sqlalchemy / sqlalchemy

The Database Toolkit for Python
https://www.sqlalchemy.org
MIT License
9.36k stars 1.4k forks source link

ON DELETE SET NULL (column) not supported at the moment? #11595

Open joidegn opened 1 month ago

joidegn commented 1 month ago

Describe the use case

Im trying to issue a ON DELETE SET NULL (column) as described in the Postgres docs since version 15.

Im getting this error from SQLAlchemy which makes me think we don't support this yet. I also can't find this mentioned in the docs.

I am using sqlalchemy 2.0.31.

sqlalchemy.exc.CompileError: Unexpected SQL phrase: 'SET NULL (applied_exchange_rate_id, base_currency_amount)' (matching against '^(?:RESTRICT|CASCADE|SET NULL|NO ACTION|SET DEFAULT)$')

Databases / Backends / Drivers targeted

postgresql and driver psycopg2

Example Use

    applied_exchange_rate_id = Column(pg_UUID, ForeignKey("exchange_rates.id", ondelete="SET NULL (applied_exchange_rate_id, base_currency_amount)"), nullable=True)
    applied_custom_exchange_rate_id = Column(pg_UUID, ForeignKey("exchange_rates.id", ondelete="SET NULL (applied_custom_exchange_rate_id, base_currency_amount)"), nullable=True)

Additional context

No response

CaselIT commented 1 month ago

Hi,

No that syntax is not supported at the moment, but PR would be accepted for it.

@zzzeek I guess we should attach the FK_ON_DELETE etc to the compiler so postgresql can override it?

joidegn commented 1 month ago

Makes sense! I have not committed code to sqlalchemy yet but I might try my hand if I find the time 🙏

CaselIT commented 1 month ago

It's likely that your code will probably work if you just comment this check https://github.com/sqlalchemy/sqlalchemy/blob/772ab0befefe0dad99db22e21ed5f37afd1a1928/lib/sqlalchemy/sql/compiler.py#L7443-L7459

Reflection would probably also need to be adapted but it could be done in a second moment

zzzeek commented 1 month ago

why do we have that filter? we dont use filters like this for anything else. PGs new syntax here is exactly the reason we dont do this kind of thing.

zzzeek commented 1 month ago

oh it's in the spirit of SQL injection prevention. which is silly as this is DDL but let's leave it

CaselIT commented 1 month ago

oh it's in the spirit of SQL injection prevention. which is silly as this is DDL but let's leave it

I think so. The blame mentions CVEs.