Skip to content

Detect augmented JSON types when skipping batch migrate CAST - #1839

Open
ArockiaRajamanickam wants to merge 1 commit into
sqlalchemy:mainfrom
ArockiaRajamanickam:fix-1120-typedecorator-json
Open

Detect augmented JSON types when skipping batch migrate CAST#1839
ArockiaRajamanickam wants to merge 1 commit into
sqlalchemy:mainfrom
ArockiaRajamanickam:fix-1120-typedecorator-json

Conversation

@ArockiaRajamanickam

Copy link
Copy Markdown

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.

The SQLite batch migration data transfer suppresses the CAST for JSON
columns, since CASTing to JSON in SQLite yields 0. The check used
isinstance(), which does not match a TypeDecorator that augments JSON,
so the CAST was emitted and the column data was replaced with 0.
Compare type affinity instead, as the preceding condition already does.

Fixes: sqlalchemy#1120
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.

1 participant