feat: add `drop constraint if exists` to compiler by miketheman · Pull Request #8161 · sqlalchemy/sqlalchemy

miketheman · 2022-06-23T12:13:19Z

Description

Add DROP CONSTRAINT ... IF EXISTS behavior to the compiler.

Fixes #8141

Checklist

This pull request is:

A documentation / typographical error fix
- Good to go, no issue or tests are needed
A short code fix
- please include the issue number, and create an issue if none exists, which
  must include a complete example of the issue. one line code fixes without an
  issue and demonstration will not be accepted.
- Please include: Fixes: #<issue number> in the commit message
- please include tests. one line code fixes without tests will not be accepted.
A new feature implementation
- please include the issue number, and create an issue if none exists, which must
  include a complete example of how the feature would look.
- Please include: Fixes: #<issue number> in the commit message
- please include tests.

Have a nice day!

zzzeek · 2022-06-23T12:43:00Z

this looks like the general idea, I wonder though since "IF EXISTS" / " IF NOT EXISTS" is generic enough that it should be in the base compiler (makes the PR even easier then). It looks like the base compiler is already doing "CASCADE" which certainly isn't covered by most backends either.

zzzeek · 2022-06-23T12:45:10Z

youd add a few tests to test/sql/test_constraints in that case, compile tests similar to test_render_drop_constraint_cascade

miketheman · 2022-06-23T12:47:26Z

Thanks! I thought about that first, but then backed out, since it seemed dialect-specific and didn't want to break any compiler rules. Will re-work the change.

zzzeek · 2022-06-23T12:49:29Z

this one is a gray area. we like to do only "SQL Standard " things in Core but I dont think create/drop of constraints is part of SQL standard anyway.

the main thing is i think if someone does "if_exists" and runs it on a backend that doesnt support it, it should fail, otherwise why did they add "if_exists". so by just running that SQL, if the backend doesnt support it, we get that error for free.

been making this kind of choice for about 17 years now...

miketheman · 2022-06-25T00:30:40Z

Thanks for the pointers - I've moved the code around to the base compiler, and rewrote the test to be similar to other tests in the module.

I did some more reading of the Postgres docs, and it seems that the only constraint operation that supports the EXISTS behavior is drop constraint, so no need to implement the ADD side.

Let me know what you think!

Add `DROP CONSTRAINT ... IF EXISTS` behavior to the compiler. Fixes sqlalchemy#8141 Signed-off-by: Mike Fiedler <miketheman@gmail.com>

CaselIT · 2022-07-02T21:56:35Z

+            drop.if_exists and "IF EXISTS " or "",
            formatted_name,
            drop.cascade and " CASCADE" or "",


these could be replaced with inline if-else (these probably date back to a python version that did not yet support it )

Thanks! I tried to remain consistent with the existing patterns rather than diverge for my changes.
Origin of this pattern cfa1a42
If it’s desirable to convert to newer patterns, happy to do so!

not really required, but I think it's safe to convert to the new syntax since we are modifying this part anyway.

If you make the change, could you also add a changelog for this pr?

Changes updated.
Changelog added as well - wasn't really certain if the location (2.0) and tags we correct - I was basing it off the opened ticket labels.

look great, thanks!

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 120171a of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2022-07-02T21:57:20Z

New Gerrit review created for change 120171a: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3970

Use single-line ternary if/else introduced in Python 2.5. Signed-off-by: Mike Fiedler <miketheman@gmail.com>

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 43276e2 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2022-07-03T21:56:13Z

Patchset 43276e2 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3970

miketheman · 2022-07-06T14:45:44Z

@zzzeek I'm not super familiar with the code review process for sqlalchemy, and having read the docs, I'm not sure if there's another action I'm meant to take?

CaselIT · 2022-07-06T19:12:21Z

Currently just wait for a review :)

sqla-tester · 2022-07-28T18:19:45Z

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3970 has been merged. Congratulations! :)

miketheman mentioned this pull request Jun 23, 2022

Support DROP CONSTRAINT IF EXISTS for Postgres Dialect #8141

Closed

miketheman force-pushed the miketheman/support-drop-constraint-8141 branch from 48ebf75 to 8c25dad Compare June 23, 2022 12:15

miketheman force-pushed the miketheman/support-drop-constraint-8141 branch 2 times, most recently from 3ee6a1f to ca90184 Compare June 25, 2022 00:19

miketheman marked this pull request as ready for review June 25, 2022 00:30

miketheman force-pushed the miketheman/support-drop-constraint-8141 branch from ca90184 to 659dc57 Compare June 25, 2022 00:34

miketheman changed the title ~~feat: add drop constraint if exists to postgres~~ feat: add drop constraint if exists to compiler Jun 25, 2022

feat: add drop constraint if exists

120171a

Add `DROP CONSTRAINT ... IF EXISTS` behavior to the compiler. Fixes sqlalchemy#8141 Signed-off-by: Mike Fiedler <miketheman@gmail.com>

miketheman force-pushed the miketheman/support-drop-constraint-8141 branch from 659dc57 to 120171a Compare June 25, 2022 00:35

CaselIT reviewed Jul 2, 2022

View reviewed changes

CaselIT requested a review from sqla-tester July 2, 2022 21:57

sqla-tester reviewed Jul 2, 2022

View reviewed changes

miketheman added 2 commits July 3, 2022 13:37

refactor: update conditional syntax

0aebcba

Use single-line ternary if/else introduced in Python 2.5. Signed-off-by: Mike Fiedler <miketheman@gmail.com>

docs: add changelog entry

43276e2

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

CaselIT requested a review from sqla-tester July 3, 2022 21:56

sqla-tester reviewed Jul 3, 2022

View reviewed changes

sqlalchemy-bot closed this in 41656f8 Jul 28, 2022

miketheman deleted the miketheman/support-drop-constraint-8141 branch July 28, 2022 18:28

Uh oh!

Conversation

miketheman commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

zzzeek commented Jun 23, 2022

Uh oh!

zzzeek commented Jun 23, 2022

Uh oh!

miketheman commented Jun 23, 2022

Uh oh!

zzzeek commented Jun 23, 2022

Uh oh!

miketheman commented Jun 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CaselIT Jul 2, 2022

Choose a reason for hiding this comment

Uh oh!

miketheman Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

CaselIT Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

miketheman Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

CaselIT Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Jul 2, 2022

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Jul 3, 2022

Uh oh!

miketheman commented Jul 6, 2022

Uh oh!

CaselIT commented Jul 6, 2022

Uh oh!

sqla-tester commented Jul 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miketheman commented Jun 23, 2022 •

edited

Loading

miketheman commented Jun 25, 2022 •

edited

Loading