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

feat: raw output w/ request.accepts #2701

Closed
wants to merge 3 commits into from

Conversation

steve-chavez
Copy link
Member

@steve-chavez steve-chavez commented Mar 11, 2023

Partly addresses #1582.

BREAKING removes the raw-media-types config
BREAKING no longer Accepts text/xml by default

This already works much better than raw-media-types. We're able to override existing media types and add new ones. For example:

CREATE OR REPLACE FUNCTION my_soap_endpoint() RETURNS text AS $$

BEGIN
  perform set_config('response.headers', '[{"Content-Type": "application/soap+xml"}]', true);

  RETURN xmlelement(
 -- ...
 )::text;
END;
$$ LANGUAGE plpgsql
set request.accepts = 'application/soap+xml, text/xml, application/xml, */*';

With that we can output xml without an explicit Accept header(because it accepts */*), removing the need for Nginx as mentioned here on the docs.

cc @fjf2002

Tasks

  • Simple request.accepts for scalar functions
    - [ ] request.accepts for aggregate functions
  • Tests
    • A wildcard content-type: / should be accepted(this is valid according to this, it's up to the user to set a correct content type)
  • Validation
    • disallow setting request.accepts on a set returning function
    • disallow setting request.accepts on a function that doesn't return text/bytea
      • Maybe this restriction can be lifted, but will leave that for another PR

@@ -1047,7 +1047,8 @@ spec actualPgVersion = do
, matchHeaders = ["Content-Type" <:> "text/plain; charset=utf-8"]
}

it "can get raw xml output with Accept: text/xml" $
it "can get raw xml output with Accept: text/xml" $ do
pendingWith "Should work later with aggregates"
request methodGet "/xmltest?select=xml" (acceptHdrs "text/xml") ""
`shouldRespondWith`
"<myxml>foo</myxml>bar<foobar><baz/></foobar>"
Copy link
Member Author

Choose a reason for hiding this comment

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

IMO, this wasn't working properly because the xmlagg function requires a column(unlike json_agg). Will replace this with the aggregate solution mentioned on #1582.

* BREAKING removes the raw-media-types config
* BREAKING no longer Accepts text/xml by default
@steve-chavez
Copy link
Member Author

I might be able to make this work without breaking the xml functionality. Will scope this PR to just scalar functions.

@steve-chavez
Copy link
Member Author

Been trying to not break existing xml functionality, but it's turning out to be complicated.

Also, I thought it would be useful to keep the /tbl?select=id Accept: text/plain, but it's a complicated interface; also is really not a scalar response as documented on https://postgrest.org/en/stable/api.html#response-formats-for-scalar-responses since it's not a single value. We would have to enforce a single row is selected, but this is even more complicated for the user and also for us.

Additionally the string_agg issue on a non-text column is always one of the most visited issues #1755 according to https://github.com/PostgREST/postgrest/graphs/traffic (private link).

The only way to get a scalar response is with a scalar returning function, so we should just keep it to that.

Will proceed with breaking the select only one column magic and instead support aggregates.

@steve-chavez
Copy link
Member Author

Closing this one as doing CREATE FUNCTION ... SET request.accepts requires SUPERUSER somehow. I'll continue with the DOMAIN approach discussed on #1582.

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.

1 participant