You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Hitesh Khamesra <hk...@pivotal.io> on 2016/05/12 20:10:18 UTC
Review Request 47323: GEODE-732 Unable to create PDXInstance from
valid JSON using JSONFormatter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47323/
-----------------------------------------------------------
Review request for geode, Bruce Schuchardt, Jianxia Chen, and Udo Kohlmeyer.
Repository: geode
Description
-------
We were not handling list inside list immediately. Fixed it in PdxListHelper, where we add java list in parent list.
Added test for it. Added one more state LIST_ENDS for readability purpose
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/pdx/JSONFormatter.java 220deaf
geode-core/src/main/java/com/gemstone/gemfire/pdx/internal/json/PdxListHelper.java 7700b30
geode-core/src/test/resources/com/gemstone/gemfire/pdx/jsonStrings/jsonListInsideList.txt PRE-CREATION
Diff: https://reviews.apache.org/r/47323/diff/
Testing
-------
Thanks,
Hitesh Khamesra
Re: Review Request 47323: GEODE-732 Unable to create PDXInstance from
valid JSON using JSONFormatter
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47323/#review133013
-----------------------------------------------------------
I don't see the test class for this. Can you please attach it.
- Udo Kohlmeyer
On May 12, 2016, 8:10 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47323/
> -----------------------------------------------------------
>
> (Updated May 12, 2016, 8:10 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Jianxia Chen, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We were not handling list inside list immediately. Fixed it in PdxListHelper, where we add java list in parent list.
> Added test for it. Added one more state LIST_ENDS for readability purpose
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/pdx/JSONFormatter.java 220deaf
> geode-core/src/main/java/com/gemstone/gemfire/pdx/internal/json/PdxListHelper.java 7700b30
> geode-core/src/test/resources/com/gemstone/gemfire/pdx/jsonStrings/jsonListInsideList.txt PRE-CREATION
>
> Diff: https://reviews.apache.org/r/47323/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 47323: GEODE-732 Unable to create PDXInstance from
valid JSON using JSONFormatter
Posted by Jianxia Chen <jc...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47323/#review133143
-----------------------------------------------------------
There must be something missing that uses jsonListInsideList.txt. I will recommend call it array instead of list.
- Jianxia Chen
On May 12, 2016, 8:10 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47323/
> -----------------------------------------------------------
>
> (Updated May 12, 2016, 8:10 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Jianxia Chen, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We were not handling list inside list immediately. Fixed it in PdxListHelper, where we add java list in parent list.
> Added test for it. Added one more state LIST_ENDS for readability purpose
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/pdx/JSONFormatter.java 220deaf
> geode-core/src/main/java/com/gemstone/gemfire/pdx/internal/json/PdxListHelper.java 7700b30
> geode-core/src/test/resources/com/gemstone/gemfire/pdx/jsonStrings/jsonListInsideList.txt PRE-CREATION
>
> Diff: https://reviews.apache.org/r/47323/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 47323: GEODE-732 Unable to create PDXInstance from
valid JSON using JSONFormatter
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47323/#review133141
-----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/pdx/JSONFormatter.java (line 198)
<https://reviews.apache.org/r/47323/#comment197441>
Just before that line we call getList method, which reads whole list. And then we add that list in Parent pDxInstance. So just setting here LIST_ENDS.
- Hitesh Khamesra
On May 12, 2016, 8:10 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47323/
> -----------------------------------------------------------
>
> (Updated May 12, 2016, 8:10 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Jianxia Chen, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We were not handling list inside list immediately. Fixed it in PdxListHelper, where we add java list in parent list.
> Added test for it. Added one more state LIST_ENDS for readability purpose
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/pdx/JSONFormatter.java 220deaf
> geode-core/src/main/java/com/gemstone/gemfire/pdx/internal/json/PdxListHelper.java 7700b30
> geode-core/src/test/resources/com/gemstone/gemfire/pdx/jsonStrings/jsonListInsideList.txt PRE-CREATION
>
> Diff: https://reviews.apache.org/r/47323/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 47323: GEODE-732 Unable to create PDXInstance from
valid JSON using JSONFormatter
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47323/#review133134
-----------------------------------------------------------
I see that JsonClientServerDUnitTest tests all of the documents in the directory containing the new array-within-an-array document. I think the changes look good, though I have one question.
geode-core/src/main/java/com/gemstone/gemfire/pdx/JSONFormatter.java (line 198)
<https://reviews.apache.org/r/47323/#comment197427>
I can understand setting currentState to LIST_ENDS in the END_ARRAY case but why does it need to be set here as well?
- Bruce Schuchardt
On May 12, 2016, 8:10 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47323/
> -----------------------------------------------------------
>
> (Updated May 12, 2016, 8:10 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Jianxia Chen, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We were not handling list inside list immediately. Fixed it in PdxListHelper, where we add java list in parent list.
> Added test for it. Added one more state LIST_ENDS for readability purpose
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/pdx/JSONFormatter.java 220deaf
> geode-core/src/main/java/com/gemstone/gemfire/pdx/internal/json/PdxListHelper.java 7700b30
> geode-core/src/test/resources/com/gemstone/gemfire/pdx/jsonStrings/jsonListInsideList.txt PRE-CREATION
>
> Diff: https://reviews.apache.org/r/47323/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>