Core: Check table UUID in RESTTableOperations #14363
Conversation
There was a problem hiding this comment.
Thank you for the change @ebyhr
it would be really nice to add an UT for it.
| if (current == null | ||
| || !Objects.equals(current.metadataFileLocation(), response.metadataLocation())) { | ||
| this.current = response.tableMetadata(); | ||
| this.current = checkUUID(current, response.tableMetadata()); |
There was a problem hiding this comment.
This seems reasonable to me
There was a problem hiding this comment.
+1 for this fix. Also +1 for adding a test
There was a problem hiding this comment.
Thanks for the suggestion. Added a unit test.
| String newUUID = newMetadata.uuid(); | ||
| if (currentMetadata != null && currentMetadata.uuid() != null && newUUID != null) { | ||
| Preconditions.checkState( | ||
| newUUID.equals(currentMetadata.uuid()), | ||
| "Table UUID does not match: current=%s != refreshed=%s", | ||
| currentMetadata.uuid(), | ||
| newUUID); | ||
| } | ||
|
|
||
| return newMetadata; |
There was a problem hiding this comment.
[optional to consider] wonder if we can move this to some place common validate(current, newMetadata) so that both BaseMetaStoreOperation and TableOperation (may be an util or just default impl of TableOps) can share it, but seems too much for ROI.
| catalog.createNamespace(TABLE.namespace()); | ||
| catalog.createTable(TABLE, SCHEMA); | ||
|
|
||
| // inject the wrong table UUID |
There was a problem hiding this comment.
nit: simulate drop and re-create the table with same name
ebyhr
force-pushed
the
ebi/rest-check-uuid
branch
from
a5e470f to
e914843
Compare
|
Rebased on main to resolve conflicts. |
3 participants
Fixes #14337