feat(s3s): add capability mechanism for opt-in semantics#533
Open
LeonWang0735 wants to merge 3 commits intos3s-project:mainfrom
Open
feat(s3s): add capability mechanism for opt-in semantics#533LeonWang0735 wants to merge 3 commits intos3s-project:mainfrom
LeonWang0735 wants to merge 3 commits intos3s-project:mainfrom
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a capability mechanism for s3s that allows S3 backends to explicitly declare which optional semantics they support (issue #475). When a request requires capabilities the backend hasn't declared, s3s returns a 501 NotImplemented error instead of silently ignoring the feature.
Changes:
- Adds a new
capabilitymodule withCapabilityenum (#[non_exhaustive], currently only__Test),Capabilitiesset wrapper, and acheck()function. - Adds codegen to emit
required_capabilities(input)stubs and capability enforcement in every operation'scall()method. - Adds
capabilities()default method to theS3trait, plus re-exports inlib.rs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/s3s/src/capability.rs |
New module: Capability enum, Capabilities set, check() function, and unit tests |
crates/s3s/src/lib.rs |
Registers capability as a public module and re-exports Capabilities/Capability |
crates/s3s/src/s3_trait.rs |
Generated: adds capabilities() default method to S3 trait |
codegen/src/v1/s3_trait.rs |
Codegen for capabilities() method in S3 trait |
codegen/src/v1/ops.rs |
Codegen for required_capabilities() stub and capability check in call() |
crates/s3s/src/ops/generated.rs |
Generated: required_capabilities + capability check for all ops |
crates/s3s/src/ops/generated_minio.rs |
Generated: same as above for MinIO variant |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of Change
Related Issues
FEAT #475
Summary of Changes
Capability mechanism (opt-in semantics)
#[non_exhaustive]). Currently only has a#[doc(hidden)] __Testvariant for tests; real capabilities can be added later.BTreeSet<Capability>withempty(),with(),contains(),missing(),check(required, supported): Returns NotImplemented if supported does not include every capability in required.S3 trait: Default fn capabilities(&self) -> Capabilities { Capabilities::empty() }.In
Operation::call(), after deserializing the request and before calling the S3 impl, the generated code runsif !required.is_empty() { capability::check(&required,&ccx.s3.capabilities())? }.Testing issues for this feature
Using a real capability for integration tests would break existing backends
If codegen gated something like part_number with a real capability (e.g. GetObjectPartNumber), any existing backend that does not declare that capability would get 501 for requests that include partNumber, which is a breaking change. So I do not gate existing fields just to make integration tests possible.
Using __Test plus a field recreates the same trade-off
I briefly gated part_number with Capability::__Test to restore the five integration tests. That made the tests pass but had the same effect as gating a real capability: requests with partNumber still returned 501 for backends that do not declare __Test. Reverting that kept backward compatibility but removed the only way to trigger the check on the real call path in tests.
Current test coverage
Only the four unit tests in capability.rs exercise check() (empty required, missing capability, declared capability, superset). The codegen-injected required_capabilities and check call are not exercised by any integration test; their shape is validated by inspection. When a real capability is added and a field is gated in codegen, integration tests can be added for that path.