-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Regression / breaking change for RPCs with SET #3242
Comments
How about making it a deny list instead? The only settings we need to ensure are not modified are the postgrest/src/PostgREST/Query.hs Line 252 in fac7aca
This will be bad for maintenance because we can't list all extensions settings. For example
This would be bad for DX. Specially since I think the deny list for search_path and role should be enough. |
Btw that won't work either because those |
We can't tell what kind of settings a user might have on their function, which they don't want to have hoisted. We should never just do "all of them" - this is just going to lead to problems.
Then we need a config option to set those settings, that should be hoisted. The config option can default to some basic PG-builtin settings.
Certainly not. As I said above, I have other cases except those two. For example it broke for me, because I had some But there are very likely other cases out there that we will just break silently - and it was very hard for me to debug this. It will be even harder for other users, who are not that familiar with postgrest. We 100% need an allowlist approach. |
I see your point. Too bad we can't have this working transparently. So how about a
|
@taimoorzaeem Could you take this one? It's pretty much a follow up of #3142 |
I don't know any such limit, I assume there's something else going wrong. |
@steve-chavez @wolfgangwalther @laurenceisla Assigning list to config in def test_multiple_func_settings(defaultenv):
"check multiple function settings are applied"
env = {
**defaultenv,
"PGRST_DB_HOISTED_TX_SETTINGS": ["work_mem", "statement_timeout"],
}
> with run(env=env) as postgrest:
.
.
.
filename = ['work_mem', 'statement_timeout']
> ???
E TypeError: expected str, bytes or os.PathLike object, not list
<frozen os>:812: TypeError |
Using a string doesn't work? env = {
**defaultenv,
"PGRST_DB_HOISTED_TX_SETTINGS": "work_mem,statement_timeout",
} |
Environment
Description of issue
#3142 introduced quite a big breaking change - which I would consider a regression / bug. Applying all settings is never going to be correct / expected.
Take this for example:
I have a few
SECURITY DEFINER
functions in my api. Best practice for security reasons is to addpg_catalog, pg_temp
as a search_path for those kind of functions. After #3142, this suddenly changes the search_path for the whole transaction. This breaks, for example, computed fields on the returned value - and probably more.I do have other cases of
SET
tings that I don't want to have applied on the transaction level. There is a reason I set them on the function and not for the authenticator role or so.While I like the idea of hoisting some
SET
tings to the transaction level.. the current implementation is a problem. I don't even see a check forrole
, so will aCREATE FUNCTION xxx SET ROLE TO y
be hoisted, too? Bad.I think we should do at least one of, but possibly both of:
pgrst.xxx
ortx.xxx
- and then set it without that prefix on the transaction level.This needs to happen before we make the next release, otherwise we can't release a minor. In the current form, this is quite a breaking change.
The text was updated successfully, but these errors were encountered: