Escape column name #168

PhilippMDoerner · 2022-08-22T17:48:27Z

This is a copy of #159 but with fixes applied by me to get the tests running.
@cmd410 @moigagoo

The main reason I didn't commit them onto the branch of @cmd410 is because I actually don't know how ^^'

For the general description, refer to #159 and a related userstory #165

GENERAL REMARK:
I noticed some worrying behaviour which I think might cause this PR to be one that break backwards compatibility.
A fair amount of the tests in postgres did not run because they either:

Started to compare column names in a case-sensitive manner which they did not before (The tests where this was the case have that remarked in their commit message)
Started to **require that specific columns (I think only FK columns?) be surrounded by quotation marks

I did not think any deeper into that or investigate why this was suddenly the case or if it doesn't matter for the library user, it was mostly an observation I made while fixing the testing errors.
To me this looks like new behaviour and a breaking change.
It might of course also just be that I ran the tests under linux and this might be fine under windows, but it is something to take a look at either way.

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.

On linux, databases tend to be case-sensitive with their tests. This test failed in part because the column is `userId` not `userid`

In the 3 ways a Foreign Key Constraint may be generated the field does not get escaped. This causes issues in the tests.

This appears to now be mandatorily necessary

…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.

…tation marks. Why that is I have no idea.

PhilippMDoerner · 2022-08-23T05:16:14Z

tests/postgres/ttables.nim

