You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/04/26 23:32:25 UTC
[GitHub] [iceberg] kbendick opened a new pull request, #4636: Core - Use singleton RESTObjectMapper in REST object test base class
kbendick opened a new pull request, #4636:
URL: https://github.com/apache/iceberg/pull/4636
There is now a global singleton object mapper for the `RESCatalog`, which already has the necessary serializers and deserializers registered.
The rest / response objects are all tested via the `RequestResponseTestBase` class, which currently repeats the object mapper instantiation and initialization logic that the `RESTObjectMapper` is ultimately responsible for.
We should be testing the rest objects using the correct object mapper (which should also reduce allocations during testing and other minor advantages).
cc @rdblue @danielcweeks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4636: Core - Use singleton RESTObjectMapper in REST RequestResponseTestBase class
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4636:
URL: https://github.com/apache/iceberg/pull/4636#discussion_r859319292
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestCreateNamespaceResponse.java:
##########
@@ -101,7 +101,8 @@ public void testDeserializeInvalidResponse() {
String jsonMisspelledKeys = "{\"namepsace\":[\"accounting\",\"tax\"],\"propertiezzzz\":{\"owner\":\"Hank\"}}";
AssertHelpers.assertThrows(
"A JSON response with the keys spelled incorrectly should fail to deserialize and validate",
- JsonProcessingException.class,
+ IllegalArgumentException.class,
+ "Invalid namespace: null",
Review Comment:
Because `RESTObjectMapper` will ignore unknown fields, the exception thrown in these tests where the keys are misspelled turn out to be more specific.
I've updated them to use the proper thrown exception type, as well as add part of the exception message to help clarify the tests purpose and intent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks merged pull request #4636: Core - Use singleton RESTObjectMapper in REST RequestResponseTestBase class
Posted by GitBox <gi...@apache.org>.
danielcweeks merged PR #4636:
URL: https://github.com/apache/iceberg/pull/4636
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4636: Core - Use singleton RESTObjectMapper in REST RequestResponseTestBase class
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4636:
URL: https://github.com/apache/iceberg/pull/4636#discussion_r859320103
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestRESTCatalogConfigResponse.java:
##########
@@ -156,14 +156,6 @@ public void testDeserializeInvalidResponse() {
() -> deserialize(jsonOverridesHasWrongType)
);
- String jsonMisspelledKeys =
- "{\"defaultzzzzzzz\":{\"warehouse\":\"s3://bucket/warehouse\"},\"overrrrrrrrides\":{\"clients\":\"5\"}}";
- AssertHelpers.assertThrows(
- "A JSON response with the keys spelled incorrectly should fail to deserialize and validate",
- JsonProcessingException.class,
- () -> deserialize(jsonMisspelledKeys)
- );
Review Comment:
Because the RESTObjectMapper does not fail on unknown keys, this test is no longer valid.
We might consider adding a warning log in the config response's `validate` method to log if `null` was received for both (not an explicit empty collection, but null).
In practice, users should be using the config response class and its associated builder, which cannot build an incorrect payload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org