You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "jaideeppyne (via GitHub)" <gi...@apache.org> on 2026/08/31 12:40:52 UTC
[PR] fix(core): reject unsupported list options instead of ignoring them [opendal]
jaideeppyne opened a new pull request, #8205:
URL: https://github.com/apache/opendal/pull/8205
# Which issue does this PR close?
Closes #6676.
# Rationale for this change
`CorrectnessCheckLayer` validates arguments for read, write, stat, delete, copy and compose, but `list` was 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:
```rust
op.write("d/a", "x").await?;
op.write("d/b", "x").await?;
let got = op.list_with("d/").start_after("d/a").await?;
// expected ["d/b"], got ["d/", "d/a", "d/b"]
```
That is exactly #6676. `versions(true)` and `deleted(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 sets `list_with_start_after: false` and is affected too.
The `object_store` integration already hand-rolls this guard (`store.rs` checks `list_with_start_after` and 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 `Unsupported` error 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::list` now rejects `start_after`, `versions` and `deleted` when the matching capability is false.
Two options stay ungated on purpose:
* `limit` is documented as a backend hint.
* `recursive` is emulated by `SimulateLayer` even when `list_with_recursive` is false, which is why `fs` recursion works today. Gating it would break working listings.
Tests: unit tests in `correctness_check.rs` covering both the rejected and the supported case, plus `test_list_with_unsupported_options` in the behavior suite, which asserts the negative case for whichever of the three the service under test does not advertise.
How I checked it. My oracle was cross-service agreement: I ran a differential harness (71 probes over list, stat, read, ranges, delete, create_dir) across every backend I can run with no network, and `start_after` being ignored on `fs` was one of the divergences it turned up. Then I ran the behavior suite against all nine of those backends before and after. Reverting only the source change while keeping the new tests makes them fail.
| service | before | after |
| --- | --- | --- |
| fs | 130 | 131 |
| memory | 110 | 111 |
| sled | 110 | 111 |
| dashmap / moka / mini-moka | 109 | 110 |
| redb / cacache / persy | 86 | 86 (no `list`, test not registered) |
Also green: `cargo clippy -p opendal-core --all-targets -- -D warnings`, `cargo fmt --all --check`, `cargo test -p opendal-core --lib --doc`, `cargo doc` with no new intra-doc warnings, and the `integrations/object_store` suite including `test_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 the `breaking-changes` label, happy to add it if you would rather flag it in the changelog.
`ListOptions` and 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 only by the mock unit test and by the existing capability-gated behavior tests in CI. I also assumed `limit` and `recursive` should stay ungated for the reasons above, and that is the judgement call most worth a second opinion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(core): reject unsupported list options instead of ignoring them [opendal]
Posted by "jaideeppyne (via GitHub)" <gi...@apache.org>.
jaideeppyne commented on PR #8205:
URL: https://github.com/apache/opendal/pull/8205#issuecomment-6106780374
## Remaining CI check
I checked the full status list rather than only the first page. The remaining failure is **Go binding / WebHDFS**: `opendal-go-services/internal/generate/generate.go:108` panics with `failed to generate go file: exit status 1` while generating the binding, before service tests run. The log does not expose the underlying generator error.
This PR does not change the Go generator. I attempted to rerun that job, but GitHub reports that it cannot be rerun. Could a maintainer inspect or rerun the generator check? I have not classified it as transient without enough evidence.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(core): reject unsupported list options instead of ignoring them [opendal]
Posted by "jaideeppyne (via GitHub)" <gi...@apache.org>.
jaideeppyne commented on PR #8205:
URL: https://github.com/apache/opendal/pull/8205#issuecomment-5479194979
CI found two more things, both now handled, and one of them is worth flagging on its own.
1. `bindings/ruby/test/lister_test.rb` had a `start_after` test asserting the full unfiltered listing, with the comment "fs backend doesn't support start_after". It was pinning the bug. It now asserts the `Unsupported` error. All 57 Ruby tests pass locally via `rake test:base`.
2. I had added a behavior-suite test asserting that an unadvertised option is rejected, and it failed on three of the s3 setups. The cause is not the check: `CapabilityOverrideLayer` is applied above `CorrectnessCheckLayer`, and it is documented to change only the capability reported by `OperatorInfo`, not what the stack enforces. The s3 setups use `OPENDAL_TEST_CAPABILITY_OVERRIDES=...,list_with_versions=false,...` to mask a non-versioned MinIO bucket, so `op.info().capability()` reports false there while the check correctly sees s3's real `true` and lets the call through. A portable test cannot read the capability the check actually uses, so I dropped that test rather than assert on the advertised one. `core/tests/behavior/async_list.rs` is now byte-identical to main.
The side observation is that `CapabilityOverrideLayer` can relabel a capability but cannot restrict behavior, so a setup that sets `list_with_versions=false` to describe a non-versioned bucket still lets `versions(true)` through to the service. That is pre-existing and out of scope here, but happy to file it separately if it is news.
Also unrelated: the goosefs job failed on a registry timeout pulling `goosefs.tencentcloudcr.com`, so no OpenDAL code ran there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(core): reject unsupported list options instead of ignoring them [opendal]
Posted by "jaideeppyne (via GitHub)" <gi...@apache.org>.
jaideeppyne commented on PR #8205:
URL: https://github.com/apache/opendal/pull/8205#issuecomment-5478660707
Pushed a follow-up: `OpenDALOptionsTest.ListOptions` in the C++ binding set all five list options at once against `memory`, which only passed because the three unsupported ones were being dropped. Split it into the two options memory does support plus a new `ListOptionsUnsupportedByService` asserting the other three now throw, which also makes it a stronger plumbing test than before. All 83 C++ tests pass locally.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org