fix: reject ambiguous secondary index names - #1266
Conversation
There was a problem hiding this comment.
🟢 Ready to approve
The changes are narrowly scoped validation hardening with solid test coverage, and the only noted issue is a small doc comment accuracy fix.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR hardens secondary index handling by rejecting secondary index names containing / at public RPC boundaries and in the Go client Put option parsing, preventing ambiguous persisted index key prefixes and cross-index collisions in the data server.
Changes:
- Added server-side validation rejecting
/in secondary index names for unary Write, WriteStream, Read (Get), List, and RangeScan requests. - Added Go client-side validation for Put secondary index options, returning
ErrInvalidOptionsearly. - Updated API documentation/comments and added focused unit tests covering the new validation behavior.
File summaries
| File | Description |
|---|---|
| oxiad/dataserver/public_rpc_server.go | Adds RPC-layer validation for secondary index names to prevent ambiguous /-containing index names from entering the system via public APIs. |
| oxiad/dataserver/public_rpc_server_test.go | Adds tests ensuring unary and streaming RPC paths reject /-containing secondary index names with InvalidArgument. |
| oxia/options_put.go | Adds local Put option validation rejecting / in secondary index names and updates SecondaryIndex doc comment. |
| oxia/options_put_test.go | Adds unit test verifying Put option validation fails locally with ErrInvalidOptions for invalid index names. |
| oxia/options_list.go | Updates UseIndex doc comment to document the / restriction (but see review comment about accuracy across Get/RangeScan). |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
89dbd64 to
93ba458
Compare
mattisonchao
changed the title
93ba458 to
4cea25c
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The updated UseIndex doc comment is misleading because it describes only List semantics even though UseIndex also applies to Get and RangeScan.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
oxia/options_list.go:56
- The UseIndex doc comment describes only List semantics ("returned list") even though UseIndex also applies to Get and RangeScan. This can confuse callers about whether returned keys are primary or secondary keys for non-List operations.
// UseIndex lets users specify a secondary index to follow. Index names must not
// contain '/'. The returned list contains the primary keys of the records.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The behavioral change is localized and covered by a targeted test, with only a minor documentation wording tweak suggested.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
oxia/options_list.go:56
- UseIndex is applied not only to List but also to RangeScan and Get (see applyRangeScan/applyGet). The updated comment still suggests it only affects "the returned list", which can mislead users about how keys/records are returned when scanning or getting via a secondary index.
// UseIndex lets users specify a secondary index to follow. Index names must not
// contain '/'. The returned list contains the primary keys of the records.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
4cea25c to
aa3ae52
Compare
There was a problem hiding this comment.
🟢 Ready to approve
The change is small and targeted, enforces the intended validation at the correct point (pre-batching), updates docs, and adds a clear unit test for the new behavior.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
mattisonchao
changed the title
There was a problem hiding this comment.
🟢 Ready to approve
The change is narrowly scoped, feature-gated for safe rolling upgrades, and has targeted tests covering client validation, server no-op semantics, and feature negotiation/persistence.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (2)
- common/proto/client.pb.go: Generated file
- common/proto/replication.pb.go: Generated file
- Files reviewed: 13/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Hi @merlimat could you help review it? |
There was a problem hiding this comment.
🟡 Human review recommended
It changes core write-path behavior and rolling-upgrade feature negotiation semantics across client/server/proto layers, so a final human review is recommended despite strong test coverage.
Review details
Files not reviewed (2)
- common/proto/client.pb.go: Generated file
- common/proto/replication.pb.go: Generated file
- Files reviewed: 16/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Human review recommended
It changes replicated/protocol-level behavior and feature-gated server write semantics (with rolling-upgrade implications) that warrant final human verification beyond automated review.
Review details
Files not reviewed (2)
- common/proto/client.pb.go: Generated file
- common/proto/replication.pb.go: Generated file
- Files reviewed: 16/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The PutOption interface signature change is a breaking public API change and should be reconsidered or replaced with a backward-compatible validation approach.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (2)
- common/proto/client.pb.go: Generated file
- common/proto/replication.pb.go: Generated file
Suppressed comments (2)
oxia/options_put.go:34
- Changing PutOption.applyPut to return an error is a breaking public API change for any downstream code that implements PutOption (even though this is a "fix" PR). If you want to keep backward compatibility, consider keeping applyPut void and performing secondary-index name validation inside newPutOptions (after applying options) instead.
// PutOption represents an option for the [SyncClient.Put] operation.
type PutOption interface {
applyPut(opts *putOptions) error
}
oxiad/dataserver/controller/lead/secondary_indexes.go:39
- wrapperUpdateCallback.ValidatePut only delegates to secondaryIndexesUpdateCallback and skips sessionManagerUpdateOperationCallback.ValidatePut. Even though the current session ValidatePut always returns OK, this creates an easy-to-miss divergence from the OnPut composition order and would silently bypass any future session-level prevalidation.
func (wrapperUpdateCallback) ValidatePut(req *proto.PutRequest, features database.FeatureChecker) proto.Status {
return secondaryIndexesUpdateCallback.ValidatePut(req, features)
}
- Files reviewed: 16/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Validate each secondary index name while applying Put options, before the operation enters the client batch manager. Refs #1265 Signed-off-by: mattisonchao <mattisonchao@gmail.com>
Signed-off-by: mattisonchao <mattisonchao@gmail.com>
Signed-off-by: mattisonchao <mattisonchao@gmail.com>
Signed-off-by: mattisonchao <mattisonchao@gmail.com>
Signed-off-by: mattisonchao <mattisonchao@gmail.com>
Signed-off-by: mattisonchao <mattisonchao@gmail.com>
0ad6640 to
c0dd124
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The wrapper callback’s new ValidatePut implementation should chain validation consistently (including the session manager validate hook) to avoid future divergence and missed validations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (2)
- common/proto/client.pb.go: Generated file
- common/proto/replication.pb.go: Generated file
Suppressed comments (1)
oxiad/dataserver/controller/lead/secondary_indexes.go:40
- wrapperUpdateCallback.ValidatePut only delegates to secondaryIndexesUpdateCallback and skips sessionManagerUpdateOperationCallback.ValidatePut. Even though the current session manager implementation always returns OK, this creates an inconsistent callback chain compared to OnPut/OnDelete and risks future bugs if session validation is added later.
func (wrapperUpdateCallback) ValidatePut(req *proto.PutRequest, features feature.Checker) proto.Status {
return secondaryIndexesUpdateCallback.ValidatePut(req, features)
}
- Files reviewed: 17/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Signed-off-by: mattisonchao <mattisonchao@gmail.com>
|
Addressed the latest Copilot review in
The branch is also rebased onto current |
There was a problem hiding this comment.
🟡 Human review recommended
It changes core write-apply semantics and protocol enums/feature negotiation, so a final human review is warranted despite the added test coverage.
Review details
Files not reviewed (2)
- common/proto/client.pb.go: Generated file
- common/proto/replication.pb.go: Generated file
- Files reviewed: 15/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Signed-off-by: mattisonchao <mattisonchao@gmail.com>
Motivation
Secondary index names are embedded unescaped in persisted index keys. A name containing
/is indistinguishable from part of the secondary key, allowing two logical indexes to produce the same stored key.Go client validation catches supported client calls, but older clients and direct protocol callers can still append ambiguous entries. Server-side rejection must be deterministic during rolling upgrades and must not stop WAL replay or commit-offset advancement.
Tracks #1265.
Modifications
newPutOptionswithout changing thePutOption.applyPutcontract/in secondary index names before a Go client Put enters batchingStatus_INVALID_ARGUMENTresult and map it toErrInvalidOptionsFEATURE_SECONDARY_INDEX_NAME_VALIDATIONValidatePutpreflight hook to the update callback contractdatabase.applyPut, before sequence, version, session, index, or record mutationsSecondaryIndexandUseIndexTesting
go test -race ./oxia ./oxiad/common/feature ./oxiad/coordinator/runtime/controller/shard ./oxiad/dataserver/controller/lead ./oxiad/dataserver/database ./oxiad/dataserver ./tests/clientmainand reran the standalone regression test