[916] Correct three inaccuracies in the REST service spec - #917
Open
slachiewicz wants to merge 1 commit into
Open
[916] Correct three inaccuracies in the REST service spec#917slachiewicz wants to merge 1 commit into
slachiewicz wants to merge 1 commit into
Conversation
source-data-path was absent from ConvertTableRequest although the service reads it. It is not optional: ConversionService passes it to TargetTable.basePath, which ExternalTable declares @nonnull, with no fallback, so a request that omits it fails. Every test sets it, which is why the gap survived. Documenting it as required matches the service as written; defaulting it to source-table-path in ConversionService would be the other way to close this, and is a maintainer call. configurations claimed to carry storage credentials. Only "partition-spec" is read; every other key is discarded. The service authenticates to cloud storage from its own environment, and a request body is logged, traced and cached, so the description now says credentials do not belong there. ErrorModel.stack offered stack traces to callers. Nothing populates it -- there is no ErrorModel class and no ExceptionMapper -- and a stack trace in a public response schema is an information leak, so it is dropped rather than left as a standing invitation to implement. info.version moves to 0.0.2, since a required request field and a removed response field are both contract changes. Verified: cd spec && make lint -> rest-service-open-api.yaml: OK.
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
1 participant
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.
What is the purpose of the pull request
Closes #916. Three places where
rest-service-open-api.yamldescribes something other than whatxtable-servicedoes. The first is the one worth reviewing: a client that follows the spec aspublished sends a request the service rejects.
Brief change log
source-data-pathtoConvertTableRequest, as required.ConversionServicepasses it toTargetTable.basePath, whichExternalTabledeclares@NonNull, with no fallback -- so the fourdocumented required fields are not a valid request. A request that follows the published REST spec fails: source-data-path is required but undocumented #916 covers the alternative fix, defaulting
it in
ConversionService; this PR takes the spec-only route.configurationsdescription. Onlypartition-specis read(
ConversionService.java:195); every other key is discarded. It no longer advertises itself as aplace for storage credentials, which the service does not read from there and which a request body
should not carry -- bodies are logged, traced and cached.
ErrorModel.stack. Nothing populates it: there is noErrorModelclass and noExceptionMapperin the module. Removing it now is cheaper than removing a stack trace thatsomeone has since implemented.
info.versionto 0.0.2, since a newly required request field and a removed response fieldare both contract changes. Happy to drop this hunk if you version the spec on a different cadence.
Verify this pull request
This pull request is a trivial rework / code cleanup without any test coverage.
cd spec && make lint→rest-service-open-api.yaml: OK. No Java file is touched, so behaviour isunchanged; what changes is whether the contract matches it.
This change was created with AI assistance.