Skip to content

Ensure the headers are not modified in HTTPResponse #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 12, 2022

Conversation

spovlot
Copy link
Contributor

@spovlot spovlot commented Dec 2, 2022

Fix for issue #26 which was caused by the passed in headers dict getting modified for the response. Then the calling program reused the headers in subsequent calls. Thanks to @dhalbert and @anecdata for assistance on Discord.

@spovlot spovlot changed the title Ensure the headers are not modified Ensure the headers are not modified in HTTPResponse Dec 2, 2022
@mandar1jn
Copy link

as the author of #26 I can confirm this fixes the content length. And as far as I can see, the capitalization issue too

@anecdata
Copy link
Member

anecdata commented Dec 2, 2022

Functionally, it shouldn't matter, but should we be consistent between request and response in the use of case for headers?

@spovlot
Copy link
Contributor Author

spovlot commented Dec 5, 2022

Functionally, it shouldn't matter, but should we be consistent between request and response in the use of case for headers?

The HTTP Standard states that header names are case-insensitive - https://www.rfc-editor.org/rfc/rfc9110.html#section-5.1-3 .

The issue #27 manifested itself as a case issue. But I think that was a side effect related to the overwriting of the passed in object.

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Thanks for the fix @spovlot!

I tested this successfully on Feather S2 TFT with 8.0.0.beta4. This solution is nice so that the users dictionary doesn't get modified "silently" by the response process.

@FoamyGuy
Copy link
Contributor

agree with @spovlot as well that changing the dictionary values are the root cause of the issue noted and the fix from this change. case consistency could be discussed and / or changed in the future still independent from this fix.

@FoamyGuy FoamyGuy merged commit 11ae0bc into adafruit:main Dec 12, 2022
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Dec 13, 2022
Updating https://github.com/adafruit/Adafruit_CircuitPython_MLX90640 to 1.2.14 from 1.2.13:
  > Merge pull request adafruit/Adafruit_CircuitPython_MLX90640#30 from tcfranks/main
  > Add .venv to .gitignore

Updating https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k to 1.12.16 from 1.12.15:
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#70 from BiffoBear/add_typing
  > Add .venv to .gitignore

Updating https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer to 1.0.1 from 1.0.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#27 from spovlot/response-header-overwrite
  > Add .venv to .gitignore

Updating https://github.com/adafruit/Adafruit_CircuitPython_Logging to 5.1.0 from 5.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_Logging#44 from vladak/multiple_handlers
  > Add .venv to .gitignore

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
@spovlot spovlot deleted the response-header-overwrite branch December 15, 2022 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants