moigagoo / norm

A Nim ORM for SQLite and Postgres
https://norm.nim.town
MIT License
378 stars 34 forks source link

Escape column name #168

Open PhilippMDoerner opened 1 year ago

PhilippMDoerner commented 1 year ago

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:

1) 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) 2) 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.

PhilippMDoerner commented 1 year ago

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?