Add JSON Payload coverage tests for REST endpoints #1890

jcarranzan · 2024-07-17T09:25:31Z

Summary

This PR adds coverage tests to the http/http-advanced-reactive Quarkus module to ensure proper handling of JSON data in REST endpoints. Test scenarios include:

Successful processing
Large, complex payloads
Invalid JSON files
Missing or incorrect data types
Verify appropriate errors handling

For more detailed info see the test plan for details: PR-179

Please select the relevant options.

Bug fix (non-breaking change which fixes an issue)
Dependency update
Refactoring
Backport
New scenario (non-breaking change which adds functionality)
This change requires a documentation update
This change requires execution against OCP (use run tests phrase in comment)

Checklist:

Methods and classes used in PR scenarios are meaningful
Commits are well encapsulated and follow the best practices

jedla97

I have looked at this and have some comments what can be improved.

...vanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/FootballTeamResource.java

...http-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/JsonPayloadIT.java

jedla97 · 2024-07-17T17:55:45Z

I also looked on the TP and see that for large JSON you want to use POST and GET. Will be GET method added or should be TP updated?

jedla97 · 2024-07-17T19:10:10Z

Also one more question. In TP you state you want test it with quarkus-rest-jackson and quarkus-rest-jsonb how you want to test it with both? Or what's the plan?

jcarranzan · 2024-07-19T08:32:00Z

I also looked on the TP and see that for large JSON you want to use POST and GET. Will be GET method added or should be TP updated?

Currently is added to the PersonResource and it's used, thanks.

jcarranzan · 2024-07-19T08:41:16Z

Also one more question. In TP you state you want test it with quarkus-rest-jackson and quarkus-rest-jsonb how you want to test it with both? Or what's the plan?

I've used the approach we talked about, so currently it's tested with both libraries. Thanks.

jcarranzan · 2024-07-19T13:48:43Z

run tests

jedla97

I review it and it's look good. Just have few small suggestion.

...vanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/FootballTeamResource.java

...ttp-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/JsonBPayloadIT.java

...vanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/JsonJacksonPayloadIT.java

...nced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/OpenShiftJsonPayloadIT.java

...http-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/JsonPayloadIT.java

jedla97 · 2024-07-19T21:13:27Z

Also jsut a note before merge I would like to see full CI run. ATM it's blocked because problem with releasing new framework version.

...ttp-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/JsonBPayloadIT.java

jedla97

I add final comment.
Also can you rebase it to main as the latest framework bump was merged. So we can see tests.

...vanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/JsonJacksonPayloadIT.java

...nced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/OpenShiftJsonPayloadIT.java

jedla97

Thanks for the update. It's look good to me. Hopefully CI will be fine!

jcarranzan · 2024-07-24T07:55:33Z

run tests

jedla97 · 2024-07-24T07:55:42Z

run tests

michalvavrik

@mjurc can you look at the FootballTeamResource and PersonResource and tell me if this implements quarkus-qe/quarkus-test-plans#179 ?

Because if so, IMHO the test plan was wrong.

My opinion:

this is using Jackson library (not an extension!) everywhere, newly added tests (maybe with consumes/produces annotations) would work without rest-jackson and rest-jsonb extensions anyway
it is instantiating ObjectMapper instead of using managed one
it serializes and deserializes ~~everything~~ part of the things manually
so when test plan says https://github.com/quarkus-qe/quarkus-test-plans/blob/main/QQE-379.md?plain=1#L40 that you expect 400 for invalid paylod, well, these resources set 400 response for invalid payload, so of course you are going to receive it? what exactly is it testing, that Response object works?
I expect users to actually use message writers/readers instead of doing this manually, if we want to test com.fasterxml.jackson then it should be clear this is what these tests are about, not something like JsonBPayloadIT etc.

michalvavrik · 2024-07-24T18:44:40Z

...vanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/JsonJacksonPayloadIT.java

+
+    @Test
+    public void shouldPickJackson() {
+        assertTrue(app.logs().forQuarkus().installedFeatures().contains(REST_JACKSON));


Isn't shouldPickJackson testing that rest-jackson feature is present? If you compare logs of JsonJacksonPayloadIT:

2024-07-24T08:07:28.0863296Z 08:07:28,083 INFO [app] 08:07:26,210 Installed features: [cdi, keycloak-authorization, micrometer, oidc, reactive-routes, rest, rest-client, rest-jackson, rest-jaxb, rest-jsonb, security, smallrye-context-propagation, smallrye-health, smallrye-openapi, vertx]

with JsonBPayloadIT logs:

2024-07-24T08:07:19.4630743Z 08:07:19,461 INFO [app] 08:07:17,767 Installed features: [cdi, keycloak-authorization, micrometer, oidc, reactive-routes, rest, rest-client, rest-jaxb, rest-jsonb, security, smallrye-context-propagation, smallrye-health, smallrye-openapi, vertx]

but both of these tests have rest-jsonbfeature present as well.

Based on what can you determine that these 2 tests are using different serializers? Let alone that you are not actually using these extensions to serialize/deserialize payload, but that's a different story.

When reviewing this I didn't realized that @Dependency(artifactId = "quarkus-" + REST_JACKSON) add it but the extension defined in pom stay there as well (probably that I was looking at GreetingResourceIT and didn't look at the pom there).

