Add opt-in autogenerate support for CHECK constraint detection#1811
Add opt-in autogenerate support for CHECK constraint detection#1811bjorkbjork wants to merge 2 commits intosqlalchemy:mainfrom
Conversation
Add a new comparator plugin that detects added and removed named CHECK
constraints during autogenerate. Comparison is name-only by default;
the dialect hook compare_check_constraint exists for future per-dialect
expression comparison.
The plugin is registered under alembic.ext.checkconstraint and must be
explicitly opted into via autogenerate_plugins:
context.configure(
autogenerate_plugins=[
"alembic.autogenerate.*",
"alembic.ext.checkconstraint",
]
)
Unnamed constraints, type-bound constraints (Boolean/Enum), and dialects
that do not support check constraint reflection are handled gracefully.
Fixes: sqlalchemy#508
|
I've given a quick look, great work so far. I'll do a better review soon. @zzzeek what do you think about this feature? I know you weren't 100% sold on it the last time, but using the new plugin system I think it could be fine to have. |
|
the important part is my comment at #1761 (comment) which I've now associated with that PR since I needed to read the context for why it went that way. If I can make my comment there more concrete: name-based comparison alone is insufficient for an in-Alembic feature identified as "alembic.ext.checkconstraint" since it doesn't detect changes in existing check constraints, only adds and deletes (it looks like I used that name in my comment as "what it should be named", let me amend that because the name is too generic for what this does). Basically I saw the feature as only a half-measure which is why I proposed the whole plugin system, so that we at least can have useful half-measures included in autogen without them being the official only way to do that thing. my comment also makes my job easier here since I wrote "I will gladly pull this in and organize it to use a new entrypoint feature as I do it in such a way that supports local "ext" files also.". I will be frank I was staking out a bit of a compromise position on that, however, here we are and I see this even proposes to close #508 with just add/remove detection. I think at this point it's likely that becomes settled reality that "check constraint autogen are done" since add/removes are probably 90% of the use case. I just want to leave that "alembic compares CHECK constraints" open for some day that we have heuristics to compare SQL expressions. TL DR let's make the name a little less direct for the moment, "alembic.ext.cks_by_name". This is almost a way just to have it as a beta feature for awhile. Later when we do like alembic 1.19 or 1.20 and we've decided #508 is done forever we can just add it under "alembic.autogenerate.constraints" to have it turn on by default (can of course still be turned off in the list if someone has some custom CK compare plugin). |
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 5f02bbf of this pull request into gerrit so we can run tests and reviews and stuff
|
New Gerrit review created for change 5f02bbf: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6672 |
|
makes sense to me mike. |
|
@bjorkbjork could you take a look at the fails in the ci? |
|
Morning gents. @zzzeek That is much clearer than the previous PR — thank you! I don't have an opinion on the plugin name: whatever you two like the best / hate the least is fine by me. |
sqla-tester
left a comment
There was a problem hiding this comment.
Hi, this is sqla-tester and I see you've pinged me for review. However, user bjorkbjork is not authorized to initiate CI jobs. Please wait for a project member to do this!
please open a discussion before starting to work on it. it's a "here be dragon" thing, especially in db that don't save what the source sql but reconstruct it (like pg). The comparison of index expression is currently very fragile and does not cover all cases. I tried doing something for this for the comparison of the index expression but I never finished it.. Maybe I could try to revive it In the meantime thanks for the effort here! |
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision b0c36ed of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset b0c36ed added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6672 |
sqla-tester
left a comment
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
great work. I've left a few comments, but overall it's mostly fine for me
Could you add a changelog note?
Could you add some documentation in /autogenerate.rst?
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6672
| name=params["name"], | ||
| **impl.adjust_reflected_dialect_options(params, "check_constraint"), | ||
| ) | ||
| conn_table.append_constraint(const) |
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
I don't think this is needed.
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6672
| return sig._is_uq | ||
|
|
||
|
|
||
| class _ck_constraint_sig(_constraint_sig[CheckConstraint]): |
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
nit: let's move this before the other typeguards
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6672
| eq_(diffs[0][0], "remove_constraint") | ||
| eq_(diffs[0][1].name, "ck_t_x_positive") | ||
|
|
||
| def test_same_name_different_expression_no_change(self): |
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
not sure how hard it would be, but we could also try to test an impl that returns something different in compare_check_constraint so that we test the comparison.is_different and comparison.is_skip
using monkeypatch would be fine for this
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6672
| eq_(check_diffs, []) | ||
|
|
||
|
|
||
| class AutogenCheckConstraintRenderTest(TestBase): |
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
let's also add an add and drop test with a schema
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6672
|
|
||
| result = autogenerate.render_op_text(self.autogen_context, op_obj) | ||
|
|
||
| assert "op.create_check_constraint(" in result |
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
could you use eq_ignore_whitespace instead of these asserts?
you can look at AutogenRenderTest
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6672
| __backend__ = True | ||
| __requires__ = ("check_constraint_reflection",) | ||
|
|
||
| def test_default_plugins_do_not_detect(self): |
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
let's remove this class and just move this test method in AutogenCheckConstraintTest. the other test method of the class we can remove, since it duplicates other ones
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6672
Add a new comparator plugin that detects added and removed named CHECK constraints during autogenerate. Comparison is name-only by default; the dialect hook compare_check_constraint exists for future per-dialect expression comparison.
The plugin is registered under alembic.ext.checkconstraint and must be explicitly opted into via autogenerate_plugins:
Unnamed constraints, type-bound constraints (Boolean/Enum), and dialects that do not support check constraint reflection are handled gracefully.
Fixes: #508
Description
I'm a SQLAlchemy/Alembic user and I've wanted autogenerate to detect CHECK constraint changes for a while. This PR adds opt-in detection of added/removed named CHECK constraints using the plugin system, following the approach @zzzeek outlined in #508, and in #1761 (used .ext.checkconstraint verbatim from @zzzeek).
Comparison is name-only — same-name constraints are considered equal regardless of expression text. A compare_check_constraint dialect hook on DefaultImpl is wired up but returns Equal() by default, since normalizing reflected SQL text across dialects is a known hard problem (discussed at length in #508). The intent is that this lays the foundation, and modification detection can be brought in on top via that dialect hook.
Handles type-bound constraints (Boolean/Enum), unnamed constraints, and dialects without check constraint reflection.
@jrmalin previously implemented this via a config flag in PR #1762, which was closed without review. This PR uses the plugin system instead, adds _ck_constraint_sig to the existing constraint signature hierarchy, and wires up the _InspectorConv SQLA2 caching path
Checklist
This pull request is:
must include a complete example of the issue. one line code fixes without an
issue and demonstration will not be accepted.
Fixes: #<issue number>in the commit messageinclude a complete example of how the feature would look.
Fixes: #<issue number>in the commit messageHave a nice day!