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

Add missing support for nested JSON objects in arrays #15425

Merged
merged 1 commit into from Sep 20, 2019

Conversation

@ahmetmircik
Copy link
Member

ahmetmircik commented Aug 6, 2019

fix attempt for #15368

@ahmetmircik ahmetmircik added this to the 4.0 milestone Aug 6, 2019
@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/jsonFix branch 2 times, most recently from 8e78193 to b5b178e Aug 6, 2019
@ahmetmircik ahmetmircik changed the title wip [WIP] Missing support for nested JSON objects in arrays Aug 7, 2019
@ahmetmircik ahmetmircik changed the title [WIP] Missing support for nested JSON objects in arrays [WIP] Add missing support for nested JSON objects in arrays Aug 7, 2019
@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/jsonFix branch from b5b178e to e1cfbdd Aug 8, 2019
@ahmetmircik ahmetmircik marked this pull request as ready for review Sep 4, 2019
@ahmetmircik ahmetmircik changed the title [WIP] Add missing support for nested JSON objects in arrays Add missing support for nested JSON objects in arrays Sep 4, 2019
@mmedenjak mmedenjak self-requested a review Sep 4, 2019
Copy link
Contributor

mmedenjak left a comment

Looks good. Some changes that you might look into:

  • fix the commit message
  • if we provide an invalid path (e.g. change the path in MapAggregationJsonTest#testArrayWithNestedField to nestedArray[any].nestedArrayObject.secondary), we get a NPE. Even though the path is invalid, can you try improving this so it's a better error or behaviour?
@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/jsonFix branch from e1cfbdd to ffb0a6e Sep 6, 2019
@ahmetmircik

This comment has been minimized.

Copy link
Member Author

ahmetmircik commented Sep 6, 2019

@mmedenjak thanks fro the findings.
Fixed commit msg and re-wrote fix to not to give NPE, this is because to align behavior with non-array queries which don't throw NPE.

@ahmetmircik ahmetmircik requested a review from mmedenjak Sep 6, 2019
@mmedenjak

This comment has been minimized.

Copy link
Contributor

mmedenjak commented Sep 9, 2019

Thank you for the update. I just realised that also additional non-existant paths are ignored. For instance, in MapAggregationJsonTest#testArrayWithNestedField you can use the path nestedArray[any].nestedArrayObject.secondary.nestedObjectLongValue.nonExistant and it should return the same values. Can you check? What do other extractors do in that case?

@mmedenjak

This comment has been minimized.

Copy link
Contributor

mmedenjak commented Sep 9, 2019

Also, please address comments in a separate commit.

@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/jsonFix branch 2 times, most recently from 4433860 to b597544 Sep 9, 2019
@ahmetmircik

This comment has been minimized.

Copy link
Member Author

ahmetmircik commented Sep 9, 2019

@mmedenjak

  1. Addressed the last issue you found
  2. Unfortunately, re-wrote whole fix. A complete review is needed now.
@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/jsonFix branch 4 times, most recently from 530f707 to 7b0219f Sep 9, 2019
@hazelcast hazelcast deleted a comment from ahmetmircik Sep 13, 2019
@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/jsonFix branch 2 times, most recently from 8c76187 to 06f7e70 Sep 16, 2019
@ahmetmircik ahmetmircik force-pushed the ahmetmircik:fix/4.0/jsonFix branch from 06f7e70 to 9fd17b1 Sep 16, 2019
@ahmetmircik ahmetmircik requested a review from mmedenjak Sep 16, 2019
Copy link
Contributor

mmedenjak left a comment

Perfect 👍

@blazember blazember self-requested a review Sep 19, 2019
@ahmetmircik ahmetmircik merged commit 6ccd834 into hazelcast:master Sep 20, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@ahmetmircik ahmetmircik deleted the ahmetmircik:fix/4.0/jsonFix branch Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.