AWS: Remove deprecated class and require SIGNER_ENDPOINT - #17627
AWS: Remove deprecated class and require SIGNER_ENDPOINT #17627dramaticlly wants to merge 2 commits into
Conversation
These five classes were deprecated for removal in 1.12.0 and are now orphaned: they reference only each other and have no remaining callers in main or test sources. S3V4RestSignerClient uses the RemoteSign* equivalents. - S3SignRequest, S3SignResponse - S3SignRequestParser, S3SignResponseParser - S3ObjectMapper
Removes the legacy signer properties deprecated for removal in 1.12.0,
along with the fallback logic the source already marked with
"TODO remove in 1.12.0":
- s3.signer.uri (S3_SIGNER_URI) -> RESTCatalogProperties.SIGNER_URI
- s3.signer.endpoint (S3_SIGNER_ENDPOINT) -> RESTCatalogProperties.SIGNER_ENDPOINT
- S3_SIGNER_DEFAULT_ENDPOINT ("v1/aws/s3/sign"), which had no replacement
RESTCatalogProperties.SIGNER_ENDPOINT is now required rather than
defaulting to "v1/aws/s3/sign", matching the existing
"TODO change to required in 1.12.0" in check().
This is a behavior change: remote signing configured without an explicit
signer endpoint previously logged a warning and used the default, and now
fails at construction. Added TestS3V4RestSignerClient.signerEndpointIsRequired
to cover it, and reworked the legacySignerProperties parameterized test into
signerUriResolution now that precedence between legacy and current
properties no longer exists.
The S3RestSigner integration test already sets SIGNER_ENDPOINT explicitly
and is unaffected.
| properties().containsKey(RESTCatalogProperties.SIGNER_ENDPOINT), | ||
| "S3 signer endpoint (%s) is required", | ||
| RESTCatalogProperties.SIGNER_ENDPOINT); | ||
| } |
There was a problem hiding this comment.
The change in this file is stricter than a pure deprecation-alias removal. Beyond stripping the s3.signer.uri/s3.signer.endpoint aliases, it also removes the working "v1/aws/s3/sign" implicit default (S3_SIGNER_DEFAULT_ENDPOINT), meaning users who set s3.remote-signing-enabled=true without ever configuring an explicit endpoint now fail at construction with IllegalArgumentException. The 1.11.0 LOG.warn, the @deprecated "no replacement, remove in 1.12.0" on S3_SIGNER_DEFAULT_ENDPOINT, and the updated SIGNER_ENDPOINT Javadoc ("If remote signing has been requested, this must be set") all point toward requiring the endpoint in 1.12.0
There was a problem hiding this comment.
Just leaving a note here, although I see this has already been noted in the description - thank you!
2 participants
1. Remove five orphaned classes
S3SignRequest,S3SignResponse,S3SignRequestParser,S3SignResponseParser,S3ObjectMapper.Zero risk: verified the only references to any of the five are from each other.
S3V4RestSignerClientalready uses theRemoteSign*equivalents and no test source touches them.2. Remove the deprecated property fallbacks in
S3V4RestSignerClients3.signer.uri(S3_SIGNER_URI) →RESTCatalogProperties.SIGNER_URIs3.signer.endpoint(S3_SIGNER_ENDPOINT) →RESTCatalogProperties.SIGNER_ENDPOINTS3_SIGNER_DEFAULT_ENDPOINT("v1/aws/s3/sign"), which had no replacementThis also resolves the three
// TODO remove in 1.12.0/// TODO change to required in 1.12.0markers already in the source.RESTCatalogProperties.SIGNER_ENDPOINTis now required instead of defaulting to"v1/aws/s3/sign". Remote signing configured without an explicit signer endpoint previously logged a warning and used the default; it now fails at construction withIllegalArgumentException.Concretely, anyone setting
s3.remote-signing-enabled=truewithoutrest.signer.endpointbreaks on upgrade. That is what thecheck()TODO scheduled, but it is stricter than a pure deprecation removal — if you would rather keep the default for one more release, I can drop therequiredhalf and keep only the legacy-property removal.Three existing tests needed
SIGNER_ENDPOINTadded for exactly this reason (TestS3FileIOProperties×2, which exerciseapplySignerConfiguration). TheS3RestSignerintegration test already sets it explicitly and is unaffected.New coverage:
TestS3V4RestSignerClient.signerEndpointIsRequired. ThelegacySignerPropertiesparameterized test becamesignerUriResolution, since precedence between legacy and current properties no longer exists.One thing to note:
endpoint()keeps aPreconditionscheck thatcheck()already enforces at construction, so it is unreachable in practice. I left it as defensive cover on a@Value.Lazyaccessor rather than reshape more code in a behavior-changing PR — happy to drop it if you prefer.Split out of #16449 to reduce reviewer burden. No
.palantir/revapi.ymlentries: theawsmodule is not covered by the revapi baseline.AI Disclosure
Model: Claude Opus 5 (1M context)
Platform/Tool: Claude Code
Human Oversight: reviewed
Prompt Summary: split #16449 into smaller self-contained PRs; verify each group compiles and tests green standalone