Skip to content

Return List instead of Optional and more tests#25

Open
mez5001 wants to merge 6 commits intoPennState:developfrom
mez5001:develop
Open

Return List instead of Optional and more tests#25
mez5001 wants to merge 6 commits intoPennState:developfrom
mez5001:develop

Conversation

@mez5001
Copy link
Contributor

@mez5001 mez5001 commented May 2, 2018

  • JsonSerializationutil.deserializeList returns a list instead of an Optional
  • Added unit tests for handling Abstract Classes

public static <T> Optional<List<T>> deserializeList(String json, Class<T> clazz) throws IOException {
public static <T> List<T> deserializeList(String json, Class<T> clazz) throws IOException {
if (json == null || json.trim().isEmpty()) {
return Optional.empty();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there projects using this method at the moment? We'll need to know what (if anything) this will break.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am only aware of Worklion using this class. I will clean it up during the next release.

}

public static <T> Optional<List<T>> deserializeList(String json, Class<T> clazz) throws IOException {
public static <T> List<T> deserializeList(String json, Class<T> clazz) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to mark this method with the @nonnull JSR-305 annotation to help static analysis tools. Lately I've been favoring ErrorProne and SpotBugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added this annotation to the method

@smoyer64
Copy link
Contributor

smoyer64 commented May 2, 2018

I just realized that this is going to break one of the conventions we had discussed when starting the SCIMple project - the behavior of the jackson2 library is different when these are marchalled and we had purposely left fields (including JSON arrays) null when we could. So myList = null would produce the following JSON snippet:

{ 
}

But myList = new ArrayList() would instead produce:

{
  myList = {}
}

So our goal when fields weren't populated was to minimize the size of the JSON data that would be sent/received.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants