fix: avoid data loss when table-type custom property has column named "id" - #33044
fix: avoid data loss when table-type custom property has column named "id"#33044a-kash-singh wants to merge 4 commits into
Conversation
…named "id"
2
3 The internal row-tracking key `'id'` collided with user-defined columns of the
4 same name: the value was overwritten with a row index on load and stripped
5 entirely on save. Rename the internal sentinel to `__row_id__` across the modal
6 state initializer, the save handler, and the hook's handleAddRow so there is no
7 collision with any user-defined column.
8
9 Fixes open-metadata#32677
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| const [dataSource, setDataSource] = useState< | ||
| TableTypePropertyValueType['rows'] | ||
| >(() => rows.map((row, index) => ({ ...row, id: index + '' }))); | ||
| >(() => rows.map((row, index) => ({ ...row, __row_id__: index + '' }))); |
There was a problem hiding this comment.
💡 Quality: No regression test for the "id" column collision fix
The PR fixes silent data loss when a user-defined column is literally named id, but no test exercises that scenario. The existing tests in TableTypePropertyEditTable.test.tsx still use id as internal mock keys and only cover the child table component, not the modal's state initializer/save path where the collision occurred. Add a test that renders EditTableTypePropertyModal with a column named id, edits it, and asserts onSave preserves the user's id value (and that __row_id__ is stripped) to prevent regression.
Was this helpful? React with 👍 / 👎
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsFixes silent data loss when a table-type custom property has a column named 💡 Quality: No regression test for the "id" column collision fix📄 openmetadata-ui/src/main/resources/ui/src/components/common/CustomPropertyTable/TableTypeProperty/EditTableTypePropertyModal.tsx:54 📄 openmetadata-ui/src/main/resources/ui/src/components/common/CustomPropertyTable/TableTypeProperty/EditTableTypePropertyModal.tsx:77 The PR fixes silent data loss when a user-defined column is literally named 🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
Closing in favour of #32741 which covers a more comprehensive fix. |
1 participant
EditTableTypePropertyModal used the bare string 'id' as an internal row-tracking key. When a user defined a column literally named id, the component overwrote the column value with a row index on load and stripped it entirely via omit(row, 'id') on save, causing silent data loss.
Rename the internal sentinel to row_id in the modal state initializer, the save handler, and useGridEditController.handleAddRow so it cannot collide with any user-defined column name.
Fixes #32677