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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Sep 10, 2023
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@nhkhanh nhkhanh Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 """"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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