@@ -56,7 +56,7 @@ suite "Table creation":

    check dbConn.getAllRows(qry, "FurnitureTable") == @[
      @[?"id", ?"bigint"],
-      @[?"legcount", ?dftDbInt]
+      @[?"legCount", ?dftDbInt]


An example for sudden case-sensitivity, though that is when comparing column names and not during actual queries, not sure how much that matters.

PhilippMDoerner · 2022-08-23T05:16:21Z

tests/postgres/ttables.nim

@@ -80,7 +80,7 @@ suite "Table creation":
    ]

    check dbConn.getAllRows(qry, "Pet") == @[
-      @[?"favtoy", ?"bigint"],
+      @[?"favToy", ?"bigint"],


An example for sudden case-sensitivity, though that is when comparing column names and not during actual queries, not sure how much that matters.

PhilippMDoerner · 2022-08-23T05:16:53Z

tests/postgres/trows.nim

@@ -219,7 +220,7 @@ suite "Row CRUD":

    let
      personRow = get dbConn.getRow(sql"""SELECT name, pet, id FROM "Person" WHERE id = $1""", person.id)
-      petRow = get dbConn.getRow(sql"""SELECT species, favToy, id FROM "Pet" WHERE id = $1""", pet.id)
+      petRow = get dbConn.getRow(sql"""SELECT species, "favToy", id FROM "Pet" WHERE id = $1""", pet.id)


An example for suddenly required quotation marks, necessary only on the FK field "favToy", not on the others.

Same for Line 71

What if you put the rest in quotations for consistency? Does it break the test?

This is so weird 🤔 Possibly, this has to do with mixing cases. If the column name uses mixed case, it must be quoted. But that's just a wild guess.

Changing the line to this: sql"""SELECT "species", "favToy", "id" FROM "Pet" WHERE id = $1"""
Still works perfectly fine.

An observation I made though, is that this:
sql"""SELECT "sPecies", "favToy", "id" FROM "Pet" WHERE id = $1"""
Will cause the test to fail while this:
sql"""SELECT sPecies, "favToy", "id" FROM "Pet" WHERE id = $1"""
works perfectly fine.

I'm getting the impression that not only are suddenly the quotation marks sometimes mandatory, they cause the column names to suddenly be case-sensitive.

PhilippMDoerner · 2022-08-23T05:17:32Z

tests/postgres/tfkpragma.nim

@@ -70,6 +72,6 @@ suite "``fk`` pragma":
    for inpCustomer in inpCustomers.mitems:
      dbConn.insert inpCustomer

-    dbConn.select(outCustomers, """"userid" = $1""", userA.id)
+    dbConn.select(outCustomers, """"userId" = $1""", userA.id)


An example for sudden case-sensitivity, though that is when comparing column names and not during actual queries, not sure how much that matters.

PhilippMDoerner · 2022-08-23T05:18:26Z

tests/postgres/tdbtypes.nim

@@ -33,7 +32,7 @@ suite "Import dbTypes from norm/private/postgres/dbtypes":

  test "dbValue[DateTime] is imported":
    let users = @[newUser()].dup:
-      dbConn.select("""lastLogin <= $1""", ?now())
+      dbConn.select(""""lastLogin" <= $1""", ?now())


An example for suddenly required quotation marks, this time not on an FK field as lastLogin is of type DateTime. Not sure what that is about.

PhilippMDoerner · 2022-08-23T05:21:17Z

src/norm/postgres.nim

      else:
-        fkGroups.add "FOREIGN KEY ($#) REFERENCES $#(id)" % [fld, (obj.dot(fld).getCustomPragmaVal(fk)).table]
+        fkGroups.add """FOREIGN KEY ("$#") REFERENCES $#(id)""" % [fld, (obj.dot(fld).getCustomPragmaVal(fk)).table]


I had to add quotation marks here because otherwise the FK constraints wouldn't have any.
Please note that I ONLY added the quotation marks in these two lines.
There is a third line dealing with FK constraints, line 107 further up, where I DID NOT add quotation marks around the field, because when that line gets called by tests the field suddenly already has quotationmarks (possibly because it calls the col proc where the others don't).

when that line gets called by tests the field suddenly already has quotationmarks (possibly because it calls the col proc where the others don't).

Oof, that's dangerous behavior. col proc should be called always.

Just tried it, replacing fld on there with obj.col(fld) does work.
I'm not sure if the code that generates is correct, as stated I haven't read through this code in any intensity in order to understand what's going on, but it does pass the tests.

…eneration I do not know whether obj.col(fld) actually is correct code, but it *does* run through all tests.

PhilippMDoerner · 2022-09-10T17:28:42Z

I'm perfectly fine with leaving this PR in the air as it is, as I'm not sure myself what to do with this.
On the one hand it fixes an issue about you being unable to have certain table names that overlap with SQL keywords, on the other it introduces breaking changes and unusual behaviour for postgres users which need to put quotation marks around fields all of a sudden (and that even only sometimes).

Is this grounds for not accepting the PR?
Should the PR be accepted and we add warnings to the docs for postgres users specifically?

Crystal Melting Dot and others added 13 commits July 14, 2022 19:54

escape column name

463368b

not escape, quote

dd67ee1

test for quoted fields in tmodel

363d225

Add changelog entry and bump version

9c677bc

ok, no version bump, then

12cf790

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.

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`

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.

moigagoo#159 escaped fieldnames in tests

146c0ab

This appears to now be mandatorily necessary

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.

Apparently the FK field and ONLY the FK field "favToy" must be in quo…

efe9499

…tation marks. Why that is I have no idea.

The field comparison is case sensitive for some reason?

bec9c0f

Merge branch 'develop' into escape_column_name

502e34e

PhilippMDoerner commented Aug 23, 2022

View reviewed changes

PhilippMDoerner added 2 commits August 23, 2022 07:22

moigagoo#159 Removed some unnecessary changes I made for debugging

5ebf1d3

Removed dangerous inconsistent behaviour for Foreign Key constraint g…

fc6ade8

…eneration I do not know whether obj.col(fld) actually is correct code, but it *does* run through all tests.

PhilippMDoerner mentioned this pull request Jan 25, 2023

Escape column name #159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Escape column name #168

Escape column name #168

PhilippMDoerner commented Aug 22, 2022 •

edited

Loading

PhilippMDoerner Aug 23, 2022

PhilippMDoerner Aug 23, 2022

PhilippMDoerner Aug 23, 2022 •

edited

Loading

moigagoo Aug 23, 2022

PhilippMDoerner Aug 23, 2022

PhilippMDoerner Aug 23, 2022

PhilippMDoerner Aug 23, 2022

PhilippMDoerner Aug 23, 2022

moigagoo Aug 23, 2022

PhilippMDoerner Aug 23, 2022 •

edited

Loading

PhilippMDoerner commented Sep 10, 2022

Escape column name #168

Are you sure you want to change the base?

Escape column name #168

Conversation

PhilippMDoerner commented Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippMDoerner Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippMDoerner Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

PhilippMDoerner commented Sep 10, 2022

PhilippMDoerner commented Aug 22, 2022 •

edited

Loading

PhilippMDoerner Aug 23, 2022 •

edited

Loading

PhilippMDoerner Aug 23, 2022 •

edited

Loading