Render the PostgreSQL DOMAIN type in autogenerate - #1831
Conversation
MohammedAlkindi
left a comment
There was a problem hiding this comment.
Checked this out and ran it on Windows 11, CPython 3.13.13, against main at c116cbc0.
Test evidence:
pytest tests/test_postgresql.py |
result |
|---|---|
main |
69 passed |
| this PR | 71 passed |
| product change reverted, new tests kept | 2 failed, 69 passed |
The two failures in the reverted run are exactly this PR's new tests, so they discriminate the fix rather than describing it. No existing test changes state. (Render-path only on my machine — no live PostgreSQL here, so the runtime DDL side wasn't exercised; these tests don't open a connection, which is what makes them runnable on a bare box.)
On the shape of the fix: following the existing _render_*_type dispatch pattern and emitting a constructor call rather than repr() matches how the other PG types are handled, and the field-by-field emission (collation, default, constraint_name, not_null, check, create_type) reads complete against SQLAlchemy's DOMAIN.__init__ signature.
Two questions, neither blocking:
create_type is not True— deliberate choice overnot type_.create_type? Ifcreate_typecan arrive as a non-bool truthy value from reflection, the identity test would emitcreate_type=Falsefor values that are actually truthy. If it is always a real bool, the current form is fine and arguably more explicit.- The
checkexpression renders withwrap_in_element=Falsewhiledefaultuseswrap_in_element=True— a line in the PR description on why they differ would save the next reader a trip into_render_potential_expr.
The changelog entry names the right ticket and follows the house format.
|
thanks for taking the time to actually run it, the revert table is exactly the check I was hoping someone would do. on 1: SQLAlchemy stores create_type as a plain bool so the two forms are equivalent, I just went with the explicit one to make "emit only when it differs from the default" obvious. happy to change it if a maintainer prefers. on 2: default ends up as a server-side expression in DDL so it needs the sa.text() wrapper to round-trip, while check is taken as a plain string that DOMAIN coerces itself, wrapping it would just add sa.text() noise in the rendered call. I'll add a line to the PR description on that. |
Fixes: #1360
Autogenerate had no renderer for the PostgreSQL
DOMAINtype, so_repr_type()fell back to plainrepr(). That dropped every constructor argument except name/data_type and left the underlying type unqualified (e.g.DOMAIN('email', CITEXT(), check="...")rendered aspostgresql.DOMAIN('email', CITEXT())), silently losing thecheckconstraint.Adds
_render_DOMAIN_typetoPostgresqlImpl(whichrender_typealready dispatches to via_render_<visit_name>_type). It renders the name, the underlyingdata_typethroughrender._repr_typefor the right prefix/import, and each ofcollation,default,constraint_name,not_null,check,create_typeonly when it differs from its default. The issue's column now renders as:Tests are guarded by
config.requirements.sqlalchemy_2sinceDOMAINonly exists on SQLAlchemy 2.0+. With the source change stashed both new tests fail (missing prefix and kwargs); with the fix they pass.pytest -q -k domain→ 2 passed, andtests/test_postgresql.py tests/test_autogen_render.py→ 249 passed, 2 skipped. Lint clean on the changed files.Note on rendering:
defaultis rendered with thesa.text()wrapper because it round-trips as a server-side expression in DDL, whilecheckis emitted as a plain string sinceDOMAINcoerces it itself.