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

PATCH endpoint behaves differently #126

Closed
trondhindenes opened this issue Jan 3, 2022 · 3 comments · Fixed by #127
Closed

PATCH endpoint behaves differently #126

trondhindenes opened this issue Jan 3, 2022 · 3 comments · Fixed by #127

Comments

@trondhindenes
Copy link
Contributor

trondhindenes commented Jan 3, 2022

Noticing that the response from PATH seems to be a text-type instead of json-type response. I'm noticing in the code that the PATH response uses JSONResponse instead of the CustomJSONResponse used by many other endpoints.
As far as I can see, functions either use json response with a dict input:

JSONResponse(self.pydantic_model.schema()

or CustomJSONResponse with a string input:

        json = self.pydantic_model_plural(
            include_readable=include_readable,
            include_columns=tuple(visible_columns),
            nested=nested,
        )(rows=rows).json()
        return CustomJSONResponse(json, headers=headers)

the PATCH response is the only one using JSONResponse with a string param.

I'm guessing this is just a bug (and not a critical one at that) but before I submit a PR to fix I just wanted to make sure.

@dantownsend
Copy link
Member

@trondhindenes Yeah, it should be CustomJSONResponse.

JSONResponse uses json.dumps to convert the Python dict into a JSON string, and it doesn't handle some types. Pydantic's JSON conversion covers way more types, so I tend to use this instead with CustomJSONResponse.

So yeah, it's a bug - well spotted.

@trondhindenes
Copy link
Contributor Author

yup, I assumed something like that. Should be fairly easy to fix, I can take a stab at it.

@dantownsend
Copy link
Member

@trondhindenes Great, thanks

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 a pull request may close this issue.

2 participants