[#12927] fix(function): Reject unresolved data types in new definitions - #12928
Open
mchades wants to merge 2 commits into
Open
[#12927] fix(function): Reject unresolved data types in new definitions#12928mchades wants to merge 2 commits into
mchades wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is well-scoped, aligns with the stated contract split (read vs write), and is backed by targeted DTO/REST/core tests with only a minor error-message wording nit.
Pull request overview
This PR tightens function-metadata write validation so new function definitions can no longer persist unresolved UnparsedType (while preserving backward-compatible read behavior for legacy metadata). It also updates the OpenAPI contract to distinguish read-compatible DataType from write-only WritableDataType.
Changes:
- Add DTO-level validation that rejects
UnparsedType(recursively) and requires non-blankExternalType.catalogStringfor function register/addDefinition requests. - Extend REST and managed-metadata tests to cover legacy readability, selector compatibility, and 400 responses for invalid write inputs.
- Update OpenAPI schemas to introduce
WritableDataType/ writable function-definition shapes for write requests.
File summaries
| File | Description |
|---|---|
| server/src/test/java/org/apache/gravitino/server/web/rest/TestFunctionOperations.java | Adds REST tests for legacy read compatibility and 400 rejection of unparsed types on writes. |
| docs/open-api/functions.yaml | Updates function write request schemas to use writable function-definition types; documents 400 responses and legacy read behavior. |
| docs/open-api/datatype.yaml | Introduces WritableDataType and writable type variants excluding UnparsedType. |
| core/src/test/java/org/apache/gravitino/storage/relational/po/TestFunctionPO.java | Verifies legacy persisted type strings deserialize as UnparsedType and remain readable. |
| core/src/test/java/org/apache/gravitino/catalog/TestManagedFunctionOperations.java | Ensures selector-based management operations still work with legacy UnparsedType definitions. |
| common/src/test/java/org/apache/gravitino/dto/requests/TestFunctionUpdatesRequest.java | Validates indexed update-paths appear in addDefinition validation errors; confirms selectors allow legacy unparsed types. |
| common/src/test/java/org/apache/gravitino/dto/requests/TestFunctionUpdateRequest.java | Adds coverage for addDefinition rejecting UnparsedType and allowing selector requests with legacy types. |
| common/src/test/java/org/apache/gravitino/dto/requests/TestFunctionRegisterRequest.java | Adds coverage for rejecting unknown/unparsed/nested-unparsed types with field-paths; validates ExternalType acceptance. |
| common/src/test/java/org/apache/gravitino/dto/function/TestFunctionDTO.java | Confirms function DTO reads still accept legacy unresolved types (UnparsedType). |
| common/src/main/java/org/apache/gravitino/dto/requests/FunctionUpdatesRequest.java | Adjusts validation to include update-indexed field paths for addDefinition validation. |
| common/src/main/java/org/apache/gravitino/dto/requests/FunctionUpdateRequest.java | Routes addDefinition validation through the shared data-type validator with field-path support. |
| common/src/main/java/org/apache/gravitino/dto/requests/FunctionRegisterRequest.java | Adds per-definition field-paths and calls shared data-type validation for register requests. |
| common/src/main/java/org/apache/gravitino/dto/requests/FunctionDataTypeValidator.java | New shared validator that rejects UnparsedType on writes and validates nested/native/external types. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code Coverage Report
Files
|
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.
What changes were proposed in this pull request?
ExternalTypevalues.UnparsedTyperecursively with indexed field paths and HTTP 400 responses.WritableDataTypeOpenAPI schema for function write requests.Why are the changes needed?
Unknown strings and malformed type objects currently deserialize as
UnparsedTypeand can be persisted as new function metadata because request validation only checks for null.Part of #12927.
Does this PR introduce any user-facing change?
Yes. Function register and
AddDefinitionnow reject unknown, malformed, or explicitUnparsedTypeinputs with HTTP 400. Existing function responses and selector-based management operations remain backward compatible. Table APIs are unchanged.How was this patch tested?
TestFunctionPOTestManagedFunctionOperationsTestFunctionOperationsREST tests./gradlew spotlessApply./gradlew :docs:buildgit diff --check