Add a df.with_http_options combinator - #380
Add a df.with_http_options combinator#380Thom Chiovoloni (thomcc-work) wants to merge 2 commits into
df.with_http_options combinator#380Conversation
a98ff15 to
891c6c7
Compare
df.with_http_options combinator
6524b60 to
ad6874a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
e70ae4b to
558a875
Compare
This comment was marked as outdated.
This comment was marked as outdated.
7d78420 to
def1068
Compare
|
As for the motivation for why I want the ability to add more arguments, there are a few (I mention a few up there, although perhaps those can be handled by a more general policy mechanism), but in #383 I'll be adding a SELECT df.start(
df.with_http_options(
df.http(
df.endpoint('myserver', '/oauth2/token'),
'POST',
body => 'grant_type=client_credentials'
|| '&client_id=a1b2c3d4'
|| '&client_secret='
-- `encoding => 'form'` just tells us that after
-- resolving the secret, that it must be form encoded.
|| df.secret('myserver', 'client_secret', encoding => 'form'),
headers => '{"Content-Type": "application/x-www-form-urlencoded"}'::jsonb,
),
-- The options:
'{"resolve_secrets": ["body"]}'::jsonb
) |=> 'tok',
'oauth-exchange'
);This passes That said, this design admittedly has the problem of resolving secrets in the whole body, which may be fine for this case, but is... generally extremely problematic (at least potentially). But regardless, many of the solutions I've thought of for it will still require |
Several of my tasks require passing additional options to `df.http` and `df.http_multipart`. Unfortunately, adding new parameters to these functions is Hard. The normal approach (for example taken in microsoft#377 with the `df.loop`) is to keep the old function around, renamed (but with the same wrapper name). This is fine (and even avoids issues when `ALTER EXTENSION UPDATE` is not run), but `df.http` and `df.http_multipart` are functions that will likely have a bunch of `GRANT`s (and those grants are semantically meaningful to `pg_durable` beyond our ability to call the functions). If we try to capture and re-issue[^1] those grants from an extension, PG will record the grants as coming from the extension update script, and assume it doesn't need to provide them in pg_dump, so then the pg_restore won't have them, meaning logical restore and/or PG upgrades will be broken. The AI suggested the right approach was some catalog feng shui but that it would take a while to engineer. I asked on the PostgreSQL discord, and one of the PG committers (rhass) told me that this (trying to copy grants from one function to another) was something that you should never do, and to just have users reissue the grants on update. So... instead of that, we just add a combinator function that manipulates the durofut JSON directly to add the options. For example, you'd use it like: ```sql df.with_http_options( df.http(...), '{"options": "here"}'::jsonb ); ``` This admittedly is less ergonomic than adding an `options =>` parameter for `df.http`, but... well, yeah. If we want, we could make this into an operator, e.g. allowing `df.http(...) <some-operator> '{"options": "here"}'` or something like that? I don't have strong feelings. [^1]: This is ignoring the fact that the permissions may be different now, for example even ignoring the next issue this won't work quite right if a delegator of a grant became a superuser.
def1068 to
dcd046f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The helper rejects every substantive option and accepts some malformed HTTP configurations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an HTTP-node modifier while preserving existing HTTP function OIDs and ACLs. However, no functional options are currently supported.
Changes:
- Adds
df.with_http_options()and upgrade DDL. - Documents validation, permissions, and upgrade behavior.
- Adds E2E and upgrade compatibility coverage.
File summaries
| File | Description |
|---|---|
src/dsl.rs |
Implements the modifier and validation. |
sql/pg_durable--0.2.7--0.2.8.sql |
Adds upgrade DDL. |
tests/e2e/sql/69_http_options.sql |
Tests validation, execution, and privileges. |
scripts/test-upgrade.sh |
Verifies HTTP OID and ACL preservation. |
USER_GUIDE.md |
Documents user-facing usage. |
docs/api-reference.md |
Adds API reference details. |
docs/http-security.md |
Explains the permission model. |
docs/upgrade-testing.md |
Records upgrade considerations. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
e4efebf to
eb860b1
Compare
Several of my tasks require passing additional options1 to
df.httpanddf.http_multipart. Unfortunately, adding new parameters to these functions is Hard. The normal approach (for example taken in #377 with thedf.loop) is to keep the old function around, renamed (but with the same wrapper name). This is fine (and even avoids issues whenALTER EXTENSION UPDATEis not run), butdf.httpanddf.http_multipartare functions that will likely have a bunch ofGRANTs (and those grants are semantically meaningful topg_durablebeyond the user's ability to call the functions).If we try to capture and re-issue2 those grants from an extension, PG will record the grants as coming from the extension update script, and assume it doesn't need to provide them in pg_dump, so then the pg_restore won't have them, meaning logical restore and/or PG upgrades will be broken.
The AI suggested the right approach was some catalog feng shui but that it would take a while to engineer. I asked on the PostgreSQL discord, and one of the PG committers (rhass) told me that this (trying to copy grants from one function to another) was something that you should never do, and to just have users reissue the grants on update.
So... instead of that, we just add a combinator function that manipulates the durofut JSON directly to add the options. For example, you'd use it like:
This admittedly is less ergonomic than adding an
options =>parameter fordf.http, but... well, yeah.If we want, we could make this into an operator, e.g. allowing
df.http(...) <some-operator> '{"options": "here"}'or something like that? I don't have strong feelings.Anyway, this does nothing right now (all options are intentionally rejected), but this way I don't have to add it in each case where it's needed. I don't mind waiting on this until the first case where it is needed is ready to land though, I suppose.
Footnotes
Response/request length caps, separate connect/read/idle/overall timeout values, request-body source and response sink (someday, related to Keep large HTTP transfer payloads out of Duroxide history #376, for cases where we want to support huge request bodies). ↩
This is ignoring the fact that the permissions may be different now, for example even ignoring the next issue this won't work quite right if a delegator of a grant became a superuser. ↩