Skip to content

Widening an enum nulls the column: infer_transform has no (Enum, Enum) arm #140

Description

@rrrodzilla

Summary

Adding a variant to an existing enum field plans a ChangeType ... via set_null, which wipes the column on every row. The migration is purely additive at the data level (every stored value is still a valid variant of the new enum), so it should be a no-op transform.

Where

crates/schema-forge-core/src/migration.rs:852

fn infer_transform(old: &FieldType, new: &FieldType) -> ValueTransform {
    match (old, new) {
        (FieldType::Integer(_), FieldType::Float(_)) => ValueTransform::IntegerToFloat,
        (FieldType::Float(_), FieldType::Integer(_)) => ValueTransform::FloatToInteger,
        (_, FieldType::Text(_)) => ValueTransform::ToString,
        _ => ValueTransform::SetNull,
    }
}

There is no (Enum, Enum) arm, so every enum-to-enum change falls through to SetNull.

Reproduce

Given a deployed schema at @version(3):

crawl_status: enum("pending", "live", "unreachable", "no_staff_page", "excluded") default("pending") indexed

bump to @version(4) adding one variant:

crawl_status: enum("pending", "live", "unreachable", "no_staff_page", "blocked", "excluded") default("pending") indexed

then:

schemaforge migrate schemas/ --schema Outlet

Plan:

Outlet (1 steps, requires_confirmation)
  1. CHANGE TYPE of 'crawl_status' from Enum[pending, live, unreachable, no_staff_page, excluded]
     to Enum[pending, live, unreachable, no_staff_page, blocked, excluded] via set_null [requires_confirmation]

Impact

Hit on a live corpus of 67,631 rows. Applying it would have nulled crawl_status on all of them (10,221 live, 10,184 no_staff_page, 1,748 unreachable, 45,496 pending), destroying the recorded outcome of a multi-day crawl. The values are all still legal under the new enum. Worked around by snapshotting the column to a file first and planning to restore it after the fact.

requires_confirmation is set, so it is not silent, but the flag reads as ordinary caution about a type change rather than "this deletes your data", and --force in a script would take it without comment.

Suggested fix

Add an (Enum, Enum) arm that compares variant sets:

  • new variants are a superset of old (pure widening): no data transform needed, and arguably not requires_confirmation either.
  • new variants drop one or more old variants: only then is a transform required, and it should be SetNull scoped to rows holding a removed variant, not the whole column. Reporting how many rows are affected in the plan would make the confirmation prompt meaningful.
  • rename/reorder with the same set: no-op.

A narrower fix that would still have prevented this: keep SetNull but make the plan state the row count it will null, so the confirmation carries the actual cost.

Backend note

On the PostgreSQL backend the stored representation makes a widening especially cheap: nothing about existing rows needs to change. The plan is conservative in a way the storage layer does not require.

Version

schemaforge 0.37.2, PostgreSQL backend, repo at 623d128.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions