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: don't interpret simple parameter as deepObject #1570

Merged

Conversation

fgreinacher
Copy link
Contributor

Fixes #1565

@fgreinacher
Copy link
Contributor Author

@RobbeSneyders Here is my take on a fix. It works fine in our application:

$ http :8080/api "filter[surname]_bogus"=="greinacher"
HTTP/1.1 400 Bad Request
Content-Length: 235
Content-Type: application/vnd.api+json; charset=utf-8
Date: Tue, 02 Aug 2022 14:56:31 GMT

{
    "errors": [
        {
            "code": "xrn:err:core:invalidRequestParameter",
            "detail": "Extra query parameter(s) filter[surname]_bogus not in spec",
            "id": "a6a0b17e-e52f-37e4-a7d9-6c2e926ed4f8",
            "status": "400",
            "title": "Invalid Request Parameter"
        }
    ]
}

Comment on lines +204 to +206
if not self._is_deep_object_style_param(root_key):
return k, v, False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the first place where we know the root parameter name and have the schema information available. We already have a lot of special casing here, so that follow-up code can assume everything is fine. Let me know if you prefer to move that checker somewhere else.

@fgreinacher fgreinacher force-pushed the fix/deep-object-style-only-if-needed branch from cf35777 to 10b021d Compare August 2, 2022 21:23
@fgreinacher fgreinacher force-pushed the fix/deep-object-style-only-if-needed branch from 10b021d to 6327904 Compare August 2, 2022 21:37
@RobbeSneyders
Copy link
Member

Thanks @fgreinacher!
I've submitted a PR to fix the failing tests due to a new Flask version: #1572. I'd like to merge that one first. Feel free to review it.

@fgreinacher
Copy link
Contributor Author

fgreinacher commented Aug 8, 2022

Thanks @fgreinacher! I've submitted a PR to fix the failing tests due to a new Flask version: #1572. I'd like to merge that one first. Feel free to review it.

Thanks @RobbeSneyders, I was wondering how this failure could be related to my change :)

The MR looks good to me!

@fgreinacher
Copy link
Contributor Author

@RobbeSneyders I'm planning the last days before my summer vacation. Do you think we could get this in early next week so I can verify it with our services? 🙇

@RobbeSneyders
Copy link
Member

Sorry for the delay, I was on vacation myself :)
Should be good to go now. I'll prep a release.

@RobbeSneyders RobbeSneyders merged commit cdc8af1 into spec-first:v2 Aug 23, 2022
RobbeSneyders added a commit that referenced this pull request Aug 29, 2022
* fix: don't interpret simple parameter as deepObject

* Fix flake8 issues

Co-authored-by: Robbe Sneyders <robbe.sneyders@ml6.eu>
RobbeSneyders added a commit that referenced this pull request Aug 30, 2022
* fix: don't interpret simple parameter as deepObject

* Fix flake8 issues

Co-authored-by: Robbe Sneyders <robbe.sneyders@ml6.eu>
@fgreinacher fgreinacher deleted the fix/deep-object-style-only-if-needed branch September 11, 2022 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants