[#158] Update Jackson dependencies to the latest version (v2.17.2) #219
Conversation
jackson-databind v2.7.0 and jackson-datatype-jsr310 v2.7.4 are severely outdated, and have critical security vulnerabilities. However, newer versions of the library serialize relative paths as absolute, causing tests to fail when updating the dependencies. This is fixed by modifying the ObjectMapper to use ToStringSerializer for the Path class, which serializes relative paths correctly. Let's update the Jackson dependencies to the latest version (v2.17.2) to resolve the security vulnerabilities and remove the IntelliJ warning.
|
Click here to submit a new iteration when this PR is ready for review. See this repository's contribution guide for more information. |
aureliony
changed the title
v1@aureliony submitted v1 for review. Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/219/1/head:BRANCHNAMEwhere |
| @@ -37,7 +37,8 @@ public class JsonUtil { | |||
| .setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY) | |||
| .registerModule(new SimpleModule("SimpleModule") | |||
| .addSerializer(Level.class, new ToStringSerializer()) | |||
| .addDeserializer(Level.class, new LevelDeserializer(Level.class))); | |||
| .addDeserializer(Level.class, new LevelDeserializer(Level.class)) | |||
| .addSerializer(Path.class, new ToStringSerializer())); | |||
There was a problem hiding this comment.
Would there be a need to deserialise the Path.class as well?
| .addSerializer(Path.class, new ToStringSerializer())); | |
| .addSerializer(Path.class, new ToStringSerializer()) | |
| .addDeserializer(Path.class, new PathDeserializer(Path.class))); |
There was a problem hiding this comment.
By default, Jackson will use com.fasterxml.jackson.databind.ext.NioPathDeserializer to deserialize the Path class, which we should not override as it is working as expected for both absolute and relative paths.
|
LGTM. However I am not sure whether it is the best practice or not. ObjectMapper maps the file to its absolute path, therefore we have no choice but to teach our objectmapper to serialize the file to their relative paths, which is to add a Path serializer to our objectmapper module. Here comes the question, should we use ToStringSerializer or should we design a PathSerializer? I think both works fine, it's just an issue of good practices. Otherwise, if the severity issue can be waited, I am not sure whether it is fine to just stick to the old version first. |
I am in favor of using |
There was a problem hiding this comment.
LGTM.
The code correctly adds the Path serializer using ToStringSerializer to handle relative path serialization. The implementation is clean and follows a consistent style. Given the context, using ToStringSerializer for Path seems appropriate. The approach seems correct and should work as intended.
|
Thanks for investigating the issue regarding path serialisation—this was a major blocker when upgrading the Jackson dependencies! A minor question: Why are we upgrading specifically to 2.17.2 instead of a slightly older version that does not contain the security vulnerability (which, if I recall correctly, is any version that is at least 2.13)? Can we guarantee that it is compatible with the existing modules, and are there any tests which can prove that no unintentional regressions have been introduced? Thanks! |
|
LGTM, just wondering if we need to test this |
Fixes #158.
To fix the issue with relative path serialization,
ObjectMapperis modified to useToStringSerializerfor thePathclass, which serializes relative paths correctly.