Skip to content
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

DATAREST-956 handle add and remove item to nested collections #245

Closed

Conversation

mduesterhoeft
Copy link

@mduesterhoeft mduesterhoeft commented Dec 7, 2016

Copy link
Member

@odrotbohm odrotbohm left a comment

Choose a reason for hiding this comment

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

I've left a few comments.

@@ -240,13 +241,18 @@ private boolean handleArrayNode(ArrayNode array, Collection<Object> collection,
for (JsonNode jsonNode : array) {

if (!value.hasNext()) {
if (componentType != null) {
collection.add(mapper.treeToValue(jsonNode, componentType));
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we have to extend the algorithm here as this will only work if there's exactly one more item provided. It will get added but then the iteration is stopped so that all subsequent incoming elements are ignored, right? I guess we could just trigger a continue if there are more elements in the array only resorting to the eventual return if we have maxed out the array elements.

I am kind of wondering whether we could or should actually invert the processing here: iterating over the existing elements first, removing items if we max out the array first and resort to adding everything thats left in the array after that?

return nestedObjectFound;
}

Object next = value.next();

if (ArrayNode.class.isInstance(jsonNode)) {
return handleArrayNode(array, asCollection(next), mapper);
final Collection<Object> nestedCollection = asCollection(next);
Copy link
Member

Choose a reason for hiding this comment

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

No finals needed here.

@odrotbohm
Copy link
Member

That said, I am happy to take it from here if you don't fancy investing more time :).

@mduesterhoeft
Copy link
Author

@olivergierke thanks for the feedback - sure I am willing to do the improvements - just overlooked that I missed the case for adding multiple items.

@mduesterhoeft
Copy link
Author

@olivergierke what do you think. Is my fix already considered a hack? ;-)

odrotbohm pushed a commit that referenced this pull request Dec 8, 2016
…val for PUT requests.

DomainObjectMerger now properly adds and removes elements to and from collections.

Original pull request: #245.
odrotbohm added a commit that referenced this pull request Dec 8, 2016
Some tiny refactorings in DomainObjectReader. We're now using TypeInformation instead of Class to preserve more generics information when it comes to deeper nesting.

Moved some code around in the unit tests.

Original pull request: #245.
odrotbohm pushed a commit that referenced this pull request Dec 8, 2016
…val for PUT requests.

DomainObjectMerger now properly adds and removes elements to and from collections.

Original pull request: #245.
odrotbohm added a commit that referenced this pull request Dec 8, 2016
Some tiny refactorings in DomainObjectReader. We're now using TypeInformation instead of Class to preserve more generics information when it comes to deeper nesting.

Moved some code around in the unit tests.

Original pull request: #245.
odrotbohm pushed a commit that referenced this pull request Dec 8, 2016
…val for PUT requests.

DomainObjectMerger now properly adds and removes elements to and from collections.

Original pull request: #245.
odrotbohm added a commit that referenced this pull request Dec 8, 2016
Some tiny refactorings in DomainObjectReader. We're now using TypeInformation instead of Class to preserve more generics information when it comes to deeper nesting.

Moved some code around in the unit tests.

Original pull request: #245.
@odrotbohm
Copy link
Member

That's polished, merged, bacl- and forward-ported, thanks!

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