Fix bug where already-quoted search path components could get double-quoted when creating the migrations table. #47

GriffinSchneider · 2019-12-25T23:51:56Z

Postgres's SHOW search_path returns the search path as a comma-separated string with spaces, like "$user", public, "another-quoted-schema", so checking whether the first character of a search path component is " is not sufficient. Instead, just check whether the search path component contains quotes at all, and don't add any quotes if it does.

To reproduce the bug:

Run:

ALTER ROLE <username> SET search_path TO "$user",public,"something-with-quotes";

Apply migrations

db-migrate-pg tries to run something like this (note the ""):

SET search_path TO "$user","public",""something-with-quotes""

Get an error like:

[ERROR] error: zero-length delimited identifier at or near """"
    at Connection.parseE (/Users/griffinschneider/dev/core/node_modules/pg/lib/connection.js:604:13)
    at Connection.parseMessage (/Users/griffinschneider/dev/core/node_modules/pg/lib/connection.js:403:19)
    at Socket.<anonymous> (/Users/griffinschneider/dev/core/node_modules/pg/lib/connection.js:123:22)
    at Socket.emit (events.js:305:20)
    at Socket.EventEmitter.emit (domain.js:483:12)
    at addChunk (_stream_readable.js:341:12)
    at readableAddChunk (_stream_readable.js:316:11)
    at Socket.Readable.push (_stream_readable.js:250:10)
    at TCP.onStreamRead (internal/stream_base_commons.js:186:23)

commitlint-wzrdtales · 2019-12-25T23:52:01Z

There were the following issues with this Pull Request

Commit: bba89f4
- ✖ message may not be empty
  , - ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.

🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

…components could get double-quoted when creating the mirations table. Postgres's `SHOW search_path` returns the search path as a comma-separated string with spaces, like `"$user", public, "another-quoted-schema"`, so checking whether the _first character_ of a search path component is `"` is not sufficient. Instead, just check whether the search path component contains quotes at all, and don't add any quotes if it does. Signed-off-by: Girffin Schneider <griffinschneider@gmail.com>

mgabeler-lee-6rs · 2020-01-09T20:57:35Z

Any outlook on when this can merge? This bug is giving me hell right now 😉

wzrdtales · 2020-01-09T21:13:22Z

Sry went under the radar.

This is missing a test, otherwise that might be fine to suggest. I doubt that anyone names their tables with an escaped quote, nor it should be allowed by most db systems using the pg wire protocol.

mgabeler-lee-6rs · 2020-01-09T21:23:41Z

My failure for this is far more simple.

Using pgBouncer for connection pooling
Using a non-default schema for one app
The default postgresql search_path of "$user", public

Result:
First use of a connection does:
SET search_path TO "myschema","$user","public"
Second use of a connection from the bouncer does:
SET search_path TO "myschema","myschema",""$user"","$public"
Which fails with a syntax error

Side thought: if the schema name didn't need to be quoted in the output of SHOW search_path, why bother trying to add quotes to it for SET search_path? It seems only the path element being added may need quoting? Is there a corner case I'm missing?

mgabeler-lee-6rs · 2020-01-09T21:27:07Z

Side Side note: The SET search_path this is doing isn't even effective for my use case, because it may run against a different "real" connection on the far side of the bouncer from the actual migrations. To be safe against such situations, it would need to be done as SET LOCAL search_path TO ... and happen inside a / each transaction, not at the global connection level.

... which also means it ends up unwittingly putting the migrations table in the wrong schema...

jmeirelesfood · 2020-01-17T17:29:03Z

index.js

@@ -219,7 +219,7 @@ var PgDriver = Base.extend({
          var searchPathes = result[0].search_path.split(',');

          for (var i = 0; i < searchPathes.length; ++i) {
-            if (searchPathes[i].indexOf('"') !== 0) {
+            if (searchPathes[i].indexOf('"') !== -1) {


The indexOf() method returns the first index at which a given element can be found in the array, or -1 if it is not present.

So in this case shouldn't we use searchPathes[i].indexOf('"') === -1 instead? If the search_path does not have a " we will surround with one.

I'm not convinced this quoting code is needed at all. The return from postgres of SHOW search_path quotes anything that needs it, so it shouldn't be necessary to apply quoting to anything that came from that, only to the new entry being added.

... I think.

If there is no BC then there is no need for quote at all. But the bug fix that is on this PR is still not fixing anything at all, it's still duplicating the quotes.

yes that is true, b/c it would need to be === -1. However I think it might be true that SHOW search_path will actually not return anything that needs to be quoted. But can't guarantee it as well. Since the pg driver is also the base for pgwire drivers it could have side effects on those. Since I have no guarantee I will rather keep this to avoid a breaking change, but accept a bugfix for this of course.

SHOW search_path; could return "$user" (with quotes) https://www.postgresql.org/docs/current/ddl-schemas.html. So this PR must be accepted to prevent throwing an exception.

[SQL] SET search_path TO "some_schema",""$user"","public" [ERROR] AssertionError [ERR_ASSERTION]: ifError got unwanted exception: zero-length delimited identifier at or near """"

