Detect augmented JSON types when skipping batch migrate CAST

[ArockiaRajamanickam]

Fixes the data loss reported in #1120.

cast_for_batch_migrate skips the data transfer CAST for JSON, because CASTing to JSON in SQLite yields 0. The check was isinstance(new_type, JSON), which a TypeDecorator wrapping JSON does not satisfy, so the CAST went out and every value in the column became 0.

Reproduced on an in-memory SQLite database with one row holding {"keep": "me", "n": 123}:

control, sa.JSON      value after: '{"keep": "me", "n": 123}'
                      INSERT INTO _alembic_tmp_bar (foo) SELECT bar.foo FROM bar

TypeDecorator(JSON)   value after: 0
                      INSERT INTO _alembic_tmp_bar (foo) SELECT CAST(bar.foo AS JSON) AS foo FROM bar

The change compares type affinity, which the condition on the line above already does:

existing.type._type_affinity is not new_type._type_affinity
and new_type._type_affinity is not JSON

One note on the approach suggested in the issue. isinstance(typ.dialect_impl(dialect), JSON) does not catch this case, because for a TypeDecorator dialect_impl() returns the decorator itself rather than its impl. I measured each candidate against the SQLite dialect:

type	isinstance	dialect_impl	_type_affinity is JSON
sa.JSON()	True	True	True
TypeDecorator(impl=JSON)	False	False	True
sqlite.JSON()	True	True	True
postgresql.JSONB()	True	True	True
String, Integer, TypeDecorator(impl=String)	False	False	False

Affinity is the only one that catches the TypeDecorator while still excluding non-JSON types. impl_instance also works, but only for the decorator, so it would have to be combined with the existing isinstance check.

Worth flagging: this is a behaviour change in one narrow direction. CAST suppression now covers any type whose affinity is JSON, not only JSON instances, so a change from a genuinely different affinity to an augmented JSON type no longer emits the CAST. That is what the original guard was for, just applied to the augmented case too.

The test is tests/test_batch.py::CopyFromTest::test_change_type_json_typedecorator, next to test_change_type as suggested in the issue. It fails on main with CAST(foo.toj AS JSON) AS toj in the transfer statement and passes with the fix. Full suite is 1770 passed, 133 skipped, and there is a changelog fragment in docs/build/unreleased/1120.rst.

I used an AI assistant while working on this. The reproduction and the comparison table are things I ran myself.

[MohammedAlkindi]

Checked this out and ran it on Windows 11 (10.0.26200), CPython 3.13.13, against main at c116cbc0.

The diagnosis holds and the failure mode is nasty — silent data loss rather than an error. CAST(x AS JSON) in SQLite yields 0, so suppressing the CAST for JSON is load-bearing, and isinstance(new_type, JSON) genuinely misses a TypeDecorator whose impl is JSON: the decorator is not a JSON subclass. Every value in the column becomes 0 with no diagnostic.

_type_affinity is the right instrument here, and it is already this codebase's idiom — worth saying because it is a private SQLAlchemy attribute and might otherwise look like a reach:

alembic/autogenerate/compare/types.py:35   conn_type._type_affinity is sqltypes.NullType
alembic/autogenerate/compare/types.py:42   metadata_type._type_affinity is sqltypes.NullType
alembic/ddl/impl.py:645                    existing.type._type_affinity is not new_type._type_affinity
alembic/ddl/mysql.py:211                   type_._type_affinity is sqltypes.DateTime

So this is consistent with four existing call sites rather than introducing a new dependency, and impl.py:645 is doing the same affinity-not-isinstance comparison in the adjacent code path.

Test evidence — failing-test set diff against base:

	failing tests
main @ c116cbc0	13
this PR	13

The sets are identical — no regressions, none fixed. (The two clusters are test_post_write.py::RunHookTest and test_script_production.py::ScriptNamingTest timezone tests; both fail on main on this machine and are unrelated to this change.) I'm reporting the set rather than the counts deliberately: raw pass counts differ between base and PR here (1805 vs 1758) only because the PR branches from an older commit with fewer tests, which would look like a regression if read as a delta.

