Core: Tolerate explicit null config in LoadViewResponseParser#17250
Open
thswlsqls wants to merge 2 commits into
Open
Core: Tolerate explicit null config in LoadViewResponseParser#17250thswlsqls wants to merge 2 commits into
thswlsqls wants to merge 2 commits into
Conversation
A REST server may send "config": null in a load view response. The reader used json.has(CONFIG), so a null value fell through to getStringMap and threw IllegalArgumentException. Use json.hasNonNull to match the sibling LoadTableResponseParser, which already skips a null config and yields an empty config map. Generated-by: Claude Code
Contributor
|
Thanks for spotting this, but I think we have a few more places that have a similar pattern that we should fix too:
|
The same has() + non-null getter pattern that broke LoadViewResponseParser on an explicit "config": null exists in other REST parsers. JsonUtil.getString, getBool, getStringMap and JsonUtil.get all reject a NullNode, while has() only checks that the key is present, so an explicit null throws instead of falling back to the absent-field behavior. Switch to hasNonNull() for the optional fields where treating an explicit null as absent matches the field's documented default: CreateViewRequestParser properties RemoteSignRequestParser properties, body, provider PlanTableScanRequestParser case-sensitive, use-snapshot-schema TableScanResponseParser delete-files, file-scan-tasks RESTFileScanTaskParser delete-file-references OAuth2Util expires_in, scope The filter fields (PlanTableScanRequestParser.filter and RESTFileScanTaskParser.residual-filter) are left alone: apache#14657 added testFilterFieldWithExplicitNullThrowsError to pin the throw when aligning the field with the OpenAPI spec, and silently reading a null filter as "no filter" would turn a malformed request into a full scan rather than an error. Each parser gets a test covering the explicit-null input. Generated-by: Claude Code
Contributor
Author
|
Fixed all of them except the two filter fields. I left |
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
2 participants
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.
Closes #17249
Summary
LoadViewResponseParser.fromJson()guards the optionalconfigfield withjson.has(CONFIG), so an explicit"config": nullfrom a REST server reachesJsonUtil.getStringMapand throwsIllegalArgumentException.json.hasNonNull(CONFIG), which treats a null config as absent and yields an empty config.LoadTableResponseParser.fromJson()(line 96), which already useshasNonNullfor the same optionalconfigmap; both parsers writeconfigonly when non-empty.Testing done
TestLoadViewResponseParser#nullConfigasserting"config": nullparses to an empty config (red before the fix, green after)../gradlew :iceberg-core:test --tests "*TestLoadViewResponseParser*" :iceberg-core:spotlessCheck :iceberg-core:checkstyleMain :iceberg-core:checkstyleTest :iceberg-core:revapi— all passed.:iceberg-core:checknot run.