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

[taimoorzaeem]

About 5 media type related test cases in our spec tests are actually bugs, but it is not documented. This commit adds a comment to point it out and that they should be fixed.

I think before working on #4498, we need to discuss and establish that these test cases are bugs. Looking for feedback.

[wolfgangwalther]

The case in #3391 is different - there the function explicitly says it can only return a specific mimetype. Aka, the function returns it immediately.

All cases here are about "structured datatypes" (rows) which can be transformed to potentially multiple mimetypes via aggregate handlers. None of these are bugs.

[wolfgangwalther]

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

[taimoorzaeem]

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?

[wolfgangwalther]

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)

[taimoorzaeem]

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?

[wolfgangwalther]

sure!

[wolfgangwalther]

Why is this a bug? I don't get it.

When text/xml is requested, surely we should not answer this text/plain?

The same applies to the other two cases in here.

[taimoorzaeem]

I am mostly confused on our approach for handling the Accept: header.

RFC mentions:

If the header field is present in a request and none of the available representations
for the response have a media type that is listed as acceptable, the origin server
can either honor the header field by sending a 406 (Not Acceptable) response or
disregard the header field by treating the response as if it is not subject to content
negotiation.

However, I see that we handle these cases in both approaches but it is not entirely clear on the rationale behind this.

postgrest/test/spec/Feature/Query/QuerySpec.hs

Lines 1179 to 1189 in 50eec77

	it "should respond an unknown accept type with 406" $
	request methodGet "/simple_pk"
	(acceptHdrs "text/unknowntype") ""
	`shouldRespondWith`
	[json|{"message":"None of these media types are available: text/unknowntype","code":"PGRST107","details":null,"hint":null}|]
	{ matchStatus = 406
	, matchHeaders = [
	"Content-Length" <:> "116",
	matchContentTypeJson
	]
	}

postgrest/test/spec/Feature/Query/CustomMediaSpec.hs

Lines 379 to 384 in 50eec77

	it "accepts any media type and sets it as a header" $ do
	request methodGet "/some_numbers?val=eq.2" (acceptHdrs "magic/number") ""
	`shouldRespondWith` "magic\n2"
	{ matchStatus = 200
	, matchHeaders = ["Content-Type" <:> "magic/number"]
	}

So, I assumed that we handle it leniently for requests on resources where aggregates are defined which is why I think they might be a bug and I wanted to discuss this for possible resolution.

[wolfgangwalther]

some_numbers

Note that this has a special handler defined as this:

create or replace function some_final (data "*/*")
returns "*/*" as $$
declare
  req_accept text := current_setting('request.headers', true)::json->>'accept';
  prefix bytea;
begin
  case req_accept
    when 'magic/number' then
      perform set_config('response.headers', json_build_array(json_build_object('Content-Type', req_accept))::text, true);
      prefix := 'magic';
    when 'crazy/bingo'  then
      perform set_config('response.headers', json_build_array(json_build_object('Content-Type', req_accept))::text, true);
      prefix := 'crazy';
    else
      prefix := 'anything';
  end case;
  return (prefix || data)::"*/*";
end; $$ language plpgsql;

This means that this is a function-defined mimetype. We don't treat the requested mimetype leniently - we just let the function say what it returns. This function just happens to implement a logic that returns the same mimetype that was requested. So the thing you are confused about is part of our test fixtures, not of our code.