Based on what can you determine that these 2 tests are using different serializers?

Now I can say we can't say for sure. As I described above I was thinking that there is only that one extension.

I think it still can be sure that Quarkus REST Jackson serializers have a higher priority, but I simply don't know it and we don't assert it, therefore we need a way to assert it. Thank you

michalvavrik · 2024-07-24T19:01:37Z

...vanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/FootballTeamResource.java

+            return Uni.createFrom()
+                    .item(() -> {
+                        Map<String, Object> responseMap = new HashMap<>();
+                        responseMap.put("teams", footballTeamSet);


here I can see object that is going to be serialized, I missed that one before and I am going to correct my comment above. @jedla97 @jcarranzan how do you know whether Jackson / JsonB serializers are going to be used?

michalvavrik · 2024-07-24T19:03:58Z

...ttp-advanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/PersonResource.java

+        if (personSet.isEmpty()) {
+            return Uni.createFrom().item(Response.status(Response.Status.NOT_FOUND).build());
+        } else {
+            return Uni.createFrom().item(Response.ok(personSet).build());


@jedla97 @jcarranzan I also missed this one, apologizes. How do you know which serializer if going to be used? Jackson or JsonB?

michalvavrik · 2024-07-24T19:06:53Z

...ttp-advanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/PersonResource.java

+    @POST
+    @Consumes(MediaType.APPLICATION_JSON)
+    @Produces(MediaType.APPLICATION_JSON)
+    public Uni<Response> processJson(String jsonData) {


@jcarranzan @jedla97 TP https://github.com/quarkus-qe/quarkus-test-plans/blob/main/QQE-379.md?plain=1#L31 says Deserialization: Ensure accurate conversion of JSON payloads, that it's performed using the configured JSON libraries (quarkus-rest-jackson, quarkus-rest-jsonb).

I don't see any deserialization performed by quarkus-rest-jackson or quarkus-rest-jsonb. AFAICT it is always performed manually. Am I wrong?

michalvavrik · 2024-07-24T19:10:47Z

@jcarranzan @jedla97 I have corrected my original comment as I can see that serialization is partially tested, I apologize. Still, I am unhappy with this PR. Please have a look at my comments and answer. I suppose it can be determined from writers/readers priority which one is picked, but you actually don't test that. I think the way to test this is to actually fail serialization/deserialization and inspect stacktrace in the exception mapper.

michalvavrik · 2024-07-24T19:20:59Z

...http-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/JsonPayloadIT.java

+    }
+
+    @Test
+    public void sendSpecialCharactersJSONPayload() throws Exception {


This test is sending plain text (it is a String) via client, received String in REST endpoint. Special characters or not, you are testing your own instance of ObjectMapper.

As far as extensions deserializers are concerned - none used
As far as extension serializers are concerned - teams should be serialized, but that is not tested at all, what is tested is that text containes Teams added successfully. So you are not testing whether special characters are handled properly by serializers, but you are testing that no exception is raised. If there was any encoding issue, you wouldn't know.

michalvavrik · 2024-07-24T19:31:11Z

...vanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/FootballTeamResource.java

+/**
+ *
+ * When a JSON extension is installed such as quarkus-rest-jackson or quarkus-rest-jsonb
+ * Quarkus will use the application/json media type by default for most return values


This comment is true https://quarkus.io/guides/rest#declaring-endpoints-representation-content-types however I can't see relation to this resource as there is just one endpoint that consumes/produces something else than HTTP status (no entity), that is uploadFootballJson and there, you explicitly delcare these annotations.

So, why to state what is default if you override it and don't test it?

mjurc · 2024-07-24T19:32:41Z

@mjurc can you look at the FootballTeamResource and PersonResource and tell me if this implements quarkus-qe/quarkus-test-plans#179 ?

Because if so, IMHO the test plan was wrong.

My opinion:

* this is using Jackson library (not an extension!) everywhere, newly added tests (maybe with consumes/produces annotations) would work without rest-jackson and rest-jsonb extensions anyway

* it is instantiating `ObjectMapper` instead of using managed one

* it serializes and deserializes ~everything~ part of the things  manually

* so when test plan says https://github.com/quarkus-qe/quarkus-test-plans/blob/main/QQE-379.md?plain=1#L40 that you expect 400 for invalid paylod, well, these resources set 400 response for invalid payload, so of course you are going to receive it? what exactly is it testing, that `Response` object works?

* I expect users to actually use message writers/readers instead of doing this manually, if we want to test `com.fasterxml.jackson` then it should be clear this is what these tests are about, not something like `JsonBPayloadIT` etc.

This is definitely a correct assessment, as in we don't support standalone libs added to Quarkus. This is not an extension test. We need to add integration tests rather than unit tests for the library.

jcarranzan · 2024-07-25T08:19:28Z

Ok, thank you @michalvavrik for your question points, I'll do a refactor of these tests.

michalvavrik · 2024-07-25T08:20:57Z

Ok, thank you @michalvavrik for your question points, I'll do a refactor of these tests.

Cool, let's give me 10 more minutes, I'll add few comments that you can consider. It is optional.

michalvavrik · 2024-07-25T08:27:19Z

...vanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/FootballTeamResource.java

+
+    private static final Logger LOG = Logger.getLogger(FootballTeamResource.class);
+
+    private Set<FootballTeam> footballTeamSet = Collections.synchronizedSet(new LinkedHashSet<>());


flow is:

String data is deserialized with Set<FootballTeam> teamSet = objectMapper.readValue(data, new TypeReference<Set<FootballTeam>>() {});

you have Set<FootballTeam> teamSet

then you do validation (that part is ok)

then you copy it into a class member synchronized set

then you use the synchronized set to return the response

then you have separate call that deletes a football team set

Things that I don't understand:

it's synchronized but there is no concurrency, you never call it more than once at time

you copy teamSet into footballTeamSet that is kept outside of /upload-football-json, but you never use it; instead you only use it to "delete it"; what is the point of copying it into the footballTeamSet

you need to manage footballTeamSet but it's not useful for you

michalvavrik · 2024-07-25T08:33:40Z

...vanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/FootballTeamResource.java

+        } catch (JsonProcessingException e) {
+            LOG.error(e.getMessage());
+            return Uni.createFrom().item(
+                    Response.status(400).entity("Invalid JSON - JsonProcessingException : " + e.getOriginalMessage()).build());


IMO your intention is to see that Quarkus responds with some response status. I say some, because I suspect that it is going to be 500 unless you customize that exception. For which reason, I think when test plan says HTTP Status Code: Expect 400 Bad request for invalid payloads. that is wrong expectation.

The point is, you know that you are able to set a response status based on business logic (like invalid argument), but this PR is verifying JSON payload handling and that is different then testing feature that allows you to set a response status.

michalvavrik · 2024-07-25T08:35:46Z

http/http-advanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/Person.java

+        this.favoriteFruit = favoriteFruit;
+    }
+
+    public String get_id() {


if you verify a JSON payload, you are probably interested in commonly used features like ignored field etc. Anyway, that's a side note. Why is this called get_id? I wonder into which field is it going to be turned by a serializer. Is it really going to be id field? You might want to check.

michalvavrik · 2024-07-25T08:38:13Z

...http-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/JsonPayloadIT.java

+                .post("/football/upload-football-json")
+                .then()
+                .statusCode(CREATION_STATUS_CODE)
+                .body(containsString("Teams added successfully"));


None of tests in this test class is testing that response was serialized correctly. You don't test that JSON response is valid, just that response contains some string. Additionally,if tests are sending special chars, you might want to check that they are serialized/deserialized correctly.

michalvavrik · 2024-07-25T08:39:25Z

Done, I don't have more comments, you can start working on this. Please make sure that @jedla97 is a reviwer as he knows requirements best and @mjurc is OOO. Thank you

jcarranzan force-pushed the QQE-379-JSON-payloads branch from 1472260 to 9e807be Compare July 17, 2024 11:05

jcarranzan requested a review from jedla97 July 17, 2024 11:08

jcarranzan force-pushed the QQE-379-JSON-payloads branch from df1bf77 to 50612a1 Compare July 17, 2024 11:35

jedla97 requested changes Jul 17, 2024

View reviewed changes

jcarranzan force-pushed the QQE-379-JSON-payloads branch 4 times, most recently from 633647c to 9d1c50e Compare July 19, 2024 06:45

jcarranzan requested a review from jedla97 July 19, 2024 06:46

jcarranzan marked this pull request as ready for review July 19, 2024 12:45

jcarranzan force-pushed the QQE-379-JSON-payloads branch 3 times, most recently from 9d432f9 to dd80888 Compare July 19, 2024 13:26

jedla97 requested changes Jul 19, 2024

View reviewed changes

jcarranzan force-pushed the QQE-379-JSON-payloads branch from 45f0aff to 5697363 Compare July 20, 2024 07:14

jedla97 reviewed Jul 23, 2024

View reviewed changes

...ttp-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/JsonBPayloadIT.java Outdated Show resolved Hide resolved

jedla97 requested changes Jul 23, 2024

View reviewed changes

...vanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/JsonJacksonPayloadIT.java Outdated Show resolved Hide resolved

...nced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/OpenShiftJsonPayloadIT.java Show resolved Hide resolved

jcarranzan force-pushed the QQE-379-JSON-payloads branch 2 times, most recently from 9e3a389 to 15e5924 Compare July 24, 2024 06:08

Add JsonPayload coverage tests

9fd51dd

jcarranzan force-pushed the QQE-379-JSON-payloads branch from 1cdb27e to 9fd51dd Compare July 24, 2024 07:31

jedla97 approved these changes Jul 24, 2024

View reviewed changes

jedla97 merged commit ca22d18 into quarkus-qe:main Jul 24, 2024
10 checks passed

michalvavrik reviewed Jul 24, 2024

View reviewed changes

michalvavrik reviewed Jul 25, 2024

View reviewed changes

jcarranzan mentioned this pull request Jul 25, 2024

Refactor of QQE-379 JSON payloads #1907

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JSON Payload coverage tests for REST endpoints #1890

Add JSON Payload coverage tests for REST endpoints #1890

jcarranzan commented Jul 17, 2024 •

edited

Loading

jedla97 left a comment

jedla97 commented Jul 17, 2024

jedla97 commented Jul 17, 2024

jcarranzan commented Jul 19, 2024

jcarranzan commented Jul 19, 2024

jcarranzan commented Jul 19, 2024

jedla97 left a comment

jedla97 commented Jul 19, 2024

jedla97 left a comment

jedla97 left a comment

jcarranzan commented Jul 24, 2024

jedla97 commented Jul 24, 2024

michalvavrik left a comment •

edited

Loading

michalvavrik Jul 24, 2024 •

edited

Loading

jedla97 Jul 25, 2024 •

edited

Loading

michalvavrik Jul 25, 2024

michalvavrik Jul 24, 2024

michalvavrik Jul 24, 2024 •

edited

Loading

michalvavrik Jul 24, 2024

michalvavrik commented Jul 24, 2024

michalvavrik Jul 24, 2024

michalvavrik Jul 24, 2024

mjurc commented Jul 24, 2024

jcarranzan commented Jul 25, 2024

michalvavrik commented Jul 25, 2024

michalvavrik Jul 25, 2024

michalvavrik Jul 25, 2024

michalvavrik Jul 25, 2024

michalvavrik Jul 25, 2024

michalvavrik commented Jul 25, 2024


		private static final Logger LOG = Logger.getLogger(FootballTeamResource.class);

		private Set<FootballTeam> footballTeamSet = Collections.synchronizedSet(new LinkedHashSet<>());

Add JSON Payload coverage tests for REST endpoints #1890

Add JSON Payload coverage tests for REST endpoints #1890

Conversation

jcarranzan commented Jul 17, 2024 • edited Loading

Summary

Checklist:

jedla97 left a comment

Choose a reason for hiding this comment

jedla97 commented Jul 17, 2024

jedla97 commented Jul 17, 2024

jcarranzan commented Jul 19, 2024

jcarranzan commented Jul 19, 2024

jcarranzan commented Jul 19, 2024

jedla97 left a comment

Choose a reason for hiding this comment

jedla97 commented Jul 19, 2024

jedla97 left a comment

Choose a reason for hiding this comment

jedla97 left a comment

Choose a reason for hiding this comment

jcarranzan commented Jul 24, 2024

jedla97 commented Jul 24, 2024

michalvavrik left a comment • edited Loading

Choose a reason for hiding this comment

michalvavrik Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

jedla97 Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalvavrik Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalvavrik commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjurc commented Jul 24, 2024

jcarranzan commented Jul 25, 2024

michalvavrik commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalvavrik commented Jul 25, 2024

jcarranzan commented Jul 17, 2024 •

edited

Loading

michalvavrik left a comment •

edited

Loading

michalvavrik Jul 24, 2024 •

edited

Loading

jedla97 Jul 25, 2024 •

edited

Loading

michalvavrik Jul 24, 2024 •

edited

Loading