-
Notifications
You must be signed in to change notification settings - Fork 152
Replace com.fasterxml.jackson with com.azure.json #948
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces the remaining usages of com.fasterxml.jackson with com.azure.json by updating serialization and deserialization logic for core cache and token classes. Key changes include:
- Implementing azure-json’s JsonSerializable interface in multiple classes
- Refactoring all JSON read/write methods and adapting helper methods and tests accordingly
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
AuthorizationRequestUrlParametersTest.java | Updated expected JSON string for "claims" parameter |
TokenCache.java | Refactored JSON deserialization/serialization and cache merge logic |
RefreshTokenCacheEntity.java, IdTokenCacheEntity.java, etc. | Updated serialization/deserialization methods to use azure.json |
ClientInfo.java, Credential.java, AccountCacheEntity.java, etc. | Replaced Jackson annotations with Azure JSON patterns |
CachePersistenceIT.java | Adjusted test assertions to match new cache token counts |
msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AuthorizationRequestUrlParametersTest.java
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenCache.java
Outdated
Show resolved
Hide resolved
if (jsonNode != null && jsonNode.isObject()) { | ||
mergeJSONNode(jsonNode, addNode.get(fieldName)); | ||
if (mainMap.containsKey(key) && mainMap.get(key) instanceof Map && value instanceof Map) { | ||
mergeJsonMaps((Map<String, Object>) mainMap.get(key), (Map<String, Object>) value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all the inputs to this function controlled? Could malformed / malicious json be a problem here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the recursive call, mergeJsonMaps
is only used in the mergeJSONString
, which converts the two json strings to a map using JsonReader so I don't think malformed json would be an issue.
Malicious json could be, as this is used for claims and client capabilities, which are ultimately strings that come from the customer. However, the values in those jsons aren't something we can control or really verify, so I don't think there's much more we can do beyond JsonReader's parsing.
Replace final usages of com.fasterxml.jackson with com.azure.json, as per #909
This PR makes changes to a lot of files, as they were unfortunately very interconnected and could not be split up into to multiple PRs like previous dependency removals.
However, most of the files received the same set of changes:
toJson()
, which creates a JsonWriter that can be used to create a JSON-formatted StringfromJson()
, which creates an instance of that class from a JsonReader (which is give a JSON-formatted string)The following files are the exception, and have more bespoke changes that need a more thorough review:
JsonHelper
: Received new methods and refactoring in order to better provide JSON helper methods to the rest of the codebaseTokenCache
: Complete overhaul of the serialization/deserialization codeFinally, there were a few files that only received small changes to use the new JsonHelper methods.
For testing, many integration tests deal with the cache and received some small changes to work with the new helper methods and exact serialization results, and new unit tests were added to
CacheTests
specifically to ensure the serialization/deserialization changes inTokenCache
were covered