The new test_change_type_json_typedecorator is the right shape — a TypeDecorator with impl = JSON and cache_ok = True, asserted through the batch path rather than by unit-testing the predicate.

One question, not a blocker. _type_affinity walks to the underlying type for a TypeDecorator, but for a doubly wrapped decorator (TypeDecorator whose impl is another TypeDecorator over JSON) — does affinity still resolve through both layers? I did not construct that case, so I am asking rather than asserting. If it does resolve, a one-line note in the test docstring would save the next reader the same question.

I could not exercise the MySQL or PostgreSQL paths — no server on this machine — so my run covers the SQLite batch path only, which is what this PR touches.

[ArockiaRajamanickam]

Thanks, and the point about reporting the failing-test set rather than the counts is well made. The 1805 vs 1758 gap would have looked like a regression to anyone reading it as a delta.

On your question, it does resolve through both layers. I built the case you did not:

class SingleJSON(TypeDecorator):
    impl = JSON
    cache_ok = True

class DoubleJSON(TypeDecorator):
    impl = SingleJSON      # decorator over a decorator over JSON
    cache_ok = True

type	_type_affinity	is JSON
JSON()	JSON	True
SingleJSON()	JSON	True
DoubleJSON()	JSON	True
TripleJSON()	JSON	True

TypeDecorator._type_affinity delegates to self.impl_instance._type_affinity, so it recurses to the bottom of the chain however deep it goes. Running the PR predicate directly: String -> DoubleJSON and String -> TripleJSON both come out as CAST suppressed, which is the safe answer. As a negative control, JSON -> TypeDecorator(impl=String) still emits the CAST, so the check has not simply become permissive. SQLAlchemy 2.0.52.

Added your suggested note to the test docstring in 91421ed rather than adding a second test, since the existing one already exercises the code path and the nesting depth is a property of _type_affinity rather than of this fix. Happy to add the explicit doubly-wrapped case as a test if you would rather see it asserted than described.

tests/test_batch.py is 119 passed, 7 skipped, flake8 clean.

Agreed on _type_affinity being the codebase idiom, and thanks for listing the four call sites. impl.py:645 was in fact what I copied, it is the line directly above the one this PR changes, which is what made the inconsistency stand out in the first place.

[MohammedAlkindi]

@ArockiaRajamanickam I'd take the asserted version — it's cheap where you already are, and it pins the one claim in the docstring that a future refactor could silently falsify (the recursion, not the single wrap). Inside the same test, something like:

class DoublyWrappedJson(TypeDecorator):
    impl = CustomJson
    cache_ok = True

with one more alter_column("toj", type_=DoublyWrappedJson) block asserting the same non-CAST SQL. Mild preference, not a blocker — the note you added in 91421ed is honest as-is, and the single-wrap test already exercises the code path.

Glad the set-vs-count point was useful — that one has saved me from reporting phantom regressions more than once.

[ArockiaRajamanickam]

Taken, in c97b364. You were right that it is worth asserting rather than describing, and I can show it rather than just claim it.

DoublyWrappedJson(impl=CustomJson) is now exercised in the same test, asserting the same non-CAST SQL, with the docstring reduced to pointing at both depths.

To check the new assertion actually earns its place I ran two negative controls against it:

1. Reverting to the original isinstance(new_type, JSON) fails the test, as before.
2. Simulating exactly the refactor you were guarding against, a fix that unwraps only one decorator layer (isinstance(getattr(new_type, "impl", new_type), JSON)), passes the single-wrap assertion and fails at the doubly wrapped one. The failure lands on the context.assert_ right after the DoublyWrappedJson block, so that case is what catches it and the original test alone would not have.

That is the scenario you described, so the assertion is doing the job you wanted from it rather than just restating the passing case.

One wrinkle worth noting for anyone reading the diff: the second block needs its own self.table.append_column(Column("toj", Text)), because CopyFromTest._fixture() rebuilds self.table from scratch. My first attempt reused the table and died with KeyError: toj. The surrounding tests in that class do the same append-after-fixture, so this follows the file.

tests/test_batch.py: 119 passed, 7 skipped, flake8 clean.