we tried and merged it, but it causes the test suite to fail. this is breaking not fixing things. if anyone wants to jump on this again feel free to.

wzrdtales · 2023-09-10T20:28:48Z

this change causes the exact error message the creator was getting. zero-length delimited identifier at or near """"

test suite is finally running again now, just merged for testing and reverted

mgabeler-lee-6rs · 2023-09-11T15:27:51Z

The CI failure is sadly very difficult to interpret, it's hard to tell where the error is coming from at all :(

Given that PG seems to return the search_path value always already quoted, I retain my earlier suggestion that this entire chunk of code can probably get deleted and just prepend the desired search path to the current one unconditionally. Having a schema listed multiple times in the search path doesn't seem to be an error, so this seems like it should be safe even in cases where it is redundant.

wzrdtales · 2023-09-11T15:41:23Z

Actually not really.

The last commands executed with your attempted fix are

[SQL] show server_version_num
[SQL] SHOW search_path
[SQL] SET search_path TO ""$user"", public
xx[SQL] SET search_path TO test_schema2
[SQL] SHOW search_path

and without

[SQL] show server_version_num
[SQL] SHOW search_path
[SQL] SET search_path TO "$user","public"
[SQL] SELECT table_name FROM information_schema.tables WHERE table_name = 'migrations' AND table_schema = 'public'

The $user search path returned by SHOW search_path by postgres is

postgres=# SHOW search_path;
   search_path   
-----------------
 "$user", public
(1 Zeile)

So the check for a starting quote, does its job as it should.

If you want to try get this fix done, the test suite is working now again and is rewritten on lab. You can add global.verbose = true to the tests to temporarily see all the executed queries.

wzrdtales · 2023-09-11T15:43:31Z

I am not a fan of this search path logic anyways, so if there is a better way to handle, I would be glad to discuss 😅

wzrdtales · 2023-09-11T15:46:48Z

Ultimately:

https://github.com/db-migrate/pg/pull/47/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R222

Your change here caused the check if "first char is not a quote" to become, there is a quote anywhere in the string. Causing the quoting to be triggered. If you actually wanted to avoid quoting this was doing the inverse of it.

mgabeler-lee-6rs · 2023-09-11T15:51:28Z

So the check for a starting quote, does its job as it should.

This only works for the first element of the search path as written:

postgres=# set search_path = public,"$user";
SET
postgres=# show search_path ;
   search_path   
-----------------
 public, "$user"
(1 row)

oops, the first char of the second element is ... a space due to not trimming the paths after splitting on commas.

We can get more fun if we e.g. start with set search_path = "$user", public, "bobby, tables"; where now the split is just completely wrong.

Your change

I'm not the PR author ;)

caused the check if "first char is not a quote" to become, there is a quote anywhere in the string. Causing the quoting to be triggered. If you actually wanted to avoid quoting this was doing the inverse of it

Yep ... though as we can see with the formatting issue above, fixing this logic inversion would also have caused "$user", public to become "$user", " public" which is also wrong.

My proposal is to replace this entire block: https://github.com/db-migrate/pg/blob/master/index.js#L252-L272

With:

return this.all(`SET search_path TO "${this.schema}", ${result[0].search_path}`);

wzrdtales · 2023-09-11T15:52:15Z

I'm not the PR author ;)

I didn't exactly scroll through and check :)

wzrdtales · 2023-09-11T15:54:14Z

My proposal is to replace this entire block

The thing is, those things were added for a reason. I don't know them anymore, but I am certainly breaking someones setup if I'm going to remove this. Generally I would say it should be save to assume, the db returns a usable string that itself understands though.

wzrdtales · 2023-09-11T15:55:57Z

It is probably all this tango before, trying to get a version from various amounts of different postgres versions and variations. So I guess, we really can't remove this safely, just trying to actually make it work.

wzrdtales · 2023-09-11T15:58:08Z

The search path actually gets trimmed here.

pg/index.js

Line 257 in 00aa56b

searchPathes[i] = '"' + searchPathes[i].trim() + '"';

This is probably just too late.

wzrdtales · 2023-09-11T16:06:37Z

yes its too late. added a test reproducing this, and putting trim before actually fixes this issue.

wzrdtales · 2023-09-11T16:27:06Z

fixed in 13e956b
fixed in db-migrate-pg@1.5.1

GriffinSchneider force-pushed the patch-1 branch from bba89f4 to 15faa5a Compare December 25, 2019 23:54

GriffinSchneider force-pushed the patch-1 branch from 15faa5a to 79436f3 Compare December 25, 2019 23:55

GriffinSchneider changed the title ~~Fix bug where already-quoted search path components could get double-quoted when creating the mirations table.~~ Fix bug where already-quoted search path components could get double-quoted when creating the migrations table. Dec 26, 2019

jmeirelesfood suggested changes Jan 17, 2020

View reviewed changes

wzrdtales merged commit c3588aa into db-migrate:master Sep 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug where already-quoted search path components could get double-quoted when creating the migrations table. #47

Fix bug where already-quoted search path components could get double-quoted when creating the migrations table. #47

GriffinSchneider commented Dec 25, 2019 •

edited

commitlint-wzrdtales bot commented Dec 25, 2019

mgabeler-lee-6rs commented Jan 9, 2020

wzrdtales commented Jan 9, 2020

mgabeler-lee-6rs commented Jan 9, 2020

mgabeler-lee-6rs commented Jan 9, 2020 •

edited

jmeirelesfood Jan 17, 2020

mgabeler-lee-6rs Jan 17, 2020

jmeirelesfood Jan 17, 2020

wzrdtales Jan 22, 2020

nhkhanh Sep 5, 2022 •

edited

wzrdtales Sep 10, 2023

wzrdtales commented Sep 10, 2023

mgabeler-lee-6rs commented Sep 11, 2023

wzrdtales commented Sep 11, 2023 •

edited

wzrdtales commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

mgabeler-lee-6rs commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

Fix bug where already-quoted search path components could get double-quoted when creating the migrations table. #47

Fix bug where already-quoted search path components could get double-quoted when creating the migrations table. #47

Conversation

GriffinSchneider commented Dec 25, 2019 • edited

commitlint-wzrdtales bot commented Dec 25, 2019

mgabeler-lee-6rs commented Jan 9, 2020

wzrdtales commented Jan 9, 2020

mgabeler-lee-6rs commented Jan 9, 2020

mgabeler-lee-6rs commented Jan 9, 2020 • edited

jmeirelesfood Jan 17, 2020

Choose a reason for hiding this comment

mgabeler-lee-6rs Jan 17, 2020

Choose a reason for hiding this comment

jmeirelesfood Jan 17, 2020

Choose a reason for hiding this comment

wzrdtales Jan 22, 2020

Choose a reason for hiding this comment

nhkhanh Sep 5, 2022 • edited

Choose a reason for hiding this comment

wzrdtales Sep 10, 2023

Choose a reason for hiding this comment

wzrdtales commented Sep 10, 2023

mgabeler-lee-6rs commented Sep 11, 2023

wzrdtales commented Sep 11, 2023 • edited

wzrdtales commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

mgabeler-lee-6rs commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

wzrdtales commented Sep 11, 2023

GriffinSchneider commented Dec 25, 2019 •

edited

mgabeler-lee-6rs commented Jan 9, 2020 •

edited

nhkhanh Sep 5, 2022 •

edited

wzrdtales commented Sep 11, 2023 •

edited