juneau-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Bognar <jamesbog...@gmail.com>
Subject Re: JsonParser fails when getters/setters of bean have inconsistent types
Date Tue, 22 Aug 2017 01:53:48 GMT
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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message