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.

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

Enabled capitzalized table names for sequence reset #19825

Merged
merged 6 commits into from
Sep 27, 2023

Conversation

jaads
Copy link
Member

@jaads jaads commented Sep 27, 2023

Closes #19817

When looking up the auto increment sequence name for a table which is capitalized, PostgreSQL throwed an error saying relation xy does not exist. The table name needed explicit quotes, otherwise PostgreSQL used a lowercase variant of the table name automatically and couldn't resolve some (internal) relationship (I guess).

So the table name for the pg_get_serial_sequence function call, which need to be passed as string, needed to be escaped.

In addition of just escaping the value to the pg_get_serial_sequence, now all the values get passed as parameters to the database (using Knex's ?) and all identifiers now get escaped via Knex using ??.

This is an example of how the statement now looks:

WITH sequence_infos AS (SELECT pg_get_serial_sequence($1, $2) AS seq_name, MAX("id") as max_val FROM "articles") SELECT SETVAL(seq_name, max_val) FROM sequence_infos; ["articles", id]

I also tested with a capitalized column name, but this worked fine without the explicit double quotes like it was needed for the table name.

@changeset-bot
Copy link

changeset-bot bot commented Sep 27, 2023

🦋 Changeset detected

Latest commit: 0c5b7a4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@directus/api Minor
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@paescuj paescuj changed the title enabled capitzalized table names for sequence reset Enabled capitzalized table names for sequence reset Sep 27, 2023
@jaads
Copy link
Member Author

jaads commented Sep 27, 2023

shout out to @paescuj for the support 🎉

@jaads jaads requested review from a team, paescuj, rijkvanzanten and azrikahar and removed request for a team September 27, 2023 10:38
@jaads jaads marked this pull request as ready for review September 27, 2023 10:39
Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

LGTM! ❤️

@jaads jaads requested a review from licitdev September 27, 2023 12:58
Copy link
Contributor

@azrikahar azrikahar left a comment

Choose a reason for hiding this comment

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

LGTM! Out of curiosity, tried to looked up more info from the official docs regarding this and they do seem to support the actual fix and reasoning behind the quotes around table name, but not needed for column name.

pg_get_serial_sequence ( table text, column text ) → text
Because the first parameter potentially contains both schema and table names, it is parsed per usual SQL rules, meaning it is lower-cased by default. The second parameter, being just a column name, is treated literally and so has its case preserved.
Link to above reference

Quoting an identifier also makes it case-sensitive, whereas unquoted names are always folded to lower case. For example, the identifiers FOO, foo, and "foo" are considered the same by PostgreSQL, but "Foo" and "FOO" are different from these three and each other. (The folding of unquoted names to lower case in PostgreSQL is incompatible with the SQL standard, which says that unquoted names should be folded to upper case. Thus, foo should be equivalent to "FOO" not "foo" according to the standard. If you want to write portable applications you are advised to always quote a particular name or never quote it.)
Link to above reference

.changeset/silver-cars-hear.md Outdated Show resolved Hide resolved
@jaads jaads removed the request for review from licitdev September 27, 2023 12:59
@azrikahar azrikahar enabled auto-merge (squash) September 27, 2023 12:59
@azrikahar azrikahar merged commit aeed9c0 into main Sep 27, 2023
7 checks passed
@azrikahar azrikahar deleted the reset-seqeuence-uppercase-identifiers-fix branch September 27, 2023 13:12
@github-actions github-actions bot added this to the Next Release milestone Sep 27, 2023
br-rafaelbarros pushed a commit to personal-forks/directus-source that referenced this pull request Nov 7, 2023
* escaped identifiers

* fix

* used parameters for values and knex for escaping identifiers

* changeset

* Update api/src/database/helpers/sequence/dialects/postgres.ts

* Update .changeset/silver-cars-hear.md

---------

Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
Co-authored-by: Azri Kahar <42867097+azrikahar@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: postgresql reset auto-increment sequence
3 participants