fix: db-pre-config function failing with pg reserved words #4420
Conversation
taimoorzaeem
force-pushed
the
fix/escape-db-pre-config
branch
3 times, most recently
from
43e90b1 to
4643eaf
Compare
steve-chavez
left a comment
steve-chavez
left a comment
There was a problem hiding this comment.
Tested it manually. Nice work 💯
When db-pre-config is accidentally set to a pg reserved word like "true", it fails with a confusing error. The function names should be properly quoted to avoid such errors. This commit resolves this by quoting the pre-config function name. Signed-off-by: Taimoor Zaeem <[email protected]>
taimoorzaeem
force-pushed
the
fix/escape-db-pre-config
branch
from
4643eaf to
3ddb343
Compare
| dumpQi (QualifiedIdentifier s i) = | ||
| (if T.null s then mempty else s <> ".") <> i | ||
|
|
||
| quoteQi :: QualifiedIdentifier -> Text | ||
| quoteQi (QualifiedIdentifier s i) = | ||
| (if T.null s then mempty else escapeIdent s <> ".") <> escapeIdent i |
There was a problem hiding this comment.
Just noticed the dumpQi is too similar to quoteQi.
Shouldn't we stay with quoteQi and drop dumpQi?
postgrest/src/PostgREST/SchemaCache/Identifiers.hs
Lines 38 to 40 in 23750e6
There was a problem hiding this comment.
The dumpQi function is used in the config dumping, here:
postgrest/src/PostgREST/Config.hs
Line 171 in 75d4131
With quoteQi we set the config to db-pre-config="true", but then get db-pre-config="\"true\"" in the config dump, which is different, right?
There was a problem hiding this comment.
Right, on second thought we shouldn't change usage on those other places on this PR.
But we definitely some clarification between dumpQi/quoteQi, even if it's just a comment on the function saying why we have two functions that are so similar. That can be done on another PR.
(if we can stick to a single function that'd be even better)
There was a problem hiding this comment.
Alright. I think we can move dumpQi to Config.hs module because that's the only place it's used. This should eliminate at least some confusion. Done that on #4435.
|
@wolfgangwalther To backport this to |
Added that label to the other PR. |
|
Successfully created backport PR for |
3 participants
When
db-pre-configis accidentally set to a pg reserved word like "true", it fails with a confusing error. The function names should be properly quoted to avoid such errors. This commit resolves the mentioned issue by quoting the pre-config function name.Closes #4380.