Skip to content

Render the PostgreSQL DOMAIN type in autogenerate - #1831

Open
cycsmail wants to merge 1 commit into
sqlalchemy:mainfrom
cycsmail:domain-render-1360
Open

Render the PostgreSQL DOMAIN type in autogenerate#1831
cycsmail wants to merge 1 commit into
sqlalchemy:mainfrom
cycsmail:domain-render-1360

Conversation

@cycsmail

@cycsmail cycsmail commented Jul 15, 2026

Copy link
Copy Markdown

Fixes: #1360

Autogenerate had no renderer for the PostgreSQL DOMAIN type, so _repr_type() fell back to plain repr(). That dropped every constructor argument except name/data_type and left the underlying type unqualified (e.g. DOMAIN('email', CITEXT(), check="...") rendered as postgresql.DOMAIN('email', CITEXT())), silently losing the check constraint.

Adds _render_DOMAIN_type to PostgresqlImpl (which render_type already dispatches to via _render_<visit_name>_type). It renders the name, the underlying data_type through render._repr_type for the right prefix/import, and each of collation, default, constraint_name, not_null, check, create_type only when it differs from its default. The issue's column now renders as:

sa.Column('email', postgresql.DOMAIN('email', postgresql.CITEXT(),
    check="value ~ '^my_.*$'"), nullable=False)

Tests are guarded by config.requirements.sqlalchemy_2 since DOMAIN only 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, and tests/test_postgresql.py tests/test_autogen_render.py → 249 passed, 2 skipped. Lint clean on the changed files.

Note on rendering: default is rendered with the sa.text() wrapper because it round-trips as a server-side expression in DDL, while check is emitted as a plain string since DOMAIN coerces it itself.

@MohammedAlkindi MohammedAlkindi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. create_type is not True — deliberate choice over not type_.create_type? If create_type can arrive as a non-bool truthy value from reflection, the identity test would emit create_type=False for values that are actually truthy. If it is always a real bool, the current form is fine and arguably more explicit.
  2. The check expression renders with wrap_in_element=False while default uses wrap_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.

@cycsmail

Copy link
Copy Markdown
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rendering postgresql dialect DOMAIN field

2 participants