Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Proposed Cleanup items for BeanPropertyIntrospector #133

Open
wants to merge 7 commits into
base: 2.17
Choose a base branch
from
Open

Proposed Cleanup items for BeanPropertyIntrospector #133

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
64f6d1a
Proposed Cleanup items for BeanPropertyIntrospector to reduce complex…
Shounaks Mar 8, 2024
ae177f4
Fix: isGetter was missed
Shounaks Mar 16, 2024
2d2c186
Fix: merge conflicts
Shounaks Mar 16, 2024
b2696e3
Fix: Public condition fix
Shounaks Mar 16, 2024
647e4ab
Merge branch '2.17' into 2.17-bean-property-introspector-cleanup
Shounaks Mar 21, 2024
e476878
Merge branch '2.17' into 2.17-bean-property-introspector-cleanup
Shounaks Mar 24, 2024
8202427
Merge branch '2.17' into 2.17-bean-property-introspector-cleanup
Shounaks Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
274 changes: 131 additions & 143 deletions jr-objects/src/main/java/com/fasterxml/jackson/jr/ob/impl/BeanPropertyIntrospector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,40 +1,42 @@
package com.fasterxml.jackson.jr.ob.impl;

import java.lang.reflect.*;
import com.fasterxml.jackson.jr.ob.impl.POJODefinition.Prop;
import com.fasterxml.jackson.jr.ob.impl.POJODefinition.PropBuilder;

import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.HashMap;
import java.util.Map;
import java.util.TreeMap;

import com.fasterxml.jackson.jr.ob.JSON;
import com.fasterxml.jackson.jr.ob.impl.POJODefinition.Prop;
import com.fasterxml.jackson.jr.ob.impl.POJODefinition.PropBuilder;
import static com.fasterxml.jackson.jr.ob.JSON.Feature.INCLUDE_STATIC_FIELDS;
import static com.fasterxml.jackson.jr.ob.JSON.Feature.USE_FIELD_MATCHING_GETTERS;
import static java.lang.Character.isLowerCase;
import static java.lang.Character.toLowerCase;
import static java.lang.reflect.Modifier.*;

