Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add JSON Payload coverage tests for REST endpoints #1890

Merged
merged 1 commit into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package io.quarkus.ts.http.advanced.reactive;

import java.util.Date;
import java.util.List;

import io.quarkus.runtime.annotations.RegisterForReflection;

@RegisterForReflection
public class FootballTeam {

private String name;
private String colorTShirt;
private int euroCups;
private Date foundationDate;
private String stadium;
private List<String> keyPlayers;

public FootballTeam() {
}

public FootballTeam(String name, String colorTShirt, int euroCups, Date foundationDate, String stadium,
List<String> keyPlayers) {
this.name = name;
this.colorTShirt = colorTShirt;
this.euroCups = euroCups;
this.foundationDate = foundationDate;
this.stadium = stadium;
this.keyPlayers = keyPlayers;
}

public Date getFoundationDate() {
return foundationDate;
}

public void setFoundationDate(Date foundationDate) {
this.foundationDate = foundationDate;
}

public String getStadium() {
return stadium;
}

public void setStadium(String stadium) {
this.stadium = stadium;
}

public List<String> getKeyPlayers() {
return keyPlayers;
}

public void setKeyPlayers(List<String> keyPlayers) {
this.keyPlayers = keyPlayers;
}

public String getName() {
return name;
}

public void setName(String name) {
this.name = name;
}

public String getColorTShirt() {
return colorTShirt;
}

public void setColorTShirt(String colorTShirt) {
this.colorTShirt = colorTShirt;
}

public int getEuroCups() {
return euroCups;
}

public void setEuroCups(int euroCups) {
this.euroCups = euroCups;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package io.quarkus.ts.http.advanced.reactive;

import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;

import jakarta.ws.rs.Consumes;
import jakarta.ws.rs.DELETE;
import jakarta.ws.rs.POST;
import jakarta.ws.rs.Path;
import jakarta.ws.rs.Produces;
import jakarta.ws.rs.core.MediaType;
import jakarta.ws.rs.core.Response;

import org.jboss.logging.Logger;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.ObjectMapper;

import io.smallrye.mutiny.Uni;

/**
*
* 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
Copy link
Member

Choose a reason for hiding this comment

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

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?

*/
@Path("/football")
public class FootballTeamResource {

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

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

Choose a reason for hiding this comment

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

flow is:

  1. String data is deserialized with Set<FootballTeam> teamSet = objectMapper.readValue(data, new TypeReference<Set<FootballTeam>>() {});
  2. you have Set<FootballTeam> teamSet
  3. then you do validation (that part is ok)
  4. then you copy it into a class member synchronized set
  5. then you use the synchronized set to return the response
  6. 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


@POST
@Path("/upload-football-json")
@Consumes(MediaType.APPLICATION_JSON)
@Produces(MediaType.APPLICATION_JSON)
public Uni<Response> uploadFootballJson(String data) {
try {
ObjectMapper objectMapper = new ObjectMapper();
Set<FootballTeam> teamSet = objectMapper.readValue(data, new TypeReference<Set<FootballTeam>>() {
});
jedla97 marked this conversation as resolved.
Show resolved Hide resolved
jcarranzan marked this conversation as resolved.
Show resolved Hide resolved
for (FootballTeam footballTeam : teamSet) {
if (footballTeam == null || footballTeam.getName().isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be nice to comment why there is only check for footballTeam.getName().isEmpty() and not other attributes. I found it out but possibly when someone fixing the tests, it can be faster to read that it's to test missing value in football-empty-teams.json. (First time going through that file I miss it.)

return Uni.createFrom().item(Response.status(400).entity("Some fields are empty or null").build());
} else {
footballTeamSet.add(footballTeam);
}
}
return Uni.createFrom()
.item(() -> {
Map<String, Object> responseMap = new HashMap<>();
responseMap.put("teams", footballTeamSet);
Copy link
Member

Choose a reason for hiding this comment

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

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?

responseMap.put("message", "Teams added successfully");
return Response.status(201).entity(responseMap).build();
});
} catch (JsonMappingException ex) {
LOG.error(ex.getMessage());
return Uni.createFrom()
.item(Response.status(400).entity("JsonMappingException : " + ex.getMessage()).build());
} catch (JsonProcessingException e) {
LOG.error(e.getMessage());
return Uni.createFrom().item(
Response.status(400).entity("Invalid JSON - JsonProcessingException : " + e.getOriginalMessage()).build());
Copy link
Member

Choose a reason for hiding this comment

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

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.

}

}

@DELETE
@Path("/clear")
public Response clearTeams() {
footballTeamSet.clear();
return Response.ok().build();
}

}
Loading