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

Expose raw body in request #1661

Open
nathancahill opened this issue Nov 21, 2020 · 11 comments
Open

Expose raw body in request #1661

nathancahill opened this issue Nov 21, 2020 · 11 comments
Labels
enhancement a feature, ready for implementation

Comments

@nathancahill
Copy link

nathancahill commented Nov 21, 2020

It would be useful to expose the raw body of a request the same way that headers are exposed:

current_setting('request.header.x', true);

Maybe:

current_setting('request.body', true);

Useful for verifying signed payloads.

I don't have much experience with Haskell but I believe it would fit in here: https://github.com/PostgREST/postgrest/blob/master/src/PostgREST/Middleware.hs#L46

@alexStrickner00
Copy link

I was also just looking for a possibility to get the request body in a pre-request function.

@wolfgangwalther
Copy link
Member

It would be useful to expose the raw body of a request the same way that headers are exposed:

I'm not sure this is a good idea. GUCs are already a bit abused by us and are not really meant to be used that way. Putting potentially huge request bodies with possibly binary data in there, doesn't sound like a good idea. Plus there would need to be an opt-out.

I was also just looking for a possibility to get the request body in a pre-request function.

But this is something that I could see happening.

What about:

  • if the pre_request function is defined without arguments, don't pass the body
  • it the pre_request function has a json argument, pass the json body (in some cases this is the raw body, in other cases this is transformed to json from some of the other types, like csv, form-data or the query string for RPCs)
  • if the pre_request function has a text (or better bytea?) argument, pass the raw request body without any changes

This would have opt-in included.

Would something like this solve your use-case, @nathancahill, as well?

I can easily see this extended to allow the pre-request function to RETURN json as well. This would even allow some sort of transformation on the input before it is passed to the query. In this case the pre_request function would have to be called in the main query, instead of in the SET LOCAL part.

@nathancahill
Copy link
Author

That would be fantastic, much better than adding it to GUCs.

@alexStrickner00
Copy link

What about:
if the pre_request function is defined without arguments, don't pass the body
it the pre_request function has a json argument, pass the json body (in some cases this is the raw body, in other cases this is transformed to json from some of the other types, like csv, form-data or the query string for RPCs)
if the pre_request function has a text (or better bytea?) argument, pass the raw request body without any changes

That would be an optimal solution for my usecase. 👍

@steve-chavez
Copy link
Member

@wolfgangwalther The pre-request can also be used for things like changing the role and considering #1600 (comment) we'd need the pre-request to be called before the query right?

So maybe we should use a different function for this case, maybe a pre-body.

@wolfgangwalther
Copy link
Member

@wolfgangwalther The pre-request can also be used for things like changing the role and considering #1600 (comment) we'd need the pre-request to be called before the query right?

Yes, that's true.

Taking this:

So maybe we should use a different function for this case, maybe a pre-body.

And this, from the other thread:

@wolfgangwalther The pre-request way seems more flexible. But changing the payload would have to be handled on a per-path basis(with conditionals).

It looks like we are on the way to some kind of a hook system. If we have pre-body, there are certainly other use-cases that could take a hook as well. If we can have different hooks on different paths, that would be very flexible. We could extend that to pre-request, which could be different by path!

All those hooks have in common, that they are optional, so that the regular case is just exactly the same query as without the hooks.

Of course SET on the function definition immediately comes to mind to declare some functions as hooks. I'm a bit hesitant, however, because of the whole inlining stuff. We'd have to do some brainstorming first to get a good idea of where hooks could be used (even without implementing them from the start) to see whether any of those would need inlining for performance.

So currently we have pre-request and pre-body. None of those needs inlining, so we could do something like:

-- pre-request hook just for the /clients table
CREATE FUNCTION clients_pre_request() RETURNS void
SET pgrst.hook.pre-request='clients'
LANGUAGE SQL AS $$ <big time protection> $$;

-- pre-body hook for clients and projects
CREATE FUNCTION pre_body(body JSON) RETURNS JSON
SET pgrst.hook.pre-body='clients,projects'
LANGUAGE SQL AS $$ <big time transformation> $$;

Not sure what kind of values we'd accept in this case for the hook SET, the most flexible way would be some kind of regex.

@wolfgangwalther
Copy link
Member

wolfgangwalther commented Dec 11, 2020

I thought a bit more about it... and I don't like it :D. I don't like setting the hooks config on the function definition. The inlining is one thing, but this will easily lead to situations where multiple functions have the same setting. Should we run all of them? In which order? One of them? Which? Complicated!

A more natural extension of what we currently have, would be to allow more config options, like this:

db-pre-request = "default_pre_request_fn"
db-pre-request.clients = "clients_pre_request_fn"
db-pre-body.projects = "projects_pre_body_fn"
db-pre-body = "default_pre_body_fn"
db-pre-body.rpc.do_stuff = "do_stuff_pre_body_fn"

I'm not sure about the last one... - we might just not implement this for RPCs, because this could get ugly with finding the right overloaded function - the pre-body fn must not change the name of the arguments, etc... much better would be to allow arbitrary bodies for RPC calls (e.g. via unnamed BYTEA argument, similar to the "single json object" approach).

Rules are simple: /clients gets the clients_pre_request_fn, all others the default. So more specific overrides less specific. If both should apply, the clients_pre_request_fn can still make a call to the default.

And once we have in-database configuration we can still put a ALTER DATABASE .. SET db.pre-body.clients='...' command right next to our function definition, so that we have all the code together in one place.

This be a lot less ambiguous, extend the pre-request config option we currently have nicely, avoid all problems with inlining... I like it!


If that's the way to go, we might think about renaming our config options to use dots as separators instead of -. E.g. db.schemas instead of db-schemas and so on. This is because app.settings.xyz has to use dots because of the config parser we use. This would be more consistent. We already have the machinery in place to add the current names as aliases, so this would be no breaking change!

@nathancahill
Copy link
Author

Yes, that last option makes the most sense to me @wolfgangwalther.

For RPCs, an unnamed BYTEA argument would be awesome.

@FGRibreau
Copy link
Contributor

FGRibreau commented Mar 2, 2021

@steve-chavez hello steve! I'm working for a company interested by this feature, is there any way we could sponsor its implementation?

@steve-chavez
Copy link
Member

@FGRibreau Hey Francois. Sure! I've sent you an email to github@fgribreau.com, let's follow up there.

@nathancahill
Copy link
Author

The unnamed JSON parameter fulfills my use case for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement a feature, ready for implementation
Development

No branches or pull requests

5 participants