/**
* Helper class that jackson-jr uses by default to introspect POJO properties
* (represented as {@link POJODefinition}) to build general POJO readers
* (deserializers) and writers (serializers).
*<p>
* <p>
* Note that most of the usage is via {@link ValueReaderLocator} and
* {@link ValueWriterLocator}
*
* @since 2.11
*/
public class BeanPropertyIntrospector
{
protected final static Prop[] NO_PROPS = new Prop[0];

private final static BeanPropertyIntrospector INSTANCE = new BeanPropertyIntrospector();

public BeanPropertyIntrospector() { }

public static BeanPropertyIntrospector instance() { return INSTANCE; }
public final class BeanPropertyIntrospector {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably can't make a class final in a patch - needs a minor release ideally

private BeanPropertyIntrospector() {
}

public POJODefinition pojoDefinitionForDeserialization(JSONReader r, Class<?> pojoType) {
return _introspectDefinition(pojoType, false, r.features());
public static POJODefinition pojoDefinitionForDeserialization(JSONReader r, Class<?> pojoType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't change a public method to be static in a patch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get your concern, this change can have unforeseen side-effects.
Currently this is just a proof of concept, to simplify and cleanup this class.

we can take items from here individually (if not all). or not use this at all (whatever is feasible)

return introspectDefinition(pojoType, false, r.features());
}

public POJODefinition pojoDefinitionForSerialization(JSONWriter w, Class<?> pojoType) {
return _introspectDefinition(pojoType, true, w.features());
public static POJODefinition pojoDefinitionForSerialization(JSONWriter w, Class<?> pojoType) {
return introspectDefinition(pojoType, true, w.features());
}

/*
Expand All @@ -43,152 +45,73 @@ public POJODefinition pojoDefinitionForSerialization(JSONWriter w, Class<?> pojo
/**********************************************************************
*/

private POJODefinition _introspectDefinition(Class<?> beanType,
boolean forSerialization, int features)
{
Map<String,PropBuilder> propsByName = new TreeMap<>();
_introspect(beanType, propsByName, features);

final BeanConstructors constructors;

if (forSerialization) {
constructors = null;
} else {
constructors = new BeanConstructors(beanType);
for (Constructor<?> ctor : beanType.getDeclaredConstructors()) {
Class<?>[] argTypes = ctor.getParameterTypes();
if (argTypes.length == 0) {
constructors.addNoArgsConstructor(ctor);
} else if (argTypes.length == 1) {
Class<?> argType = argTypes[0];
if (argType == String.class) {
constructors.addStringConstructor(ctor);
} else if (argType == Integer.class || argType == Integer.TYPE) {
constructors.addIntConstructor(ctor);
} else if (argType == Long.class || argType == Long.TYPE) {
constructors.addLongConstructor(ctor);
}
}
}
}

final int len = propsByName.size();
Prop[] props;
if (len == 0) {
props = NO_PROPS;
} else {
props = new Prop[len];
int i = 0;
for (PropBuilder builder : propsByName.values()) {
props[i++] = builder.build();
}
}
return new POJODefinition(beanType, props, constructors);
}

private static void _introspect(Class<?> currType, Map<String, PropBuilder> props,
int features)
{
/**
* Brain Method of {@link BeanPropertyIntrospector}, used to get the list of props
*/
private static void _introspect(Class<?> currType, Map<String, PropBuilder> props, int features) {
if (currType == null || currType == Object.class) {
return;
}
// First, check base type
_introspect(currType.getSuperclass(), props, features);

final boolean noStatics = JSON.Feature.INCLUDE_STATIC_FIELDS.isDisabled(features);
final boolean isFieldNameGettersEnabled = JSON.Feature.USE_FIELD_MATCHING_GETTERS.isEnabled(features);

final boolean isFieldNameGettersEnabled = USE_FIELD_MATCHING_GETTERS.isEnabled(features);
final Map<String, Field> fieldNameMap = isFieldNameGettersEnabled ? new HashMap<>() : null;
_populatePropsWithField(fieldNameMap, currType, props, features);
_populatePropWithGettersAndSetters(isFieldNameGettersEnabled, fieldNameMap, currType, props);
}

// then public fields (since 2.8); may or may not be ultimately included
// but at this point still possible
for (Field f : currType.getDeclaredFields()) {
if (fieldNameMap != null) {
fieldNameMap.put(f.getName(), f);
}
if (!Modifier.isPublic(f.getModifiers()) || f.isEnumConstant() || f.isSynthetic()) {
continue;
}
// Only include static members if (a) inclusion feature enabled and
// (b) not final (cannot deserialize final fields)
if (Modifier.isStatic(f.getModifiers()) && (noStatics || Modifier.isFinal(f.getModifiers()))) {
continue;
}
_propFrom(props, f.getName()).withField(f);
}

private static void _populatePropWithGettersAndSetters(boolean isFieldNameGettersEnabled, Map<String, Field> fieldMap, Class<?> currType, Map<String, PropBuilder> props) {
// then get methods from within this class
for (Method m : currType.getDeclaredMethods()) {
final int flags = m.getModifiers();
// 13-Jun-2015, tatu: Skip synthetic, bridge methods altogether, for now
// at least (add more complex handling only if absolutely necessary)
if (Modifier.isStatic(flags) || m.isSynthetic() || m.isBridge() || isGroovyMetaClass(m.getReturnType())) {
final Class<?> returnType = m.getReturnType();

// 13-Jun-2015, tatu:
// Skip synthetic, bridge methods altogether, for now
// at least (add more complex handling only if absolutely necessary)
if (isStatic(flags) || m.isSynthetic() || m.isBridge() || isGroovyMetaClass(returnType)) {
continue;
}
Class<?> argTypes[] = m.getParameterTypes();
if (argTypes.length == 0) { // getter?
// getters must be public to be used
if (!Modifier.isPublic(flags)) {
continue;
}

Class<?> resultType = m.getReturnType();
if (resultType == Void.class) {
continue;
}
String name = m.getName();
if (name.startsWith("get")) {
if (name.length() > 3) {
name = decap(name.substring(3));
_propFrom(props, name).withGetter(m);
}
} else if (name.startsWith("is")) {
if (name.length() > 2) {
// May or may not be used, but collect for now all the same:
name = decap(name.substring(2));
_propFrom(props, name).withIsGetter(m);
}
} else if (isFieldNameGettersEnabled) {
// 10-Mar-2024: [jackson-jr#94]:
// This will allow getters with field name as their getters,
// like the ones generated by Groovy (or JDK 17 for Records).
// If method name matches with field name, & method return
// type matches the field type only then it can be considered a getter.
Field field = fieldNameMap.get(name);
if (field != null && Modifier.isPublic(m.getModifiers()) && m.getReturnType().equals(field.getType())) {
// NOTE: do NOT decap, field name should be used as-is
_propFrom(props, name).withGetter(m);
}
}
final Class<?>[] argTypes = m.getParameterTypes();
if (argTypes.length == 0 && returnType != Void.class) { // getter?
if(!isPublic(flags)) continue;
generatePropsWithGetter(m, props);
generatePropsWithIsGetter(m, props);
generatePropsWithFieldMatchingGetter(isFieldNameGettersEnabled, fieldMap, m, props);
} else if (argTypes.length == 1) { // setter?
// Non-public setters are fine if we can force access, don't yet check
// let's also not bother about return type; setters that return value are fine
String name = m.getName();
if (!name.startsWith("set") || name.length() == 3) {
continue;
}
name = decap(name.substring(3));
_propFrom(props, name).withSetter(m);
generatePropsWithSetter(m, props);
}
}
}

private static void _populatePropsWithField(Map<String, Field> fieldNameMap, Class<?> currType, Map<String, PropBuilder> props, int features) {
// then public fields (since 2.8); may or may not be ultimately included but at this point still possible
// Also, only include static members if
// (a) inclusion feature enabled and
// (b) not final (cannot deserialize final fields)
for (Field f : currType.getDeclaredFields()) {
if (fieldNameMap != null) {
fieldNameMap.put(f.getName(), f);
}
if (!isPublic(f.getModifiers()) || f.isEnumConstant() || f.isSynthetic() || (isStatic(f.getModifiers()) && (INCLUDE_STATIC_FIELDS.isDisabled(features) || isFinal(f.getModifiers())))) {
continue;
}
propFrom(props, f.getName()).withField(f);
}
}

private static PropBuilder _propFrom(Map<String,PropBuilder> props, String name) {
private static PropBuilder propFrom(Map<String, PropBuilder> props, String name) {
return props.computeIfAbsent(name, Prop::builder);
}

private static String decap(String name) {
char c = name.charAt(0);
char lowerC = Character.toLowerCase(c);

if (c != lowerC) {
// First: do NOT lower case if more than one leading upper case letters:
if ((name.length() == 1)
|| !Character.isUpperCase(name.charAt(1))) {
char chars[] = name.toCharArray();
chars[0] = lowerC;
return new String(chars);
}
private static String _decap(String name) {
if (!isLowerCase(name.charAt(0)) && ((name.length() == 1) || !Character.isUpperCase(name.charAt(1)))) {
final char[] chars = name.toCharArray();
chars[0] = toLowerCase(name.charAt(0));
return new String(chars);
}
return name;
}
Expand All @@ -199,7 +122,72 @@ private static String decap(String name) {
* @implNote Groovy MetaClass have cyclic reference, and hence the class containing it should not be serialised without
* either removing that reference, or skipping over such references.
*/
protected static boolean isGroovyMetaClass(Class<?> clazz) {
private static boolean isGroovyMetaClass(Class<?> clazz) {
return "groovy.lang.MetaClass".equals(clazz.getName());
}
}

private static void generatePropsWithFieldMatchingGetter(boolean isFieldNameGettersEnabled,Map<String, Field> fieldNameMap, final Method m, final Map<String, PropBuilder> props) {
final String name = m.getName();
if (isFieldNameGettersEnabled) {
// 10-Mar-2024: [jackson-jr#94]:
// This will allow getters with field name as their getters,
// like the ones generated by Groovy (or JDK 17 for Records).
// If method name matches with field name, & method return
// type matches the field type only then it can be considered a getter.
Field field = fieldNameMap.get(name);
if (field != null && Modifier.isPublic(m.getModifiers()) && m.getReturnType().equals(field.getType())) {
// NOTE: do NOT decap, field name should be used as-is
propFrom(props, name).withGetter(m);
}
}
}

private static void generatePropsWithGetter(final Method method, final Map<String, PropBuilder> props) {
final String getterPrefix = "get";
final String name = method.getName();
if (name.startsWith(getterPrefix) && name.length() > getterPrefix.length()) {
propFrom(props, _decap(name.substring(getterPrefix.length()))).withGetter(method);
}
}

private static void generatePropsWithIsGetter(final Method method, final Map<String, PropBuilder> props) {
final String isGetterPrefix = "is";
final String name = method.getName();
if (name.startsWith(isGetterPrefix) && name.length() > isGetterPrefix.length()) {
propFrom(props, _decap(name.substring(isGetterPrefix.length()))).withIsGetter(method);
}
}

private static void generatePropsWithSetter(final Method method, final Map<String, PropBuilder> props) {
final String setterPrefix = "set";
final String name = method.getName();
if (name.startsWith(setterPrefix) && name.length() > setterPrefix.length()) {
propFrom(props, _decap(name.substring(setterPrefix.length()))).withSetter(method);
}
}

private static POJODefinition introspectDefinition(Class<?> beanType, boolean forSerialization, int features) {
final Map<String, PropBuilder> propsByName = new TreeMap<>();
_introspect(beanType, propsByName, features);

final BeanConstructors constructors = new BeanConstructors(beanType);
for (Constructor<?> ctor : beanType.getDeclaredConstructors()) {
final Class<?>[] argTypes = ctor.getParameterTypes();
if (argTypes.length == 0) {
constructors.addNoArgsConstructor(ctor);
} else if (argTypes.length == 1) {
final Class<?> argType = argTypes[0];
if (argType == String.class) {
constructors.addStringConstructor(ctor);
} else if (argType == Integer.class || argType == Integer.TYPE) {
constructors.addIntConstructor(ctor);
} else if (argType == Long.class || argType == Long.TYPE) {
constructors.addLongConstructor(ctor);
}
}
}

final Prop[] props = propsByName.values().stream().map(PropBuilder::build).toArray(Prop[]::new);
return new POJODefinition(beanType, props, forSerialization ? null : constructors);
}
}
15 changes: 10 additions & 5 deletions jr-objects/src/main/java/com/fasterxml/jackson/jr/ob/impl/ValueReaderLocator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
package com.fasterxml.jackson.jr.ob.impl;

import java.lang.reflect.*;
import java.util.*;
import java.util.concurrent.ConcurrentHashMap;

import com.fasterxml.jackson.jr.ob.JSON;
import com.fasterxml.jackson.jr.ob.api.ReaderWriterModifier;
import com.fasterxml.jackson.jr.ob.api.ReaderWriterProvider;
Expand All @@ -12,6 +8,15 @@
import com.fasterxml.jackson.jr.type.TypeBindings;
import com.fasterxml.jackson.jr.type.TypeResolver;

import java.lang.reflect.Field;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.lang.reflect.Type;
import java.util.*;
import java.util.concurrent.ConcurrentHashMap;

import static com.fasterxml.jackson.jr.ob.impl.BeanPropertyIntrospector.pojoDefinitionForDeserialization;

/**
* Helper object used for efficient detection of type information relevant
* to our conversion needs when writing out Java Objects as JSON.
Expand Down Expand Up @@ -430,7 +435,7 @@ protected POJODefinition _resolveBeanDef(Class<?> raw) {
return def;
}
}
return BeanPropertyIntrospector.instance().pojoDefinitionForDeserialization(_readContext, raw);
return pojoDefinitionForDeserialization(_readContext, raw);
} catch (Exception e) {
throw new IllegalArgumentException(String.format
("Failed to introspect ClassDefinition for type '%s': %s",
Expand Down