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: $ in front of column name causes issue with definition tab #17087

Closed
wants to merge 5 commits into from
Closed

fix: $ in front of column name causes issue with definition tab #17087

wants to merge 5 commits into from

Conversation

Bhavyawahie
Copy link
Contributor

@Bhavyawahie Bhavyawahie commented Aug 31, 2023 •
edited
Loading

What kind of change does this PR introduce?

Fixes #17013. The given PR brings about client-side validation on column's name field for a given table's definition made using the Table Editor.

What is the current behavior?

The current behavior allows for naming columns of a given table without any validation for special characters (case in point being column names beginning with'$') which cause a syntax error with the supabase-meta container running the postgres-meta service.

Replicating the error

ezgif com-video-to-gif (11)

Error from Postgres-meta container's logs

"request":{"method":"POST","url":"/query","pg":"db","opt":""}}
{"level":"error","time":"2023-08-31T11:31:36.849Z","pid":19,"hostname":"a26f486d93cb","reqId":"req-2st","error":{"message":"syntax error at or near \"$\""}," \"$\""}

What is the new behavior?

The changes in validation allow for users to not create a table until the said column name within that table does not begin with a '$'. Additionally, any previously created column names are also validated on any subsequent updation to them.

Table Creation

ezgif com-video-to-gif (13)

Table Updation

ezgif com-video-to-gif (12)

Accessing Table Definition

ezgif com-video-to-gif (14)

This also solves the problem of Application error that was earlier caused by column names beginning with '$' on accessing the Table Definition tab inside the Table Editor.

@Bhavyawahie Bhavyawahie requested a review from a team as a code owner August 31, 2023 21:57
@vercel
Copy link

vercel bot commented Aug 31, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 1, 2023 8:38am
studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 1, 2023 8:38am
studio-self-hosted ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 1, 2023 8:38am
studio-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 1, 2023 8:38am
zone-www-dot-com ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 1, 2023 8:38am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
ui-storybook ⬜️ Ignored (Inspect) Visit Preview Sep 1, 2023 8:38am

@vercel
Copy link

vercel bot commented Aug 31, 2023

@Bhavyawahie is attempting to deploy a commit to the Supabase Team on Vercel.

A member of the Team first needs to authorize it.

@Bhavyawahie
Copy link
Contributor Author

@alaister have a look!

alaister
alaister requested changes Sep 1, 2023
View reviewed changes
Copy link
Member

@alaister alaister left a comment

Choose a reason for hiding this comment

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

Hey @Bhavyawahie,

Thanks for the PR!

While not recommended, it is supported in postgres to name columns beginning with $. For example, we also allow columns with spaces in the name. Additionally, this doesn't seem to fix the definitions tab error for people who already have columns named with $.

With this in mind, could you remove this new validation and fix the bug in the definitions tab? I'm happy to help out with some pointers if it isn't clear where the error might be (I have a feeling it might be in our SQL formatted library 😕)

Thanks!

@Bhavyawahie
Copy link
Contributor Author

Bhavyawahie commented Sep 1, 2023 •
edited
Loading

Thanks for the feedback @alaister. Happy to work as instructed although I feel this validation check is also necessary at this point until the said changes in the definitions tab are proposed/made.

The SQL editor also disallows to name/rename column names with column_name beginning with $ sign or any special character for that matter (/^[a-zA-Z_][a-zA-Z0-9_]*$/) thus may cause irregularities in the UI as far as SQL column nomenclature rules across the platform are concerned.

image

@alaister
Copy link
Member

alaister commented Sep 4, 2023

Postgres (SQL) is doing the validation, which is causing that error to happen in the SQL editor. If you wrap the column name containing the $ or spaces with double quotes (") it will work. So do you think you can make the changes to the definitions tab here in this PR? 🙏🏻

@Bhavyawahie
Copy link
Contributor Author

Sure, given if you could point me to where the error might be 😅🙏

@rmrt1n
Copy link
Contributor

rmrt1n commented Sep 20, 2023

This is sql-formatter's parsing error caused by pg_get_tabledef returning unquoted $column_name. Updating pg_get_tabledef to quote column/table names starting with $ should fix the issue (like below):
image

However, column names with spaces in them, as shown above, are still broken. Handling quoted identifiers is tricky. Currently, pg_get_tabledef quotes columns that are: 1. contains a capital letter, and 2. is a reserved keyword. A quick workaround is to quote every column in the table.

A better(?) approach might look like this:

  1. quote reserved keywords
  2. quote all columns that doesn't follow postgres's valid identifer syntax: /^[a-z_][a-zA-Z0-9_$]$/. This should include columns that are camelCase, $dollar_sign, and "contains spaces".

This should probably fix most parse issues related to table/column names. Unfortunately this doesn't take into account identifiers that contain non-latin characters. Any thoughts @alaister ?

@alaister
Copy link
Member

Hi,

Unfortunately, this has got a little stale, so I'm going to go ahead and close it.

I think this issue should be solved on the https://github.com/MichaelDBA/pg_get_tabledef side, so maybe you can consider opening up an issue there? Thanks 🙏🏻

@alaister alaister closed this Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kiwicopple kiwicopple kiwicopple left review comments

@alaister alaister alaister requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

$ in front of column name causes issue with definition tab
4 participants
@Bhavyawahie @alaister @rmrt1n @kiwicopple