feat(attributes): add data migration utilities#454
Conversation
Add shared data migration APIs for deprecated attributes across the JavaScript, Python, and Rust packages. The utilities expose single-value and attribute-map migration helpers, generated metadata support, and documentation for adding future migrations without hard-coding migration ids in schemas or generated types.
constantinius
requested review from
a team,
Lms24,
cleptric,
mjq and
nsdeschenes
as code owners
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
🤖 This preview updates automatically when you update the PR. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| for migration in ATTRIBUTE_MIGRATIONS: | ||
| if len(migration.sources) == 1 and migration.sources[0] == attribute_name: | ||
| return migration.migrate({attribute_name: value}) or value |
There was a problem hiding this comment.
Bug: The migrate_attribute_value function incorrectly handles falsy return values (like 0, False, or "") from migrations, causing it to discard the migrated value and return the original one.
Severity: HIGH
Suggested Fix
Replace the or value logic with an explicit check for None. For example: migrated_value = migration.migrate({attribute_name: value}); return migrated_value if migrated_value is not None else value;. This ensures that only a None return is treated as a failed migration.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: python/src/sentry_conventions/migrations.py#L73
Potential issue: The function `migrate_attribute_value` uses the `or` operator to
provide a fallback value. This is incorrect because valid migrated attribute values can
be "falsy" in Python (e.g., `0`, `False`, `""`, `[]`). When a migration correctly
returns a falsy value, the `or` operator will incorrectly discard it and return the
original, un-migrated value instead. This leads to silent data corruption, as the
migration appears to fail when it has actually succeeded. This behavior is inconsistent
with the JavaScript and Rust implementations, which correctly handle this case.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a768c83. Configure here.
|
|
||
| for migration in ATTRIBUTE_MIGRATIONS: | ||
| if len(migration.sources) == 1 and migration.sources[0] == attribute_name: | ||
| return migration.migrate({attribute_name: value}) or value |
There was a problem hiding this comment.
Python falsy migration results dropped
Medium Severity
In migrate_attribute_value, the migrated result is combined with or value, so legitimate values such as 0, False, or "" are treated as failure and the original deprecated value is returned instead of the replacement.
Reviewed by Cursor Bugbot for commit a768c83. Configure here.
| if migration.remove_sources { | ||
| for source in migration.sources { | ||
| attributes.remove(*source); | ||
| } |
There was a problem hiding this comment.
Rust ignores deprecation normalize status
High Severity
Rust migrate_deprecated_attributes removes sources only when each migration’s static remove_sources flag is set, not from generated deprecation normalize/backfill metadata. Docs and CONTRIBUTING describe metadata-driven behavior like JavaScript and Python, so flipping _status in JSON will not update Rust (including Relay) unless code is edited separately.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a768c83. Configure here.
| if (migration.sources.some((source) => ATTRIBUTE_METADATA[source]?.deprecation?.status === 'normalize')) { | ||
| for (const source of migration.sources) { | ||
| delete attributes[source]; | ||
| } |
There was a problem hiding this comment.
Normalize drops sources without replacement
Medium Severity
When any migration source has deprecation status normalize, migrateDeprecatedAttributes deletes all configured source keys even if the migration did not produce a replacement value and the replacement key is still absent from the map.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a768c83. Configure here.
|
Hi, we're going to discuss in the ingest team how this API would fit into Relay. In the meantime, a question: is the migration machinery intended to eventually replace the existing backfill/normalize logic? |
|
Thank you
No, I would say it complements it. The migrations are only used when we change the data layout. But we should discuss how it works together: e.g backfill + migration keeps the old value on the old field, and sets the new value on the new field. |
loewenheim
left a comment
loewenheim
left a comment
There was a problem hiding this comment.
This needs some further offline discussion.
|
My knee-jerk reaction to this: why don't we design schemas that are forwards compatible instead? |
3 participants
Summary
Contributes to TET-2369.
Testing
yarn test