Skip to content
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

Fix qvalue parsing #2753

Merged
merged 1 commit into from
Aug 13, 2023
Merged

Fix qvalue parsing #2753

merged 1 commit into from
Aug 13, 2023

Conversation

jcharlytown
Copy link

@jcharlytown jcharlytown commented Jul 20, 2023

Fixes #2751. Introduce proper regex for quality values as specified by RFC9110

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@jcharlytown
Copy link
Author

Question: Introducing a regex which tests explicitly for RFC9110 makes the parsing very strict. Alternative would be to just allow plain floats and plain ints. I'd be happy to change the PR if needed

@pgjones
Copy link
Member

pgjones commented Aug 12, 2023

I think this is good, and the right level of strictness. Yet, I also believe I'll change my mind after we find users caught out by it :conflicted: . If we do stay this strict, I don't think it can be a patch release.

@davidism
Copy link
Member

I like the idea of naming this specifically as _qvalue instead of _plain_float, since _plain_float only ended up being used in that one place anyway. I'll lift the decimal length restriction though and make it part of the next fix release.

@ThiefMaster
Copy link
Member

I think this is good, and the right level of strictness.

I think generally you should be lenient in what you accept and strict in what you generate. Is there really a good reason to become more strict there when parsing?

@davidism
Copy link
Member

davidism commented Aug 13, 2023

Nowadays I've observed more projects and maintainers leaning towards not accepting everything, because otherwise nothing gets fixed and no client/server can ever really conform to the standards. But here it doesn't really matter either way (I doubt most clients use more than one decimal place in practice), so I'm just staying lenient.

@davidism davidism changed the base branch from main to 2.3.x August 13, 2023 19:59
@davidism davidism merged commit ac9974c into pallets:2.3.x Aug 13, 2023
11 checks passed
@davidism davidism added this to the 2.3.7 milestone Aug 13, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parse_options_header rejects q values "0" and "1"
4 participants