auth0-java icon indicating copy to clipboard operation
auth0-java copied to clipboard

User object blocked field should be primitive or accessor renamed

Open scarter93 opened this issue 3 years ago • 4 comments

Describe the problem you'd like to have solved

On the com.auth0.json.mgmt.users.User class there is a field private Boolean blocked. The accessor for this field is public Boolean isBlocked() {...}. This does not conform to Java bean specification.

This causes an issue in java.beans.PropertyDescriptor::getReadMethod() so the field cannot be found via reflection. This can in turn affects libraries using the Java Beans specification which rely on this method to reflectively populate dynamic data at runtime.

Describe the ideal solution

Make the field blocked a primitive type as false is effectively equal to a non-existent value in this case.

Alternatives and current work-arounds

Alternative: Deprecate public Boolean isBlocked() {...} and add public Boolean getBlocked() {...}

Depending on the library it may be possible to work around the issue, by providing a custom field and look up to use the isBlocked() method.

Additional information, if any

Java beans specification is arguably "outdated" as it does not explicitly define behavior around primitive wrappers such as Boolean or handle it as a boolean. This is why I recommend leveraging the primitive if possible, rather than the alternative of changing to getBlocked(). I believe this should have minimal consequence.

https://download.oracle.com/otndocs/jcp/7224-javabeans-1.01-fr-spec-oth-JSpec/

scarter93 avatar Feb 07 '22 22:02 scarter93

Thanks for the details @scarter93. I don't think we can change the field to be a primitive type, as when serializing the object we don't serialize null fields. If we change to a primitive instead of omitting unset Boolean fields, we'd send false when calling endpoints, which could lead to unexpected and breaking change behavior.

Regarding adding a get<Field>, I think that would conflict with the JavaBeans convention of using is<Field> as accessors for boolean properties.

Depending on the library it may be possible to work around the issue, by providing a custom field and look up to use the isBlocked() method.

Can you expand on this workaround for mine and other's full understanding? Thanks!

jimmyjames avatar Feb 08 '22 14:02 jimmyjames

@jimmyjames Understood, agreed the Boolean Object makes sense in that case!

I agree that boolean accessor should be is is<Field>, but Java Beans doesn't account for the primitive wrapper Boolean which is where the issues arises. Code in question:

/**
 * Gets the method that should be used to read the property value.
 *
 * @return The method that should be used to read the property value.
 * May return null if the property can't be read.
 */
public synchronized Method getReadMethod() {
    Method readMethod = this.readMethodRef.get();
    if (readMethod == null) {
        Class<?> cls = getClass0();
        if (cls == null || (readMethodName == null && !this.readMethodRef.isSet())) {
            // The read method was explicitly set to null.
            return null;
        }
        String nextMethodName = Introspector.GET_PREFIX + getBaseName();
        if (readMethodName == null) {
            Class<?> type = getPropertyType0();
            if (type == boolean.class || type == null) {
                readMethodName = Introspector.IS_PREFIX + getBaseName();
            } else {
                readMethodName = nextMethodName;
            }
        }

        // Since there can be multiple write methods but only one getter
        // method, find the getter method first so that you know what the
        // property type is.  For booleans, there can be "is" and "get"
        // methods.  If an "is" method exists, this is the official
        // reader method so look for this one first.
        readMethod = Introspector.findMethod(cls, readMethodName, 0);
        if ((readMethod == null) && !readMethodName.equals(nextMethodName)) {
            readMethodName = nextMethodName;
            readMethod = Introspector.findMethod(cls, readMethodName, 0);
        }
        try {
            setReadMethod(readMethod);
        } catch (IntrospectionException ex) {
            // fall
        }
    }
    return readMethod;
}

From java.beans.PropertyDescriptor.

The workaround I mentioned would be library specific, for my use case the data was being displayed in a Vaadin Grid (Version 21). They provide a functional interface that allows a custom "Value Provider" for a column, I was able to create a column in the grid and then provide the User::isBlocked as the value source.

~~I did find another alternative to register a custom Boolean bean introspector and register this with Java Beans. BeanUtilsBean.getInstance().getPropertyUtils().addBeanIntrospector(new BooleanIntrospector()); I would say this is the cleanest solution on my side to get it to work without adjusting the underlying data model.~~

I wanted to submit an issue to see if the primitive change was a quick win, if not I think the cleanest alternative is the above.

EDIT: Looks like the custom introspector requires apache bean utils. Need to research more to see if its possible with the base Java Beans Implementation

scarter93 avatar Feb 08 '22 15:02 scarter93

Thanks for the info @scarter93, it may help others if they happen to run into this issue. It may not be too bad to add get<BooleanField> accessors. I don't want to signal right now that this library entirely conforms to the JavaBean spec though, so if we do add those accessors it would be a one-off; do you know of other property getters/setters that cause issues?

jimmyjames avatar Feb 16 '22 23:02 jimmyjames

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

stale[bot] avatar Jul 31 '22 07:07 stale[bot]

Closing due to inactivity, but we will evaluate how we handle boolean fields in the next release to either better conform to JavaBeans or provide workarounds discussed above.

jimmyjames avatar Aug 30 '22 01:08 jimmyjames