fix(core): reject unsupported list options instead of ignoring them - #8205
fix(core): reject unsupported list options instead of ignoring them#8205jaideeppyne wants to merge 3 commits into
Conversation
CorrectnessCheckLayer validates arguments for read, write, stat, delete, copy and compose, but forwarded list arguments unchecked. A service that does not support start_after, versions or deleted silently dropped them and returned a full listing. Gate those three on their capability. limit stays ungated because it is documented as a backend hint, and recursive stays ungated because SimulateLayer emulates it when list_with_recursive is false.
|
Pushed a follow-up: I swept the other bindings for the same pattern; Java, Python, Go and the Rust behavior suite all gate these three on the capability already, so that C++ test was the only unguarded caller. |
The Ruby lister test asserted the unfiltered listing that fs returned while start_after was being dropped; it now asserts the Unsupported error. Drop the behavior-suite test. CapabilityOverrideLayer is documented to change only the capability reported by OperatorInfo, not what the stack enforces, and the s3 test setups use it to mask a non-versioned bucket. A portable test cannot read the capability the check actually uses.
|
CI found two more things, both now handled, and one of them is worth flagging on its own.
The side observation is that Also unrelated: the goosefs job failed on a registry timeout pulling |
Which issue does this PR close?
Closes #6676.
Rationale for this change
CorrectnessCheckLayervalidates arguments for read, write, stat, delete, copy and compose, butlistwas forwarded unchecked. So a list option the service cannot honor is dropped silently and the caller gets a full listing back with no signal that anything was ignored.Reproduced on
fs, no network needed:That is exactly #6676.
versions(true)anddeleted(true)fail the same way everywhere: they are only read by the s3, cos, tos, oss and onedrive listers and dropped by every other service. This is not fs-only either, s3-express setslist_with_start_after: falseand is affected too.Two existing tests had encoded the old behavior as expected.
bindings/ruby/test/lister_test.rbasserted the full unfiltered listing with the comment "fs backend doesn't support start_after", and the C++OpenDALOptionsTest.ListOptionsset all five options at once againstmemoryand passed only because three of them were dropped. Both are updated here.The
object_storeintegration already hand-rolls this guard (store.rscheckslist_with_start_afterand falls back to client-side filtering when it is false), which is a fair sign that callers should not have to discover the limitation themselves.@Xuanwo, on #6676 you said you were open to this if we can inform users about the limitation clearly. An
Unsupportederror is the signal the layer already gives for every other operation, so this reuses it rather than inventing a new mechanism.What changes are included in this PR?
CorrectnessService::listnow rejectsstart_after,versionsanddeletedwhen the matching capability is false.Two options stay ungated on purpose:
limitis documented as a backend hint.recursiveis emulated bySimulateLayereven whenlist_with_recursiveis false, which is whyfsrecursion works today. Gating it would break working listings.Tests: unit tests in
correctness_check.rscovering all three options in both directions, plus a separate test pinning thatlimitandrecursivestay ungated. The Ruby test now asserts the error, and the C++ test is split into the two optionsmemorysupports plus a newListOptionsUnsupportedByServicefor the three it does not.There is deliberately no behavior-suite test. I tried one and CI showed why it cannot work:
CapabilityOverrideLayeris documented to change only the capability reported byOperatorInfo, not what the stack enforces, and it is applied aboveCorrectnessCheckLayer. The s3 setups use it (OPENDAL_TEST_CAPABILITY_OVERRIDES=...,list_with_versions=false,...) to mask a non-versioned MinIO bucket, soop.info().capability()there reportslist_with_versions: falsewhile the check correctly sees the service's realtrue. A portable test cannot read the capability the check actually uses, so asserting on the advertised one is wrong. Worth knowing separately: that layer can relabel a capability but cannot restrict behavior.How I found it. My oracle was cross-service agreement: a 71-probe differential harness over list, stat, read, ranges, delete and create_dir, run across every backend I can exercise with no network (fs, memory, dashmap, moka, mini-moka, sled, redb, cacache, persy).
start_afterbeing silently ignored onfswas one of the divergences it turned up.Verified locally:
cargo test -p opendal-core --lib(11 correctness_check tests), the behavior suite unchanged at 130 passed on fs and 110 on memory, all 83 C++ tests via cmake/ninja, all 57 Ruby tests viarake test:base,cargo clippy -p opendal-core --all-targets -- -D warnings,cargo fmt --all --check,cargo test -p opendal-core --doc,cargo docwith no new intra-doc warnings, and theintegrations/object_storesuite includingtest_list_with_offset.Are there any user-facing changes?
Yes. Anyone passing one of those three options to a service that does not support it used to get a successful but wrong result and now gets
Unsupported. No public API signature changes, so I did not add thebreaking-changeslabel, happy to add it if you would rather flag it in the changelog.ListOptionsand the list futures now document the capability requirement.AI Usage Statement
AI-assisted. I used Claude Code to build and run the cross-backend differential harness that surfaced the divergence, and to draft the patch; I reviewed the result, chose the scope, and ran the verification above myself.
Assumptions and unknowns worth your review: every service that supports these three options is network-backed, so I could not exercise the positive path against a real backend. It is covered by the mock unit test and by the existing capability-gated behavior tests in CI. I also assumed
limitandrecursiveshould stay ungated for the reasons above, and that is the judgement call most worth a second opinion.