Ensure TreeMapMetaStore range reads return copies#5027
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes TreeMapMetaStore range reads to return defensive copies of stored records (via the slice’s existing copyRecord function) so callers can’t mutate the in-memory store by modifying objects returned from readRange.
Changes:
- Update
Slice.readRangeto copy each returned value for both empty-prefix and prefix-bounded range reads. - Add a
copyValues(...)helper to centralize the per-record copying logic. - Add unit tests covering empty-prefix and non-empty-prefix
readRangecopy behavior usingPolarisGrantRecord.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStore.java | Make readRange return per-record copies using copyRecord (via new copyValues). |
| polaris-core/src/test/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStoreTest.java | Add tests verifying readRange returns copies for empty and non-empty prefixes. |
| private List<T> copyValues(Collection<T> values) { | ||
| List<T> copied = new ArrayList<>(values.size()); | ||
| for (T value : values) { | ||
| copied.add(this.copyRecord.apply(value)); | ||
| } |
There was a problem hiding this comment.
I updated deleteRange to avoid calling readRange now that readRange defensively copies returned records.
I also saw the dev discussion about eventually deprecating/removing TreeMapMetaStore. So I kept this change narrow.
Thanks you!
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for your contribution, @iting0321 !
Changes LGTM, but please see https://lists.apache.org/thread/nzoljc1ohnsq4f5o28dh4opqkqw3p09h
| private List<T> copyValues(Collection<T> values) { | ||
| List<T> copied = new ArrayList<>(values.size()); | ||
| for (T value : values) { | ||
| copied.add(this.copyRecord.apply(value)); | ||
| } |
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
flyrain
left a comment
flyrain
left a comment
There was a problem hiding this comment.
LGTM. Thanks @iting0321 !
|
|
||
| String endKey = | ||
| prefix.substring(0, prefix.length() - 1) | ||
| + (char) (prefix.charAt(prefix.length() - 1) + 1); |
There was a problem hiding this comment.
nit: The endKey prefix-successor computation is now duplicated verbatim between readRange (lines 87-89) and here. Worth pulling into a small private helper (e.g. rangeEndKey(prefix), or a shared subMap accessor both paths call) so the two range computations can't drift.
There was a problem hiding this comment.
Thanks for your feedback! I just fixed it!
Summary
Implement the TODO
TODO: return a copy of each object to avoid mutating the recordsFix
TreeMapMetaStorerange reads so they return copied records instead of references to the records stored in the underlying slice.Previously,
readRangereturned a new list but reused the stored record objects inside that list. Callers could mutate those returned records and accidentally modify the in-memory store outside a write transaction.This change makes
readRangecopy each returned value with the slice’s existingcopyRecordfunction for both:Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)