Core: Add explicit JSON parser for ConfigResponse #9952
Conversation
core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponseParser.java
Show resolved
Hide resolved
|
|
||
| gen.writeStartObject(); | ||
|
|
||
| JsonUtil.writeStringMap(DEFAULTS, response.defaults(), gen); |
There was a problem hiding this comment.
existing tests in TestConfigResponse expect the maps to be always written (even if empty), so I omitted having a isEmpty() check
There was a problem hiding this comment.
Should we just change this to not send back the key in the response in the value is null (makes for a cleaner response? I don't really think that's a behavior change since in the end a client needs to check that the value is not null.
There was a problem hiding this comment.
NVM these two fields are spec'd out so it seems like servers should always send it even though it may be null
https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L83
| ConfigResponse.Builder builder = ConfigResponse.builder(); | ||
|
|
||
| if (json.hasNonNull(DEFAULTS)) { | ||
| builder.withDefaults(JsonUtil.getStringMapNullableValues(DEFAULTS, json)); |
There was a problem hiding this comment.
this seems to specifically require a Map where the value can be null (see also #4184 (comment)) and there are also tests in TestConfigResponse that set values in a defaults/overrides map to null in order to be able to disable a particular configuration
| property, | ||
| pNode); | ||
|
|
||
| Map<String, String> map = Maps.newHashMap(); |
There was a problem hiding this comment.
can't use ImmutableMap here as it doesn't support null values
|
|
||
| gen.writeStartObject(); | ||
|
|
||
| JsonUtil.writeStringMap(DEFAULTS, response.defaults(), gen); |
There was a problem hiding this comment.
Should we just change this to not send back the key in the response in the value is null (makes for a cleaner response? I don't really think that's a behavior change since in the end a client needs to check that the value is not null.
|
|
||
| gen.writeStartObject(); | ||
|
|
||
| JsonUtil.writeStringMap(DEFAULTS, response.defaults(), gen); |
There was a problem hiding this comment.
NVM these two fields are spec'd out so it seems like servers should always send it even though it may be null
https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L83
|
thanks for the review @amogh-jahagirdar |
3 participants
This introduces an explicit JSON parser for
ConfigResponseas a preparation step for thecapabilitiesthat are being introduced by #9940.Currently,
ConfigResponseis relying on reflection to properly do JSON <-->ConfigResponse.Having an explicit JSON parser has the advantage that the underlying
capabilitiescan be parsed in a way that gives us more flexiblity and allows to be fully forward/backward compatible when new capabilities are being added.