perseas / Pyrseas

Provides utilities for Postgres database schema versioning.
https://perseas.github.io/
BSD 3-Clause "New" or "Revised" License
395 stars 67 forks source link

Table named with PG reserved word and constraint causes error in dbtoyaml #177

Open jmafc opened 6 years ago

jmafc commented 6 years ago

Test case:

CREATE TABLE "order" (
    c1 integer UNIQUE,
    c2 text);

Running dbtoyaml causes error in https://github.com/perseas/Pyrseas/blob/fdbb76d05c87dcd92941f3b48309f7dfe4ccf45f/pyrseas/dbobject/table.py#L927 as follows: KeyError: ('public', '"order"').

The same problem occurs if UNIQUE is replaced by PRIMARY KEY.

rmg commented 4 years ago

https://github.com/perseas/Pyrseas/blob/171d703ce52bd61055058ed48dca3c62654b5f17/pyrseas/dbobject/__init__.py#L37-L39

The identifier quoting is currently only done for what Pyrseas thinks are reserved words. Unfortunately, that criteria misses some other reserved keywords:

SELECT word FROM pg_get_keywords() WHERE catdesc like 'reserved%' and catcode != 'R';
      word
----------------
 authorization
 binary
 collation
 concurrently
 cross
 current_schema
 freeze
 full
 ilike
 inner
 is
 isnull
 join
 left
 like
 natural
 notnull
 outer
 overlaps
 right
 similar
 tablesample
 verbose
(23 rows)

In my experience, query builders (as found in things like django, Ruby on Rails, etc.) generally unconditionally quote all identifiers.

https://github.com/perseas/Pyrseas/blob/171d703ce52bd61055058ed48dca3c62654b5f17/pyrseas/dbobject/__init__.py#L58

I can certainly see how it could be a breaking change to switch to it, but that aside it seems like a much safer approach. Is there a reason this is only done conditionally in Pyrseas that I'm missing?

Even if the quoting is left as conditional, the reserved words list is definitely incomplete.

jmafc commented 4 years ago

It's been a while, but I just tested the example that I created against both PG 9.6.16 and 11.6-1 database, with both the Pyrseas stable release and master, and using both Python 2.7.16 and 3.7.3, and dbtoyaml does not fail. Do you have an example of where it's happening to you?

dbtoyaml is not a query builder: its output is YAML. The error that was happening before was purely internal. IIRC, that list of RESERVED_WORDS is only used by yamltodb.

rmg commented 4 years ago

Sorry, I misread the issue title. I am indeed talking about yamltodb.

jmafc commented 4 years ago

OK, so if you do have a problem with yamltodb, please open another issue and provide an example.

rmg commented 4 years ago

Sure thing; I was just trying to avoid opening a duplicate :-)