norm icon indicating copy to clipboard operation
norm copied to clipboard

Escape column name

Open cmd410 opened this issue 1 year ago • 8 comments

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

cmd410 avatar Jul 14 '22 17:07 cmd410

@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 & """.

moigagoo avatar Jul 14 '22 19:07 moigagoo

@moigagoo yep, makes sense, fixed this

cmd410 avatar Jul 14 '22 19:07 cmd410

Pls update the tests.

moigagoo avatar Jul 15 '22 03:07 moigagoo

@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 avatar Jul 18 '22 20:07 moigagoo

@cmd410 some tests are failing.

moigagoo avatar Jul 19 '22 10:07 moigagoo

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

cmd410 avatar Jul 19 '22 15:07 cmd410

@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 avatar Jul 20 '22 10:07 moigagoo

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 avatar Aug 22 '22 16:08 PhilippMDoerner

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.

PhilippMDoerner avatar Jan 25 '23 20:01 PhilippMDoerner