Added option to validate incoming URL query parameters #1122

JPBergsma · 2022-04-18T14:35:35Z

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.

codecov · 2022-04-18T14:44:28Z

Codecov Report

Merging #1122 (59933df) into master (0fb4423) will increase coverage by 0.03%.
The diff coverage is 94.28%.

@@            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

Flag	Coverage Δ
project	`92.37% <94.28%> (+0.03%)`	⬆️
validator	`92.37% <94.28%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
optimade/server/warnings.py	`88.88% <50.00%> (+0.65%)`	⬆️
optimade/server/query_params.py	`98.03% <96.66%> (-1.97%)`	⬇️
optimade/server/config.py	`91.39% <100.00%> (+0.09%)`	⬆️
optimade/server/routers/utils.py	`98.13% <100.00%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fb4423...59933df. Read the comment docs.

ml-evs

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:

ml-evs · 2022-04-18T15:35:02Z

optimade/server/routers/utils.py

+                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."
+                )


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).

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)?

ml-evs · 2022-04-18T15:43:20Z

optimade/server/routers/utils.py

+    for param in request.query_params.keys():
+        if not hasattr(params, param):
+            split_param = param.split("_")
+            if param.startswith("_") and len(split_param) > 2:


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.

JPBergsma · 2022-04-19T15:34:39Z

The function has been moved to query_params.py and is part of a new BaseQueryParam class, from which the EntryListingQueryParams and SingleEntryQueryParams classes inherit it.
I hope you find this clean enough, and I wonder if we could also move the query parameters that belong to both these classes to this common base class.

I mostly used your suggestion, so the errors and warnings are now collected.
I have also defined a flag in config.py which can be used to turn the test off. I think it is good that servers perform this test, so I have set the default to True. As this may break things for servers that have implemented their own query parameters without properly declaring them in the EntryListingQueryParams and SingleEntryQueryParams, we should probably bump the OPTIMADE python tools version to 0.17.1 when this change is released.

ml-evs

Looking good now @JPBergsma, couple of minor comments below!

ml-evs · 2022-04-25T14:11:33Z

optimade/server/config.py

@@ -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.",


Suggested change

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/config.py

optimade/server/query_params.py

ml-evs · 2022-04-25T14:13:35Z

optimade/server/query_params.py

+
+
+class BaseQueryParams:


This should be made an abstract class

Suggested change

class BaseQueryParams:

from abc import ABC

@ABC

class BaseQueryParams:

Co-authored-by: Matthew Evans <7916000+ml-evs@users.noreply.github.com>

ml-evs

Very minor changes to the docstrings so they look good in the rendered docs, otherwise LGTM!

optimade/server/query_params.py

optimade/server/warnings.py

… files.

Co-authored-by: Matthew Evans <7916000+ml-evs@users.noreply.github.com>

JPBergsma · 2022-04-29T15:09:12Z

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.

ml-evs

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!

ml-evs · 2022-04-30T11:38:36Z

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.

JPBergsma added 2 commits April 18, 2022 15:51

Added a check to determine if query parameters are correct.

bb5e65a

Now I have also added the check for the single entry entry point.

7934450

JPBergsma requested review from CasperWA and ml-evs as code owners April 18, 2022 14:35

ml-evs reviewed Apr 18, 2022

View reviewed changes

This was referenced Apr 18, 2022

Add missing documentation for serving custom query params and fields #1123

Closed

Add server check for typos in query parameters #1120

Closed

JPBergsma added 4 commits April 19, 2022 16:03

Moved the check param function to query_params.py, added a flag to tu…

44b7955

…rn the check on or off and the check now accumulates the warnings/errors.

Now only the parameters as determined by FAST API are transferred to …

22cf3cf

…the check_params method, instead of the whole request.

Added docstring and now any iterable is allowed as input.

425d489

Added docstring and now any iterable is allowed as input.

b732778

Removed accidentaly added blanc line.

641ac7b

ml-evs requested changes Apr 25, 2022

View reviewed changes

JPBergsma and others added 3 commits April 25, 2022 16:40

Apply suggestions from code review

1bcff25

Co-authored-by: Matthew Evans <7916000+ml-evs@users.noreply.github.com>

Processed suggestions ml-evs, made check_params an ABC class.

5523405

Merge branch 'master' into JPBergsma_add_parameter_check

b7022ac

JPBergsma mentioned this pull request Apr 26, 2022

Raise error/warning when using unsupported pagination method #1132

Closed

JPBergsma requested a review from ml-evs April 29, 2022 12:37

ml-evs requested changes Apr 29, 2022

View reviewed changes

optimade/server/query_params.py Outdated Show resolved Hide resolved

optimade/server/warnings.py Outdated Show resolved Hide resolved

JPBergsma and others added 3 commits April 29, 2022 15:29

By using getattr we won't break compatability with preexisting config…

9989f72

… files.

Apply suggestions from code review

7d80fab

Co-authored-by: Matthew Evans <7916000+ml-evs@users.noreply.github.com>

Letting Black do it's thing after accepting changes on Github.

59933df

ml-evs approved these changes Apr 30, 2022

View reviewed changes

ml-evs added enhancement New feature or request server Issues pertaining to the example server implementation labels Apr 30, 2022

ml-evs linked an issue Apr 30, 2022 that may be closed by this pull request

Add server check for typos in query parameters #1120

Closed

ml-evs changed the title ~~Add a check to determine if query parameters are correct.~~ Add option to validate incoming URL query parameters Apr 30, 2022

ml-evs changed the title ~~Add option to validate incoming URL query parameters~~ Added option to validate incoming URL query parameters Apr 30, 2022

ml-evs merged commit 1408a7c into Materials-Consortia:master Apr 30, 2022

JPBergsma deleted the JPBergsma_add_parameter_check branch April 30, 2022 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added option to validate incoming URL query parameters #1122

Added option to validate incoming URL query parameters #1122

JPBergsma commented Apr 18, 2022

codecov bot commented Apr 18, 2022 •

edited

ml-evs left a comment

ml-evs Apr 18, 2022

ml-evs Apr 18, 2022

ml-evs Apr 18, 2022

JPBergsma commented Apr 19, 2022

ml-evs left a comment

ml-evs Apr 25, 2022

ml-evs Apr 25, 2022

ml-evs left a comment

JPBergsma commented Apr 29, 2022

ml-evs left a comment

ml-evs commented Apr 30, 2022

	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.",

Added option to validate incoming URL query parameters #1122

Added option to validate incoming URL query parameters #1122

Conversation

JPBergsma commented Apr 18, 2022

codecov bot commented Apr 18, 2022 • edited

Codecov Report

ml-evs left a comment

Choose a reason for hiding this comment

ml-evs Apr 18, 2022

Choose a reason for hiding this comment

ml-evs Apr 18, 2022

Choose a reason for hiding this comment

ml-evs Apr 18, 2022

Choose a reason for hiding this comment

JPBergsma commented Apr 19, 2022

ml-evs left a comment

Choose a reason for hiding this comment

ml-evs Apr 25, 2022

Choose a reason for hiding this comment

ml-evs Apr 25, 2022

Choose a reason for hiding this comment

ml-evs left a comment

Choose a reason for hiding this comment

JPBergsma commented Apr 29, 2022

ml-evs left a comment

Choose a reason for hiding this comment

ml-evs commented Apr 30, 2022

codecov bot commented Apr 18, 2022 •

edited