Skip to content

test(spec): add comment on media type related bugs #4510

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.

Sign up for GitHub

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

Draft
taimoorzaeem wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

test(spec): add comment on media type related bugs #4510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions test/spec/Feature/Query/CustomMediaSpec.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ spec = describe "custom media types" $ do
simpleBody r `shouldBe` readFixtureFile "1.twkb"
simpleHeaders r `shouldContain` [("Content-Type", "application/vnd.twkb")]

-- FIXME: This is a bug, twkb_agg exists for "lines" table
it "will fail if there's no aggregate defined for the table" $ do
request methodGet "/lines" (acceptHdrs "text/plain") ""
`shouldRespondWith`
Expand Down Expand Up @@ -116,6 +117,7 @@ spec = describe "custom media types" $ do
, matchHeaders = ["Content-Type" <:> "text/xml; charset=utf-8"]
}

-- FIXME: This is a bug, this should return text/plain
it "should fail with function returning text and Accept: text/xml" $ do
request methodGet "/rpc/welcome" (acceptHdrs "text/xml") ""
`shouldRespondWith`
Expand Down Expand Up @@ -160,6 +162,7 @@ spec = describe "custom media types" $ do
simpleBody r `shouldBe` readFixtureFile "lines.twkb"
simpleHeaders r `shouldContain` [("Content-Type", "application/vnd.twkb")]

-- FIXME: This is a bug, application/vnd.twkb aggregate exists for "get_lines" table
it "fails if doesn't have an aggregate defined" $ do
request methodGet "/rpc/get_lines"
(acceptHdrs "application/octet-stream") ""
Expand Down
3 changes: 3 additions & 0 deletions test/spec/Feature/Query/RawOutputTypesSpec.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,13 @@ spec = describe "When raw-media-types config variable is missing or left empty"
`shouldRespondWith` [json| [{"id":1}] |]
{ matchHeaders= ["Content-Type" <:> "application/json; charset=utf-8"] }

-- FIXME: This shouldn't default to application/json because text/tab-separated-values aggregate exist
it "responds json to a GET request to RPC with Firefox Accept headers" $
request methodGet "/rpc/get_projects_below?id=3" firefoxAcceptHdrs ""
`shouldRespondWith` [json|[{"id":1,"name":"Windows 7","client_id":1}, {"id":2,"name":"Windows 10","client_id":1}]|]
{ matchHeaders= ["Content-Type" <:> "application/json; charset=utf-8"] }

-- FIXME: This shouldn't default to application/json because text/tab-separated-values aggregate exist
it "responds json to a GET request to RPC with Chrome Accept headers" $
Comment on lines +26 to 33
Copy link
Member

@wolfgangwalther wolfgangwalther Dec 2, 2025

Choose a reason for hiding this comment

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

I don't understand why these two should be failures. Why is defaulting to application/json for */* a bug?

Copy link
Collaborator Author

@taimoorzaeem taimoorzaeem Dec 2, 2025

Choose a reason for hiding this comment

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

This one is a bit confusing for me, as per my understanding */* should return whatever representation is available, text/tab-separated-values is available, then why default to application/json?

Copy link
Member

@wolfgangwalther wolfgangwalther Dec 3, 2025

Choose a reason for hiding this comment

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

We have two general cases of return values from "things" (tables/views/functions):

  • "un-mime-typed" values, in most of the cases this is a composite/row value, for example for tables or views, but also often for functions. It can be simple integer values or text or bytea as well, though.
  • "mime-typed" values, which specifically return a domain type that is associated to a certain mimetype. A function can say "I only return image/png" or whatever.

Let's discuss the second case first: These are simple. If you get a request with a "matching" accept header, you call the function, otherwise you return an error. If the function returns image/png, you'd accept */*, image/* and image/png. Once any of that is in the Accept header, you can handle the request.

The first case is different - you need handlers to transform the non-mime-typed values into mimetypes. We have a default builtin handler that does application/json. We can add more handlers via aggregates.

Note that the tests we are commenting on here are this first case - the function returns a project composite type, so you need handlers to make any sense of its return value. Now, let's discuss a few cases for requests to this endpoint:

  • Accept: text/tab-separated-values - simple, you use that tsv handler, ofc.
  • Accept: application/json - also simple, you return json.
  • Accept: text/tab-separated-values, application/json... we're likely going to go left to right, so do tsv? Not sure exactly what the RFC about content-negotiation says here, not really relevant for the point right now.
  • Accept: text/tab-separated-values, */* - ofc, we want the more specific handler first, so we are going to return tsv here.
  • Accept: */* - we can do anything. There is no reason why we should chose tsv here, when it was not explicitly requested. The default assumption for PostgREST is to return JSON, so we should do it here.

If we don't return JSON in the last example - what are you going to return when you have multiple custom aggregates for the same case? How are you going to prioritize between them?

(this question still needs to be solved, once we do more sophisticated matching and you have Accept: text/* and your aggregates are text/tsv and text/plain... - but that still doesn't mean we shouldn't use application/json as the default, when possible)

Copy link
Collaborator Author

@taimoorzaeem taimoorzaeem Dec 5, 2025

Choose a reason for hiding this comment

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

Many thanks for the detailed explantion! Would it be OK if I add some of these important details as comments in the code base where appropriate?

Copy link
Member

@wolfgangwalther wolfgangwalther Dec 5, 2025

Choose a reason for hiding this comment

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

sure!

request methodGet "/rpc/get_projects_below?id=3" chromeAcceptHdrs ""
`shouldRespondWith` [json|[{"id":1,"name":"Windows 7","client_id":1}, {"id":2,"name":"Windows 10","client_id":1}]|]
Expand Down