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

added option to also check for unknown query parameters #1297

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mortbauer
Copy link

Currently unknown query parameters are just ignored by fastapi, I feel that it should be at least possible to change the behaviour to return a validation error also for unknown.

It adresses #1190

  • not yet documented
  • not yet any testing
  • maybe different path to pass that option
  • maybe rename to something else, maybe could be thought to generally
    check unknowns, however for models, pydantic has to be adapted

@codecov
Copy link

codecov bot commented Apr 21, 2020

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (77f7447) 100.00% compared to head (3c0a556) 100.00%.
Report is 2207 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1297   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          240       241    +1     
  Lines         7101      7147   +46     
=========================================
+ Hits          7101      7147   +46     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jrocketdev
Copy link

Is there any progress on this? I see there are some merge conflicts and missing tests. Having this optional feature would be a nice to have for some work I am doing as well

@mortbauer
Copy link
Author

No progress from my side, I'm a bit short on time and before investing into testing and docs I thought to wait for a bit of feedback about the implementation. But I guess with more than 60 pending pull requests it is quite difficult to keep up.

@sebg1
Copy link

sebg1 commented Jul 24, 2020

I am interested with the fix, when it will be available.

@jeremyschiemann
Copy link

+1

@hawkaa
Copy link

hawkaa commented Aug 9, 2020

I'm interested in the fix too. I know it has been considered as an edge case by @tiangolo in a similar issue/PR, but I can explain my niece use case.

I'm implementing the "OGC API - Features - Part 1: Core" (http://docs.opengeospatial.org/is/17-069r3/17-069r3.html) which is a standard for fetching geospatial features over HTTP. In section 7.6 we read the following:

The server SHALL respond with a response with the status code 400, if the request URI includes a query parameter that is not specified in the API definition.

This is not a big deal, but just wanted to let you guys know of an use case where this functionality would come in handy.

* minimal documentation added
* tests added
* maybe different path to pass that option
* maybe rename to something else, maybe could be thought to generally
  check unknowns, however for models, pydantic has to be adapted
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2020

📝 Docs preview for commit 8ea5a24 at: https://5fa5acf112b34d10164c20b5--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2020

📝 Docs preview for commit 49ba6a2 at: https://5fa5b0194381b919d55b3452--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2020

📝 Docs preview for commit 5463435 at: https://5fa5b48cef870919146ebb88--fastapi.netlify.app

@mortbauer
Copy link
Author

so Í took some time to work on this again:

  • rebased
  • added tests
  • added minimal docs
  • added support to also check for unknown body params
    could it be merged?

@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2020

📝 Docs preview for commit 2c38b3b at: https://5fa5b7b1402533205fc19f18--fastapi.netlify.app

app_check = FastAPI(check_unknown=True)

client_no_check = TestClient(app_no_check)
client_ceck = TestClient(app_check)
Copy link

Choose a reason for hiding this comment

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

Not a big deal, but should probably be client_check and not client_ceck 👍

Copy link
Author

Choose a reason for hiding this comment

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

yes you are right, changed that :)

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2020

📝 Docs preview for commit 3c0a556 at: https://5fa7d2e68d2a1a3e4a180f0e--fastapi.netlify.app

@philipbjorge
Copy link

@mortbauer @boyaps @diogoduartec --
We'd love to have this feature for our API product.
Do you know what it will take to move this across the finish line?

Thanks -- Appreciate all the hard work you're putting into FastAPI!

@gergelyhunyady
Copy link

gergelyhunyady commented May 13, 2021

@mortbauer @hawkaa @boyaps @diogoduartec
I also believe this is a reasonable optional feature to add. Could serve the good in many use-cases.

As far as I understand the proposed implementation would allow users to specify check_unknown either for the whole service (via passing it to the FastAPI constructor) or alternatively they could specify it for specific APIRouters only. I think this is pretty nice. Next to that I think it would be also reasonable if one could go even more fine grained by specifying the check_unknown behavior for specific path operations exclusively if needed (@router.get("/users", ..., check_unknown=True)). I didn't properly investigate if it fits well to the implementation of fastapi or if it would be hard to implement, so this is just my view from the user perspective.

Another point I would stress is the name of the parameter. I feel the check_unknown name is a bit too general. If the behavior strictly means only that unknown parameters will be caught and an error response will be returned to the caller (which I think is the reasonable behavior), then I think something like check_unknown_parameters or maybe even better forbid_unknown_parameters (which would be in line with the forbid setting of the extra field in the pydantic model config) could reflect the behavior better.

Thanks for your work! What's your opinion on my two cents?

@hawkaa
Copy link

hawkaa commented May 16, 2021

@gergelyhunyady I agree on the naming part! And if we can align the naming with pydantic that is even better.

As for the specific path keyword, I have no strong opinions.

@gidven
Copy link

gidven commented Oct 28, 2021

Really keen to see this feature make it to the main branch! I am afraid I cannot directly contribute to the source code but would certainly like to test this thoroughly. Aligning the param name with pydantic as forbid_unknown_params definitely recommended.

@gguta
Copy link

gguta commented Jan 5, 2022

Are there any progress on this?

if received_body is not None:
left_body_params = set(received_body)
else:
left_body_params = set()
Copy link
Contributor

Choose a reason for hiding this comment

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

left_body_params = set(received_body or {})

@@ -641,6 +660,7 @@ def request_params_to_args(
async def request_body_to_args(
required_params: List[ModelField],
received_body: Optional[Union[Dict[str, Any], FormData]],
check_unknown: bool = False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to name is_unknown.

if check_unknown:
dependant_names = {param.name for param in dependant.query_params}
left_query_params.difference_update(dependant_names)
if outer_level:
Copy link
Contributor

Choose a reason for hiding this comment

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

Merge this condition to the upper one. Avoid nested conditions if you do not have to.

@pacoispaco
Copy link

pacoispaco commented Feb 21, 2022

I am also interested in this feature. However shouldn't the default status code returned be a 404 and not a 400? Query parameters are part of the URL that identify a resource. And in the case of an unexpected query parameter the URL, by definition, would be identifying a resource that does not exist.

@danhje
Copy link

danhje commented May 12, 2022

Just noticed the application I'm currently working on has been querying an internal API with an old query parameter name for months, without anyone noticing, resulting in reduced quality of it's output. Will definitely make use of this feature when it is available!

@stat-johan
Copy link

I would also greatly appreciate this feature

@grzracz
Copy link

grzracz commented Jul 5, 2022

Would also deeply appreciate this.
We have a freely available API and because of "bad" query parameters, bad actors are able to bypass cache.
Instead of

/assets/X/price?currency=USD

they are calling

/assets/X/price?currencyVNO1O1X114

adding a random string. I'd love to return a 400 when something like this is attempted.

@Toby14
Copy link

Toby14 commented Jul 20, 2023

Would also appreciate this feature as users would know what was wrong if they make a typo or unknown param.

@xsway
Copy link

xsway commented Aug 23, 2023

I also definitely see a use for this. Just wanted to say that a fail on validation is not maybe needed, if there is be a way to show a warning, that would be sufficient imo.

@tiangolo tiangolo added p3 feature New feature or request and removed investigate labels Jan 15, 2024
@RonC-LL
Copy link

RonC-LL commented Feb 1, 2024

I'm just here to lend my support for this feature. Misspelling a param bit me in the butt as well. Fortunately, it's not production code or anything.

@GidVeo
Copy link

GidVeo commented Apr 26, 2024

For anyone interested in a solution using a subclass of APIRoute, see the discussion here:
#7697 (comment)

Credit to https://github.com/jenden who provided the first example of a solution for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request p3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet