You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Bruce Schuchardt <bs...@pivotal.io> on 2016/11/07 22:13:28 UTC
Review Request 53557: GEODE-2080 Rest POST put call not working with
region valueConstrain
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53557/
-----------------------------------------------------------
Review request for geode, Barry Oglesby, Hitesh Khamesra, and Udo Kohlmeyer.
Bugs: GEODE-2080
https://issues.apache.org/jira/browse/GEODE-2080
Repository: geode
Description
-------
If you set a value constraint on a cache Region you will be unable to store objects in the region via the Rest API. This change-set modifies LocalRegion's constraint check to look for a Rest document and use its type name in the constraint check
Diffs
-----
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 3873e6e159ebba4c1a288e9fccde5dbabd2a1140
geode-core/src/test/java/org/apache/geode/pdx/PdxClientServerDUnitTest.java 9a9680a7e14ddec91f005fa0f0c6c3da8d033df2
Diff: https://reviews.apache.org/r/53557/diff/
Testing
-------
new test, precheckin
Thanks,
Bruce Schuchardt
Re: Review Request 53557: GEODE-2080 Rest POST put call not working
with region valueConstrain
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53557/#review155210
-----------------------------------------------------------
Ship it!
Ship It!
- Hitesh Khamesra
On Nov. 7, 2016, 10:13 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53557/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2016, 10:13 p.m.)
>
>
> Review request for geode, Barry Oglesby, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-2080
> https://issues.apache.org/jira/browse/GEODE-2080
>
>
> Repository: geode
>
>
> Description
> -------
>
> If you set a value constraint on a cache Region you will be unable to store objects in the region via the Rest API. This change-set modifies LocalRegion's constraint check to look for a Rest document and use its type name in the constraint check
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 3873e6e159ebba4c1a288e9fccde5dbabd2a1140
> geode-core/src/test/java/org/apache/geode/pdx/PdxClientServerDUnitTest.java 9a9680a7e14ddec91f005fa0f0c6c3da8d033df2
>
> Diff: https://reviews.apache.org/r/53557/diff/
>
>
> Testing
> -------
>
> new test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 53557: GEODE-2080 Rest POST put call not working
with region valueConstrain
Posted by Bruce Schuchardt <bs...@pivotal.io>.
> On Nov. 8, 2016, 1:10 a.m., Udo Kohlmeyer wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java, lines 3311-3326
> > <https://reviews.apache.org/r/53557/diff/1/?file=1556244#file1556244line3311>
> >
> > Maybe this is better suited on the PDXInstance. Maybe a method like "isTypeOf(Class). Then at least we don't spread the PDX logic everywhere in the code.
I had it coded like that but then LocalRegion had to import and interact with PdxInstanceImpl for the first time and I didn't want to do that. Also, since this constraint check is just for Rest objects it didn't make sense to me to further pollute PdxInstanceImpl with knowledge about Rest objects. I would rather have put it in JSonFormatter but that's a public API and this is an internal implementation detail. So, no I don't agree with putting it in PdxInstanceImpl.
- Bruce
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53557/#review155238
-----------------------------------------------------------
On Nov. 7, 2016, 10:13 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53557/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2016, 10:13 p.m.)
>
>
> Review request for geode, Barry Oglesby, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-2080
> https://issues.apache.org/jira/browse/GEODE-2080
>
>
> Repository: geode
>
>
> Description
> -------
>
> If you set a value constraint on a cache Region you will be unable to store objects in the region via the Rest API. This change-set modifies LocalRegion's constraint check to look for a Rest document and use its type name in the constraint check
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 3873e6e159ebba4c1a288e9fccde5dbabd2a1140
> geode-core/src/test/java/org/apache/geode/pdx/PdxClientServerDUnitTest.java 9a9680a7e14ddec91f005fa0f0c6c3da8d033df2
>
> Diff: https://reviews.apache.org/r/53557/diff/
>
>
> Testing
> -------
>
> new test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 53557: GEODE-2080 Rest POST put call not working
with region valueConstrain
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53557/#review155238
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java (lines 3269 - 3284)
<https://reviews.apache.org/r/53557/#comment225104>
Maybe this is better suited on the PDXInstance. Maybe a method like "isTypeOf(Class). Then at least we don't spread the PDX logic everywhere in the code.
- Udo Kohlmeyer
On Nov. 7, 2016, 10:13 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53557/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2016, 10:13 p.m.)
>
>
> Review request for geode, Barry Oglesby, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-2080
> https://issues.apache.org/jira/browse/GEODE-2080
>
>
> Repository: geode
>
>
> Description
> -------
>
> If you set a value constraint on a cache Region you will be unable to store objects in the region via the Rest API. This change-set modifies LocalRegion's constraint check to look for a Rest document and use its type name in the constraint check
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 3873e6e159ebba4c1a288e9fccde5dbabd2a1140
> geode-core/src/test/java/org/apache/geode/pdx/PdxClientServerDUnitTest.java 9a9680a7e14ddec91f005fa0f0c6c3da8d033df2
>
> Diff: https://reviews.apache.org/r/53557/diff/
>
>
> Testing
> -------
>
> new test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 53557: GEODE-2080 Rest POST put call not working
with region valueConstrain
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53557/#review155654
-----------------------------------------------------------
Ship it!
Ship It!
- Udo Kohlmeyer
On Nov. 7, 2016, 10:13 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53557/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2016, 10:13 p.m.)
>
>
> Review request for geode, Barry Oglesby, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-2080
> https://issues.apache.org/jira/browse/GEODE-2080
>
>
> Repository: geode
>
>
> Description
> -------
>
> If you set a value constraint on a cache Region you will be unable to store objects in the region via the Rest API. This change-set modifies LocalRegion's constraint check to look for a Rest document and use its type name in the constraint check
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 3873e6e159ebba4c1a288e9fccde5dbabd2a1140
> geode-core/src/test/java/org/apache/geode/pdx/PdxClientServerDUnitTest.java 9a9680a7e14ddec91f005fa0f0c6c3da8d033df2
>
> Diff: https://reviews.apache.org/r/53557/diff/
>
>
> Testing
> -------
>
> new test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 53557: GEODE-2080 Rest POST put call not working
with region valueConstrain
Posted by Bruce Schuchardt <bs...@pivotal.io>.
> On Nov. 8, 2016, 1:44 a.m., Darrel Schneider wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java, line 3315
> > <https://reviews.apache.org/r/53557/diff/1/?file=1556244#file1556244line3315>
> >
> > If pdx.getClassName() is not equals to JSON_CLASSNAME then would it make sense to set valueClassName to pdx.getClassName()? That way you could have "read-serialized" set to true and still have a meaningful value constraint with pdx
No, definitely not. That would cause it to start rejecting PdxInstances of subclasses of the constraint class.
For example, if the region is constrained to hold Person objects and a client entered an Employee object (subclass of Person) into the region a server would reject the value if we checked that the PdxInstance's class name had to match Person.
It's enough that the original cache that entered the object into the system performed a constraint check on the original object before it was serialized. That's not the case for Json objects coming from the Rest API - they need to be checked and that is the reason for this change.
> On Nov. 8, 2016, 1:44 a.m., Darrel Schneider wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java, line 3323
> > <https://reviews.apache.org/r/53557/diff/1/?file=1556244#file1556244line3323>
> >
> > Is ".equals" too specific? You want something like instanceof. For example if the value-constraint was "Number" you would want it to be satisfied if "valueClassName" was "Integer", "Long", etc.
> >
> > You could do this with Class.isAssignableFrom(this.valueConstraint) but in that case you need to load a class for "valueClassName". One of the goals of pdx is that you do not need to be able to load the domain classes on the server, so you might not want to do this check. If you leave it with just a simple equals check you should document that in the case of pdx the value-constraint needs to exactly match (i.e. no instanceof support).
In the case of Rest objects there are no classes to perform this kind of check so equals() is the correct operation to use. For non-Pdx objects we already use instanceof checks. For non-Pdx objects we perform no constraint check at all because it's already been performed and we can't be assured that we have classes for the objects held by the PdxInstance anyway.
- Bruce
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53557/#review155241
-----------------------------------------------------------
On Nov. 7, 2016, 10:13 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53557/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2016, 10:13 p.m.)
>
>
> Review request for geode, Barry Oglesby, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-2080
> https://issues.apache.org/jira/browse/GEODE-2080
>
>
> Repository: geode
>
>
> Description
> -------
>
> If you set a value constraint on a cache Region you will be unable to store objects in the region via the Rest API. This change-set modifies LocalRegion's constraint check to look for a Rest document and use its type name in the constraint check
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 3873e6e159ebba4c1a288e9fccde5dbabd2a1140
> geode-core/src/test/java/org/apache/geode/pdx/PdxClientServerDUnitTest.java 9a9680a7e14ddec91f005fa0f0c6c3da8d033df2
>
> Diff: https://reviews.apache.org/r/53557/diff/
>
>
> Testing
> -------
>
> new test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 53557: GEODE-2080 Rest POST put call not working
with region valueConstrain
Posted by Bruce Schuchardt <bs...@pivotal.io>.
> On Nov. 8, 2016, 1:44 a.m., Darrel Schneider wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java, line 3323
> > <https://reviews.apache.org/r/53557/diff/1/?file=1556244#file1556244line3323>
> >
> > Is ".equals" too specific? You want something like instanceof. For example if the value-constraint was "Number" you would want it to be satisfied if "valueClassName" was "Integer", "Long", etc.
> >
> > You could do this with Class.isAssignableFrom(this.valueConstraint) but in that case you need to load a class for "valueClassName". One of the goals of pdx is that you do not need to be able to load the domain classes on the server, so you might not want to do this check. If you leave it with just a simple equals check you should document that in the case of pdx the value-constraint needs to exactly match (i.e. no instanceof support).
>
> Bruce Schuchardt wrote:
> In the case of Rest objects there are no classes to perform this kind of check so equals() is the correct operation to use. For non-Pdx objects we already use instanceof checks. For non-Pdx objects we perform no constraint check at all because it's already been performed and we can't be assured that we have classes for the objects held by the PdxInstance anyway.
Edited: For non-Pdx objects we already use instanceof checks. For **PdxInstance objects** we perform no constraint check at all because it's already been performed and we can't be assured that we have classes for the objects held by the PdxInstance anyway.
- Bruce
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53557/#review155241
-----------------------------------------------------------
On Nov. 7, 2016, 10:13 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53557/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2016, 10:13 p.m.)
>
>
> Review request for geode, Barry Oglesby, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-2080
> https://issues.apache.org/jira/browse/GEODE-2080
>
>
> Repository: geode
>
>
> Description
> -------
>
> If you set a value constraint on a cache Region you will be unable to store objects in the region via the Rest API. This change-set modifies LocalRegion's constraint check to look for a Rest document and use its type name in the constraint check
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 3873e6e159ebba4c1a288e9fccde5dbabd2a1140
> geode-core/src/test/java/org/apache/geode/pdx/PdxClientServerDUnitTest.java 9a9680a7e14ddec91f005fa0f0c6c3da8d033df2
>
> Diff: https://reviews.apache.org/r/53557/diff/
>
>
> Testing
> -------
>
> new test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 53557: GEODE-2080 Rest POST put call not working
with region valueConstrain
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53557/#review155241
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java (line 3273)
<https://reviews.apache.org/r/53557/#comment225108>
If pdx.getClassName() is not equals to JSON_CLASSNAME then would it make sense to set valueClassName to pdx.getClassName()? That way you could have "read-serialized" set to true and still have a meaningful value constraint with pdx
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java (line 3281)
<https://reviews.apache.org/r/53557/#comment225110>
Is ".equals" too specific? You want something like instanceof. For example if the value-constraint was "Number" you would want it to be satisfied if "valueClassName" was "Integer", "Long", etc.
You could do this with Class.isAssignableFrom(this.valueConstraint) but in that case you need to load a class for "valueClassName". One of the goals of pdx is that you do not need to be able to load the domain classes on the server, so you might not want to do this check. If you leave it with just a simple equals check you should document that in the case of pdx the value-constraint needs to exactly match (i.e. no instanceof support).
- Darrel Schneider
On Nov. 7, 2016, 2:13 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53557/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2016, 2:13 p.m.)
>
>
> Review request for geode, Barry Oglesby, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-2080
> https://issues.apache.org/jira/browse/GEODE-2080
>
>
> Repository: geode
>
>
> Description
> -------
>
> If you set a value constraint on a cache Region you will be unable to store objects in the region via the Rest API. This change-set modifies LocalRegion's constraint check to look for a Rest document and use its type name in the constraint check
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 3873e6e159ebba4c1a288e9fccde5dbabd2a1140
> geode-core/src/test/java/org/apache/geode/pdx/PdxClientServerDUnitTest.java 9a9680a7e14ddec91f005fa0f0c6c3da8d033df2
>
> Diff: https://reviews.apache.org/r/53557/diff/
>
>
> Testing
> -------
>
> new test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>