Skip to content

Commit

Permalink
Development: Unify Json usage (#7457)
Browse files Browse the repository at this point in the history
  • Loading branch information
basak-akan authored Oct 29, 2023
1 parent babc1bc commit 5eb76d0
Show file tree
Hide file tree
Showing 16 changed files with 119 additions and 93 deletions.
7 changes: 7 additions & 0 deletions docs/dev/guidelines/server.rst
Original file line number Diff line number Diff line change
Expand Up @@ -648,3 +648,10 @@ Now, instead of mocking the whole Service, we can just mock the static method, l
You should notice here that we can avoid the use of a Bean and also test deeper. Instead of mocking the uppermost method we only throw the exception at the place where it could actually happen. Very important to mention is that you need to close the mock at the end of the test again.

For a real example where a SpyBean was replaced with a static mock look at the SubmissionExportIntegrationTest.java in `here <https://github.com/ls1intum/Artemis/commit/4843137aa01cfdf27ea019400c48df00df36ed45>`__.

27. Avoid Arbitrary JSON Parsing Libraries
==========================================

* Utilize well-established JSON parsing libraries such as Gson or Jackson.
* Whenever possible, use ObjectMapper for JSON parsing. If ObjectMapper is not suitable for the specific task, use to Gson's JsonParser.
* By adhering to these points, we ensure to maintain a consistent and reliable approach to JSON parsing. This helps reduce potential maintenance challenges and compatibility issues.
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@

import org.springframework.security.oauth2.core.oidc.OidcIdToken;

import com.nimbusds.jose.shaded.json.JSONArray;
import com.nimbusds.jose.shaded.json.JSONObject;
import com.google.gson.*;

/**
* A wrapper class for an LTI 1.3 Assignment and Grading Services Claim. We support the Score Publishing Service in order to transmit scores.
Expand All @@ -23,23 +22,24 @@ public class Lti13AgsClaim {
* @return an Ags-Claim if one was present in idToken.
*/
public static Optional<Lti13AgsClaim> from(OidcIdToken idToken) {
JSONObject agsClaimJson = idToken.getClaim(Claims.AGS_CLAIM);
if (agsClaimJson == null) {
if (idToken.getClaim(Claims.AGS_CLAIM) == null) {
return Optional.empty();
}

JsonObject agsClaimJson = JsonParser.parseString(idToken.getClaim(Claims.AGS_CLAIM).toString()).getAsJsonObject();

Lti13AgsClaim agsClaim = new Lti13AgsClaim();
JSONArray scopes = (JSONArray) agsClaimJson.get("scope");
JsonArray scopes = agsClaimJson.get("scope").getAsJsonArray();

if (scopes == null) {
return Optional.empty();
}

if (scopes.contains(Scopes.AGS_SCORE)) {
if (scopes.contains(new JsonPrimitive(Scopes.AGS_SCORE))) {
agsClaim.setScope(Collections.singletonList(Scopes.AGS_SCORE));
}

agsClaim.setLineItem((String) agsClaimJson.get("lineitem"));
agsClaim.setLineItem(agsClaimJson.get("lineitem").getAsString());

return Optional.of(agsClaim);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import org.springframework.security.oauth2.core.oidc.OidcIdToken;
import org.springframework.util.Assert;

import com.nimbusds.jose.shaded.json.JSONObject;
import com.google.gson.JsonParser;

public class Lti13LaunchRequest {

Expand All @@ -24,8 +24,13 @@ public Lti13LaunchRequest(OidcIdToken ltiIdToken, String clientRegistrationId) {
this.sub = ltiIdToken.getClaim("sub");
this.deploymentId = ltiIdToken.getClaim(Claims.LTI_DEPLOYMENT_ID);

JSONObject resourceLinkClaim = ltiIdToken.getClaim(Claims.RESOURCE_LINK);
this.resourceLinkId = resourceLinkClaim != null ? (String) resourceLinkClaim.get("id") : null;
var resourceLinkClaim = ltiIdToken.getClaim(Claims.RESOURCE_LINK);
if (resourceLinkClaim != null) {
this.resourceLinkId = JsonParser.parseString(resourceLinkClaim.toString()).getAsJsonObject().get("id").getAsString();
}
else {
this.resourceLinkId = null;
}
this.targetLinkUri = ltiIdToken.getClaim(Claims.TARGET_LINK_URI);

this.agsClaim = Lti13AgsClaim.from(ltiIdToken).orElse(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@
import org.springframework.web.filter.OncePerRequestFilter;
import org.springframework.web.util.UriComponentsBuilder;

import com.google.gson.JsonObject;

import de.tum.in.www1.artemis.domain.lti.Claims;
import de.tum.in.www1.artemis.exception.LtiEmailAlreadyInUseException;
import de.tum.in.www1.artemis.service.connectors.lti.Lti13Service;
import net.minidev.json.JSONObject;
import uk.ac.ox.ctl.lti13.security.oauth2.client.lti.authentication.OidcAuthenticationToken;
import uk.ac.ox.ctl.lti13.security.oauth2.client.lti.web.OAuth2LoginAuthenticationFilter;

Expand Down Expand Up @@ -102,8 +103,8 @@ private void writeResponse(String targetLinkUri, HttpServletResponse response) t
UriComponentsBuilder uriBuilder = UriComponentsBuilder.fromUriString(targetLinkUri);
lti13Service.buildLtiResponse(uriBuilder, response);

JSONObject json = new JSONObject();
json.put("targetLinkUri", uriBuilder.build().toUriString());
JsonObject json = new JsonObject();
json.addProperty("targetLinkUri", uriBuilder.build().toUriString());

response.setContentType("application/json");
response.setCharacterEncoding("UTF-8");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
import org.springframework.web.client.RestTemplate;
import org.springframework.web.util.UriComponentsBuilder;

import com.google.gson.JsonObject;

import de.tum.in.www1.artemis.domain.*;
import de.tum.in.www1.artemis.domain.lti.*;
import de.tum.in.www1.artemis.domain.participation.StudentParticipation;
Expand All @@ -35,7 +37,6 @@
import de.tum.in.www1.artemis.security.lti.Lti13TokenRetriever;
import de.tum.in.www1.artemis.service.OnlineCourseConfigurationService;
import de.tum.in.www1.artemis.web.rest.errors.BadRequestAlertException;
import net.minidev.json.JSONObject;

@Service
@Profile("lti")
Expand Down Expand Up @@ -236,15 +237,15 @@ private String getScoresUrl(String lineItemUrl) {
}

private String getScoreBody(String userId, String comment, Double score) {
JSONObject requestBody = new JSONObject();
requestBody.put("userId", userId);
requestBody.put("timestamp", (new DateTime()).toString());
requestBody.put("activityProgress", "Submitted");
requestBody.put("gradingProgress", "FullyGraded");
requestBody.put("comment", comment);
requestBody.put("scoreGiven", score);
requestBody.put("scoreMaximum", 100D);
return requestBody.toJSONString();
JsonObject requestBody = new JsonObject();
requestBody.addProperty("userId", userId);
requestBody.addProperty("timestamp", (new DateTime()).toString());
requestBody.addProperty("activityProgress", "Submitted");
requestBody.addProperty("gradingProgress", "FullyGraded");
requestBody.addProperty("comment", comment);
requestBody.addProperty("scoreGiven", score);
requestBody.addProperty("scoreMaximum", 100D);
return requestBody.toString();
}

/**
Expand Down
13 changes: 13 additions & 0 deletions src/test/java/de/tum/in/www1/artemis/ArchitectureTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,19 @@ void testLogging() {
GeneralCodingRules.NO_CLASSES_SHOULD_ACCESS_STANDARD_STREAMS.check(classes);
}

@Test
void testJSONImplementations() {
ArchRule jsonObject = noClasses().should().dependOnClassesThat(have(simpleName("JsonObject").or(simpleName("JSONObject"))).and(not(resideInAPackage("com.google.gson"))));

ArchRule jsonArray = noClasses().should().dependOnClassesThat(have(simpleName("JsonArray").or(simpleName("JSONArray"))).and(not(resideInAPackage("com.google.gson"))));

ArchRule jsonParser = noClasses().should().dependOnClassesThat(have(simpleName("JsonParser").or(simpleName("JSONParser"))).and(not(resideInAPackage("com.google.gson"))));

jsonObject.check(allClasses);
jsonArray.check(allClasses);
jsonParser.check(allClasses);
}

// Custom Predicates for JavaAnnotations since ArchUnit only defines them for classes

private DescribedPredicate<? super JavaAnnotation<?>> simpleNameAnnotation(String name) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@
import org.springframework.http.HttpStatus;
import org.springframework.security.test.context.support.WithMockUser;

import com.google.gson.JsonObject;

import de.tum.in.www1.artemis.domain.Imprint;
import de.tum.in.www1.artemis.domain.enumeration.Language;
import net.minidev.json.JSONObject;

class ImprintResourceIntegrationTest extends AbstractSpringIntegrationIndependentTest {

Expand Down Expand Up @@ -212,10 +213,10 @@ void testUpdateImprint_writesFile_ReturnsUpdatedFileContent() throws Exception {
@Test
@WithMockUser(username = TEST_PREFIX + "admin", roles = "ADMIN")
void testUpdateImprint_unsupportedLanguageBadRequest() throws Exception {
JSONObject body = new JSONObject();
body.put("text", "test");
body.put("language", "FRENCH");
request.put("/api/admin/imprint", body, HttpStatus.BAD_REQUEST);
JsonObject body = new JsonObject();
body.addProperty("text", "test");
body.addProperty("language", "FRENCH");
request.put("/api/admin/imprint", body.toString(), HttpStatus.BAD_REQUEST);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@
import org.springframework.http.HttpStatus;
import org.springframework.security.test.context.support.WithMockUser;

import com.google.gson.JsonObject;

import de.tum.in.www1.artemis.domain.PrivacyStatement;
import de.tum.in.www1.artemis.domain.enumeration.Language;
import net.minidev.json.JSONObject;

class PrivacyStatementResourceIntegrationTest extends AbstractSpringIntegrationIndependentTest {

Expand Down Expand Up @@ -220,10 +221,10 @@ void testUpdatePrivacyStatement_writesFile_ReturnsUpdatedFileContent() throws Ex
@Test
@WithMockUser(username = TEST_PREFIX + "admin", roles = "ADMIN")
void testUpdatePrivacyStatement_unsupportedLanguageBadRequest() throws Exception {
JSONObject body = new JSONObject();
body.put("text", "test");
body.put("language", "FRENCH");
request.put("/api/admin/privacy-statement", body, HttpStatus.BAD_REQUEST);
JsonObject body = new JsonObject();
body.addProperty("text", "test");
body.addProperty("language", "FRENCH");
request.put("/api/admin/privacy-statement", body.toString(), HttpStatus.BAD_REQUEST);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,8 @@
import org.springframework.web.client.RestTemplate;
import org.springframework.web.util.UriComponentsBuilder;

import com.nimbusds.jose.shaded.json.JSONObject;
import com.nimbusds.jose.shaded.json.parser.JSONParser;
import com.nimbusds.jose.shaded.json.parser.ParseException;
import com.google.gson.JsonObject;
import com.google.gson.JsonParser;

import de.tum.in.www1.artemis.domain.*;
import de.tum.in.www1.artemis.domain.lti.LtiResourceLaunch;
Expand Down Expand Up @@ -121,8 +120,8 @@ void performLaunch_exerciseFound() {
when(oidcIdToken.getClaim("sub")).thenReturn("1");
when(oidcIdToken.getClaim("iss")).thenReturn("http://otherDomain.com");
when(oidcIdToken.getClaim(Claims.LTI_DEPLOYMENT_ID)).thenReturn("1");
JSONObject jsonObject = new JSONObject();
jsonObject.put("id", "resourceLinkUrl");
JsonObject jsonObject = new JsonObject();
jsonObject.addProperty("id", "resourceLinkUrl");
when(oidcIdToken.getClaim(Claims.RESOURCE_LINK)).thenReturn(jsonObject);
prepareForPerformLaunch(courseId, exerciseId);

Expand Down Expand Up @@ -411,7 +410,7 @@ void onNewResultTokenFails() {
}

@Test
void onNewResult() throws ParseException {
void onNewResult() {
Result result = new Result();
double scoreGiven = 60D;
result.setScore(scoreGiven);
Expand Down Expand Up @@ -455,16 +454,15 @@ void onNewResult() throws ParseException {
assertThat(authHeaders).as("Score publish request must contain an Authorization header").isNotNull();
assertThat(authHeaders).as("Score publish request must contain the corresponding Authorization Bearer token").contains("Bearer " + accessToken);

JSONParser jsonParser = new JSONParser(JSONParser.MODE_JSON_SIMPLE);
JSONObject body = (JSONObject) jsonParser.parse(httpEntity.getBody());
assertThat(body.get("userId")).as("Invalid parameter in score publish request: userId").isEqualTo(launch.getSub());
assertThat(body.get("timestamp")).as("Parameter missing in score publish request: timestamp").isNotNull();
assertThat(body.get("activityProgress")).as("Parameter missing in score publish request: activityProgress").isNotNull();
assertThat(body.get("gradingProgress")).as("Parameter missing in score publish request: gradingProgress").isNotNull();
JsonObject body = JsonParser.parseString(httpEntity.getBody()).getAsJsonObject();
assertThat(body.get("userId").getAsString()).as("Invalid parameter in score publish request: userId").isEqualTo(launch.getSub());
assertThat(body.get("timestamp").getAsString()).as("Parameter missing in score publish request: timestamp").isNotNull();
assertThat(body.get("activityProgress").getAsString()).as("Parameter missing in score publish request: activityProgress").isNotNull();
assertThat(body.get("gradingProgress").getAsString()).as("Parameter missing in score publish request: gradingProgress").isNotNull();

assertThat(body.get("comment")).as("Invalid parameter in score publish request: comment").isEqualTo("Good job. Not so good");
assertThat(body.get("scoreGiven")).as("Invalid parameter in score publish request: scoreGiven").isEqualTo(scoreGiven);
assertThat(body.get("scoreMaximum")).as("Invalid parameter in score publish request: scoreMaximum").isEqualTo(100d);
assertThat(body.get("comment").getAsString()).as("Invalid parameter in score publish request: comment").isEqualTo("Good job. Not so good");
assertThat(body.get("scoreGiven").getAsDouble()).as("Invalid parameter in score publish request: scoreGiven").isEqualTo(scoreGiven);
assertThat(body.get("scoreMaximum").getAsDouble()).as("Invalid parameter in score publish request: scoreMaximum").isEqualTo(100d);

assertThat(launch.getScoreLineItemUrl() + "/scores").as("Score publish request was sent to a wrong URI").isEqualTo(urlCapture.getValue());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@
import java.net.URL;

import org.gitlab4j.api.GitLabApiException;
import org.json.simple.parser.JSONParser;
import org.json.simple.parser.ParseException;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand All @@ -22,6 +20,7 @@
import org.springframework.security.test.context.support.WithMockUser;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;

import de.tum.in.www1.artemis.AbstractSpringIntegrationJenkinsGitlabTest;
import de.tum.in.www1.artemis.domain.Commit;
Expand Down Expand Up @@ -118,9 +117,9 @@ void testGetOrRetrieveDefaultBranch() throws GitLabApiException {
}

@Test
void testGetLastCommitDetails() throws ParseException {
void testGetLastCommitDetails() throws JsonProcessingException {
String latestCommitHash = "11028e4104243d8cbae9175f2bc938cb8c2d7924";
Object body = new JSONParser().parse(GITLAB_PUSH_EVENT_REQUEST);
Object body = new ObjectMapper().readValue(GITLAB_PUSH_EVENT_REQUEST, Object.class);
Commit commit = versionControlService.getLastCommitDetails(body);
assertThat(commit.getCommitHash()).isEqualTo(latestCommitHash);
assertThat(commit.getBranch()).isNotNull();
Expand All @@ -130,9 +129,9 @@ void testGetLastCommitDetails() throws ParseException {
}

@Test
void testGetLastCommitDetailsWrongCommitOrder() throws ParseException {
void testGetLastCommitDetailsWrongCommitOrder() throws JsonProcessingException {
String latestCommitHash = "11028e4104243d8cbae9175f2bc938cb8c2d7924";
Object body = new JSONParser().parse(GITLAB_PUSH_EVENT_REQUEST_WRONG_COMMIT_ORDER);
Object body = new ObjectMapper().readValue(GITLAB_PUSH_EVENT_REQUEST_WRONG_COMMIT_ORDER, Object.class);
Commit commit = versionControlService.getLastCommitDetails(body);
assertThat(commit.getCommitHash()).isEqualTo(latestCommitHash);
assertThat(commit.getBranch()).isNotNull();
Expand All @@ -142,9 +141,9 @@ void testGetLastCommitDetailsWrongCommitOrder() throws ParseException {
}

@Test
void testGetLastCommitDetailsWithoutCommits() throws ParseException {
void testGetLastCommitDetailsWithoutCommits() throws JsonProcessingException {
String latestCommitHash = "11028e4104243d8cbae9175f2bc938cb8c2d7924";
Object body = new JSONParser().parse(GITLAB_PUSH_EVENT_REQUEST_WITHOUT_COMMIT);
Object body = new ObjectMapper().readValue(GITLAB_PUSH_EVENT_REQUEST_WITHOUT_COMMIT, Object.class);
Commit commit = versionControlService.getLastCommitDetails(body);
assertThat(commit.getCommitHash()).isEqualTo(latestCommitHash);
assertThat(commit.getBranch()).isNull();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import java.util.Arrays;
import java.util.stream.Stream;

import org.json.simple.parser.JSONParser;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand All @@ -24,6 +23,8 @@
import org.springframework.http.HttpStatus;
import org.springframework.security.test.context.support.WithMockUser;

import com.fasterxml.jackson.databind.ObjectMapper;

import de.tum.in.www1.artemis.AbstractSpringIntegrationBambooBitbucketJiraTest;
import de.tum.in.www1.artemis.domain.enumeration.ExerciseMode;
import de.tum.in.www1.artemis.domain.enumeration.ProgrammingLanguage;
Expand Down Expand Up @@ -218,7 +219,7 @@ void resumeProgrammingExercise_doesNotExist(ExerciseMode exerciseMode) throws Ex
void resumeProgrammingExerciseByPushingIntoRepo_correctInitializationState(ExerciseMode exerciseMode) throws Exception {
final String requestAsArtemisUser = BITBUCKET_PUSH_EVENT_REQUEST.replace("\"name\": \"admin\"", "\"name\": \"Artemis\"").replace("\"displayName\": \"Admin\"",
"\"displayName\": \"Artemis\"");
Object body = new JSONParser().parse(requestAsArtemisUser);
Object body = new ObjectMapper().readValue(requestAsArtemisUser, Object.class);
programmingExerciseTestService.resumeProgrammingExerciseByPushingIntoRepo_correctInitializationState(exerciseMode, body);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import java.util.Arrays;
import java.util.stream.Stream;

import org.json.simple.parser.JSONParser;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand All @@ -24,6 +23,8 @@
import org.springframework.http.HttpStatus;
import org.springframework.security.test.context.support.WithMockUser;

import com.fasterxml.jackson.databind.ObjectMapper;

import de.tum.in.www1.artemis.AbstractSpringIntegrationJenkinsGitlabTest;
import de.tum.in.www1.artemis.domain.enumeration.ExerciseMode;
import de.tum.in.www1.artemis.domain.enumeration.ProgrammingLanguage;
Expand Down Expand Up @@ -278,7 +279,7 @@ void resumeProgrammingExercise_doesNotExist(ExerciseMode exerciseMode) throws Ex
@EnumSource(ExerciseMode.class)
@WithMockUser(username = TEST_PREFIX + studentLogin, roles = "USER")
void resumeProgrammingExerciseByPushingIntoRepo_correctInitializationState(ExerciseMode exerciseMode) throws Exception {
Object body = new JSONParser().parse(GITLAB_PUSH_EVENT_REQUEST);
Object body = new ObjectMapper().readValue(GITLAB_PUSH_EVENT_REQUEST, Object.class);
programmingExerciseTestService.resumeProgrammingExerciseByPushingIntoRepo_correctInitializationState(exerciseMode, body);
}

Expand Down
Loading

0 comments on commit 5eb76d0

Please sign in to comment.