JSONObject#populateMap() calls methods that aren't valid object getters

[run2000]

JSONObject.populateMap() can be a over-enthusiastic in calling methods on an object that aren't valid bean getter methods. For instance:

• It will call static get methods that otherwise pass all the tests for validity.
• It will call get methods with a void return type
• It will call a get method that is a synthetic method -- typically the result of overriding a superclass

My opinion is that populateMap() should be more cautious about calling methods on an unknown object. There may be side-effecty behaviour within the object that's undesirable. Code to avoid doing this is trivial and fast.

[stleary]

I think the idea of more careful handling of beans is reasonable.

Don't know if we can be sure that no users will ever want to serialize static data. For example, suppose there is only one instance of the class, and its data is static?

Making sure the method has a return type sounds reasonable.

Not sure why synthetic methods must be excluded from serialization. Can you give an example?

Might also want to address enum serialization as a bean, which I think picks up some unwanted getters.

[johnjaylward]

I think this ties in a little with #264.

For Statics:
I can't really see the need to serialize static methods at all. By definition static methods indicate an application level state (or constants that never change) and outputting those as part of the bean state seems like a bug to me.

Void return types:
I agree, these should be ignored. These likely are causing side effects in the objects

Synthetic Methods:
My reflection may be a little rusty, but I'm not sure I see why we should ignore these. I agree with @stleary that a use-case for ignoring these would be helpful.

[johnjaylward]

@stleary enum serialization was corrected in PR #271. It should not be output as a bean anymore.

[erosb]

In a slightly broader context, I have doubts about if supporting bean <-> JSONObject conversions is a good idea at all. Maybe, for the sake of simplicity, org.json should support only JSONObject <-> Map conversions and let people perform the more complex task (map <-> bean) perform with a library (eg. apache beanutils) which is better suited for this purpose.

Maybe it would be a better idea to deprecate the JSONObject(Object) constructor instead of maintaining it.

[johnjaylward]

@erosb I personally do not use the constructor. I've been using my own custom bean -> JSONObject wrapper for around 10 years now. Mine does 2 of the 3 checks (no statics, and no void returns) mentioned above.

However, I think due to the handover agreement with Douglas, we can not deprecate the constructor. Bug fixes should be fine in it, but I don't think we are allowed to reduce the public API.

[johnjaylward]

For reference, here is the code I use to determine what methods to skip:

    /**
     * @param m
     *            method to check
     * @return true if the method does not match expected properties
     */
    private static boolean skip(final Method m) {
        final int modifiers = m.getModifiers();
        if (
        // skip static
        Modifier.isStatic(modifiers)
        // skip abstract
                || Modifier.isAbstract(modifiers)
                // skip any non-public methods
                || !Modifier.isPublic(modifiers)
                // skip methods with parameters
                || m.getParameterTypes().length != 0) {
            return true;
        }

        final Class<?> returnType = m.getReturnType();
        // skip void, collections, maps
        if (void.class.equals(returnType) || Collection.class.isAssignableFrom(returnType)
                || Map.class.isAssignableFrom(returnType)) {
            return true;
        }

        final String name = m.getName();
        if ("hashCode".equals(name) || "getClass".equals(name)
                || !(name.startsWith("get") || name.startsWith("is")
                    || name.startsWith("has"))
        ) {
             return true;
        }

        return false;
    }

I skip maps and collections because I use this mainly in a REST application and any associated maps/collections are loaded by other web actions. I don't think skipping them would be the right thing to do for populateMap

--edit
I added in the name checking that I use elsewhere in the code.

[johnjaylward]

Looking up synthetic methods I found this: http://www.javaworld.com/article/2073578/java-s-synthetic-methods.html

I'm still not sure that skipping them is the best option. From what I've seen, usually compiler generated methods and classes start with a $, so the current checks on names would exclude them in most cases. I suppose that would be compiler dependant though, and maybe not something we want to rely on?

[run2000]

Perhaps instead of synthetic method, read bridge method. All bridge methods are synthetic methods, but not all synthetic methods are bridge methods.

Bridge methods are generated by the javac compiler as a result of Java 5's greater flexibility in overriding methods based on return types. As an example, given a superclass that looks like this:

public class MySuperBean {
  public Number getNumber() {
    System.out.println("Super method called");
    return new BigDecimal("123.456");
  }
}

And a subclass that looks like this:

public class MySubBean extends MySuperBean {
  @Override
  public BigInteger getNumber() {
    System.out.println("Sub method called");
    return new BigInteger("10000");
  }
}

Note that getNumber() of MySubBean overrides the same method of MySuperBean, even though the return type is more specific than the method it's overriding. To deal with the difference, javac creates a bridge method that delegates from one form to another.

How this affects populateMap() is that when MySubBean is introspected, getNumber() gets called twice -- once directly, and once via the bridge method. You see this by the fact that "Sub method called" appears twice on the console.

The code I have to skip these methods looks like this:

            if (Modifier.isPublic(method.getModifiers()) &&
                    !Modifier.isStatic(method.getModifiers()) &&
                    !method.isBridge() &&
                    (method.getReturnType() != Void.TYPE)) {
                        // ...

[erosb]

+1 for omitting syntetic methods. Conceptually these are methods which are not written by the developer of the class, probably he/she is not even know about its existence. Syntetic methods exist for solely technical, java-specific reasons (see hte above bridge method explanation), therefore mapping them while working with a language-independent representation of the bean is just not useful.

I'm quite sure about that a lot of users of this library don't even know about of syntethic methods, and current behavior may result un unpleasant surprises.

[stleary]

Static methods: No plans to change
Synthetic non-bridge methods: No plans to change, assuming current checks already omit them
Bridge methods: Will consider
Check return type: OK to fix