[#13645] Adopt Jackson JSON library#13688
[#13645] Adopt Jackson JSON library#13688iZUMi-kyouka wants to merge 61 commits intoTEAMMATES:masterfrom
Conversation
There was a problem hiding this comment.
Copilot reviewed 13 out of 14 changed files in this pull request and generated no comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 47 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/main/java/teammates/ui/request/MarkNotificationAsReadRequest.java:16
MarkNotificationAsReadRequestuses an@JsonCreatorconstructor withLong endTimestampbut assigns it to a primitivelong. If the JSON omitsendTimestamp(or sets it to null), Jackson will pass null and this will NPE during unboxing, resulting in a 500 instead of a validation-driven 400. Consider taking a primitivelongin the creator, or explicitly handling null (e.g., default to 0 sovalidate()can fail cleanly).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String expectedQuestionDetailsJson = "{\n" | ||
| + " \"shouldAllowRichText\": true,\n" | ||
| + " \"questionType\": \"TEXT\",\n" | ||
| + " \"questionText\": \"Question text.\"\n" | ||
| + " \"questionText\": \"Question text.\",\n" | ||
| + " \"shouldAllowRichText\": true\n" | ||
| + "}"; | ||
|
|
||
| assertEquals(expectedQuestionDetailsJson, JsonUtils.toJson(qd)); | ||
|
|
||
| expectedQuestionDetailsJson = "{\"shouldAllowRichText\":true,\"questionType\":\"TEXT\"," | ||
| + "\"questionText\":\"Question text.\"}"; | ||
| expectedQuestionDetailsJson = "{\"questionType\":\"TEXT\",\"questionText\":\"Question text.\"," | ||
| + "\"shouldAllowRichText\":true}"; | ||
|
|
||
| assertEquals(expectedQuestionDetailsJson, JsonUtils.toCompactJson(qd)); |
There was a problem hiding this comment.
These tests assert exact JSON string output, which is brittle with Jackson (field order/pretty-printing can change across versions or mapper settings). To make the test stable, consider asserting JSON structural equality instead (e.g., parse both expected/actual into a JSON tree and compare) rather than comparing raw strings.
There was a problem hiding this comment.
Perhaps can be discussed in another issue. This is just a fix to align with Jackson's parent class field-first ordering.
… cascading type error
…ties in JsonUtils
… for FQDetails and FRDetails
- Disable default of sorting attributes alphabetically - Remove explicit registration of paramnames and javatime module - Update custom pretty printer to new API - Update custom serde to new API - Update annotation introspector to new API
# Conflicts: # src/main/java/teammates/ui/output/DeadlineExtensionData.java # src/main/java/teammates/ui/output/FeedbackSessionData.java # src/main/java/teammates/ui/output/FeedbackSessionsData.java # src/main/java/teammates/ui/request/MarkNotificationAsReadRequest.java # src/test/java/teammates/test/AbstractBackDoor.java
… of Jackson 3 in project. - Without this, Jackson 3 will fail when ./gradlew generateTypes run since it will use jackson-annotations 2.14 brought by the ts-generator which lacks some enum constants required by Jackson 3.
…regarding Jackson's implicit default behaviour
|
Summary of Changes
|
|
One thing that comes to mind is that currently not all entity in ui/output is ready for Jackson deserialisation since some lack private no-args constructor while others with suitable constructors need to be annotated. Adding these only when it becomes required i.e. when tests actually deserialise them reduce clutter but also means each time new tests that possibly deserialise one of these undeserialisable entity are written, then the annotation need to be added. Just wondering if this should be made into a separate issue to make all of them ready to be deserialised. |
c8eb160 to
81061b0
Compare
81061b0 to
fe5078c
Compare
… which is disabled by default in Jackson 3
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 50 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/main/java/teammates/common/datatransfer/logs/FeedbackSessionLogType.java:17
- FeedbackSessionLogType no longer has
@JsonValueon the label, which will change JSON serialization from label values (e.g., "access", "view result") to enum names (e.g., "ACCESS", "VIEW_RESULT"). This is likely to break callers that pass/expect the label form (e.g., CreateFeedbackSessionLogAction parses fsltype via valueOfLabel) and may also alter generated TypeScript enum values. Either restore@JsonValue(or add a@JsonValuegetter returning label) or update parsing/clients to consistently use enum names instead of labels.
// CHECKSTYLE.ON:JavadocVariable
private final String label;
FeedbackSessionLogType(String label) {
this.label = label;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes #13645
Outline of Solution
@JsonCreatorannotations for classes where the existing constructor is suitable to create the object on deserialisationIssues
FeedbackQuestionType.questionType, requiring some non-trivial amount of changes to the frontend which should be done in another PR if required.mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);, but I think this should be removed in future for security@JsonSubtypesand@JsonTypeInfoannotation for LogDetails vs custom serialiser and deserialiser for FeedbackQuestion and FeedbackResponse. For both FQ and FR, this is required since the type info is nested inside the FQDetails and FRDetails and there is no way to encode this information using Jackson's annotations. Unless we flatten it, which not sure is feasible and shold be in another PR