-
Notifications
You must be signed in to change notification settings - Fork 42
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
Added option to validate incoming URL query parameters #1122
Added option to validate incoming URL query parameters #1122
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1122 +/- ##
==========================================
+ Coverage 92.33% 92.37% +0.03%
==========================================
Files 68 68
Lines 3849 3881 +32
==========================================
+ Hits 3554 3585 +31
- Misses 295 296 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @JPBergsma, thanks! It would be nice to move this logic in to the query params classes themselves, but as I mentioned, I couldn't find a clean way to do this.
We should remember to add a note in the docs about how to add custom query parameters. I also realise the config-only extensions I added for provider fields have not yet been documented, so I will raise an issue to track this separately.
A couple of comments below:
optimade/server/routers/utils.py
Outdated
if split_param[1] not in BaseResourceMapper.KNOWN_PROVIDER_PREFIXES: | ||
warn( | ||
f"The Query parameter '{param}' has an unknown provider prefix: '{split_param[1]}'. This query parameter has been ignored.", | ||
UnknownProviderQueryParameter, | ||
) | ||
elif split_param[1] in BaseResourceMapper.SUPPORTED_PREFIXES: | ||
raise BadRequest( | ||
detail=f"The query parameter '{param}' has a prefix that is supported by this server, yet the parameter is not known." | ||
) | ||
else: | ||
raise BadRequest( | ||
detail=f"The query parameter '{param}' is not known by this entry point." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would unify the error messages for the second and third cases (I'm not sure telling the client that the prefix is supported is that much more useful). This should allow the code to be simplified quite a bit (especially wrt. my other comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only issue I can foresee is if an implementation is already using a custom query parameter without registering it in the query param model. Maybe this param check should be toggle-able and off by default (for now)?
optimade/server/routers/utils.py
Outdated
for param in request.query_params.keys(): | ||
if not hasattr(params, param): | ||
split_param = param.split("_") | ||
if param.startswith("_") and len(split_param) > 2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would suggest looping over all the parameters and accumulating all the errors and warnings before emitting them.
It would probably be fine to just raise one error for all unrecognised fields, e.g.
"The query parameter(s) ['fitler', '_exmpl_test'] are not recognised by this entrypoint."
Something like:
for param in request.query_params.keys():
errors = []
warnings = []
if not hasattr(params, param):
split_param = params.split("_")
if param.startswith("_") and len(split_param) > 2:
prefix = split_param[1]
if prefix in BaseResourceMapper.SUPPORTED_PREFIXES:
errors.append(param)
elif prefix not in BaseResourceMapper.KNOWN_PROVIDER_PREFIXES:
warnings.append(param)
else:
errors.append(param)
if warnings:
warn(f"The query parameter(s) '{warnings}' are unrecognised and have been ignored.")
if errors:
raise BadRequest(f"The query parameter(s) '{errors}' are not recognised by this endpoint.")
…rn the check on or off and the check now accumulates the warnings/errors.
…the check_params method, instead of the whole request.
The function has been moved to I mostly used your suggestion, so the errors and warnings are now collected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good now @JPBergsma, couple of minor comments below!
optimade/server/config.py
Outdated
@@ -267,6 +267,10 @@ class ServerConfig(BaseSettings): | |||
Path("/var/log/optimade/"), | |||
description="Folder in which log files will be saved.", | |||
) | |||
check_parameters: Optional[bool] = Field( | |||
True, | |||
description="If True, the code will check whether the query parameters given in the URL string are correct.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description="If True, the code will check whether the query parameters given in the URL string are correct.", | |
description="If True, the server will each request whether the query parameters given in the request are correct.", |
optimade/server/query_params.py
Outdated
|
||
|
||
class BaseQueryParams: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be made an abstract class
class BaseQueryParams: | |
from abc import ABC | |
@ABC | |
class BaseQueryParams: |
Co-authored-by: Matthew Evans <7916000+ml-evs@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor changes to the docstrings so they look good in the rendered docs, otherwise LGTM!
Co-authored-by: Matthew Evans <7916000+ml-evs@users.noreply.github.com>
I processed your changes and used getattr to get the value of validate_query_parameters. This way, things won't break if they use their own config file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I processed your changes and used getattr to get the value of validate_query_parameters. This way, things won't break if they use their own config file.
I see what you are going for, but not sure how many people have written a custom config class that doesn't inherit from our one (or if we should encourage this). Seems like a harmless change to leave in though.
I'm happy with this if you are, thanks again @JPBergsma!
I'm just going to rename the PR and the linked issue to make the changelog clearer. Feel free to squash merge once that's done. |
After the discussion in #1120 I have made this function that checks whether the query parameters are known to the server or have a known prefix. If not, an appropriate error/warning is given.
I now perform this check fairly early in the process, If you want I could extract the query parameters at a later stage from the URL string. Although this would be a bit of double work as fastAPI has already done this for us.