Skip to content

Core - Use singleton RESTObjectMapper in REST RequestResponseTestBase class#4636

Merged
danielcweeks merged 3 commits intoapache:masterfrom
kbendick:use-rest-object-mapper-in-reqresp-test-base
Apr 27, 2022
Merged

Core - Use singleton RESTObjectMapper in REST RequestResponseTestBase class#4636
danielcweeks merged 3 commits intoapache:masterfrom
kbendick:use-rest-object-mapper-in-reqresp-test-base

Conversation

@kbendick
Copy link
Contributor

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

@github-actions github-actions bot added the core label Apr 26, 2022
@kbendick kbendick changed the title Core - Use singleton RESTObjectMapper in REST object test base class Core - Use singleton RESTObjectMapper in REST RequestResponseTestBase class Apr 26, 2022
Copy link
Contributor

@danielcweeks danielcweeks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (just waiting on checks)

"A JSON response with the keys spelled incorrectly should fail to deserialize and validate",
JsonProcessingException.class,
IllegalArgumentException.class,
"Invalid namespace: null",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"A JSON response with the keys spelled incorrectly should fail to deserialize and validate",
JsonProcessingException.class,
() -> deserialize(jsonMisspelledKeys)
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

…ct mapper is currently permissive - all but one of these cases fail validation anyways
@kbendick kbendick force-pushed the use-rest-object-mapper-in-reqresp-test-base branch from 08729fe to e80c5de Compare April 27, 2022 02:26
@danielcweeks danielcweeks merged commit cba9785 into apache:master Apr 27, 2022
@kbendick kbendick deleted the use-rest-object-mapper-in-reqresp-test-base branch April 27, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants