Skip to content

bpo-25439: Added additional type checks to urllib.request.Request #10616

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

bpo-25439: Added additional type checks to urllib.request.Request #10616

wants to merge 5 commits into from

Conversation

CuriousLearner
Copy link
Member

@CuriousLearner CuriousLearner commented Nov 20, 2018 •
edited by bedevere-bot
Loading

Add additional type checks in urllib.request.Request to ensure URL is of
type str, POST data and it's keys are of type bytes.

https://bugs.python.org/issue25439

@CuriousLearner CuriousLearner force-pushed the fix-25439 branch 2 times, most recently from 068940d to 647b7aa Compare November 21, 2018 20:00
@CuriousLearner
Copy link
Member Author

Hello @ezio-melotti @vadmium

Can you please have a look at this?

vadmium
vadmium reviewed Dec 8, 2018
View reviewed changes
Copy link
Member

@vadmium vadmium left a comment

Choose a reason for hiding this comment

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

FWIW some of my earlier comments also still apply, in particular dict-of-bytearray https://bugs.python.org/issue25439#msg253815 and not mentioning the dict special case https://bugs.python.org/review/25439/diff/15859/Doc/library/urllib.request.rst#newcode181

@CuriousLearner
Copy link
Member Author

@vadmium Can you have a look now?

vadmium
vadmium reviewed Feb 22, 2019
View reviewed changes
Copy link
Member

@vadmium vadmium left a comment

Choose a reason for hiding this comment

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

I had a look, but I don’t like the documentation changes, and I don’t understand why you want to specifically whitelist bytes and memoryview dictionary keys, yet drop support for other kinds of keys.

@CuriousLearner
Merge branch 'master' into fix-25439
2cb5fd7 
* master: (1204 commits)
  bpo-31855: unittest.mock.mock_open() results now respects the argument of read([size]) (pythonGH-11521)
  Forbid creating of stream objects outside of asyncio (python#13101)
  bpo-35925: Skip SSL tests that fail due to weak external certs. (pythonGH-13124)
  Fix rst formatting for several links in ssl documentation (pythonGH-13133)
  bpo-36542: Allow to overwrite the signature for Python functions. (pythonGH-12705)
  bpo-36793: Remove unneeded __str__ definitions. (pythonGH-13081)
  bpo-36766: Typos in docs and code comments (pythonGH-13116)
  bpo-36275: enhance documentation for venv.create() (pythonGH-13114)
  Clarify the download unit in the download section (pythonGH-13122)
  bpo-30668: add missing word in license.rst (pythonGH-13115)
  Unroll import-team in CODEOWNERS (python#13118)
  bpo-36594: Fix incorrect use of %p in format strings (pythonGH-12769)
  bpo-36798: Updating f-string docs for := use case (pythonGH-13107)
  Update wsgiref.rst (python#10488)
  Doc/c-api/exceptions.rst: fix grammar (python#12091)
  bpo-36811: Fix a C compiler warning in _elementtree.c. (pythonGH-13109)
  Only count number of members once (python#12691)
  bpo-16024: Doc cleanup regarding path_fd, dir_fd, follow_symlinks (pythonGH-5505)
  bpo-36791: Safer detection of integer overflow in sum(). (pythonGH-13080)
  bpo-33530: Implement Happy Eyeballs in asyncio, v2 (pythonGH-7237)
  ...
@CuriousLearner
Copy link
Member Author

Hey @vadmium !

I whitelisted bytes and memoryview solely based on examples. Do you think we can improve this somehow? What do you think we can improve in the documentation?

@CuriousLearner
Copy link
Member Author

Hello @vadmium Do you have any suggestions on this?

MaxwellDupre
MaxwellDupre approved these changes May 12, 2022
View reviewed changes
Copy link
Contributor

@MaxwellDupre MaxwellDupre left a comment

Choose a reason for hiding this comment

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

Run tests passed for both urllib and 2.
Looks ok.

iritkatriel
iritkatriel previously requested changes Nov 26, 2022
View reviewed changes
Copy link
Member

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

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

This has a merge conflict now.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@CuriousLearner
Copy link
Member Author

Hi @iritkatriel

Seems like Ezio updated the branch with latest changes in master. Please have a look and let me know if there is anything I can do to get this merged :)

Thanks!

@iritkatriel iritkatriel dismissed their stale review November 28, 2022 09:36

conflict was resolved.

@iritkatriel
Copy link
Member

Hi @iritkatriel

Seems like Ezio updated the branch with latest changes in master. Please have a look and let me know if there is anything I can do to get this merged :)

Thanks!

You would need a core dev who's enough of an expert in this area to agree with the change. It looks like you have an unresolved discussion with @vadmium .

@encukou
Copy link
Member

encukou commented Mar 19, 2024 •
edited
Loading

Unfortunately, @vadmium isn't very active any more, and the PR went unreviewed for years. Sorry for that.

I'm OK with the check for common mistake (if isinstance(data, str): raise) in data setter. We can be reasonably sure that this doesn't break existing code.
On the other hand, allowing only bytes and memoryview closes the door for types that might work. How can we be sure that only these two types work?

The extra check for keys adds overhead to the non-error case; that's not ideal.

But overall, I think the issue made much more sense when it was opened, Python 3 was new and mixing up str and bytes was much more common than today. Also, we now have generic tools to detect type errors early -- static type checkers.
I think that today, it's best to close the PR without action. I'll do that in a month if there are no objections.

@encukou encukou closed this Apr 19, 2024
@ezio-melotti ezio-melotti mentioned this pull request Apr 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vadmium vadmium vadmium left review comments

@MaxwellDupre MaxwellDupre MaxwellDupre approved these changes

@iritkatriel iritkatriel iritkatriel left review comments

Assignees
No one assigned
Labels
awaiting core review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@CuriousLearner @bedevere-bot @iritkatriel @encukou @vadmium @MaxwellDupre @the-knights-who-say-ni @ezio-melotti