juneau-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Goddard <godd...@acm.org>
Subject Re: JsonParser fails when getters/setters of bean have inconsistent types
Date Tue, 22 Aug 2017 19:24:03 GMT
My scenario did not have explicit setters/getters, but rather a public p 
and something that looks like a getter (and no setter):

 >     public boolean isP();
 >     public String p;

Perhaps the problem is that it's neither completely one thing nor the other.

Your example is one of many where there is possible ambiguity:
 >     public boolean isP();
 >     public void setP(String);
 >     public void setP(Foo);

I'd agree in the above case, the safest option would be to assume P is 
read-only.  I'd suggest that as a rule if the apparent getter/setter 
type is different from the property type then we should consider them 
unrelated (i.e. not a getter/setter for that property).  Then in the 
above case, P would only have a getter and not a setter (because the 
other two should be ignored as having incompatible types) and hence 
would be read-only.

David


On 22/08/2017 15:55, James Bognar wrote:
> Perhaps we don't need a restriction that the getter and setter be the same
> type.  From our perspective, I can't think of a good reason why you can't
> have...
> 
>     public boolean isP();
>     public void setP(String p);
> 
> 
> Getters/setters take precedence over fields, so in the following case, the
> getter would be used by JsonSerializer, and the field would be used by
> JsonParser....
> 
>     public boolean isP();
>     public String p;
> 
> 
> One question remains about how to resolve the setter when multiple setters
> are found but none match the getter type:
> 
>     public boolean isP();
>     public void setP(String);
>     public void setP(Foo);
> 
> Ignore the setter in this case and assume the property is read-only?
> 
> 
> On Mon, Aug 21, 2017 at 9:53 PM, James Bognar <jamesbognar@gmail.com> wrote:
> 
>> Thanks David!  That's some in-depth debugging!
>>
>> I'll look at this carefully tomorrow.  It's not immediately obvious to me
>> what the correct behavior should be in this case.
>>
>> On Sat, Aug 19, 2017 at 3:52 PM David Goddard <goddard@acm.org> wrote:
>>
>>> Hi,
>>>
>>> I think this is probably a bug, and I will log a JIRA, but I thought I'd
>>> throw it out here first.  I'm finding (after a lot of time narrowing
>>> down the cause) that I am unable to parse JSON into an object that
>>> contains a method with a certain name.
>>>
>>> However, this is quite a long message, so the tldr summary is:
>>>     If the JsonParser attempts to parse an object into a class that
>>> abuses the setter/getter naming conventions by having what looks like a
>>> boolean getter for a non-boolean property, it fails, complaining that
>>> the property is unknown.
>>>
>>> Here is my test code, using 6.3.1-INCUBATING:
>>>
>>>     package test;
>>>     import org.apache.juneau.json.*;
>>>
>>>     public class ClassParseTest {
>>>
>>>       public static void main(String[] args) {
>>>
>>>         String json = "{\"p1\":\"p1val\",\"p2\":\"p2val\"}";
>>>
>>>         try {
>>>           JsonParser p = new JsonParserBuilder().build();
>>>           p.parse(json, GoodClass.class);
>>>           p.parse(json, DodgyClass.class);
>>>         } catch (Exception e) {
>>>           e.printStackTrace();
>>>         }
>>>
>>>       }
>>>
>>>     }
>>>
>>> The code is able to successfully parse GoodClass, but fails on
>>> DodgyClass with this error:
>>>
>>>     org.apache.juneau.parser.ParseException: Parse exception occurred at
>>>     {currentClass:'DodgyClass',line:1,column:7}.  Unknown property 'p1'
>>>     encountered while trying to parse into class 'test.DodgyClass'
>>>
>>> Here is the offending class:
>>>
>>>     public class DodgyClass {
>>>
>>>       public boolean isP1() {
>>>         return false;
>>>       }
>>>
>>>       public String p1;
>>>       public String p2;
>>>
>>>     }
>>>
>>> And here is the working class:
>>>
>>>     public class GoodClass {
>>>
>>>       public boolean somethingP1() {
>>>         return false;
>>>       }
>>>
>>>       public String p1;
>>>       public String p2;
>>>
>>>     }
>>>
>>> Note that the only difference here is that the former contains a method
>>> called "isP1", while the latter has had this method renamed.  Any other
>>> name at all works fine - it is just "isP1" that fails.
>>>
>>> In the debugger, I see that in BeanMap.java, for the broken class
>>> getPropertyMeta is using incorrect data:
>>>
>>>     public BeanPropertyMeta getPropertyMeta(String propertyName) {
>>>       BeanPropertyMeta bpMeta = meta.properties.get(propertyName);
>>>       if (bpMeta == null)
>>>         bpMeta = meta.dynaProperty;
>>>       return bpMeta;
>>>     }
>>>
>>> The meta variable in the BeanMap only contains p2:
>>>     test.DodgyClass {
>>>       p2: java.lang.String, field=[public java.lang.String
>>>           test.DodgyClass.p2], getter=[null], setter=[null],
>>>     }
>>>
>>> Whereas for the working class, it is complete:
>>>
>>>     test.GoodClass {
>>>       p1: java.lang.String, field=[public java.lang.String
>>>           test.GoodClass.p1], getter=[null], setter=[null],
>>>       p2: java.lang.String, field=[public java.lang.String
>>>           test.GoodClass.p2], getter=[null], setter=[null],
>>>     }
>>>
>>> Hence getPropertyMeta returns null and the parse fails, as the parser
>>> thinks that p1 does not exist in the class.
>>>
>>> During the construction of the bean meta in BeanMeta line 286 we see:
>>>
>>>     for (BeanMethod bm : bms) {
>>>       String pn = bm.propertyName;
>>>       Method m = bm.method;
>>>       if (! normalProps.containsKey(pn))
>>>           normalProps.put(pn, new BeanPropertyMeta.Builder(beanMeta, pn));
>>>       BeanPropertyMeta.Builder bpm = normalProps.get(pn);
>>>       if (! bm.isSetter)
>>>         bpm.setGetter(m);
>>>     }
>>>
>>> For the method DodgyClass.isP1(), bpm.setGetter(m) is invoked, which
>>> sets isP1() as a getter for the p1 field.
>>>
>>> In line 315 of BeanMeta, some setter/getter based validation occurs:
>>>
>>>     if (p.validate(ctx, beanRegistry, typeVarImpls)) {
>>>       if (p.getter != null)
>>>         getterProps.put(p.getter, p.name);
>>>       if (p.setter != null)
>>>         setterProps.put(p.setter, p.name);
>>>     } else {
>>>         i.remove();
>>>     }
>>>
>>> For the affected class, p.validate() returns false for p1.  It does so
>>> on this part of BeanPropertyMeta, line 171:
>>>
>>>     // Do some annotation validation.
>>>     Class<?> c = rawTypeMeta.getInnerClass();
>>>     if (getter != null) {
>>>       if (isDyna) {
>>>         if (! isParentClass(Map.class, c))
>>>           return false;
>>>       } else {
>>>         if (! isParentClass(getter.getReturnType(), c))
>>>           return false;
>>>       }
>>>     }
>>>
>>> Because isParentClass(getter.getReturnType(), c) returns false for p1 in
>>> the affected class the call to validate() returns false and hence p1 is
>>> removed from the meta.properties for the bean and this ultimately causes
>>> the problem.
>>>
>>> I've not yet dug into whether isParentClass() is behaving as expected; I
>>> guess it probably is.
>>>
>>> We should bear in mind that p1 is a String in this test, and isP1() is a
>>> boolean, and that the problem does not occur if both are boolean.  My
>>> question here is whether this is a valid bug, or simply a case of "don't
>>> do that then".  The problem originally arose for me because I added a
>>> boolean method to a class that did some logic based on a string property
>>> P, and happened to name that method "isP", causing the parser to
>>> identify it as a getter for P when it is not really.  This is probably
>>> something of an edge case because it is quite a specific circumstance
>>> that causes it, and it is also easy to resolve by renaming the method to
>>> not sound like a getter.  Given that Java naming conventions state that
>>> a getter for a boolean should begin with "is", it's a pretty good
>>> argument that abusing this in this way is asking for trouble.
>>>
>>> If we view this as actually a limitation in the parser rather than user
>>> error, then I guess one solution would be to enhance the logic for
>>> determining setters/getters to only consider a method to be a
>>> getter/setter if its return/input type matches the type of the variable.
>>>    This is something I'd be happy to look further at.  Either way, it's
>>> probably helpful to have this error and a potential cause documented in
>>> the mailing list.
>>>
>>> Regards,
>>>
>>> David
>>>
>>>
> 


Mime
View raw message