search

se-edu / addressbook-level3

:ab::three: Address Book sample application (Level 3)
https://se-education.org/addressbook-level3
MIT License
28 stars 430 forks source link

[#158] Update Jackson dependencies to the latest version (v2.17.2) #219

Open aureliony opened 3 months ago

aureliony commented 3 months ago

Fixes #158.

To fix the issue with relative path serialization, ObjectMapper is modified to use ToStringSerializer for the Path class, which serializes relative paths correctly.

canihasreview[bot] commented 3 months ago

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

canihasreview[bot] commented 3 months ago

v1

@aureliony submitted v1 for review.

(:books: Archive)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/219/1/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
jyue487 commented 3 months ago

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.

aureliony commented 3 months ago

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 ToStringSerializer. Even if we were to create our own custom serializer, it's gonna use path.toString() anyway (unless there's another way to get a string representation of the path?), so it would keep things much simpler. Additionally ToStringSerializer is already being used to serialize java.util.logging.Level.

ryanozx commented 3 months ago

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!

baskargopinath commented 3 months ago

LGTM, just wondering if we need to test this