Skip to content

feat: add optional nulls=stripped parameter for mediatypes applicatio… #2894

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

Merged
merged 3 commits into from
Aug 9, 2023

Conversation

taimoorzaeem
Copy link
Collaborator

…n/vnd.pgrst.array+json and application/vnd.pgrst.object+json

Fixes issue #1601.

@steve-chavez The codecov is not working for me yet. Let me know if any warnings show up. Also, I am not exactly sure what combinations of testcases need to be written for this feature so please let me know if I need to add more.

Comment on lines 38 to 52
MTApplicationJSON == MTApplicationJSON = True
MTArrayJSONStrip == MTArrayJSONStrip = True
MTSingularJSONStrip == MTSingularJSONStrip = True
MTSingularJSON == MTSingularJSON = True
MTGeoJSON == MTGeoJSON = True
MTTextCSV == MTTextCSV = True
MTTextPlain == MTTextPlain = True
MTTextXML == MTTextXML = True
MTOpenAPI == MTOpenAPI = True
MTUrlEncoded == MTUrlEncoded = True
MTOctetStream == MTOctetStream = True
MTAny == MTAny = True
MTOther x == MTOther y = x == y
MTPlan{} == MTPlan{} = True
_ == _ = False
Copy link
Member

@steve-chavez steve-chavez Aug 7, 2023

Choose a reason for hiding this comment

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

I realize this is quite ugly. I think the whole media type handling could be refactored to use https://hackage.haskell.org/package/http-media-0.8.0.0/docs/Network-HTTP-Media.html.

(This might be complex, so not a requirement for this PR)

Then we could remove this part.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps, maybe we should move this refactoring to a new separate issue to work on?

Copy link
Member

@steve-chavez steve-chavez Aug 12, 2023

Choose a reason for hiding this comment

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

@taimoorzaeem Could you open that one 🙏 ? I'll tag it as "refactoring".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah sure!

@steve-chavez
Copy link
Member

@taimoorzaeem The end result looks good 👍. But the code needs some refactoring. I've added some comments.

…n/vnd.pgrst.array+json and application/vnd.pgrst.object+json
@steve-chavez steve-chavez merged commit fbf9bf2 into PostgREST:main Aug 9, 2023
@taimoorzaeem taimoorzaeem deleted the issue1601 branch August 10, 2023 04:06
@steve-chavez
Copy link
Member

@taimoorzaeem Just found a bug here. So currently we accept doing:

Accept: application/vnd.pgrst.object
Accept: application/vnd.pgrst.object+json

But the array fails on

Accept: application/vnd.pgrst.array

It always requires the +json suffix, which is inconvenient.

@taimoorzaeem
Copy link
Collaborator Author

@steve-chavez Oh, I can't believe how I missed this. It's an easy fix, let me send a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants