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

Escape column name #159

Closed
wants to merge 5 commits into from
Closed

Escape column name #159

wants to merge 5 commits into from

Conversation

cmd410
Copy link

@cmd410 cmd410 commented Jul 14, 2022

This is a fix for fields that match SQL keywords to cause syntax error in the query.

For example:

import std / options

import norm / [model, sqlite]

type
  User* = ref object of Model
    email*: string
    group*: int

func newUser*(email = ""): User =
  User(email: email, group: 0)

let dbConn* = open("test.db", "", "", "")
dbConn.createTables(newUser())

Will cause Error: unhandled exception: near "group": syntax error [DbError] as group matches an SQL keyword.
However it is possible to create such fields delimiting them with quotes:

CREATE TABLE "User"(
  "email" TEXT NOT NULL,
  "group" INTEGER NOT NULL,
  "id" INTEGER NOT NULL PRIMARY KEY
)

So this fix uses strutils.escape proc to achieve that, though not sure if its a proper way to do this...

@moigagoo
Copy link
Owner

@cmd410 escaping is a different operation than quoting. It replaces chars with other chars, we don't want that.

Instead, you could just use """ & fld & """.

@cmd410
Copy link
Author

cmd410 commented Jul 14, 2022

@moigagoo yep, makes sense, fixed this

@moigagoo
Copy link
Owner

Pls update the tests.

@moigagoo
Copy link
Owner

@cmd410 thanks for the update! Now, the only thing left that I'll have to ask you to do is, please add a changelog entry for your change.

And thank you again for you contribution! 🙏

@moigagoo
Copy link
Owner

@cmd410 some tests are failing.

@cmd410
Copy link
Author

cmd410 commented Jul 19, 2022

truthfully, I got no clue why they fail... error messages are confusing, saying some field does not exist. The other is like Unhandled exception: ERROR: database "postgres" is being accessed by other users which i don't see related to field quoting. They are all related to postgres, and I don't have one installed(it just won't install on my system for some reason) so can't even really test it, unfortunately

@moigagoo
Copy link
Owner

@cmd410 most errors have to do with incorrect column names:

LINE 1: SELECT species, favToy, id FROM "Pet"
                        ^
HINT:  Perhaps you meant to reference the column "Pet.favToy".

Unhandled exception: ERROR:  column "userid" referenced in foreign key constraint does not exist

These should be easy to fix. It's either an error in the tests or in query generation for e.g. foreign key queries.

The database "postgres" is being accessed by other users error is probably caused by incorrectly closed DB connections. You can ignore them, they're likely to go away when you fix the column names.

@moigagoo moigagoo linked an issue Aug 15, 2022 that may be closed by this pull request
[Bugfix] Enable creation of tables whose fieldName otherwise clashes with SQL keywords such as group #165
Open
@PhilippMDoerner
Copy link
Collaborator

PhilippMDoerner commented Aug 22, 2022 •
edited
Loading

I've taken a look at the errors here as well and the issue this opens up seems non trivial.
For example:
tests/postgres/tdbtypes.nim will only function if you replace line 37
old: dbConn.select("""lastLogin <= $1""", ?now())
new: dbConn.select(""""lastLogin" <= $1""", ?now())

Notice the extra quotation marks around the fields.
For some reason now they're required around the SQL the user passes in as well? Is this behaviour that we want?

Edit: I noticed that, in fact, only the model fields must be in quotation marks. See postgres/trows line 225 once I've pushed my update commits

PhilippMDoerner added a commit to PhilippMDoerner/norm that referenced this pull request Aug 22, 2022
@PhilippMDoerner
moigagoo#159 fixed tdbtypes test for postgres
44af681 
The field there now requires quotation marks around itself.
This is likely because in the SELECT bit of the query the fields are
also provided with quotation marks.
PhilippMDoerner added a commit to PhilippMDoerner/norm that referenced this pull request Aug 22, 2022
@PhilippMDoerner
moigagoo#159 fixed case-sensitive test error
5638632 
On linux, databases tend to be case-sensitive with their tests.
This test failed in part because the column is `userId` not `userid`
PhilippMDoerner added a commit to PhilippMDoerner/norm that referenced this pull request Aug 22, 2022
@PhilippMDoerner
moigagoo#159 fixed createTables not escaping field names properly
31e113d 
In the 3 ways a Foreign Key Constraint may be generated
the field does not get escaped.
This causes issues in the tests.
PhilippMDoerner added a commit to PhilippMDoerner/norm that referenced this pull request Aug 22, 2022
@PhilippMDoerner
moigagoo#159 escaped fieldnames in tests
146c0ab 
This appears to now be mandatorily necessary
PhilippMDoerner added a commit to PhilippMDoerner/norm that referenced this pull request Aug 22, 2022
@PhilippMDoerner
moigagoo#159 Removed quotes for foreign key constraints based on mode…
63a14f5 
…l fields

For some reason the tests of tcount fail, because the model in there
has another model in it, which generates an FK field that then looks like this:
""favToy""
Which causes SQL errors.
@PhilippMDoerner PhilippMDoerner mentioned this pull request Aug 22, 2022
Open
PhilippMDoerner added a commit to PhilippMDoerner/norm that referenced this pull request Aug 23, 2022
@PhilippMDoerner
moigagoo#159 Removed some unnecessary changes I made for debugging
5ebf1d3
PhilippMDoerner added a commit to PhilippMDoerner/norm that referenced this pull request Aug 23, 2022
@PhilippMDoerner
moigagoo#159 Moved procs into utils module
b39f970 
Renamed "isContainer" to "isRefObject"
Added documentation to utility procs
PhilippMDoerner added a commit to PhilippMDoerner/norm that referenced this pull request Sep 3, 2022
@PhilippMDoerner
moigagoo#159 ADD info to doc commments of rawSelect
964106a 
Due to the tables in postgres being created in a case-sensitive manner
they require quotation marks around them and must be case-sensitive correct.
This is only the case with postgres, not sqlite.
https://stackoverflow.com/questions/43111996/why-postgresql-does-not-like-uppercase-table-names
PhilippMDoerner added a commit to PhilippMDoerner/norm that referenced this pull request Sep 3, 2022
@PhilippMDoerner
moigagoo#159 Removed logging from tests
00f2687
@PhilippMDoerner
Copy link
Collaborator

Given that the issues of this PR were solved in #168 I'd argue this PR belongs closed.
Further, #168 comes with its completely own set of problems that this PR would also run into, so that makes it more sensible to contain any commentary in that PR rather than this one.

@moigagoo moigagoo closed this Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bugfix] Enable creation of tables whose fieldName otherwise clashes with SQL keywords such as group
3 participants
@cmd410 @moigagoo @PhilippMDoerner