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.

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

Refactor Bean property introspection #4532

Merged
merged 27 commits into from
May 18, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
0186add
Start work on front-loading Creator detection
cowtowncoder May 17, 2024
1b90882
Minor NPE fix
cowtowncoder May 17, 2024
687d15a
Merge branch '2.18' into feature/2.18/4515-rewrite-prop-introspection
cowtowncoder May 17, 2024
180fd92
Start solving some cases (explicit properties-based)
cowtowncoder May 17, 2024
9008aee
More work on explicit creator detection
cowtowncoder May 18, 2024
38c0d67
Bit more refactoring
cowtowncoder May 18, 2024
cbecdab
Comment out invalid test
cowtowncoder May 18, 2024
77a87d8
Try to avoid bogus conflicts
cowtowncoder May 18, 2024
5c24f74
Fix 4 more test cases
cowtowncoder May 18, 2024
e40740c
Getting there: resolved all but 1 of non-Record test cases
cowtowncoder May 18, 2024
70d10e5
Fixes to tests, test mapper setup
cowtowncoder May 18, 2024
312e0e9
Resolve most of Record test failures too
cowtowncoder May 18, 2024
f28a4f9
Fix one more Record test ("empty" Record)
cowtowncoder May 18, 2024
46616bc
Merge branch '2.18' into feature/2.18/4515-rewrite-prop-introspection
cowtowncoder May 18, 2024
c5576cc
Merge branch '2.18' into feature/2.18/4515-rewrite-prop-introspection
cowtowncoder May 18, 2024
4b59d64
Temporarily disable the one non-Record test that fails; tackle at a l…
cowtowncoder May 18, 2024
dc5dd2f
Merge branch '2.18' into feature/2.18/4515-rewrite-prop-introspection
cowtowncoder May 18, 2024
77c1fb1
Fix 2 more Record tests; 5 to go
cowtowncoder May 18, 2024
9e239f4
Resolve one failing case
cowtowncoder May 18, 2024
bbdbf61
Resolve 2 more fails; only 2 remain
cowtowncoder May 18, 2024
ef7db31
Merge branch '2.18' into feature/2.18/4515-rewrite-prop-introspection
cowtowncoder May 18, 2024
324e4a2
Fix one more test (one fail remains!)
cowtowncoder May 18, 2024
9d18c1a
Fix the last Record test!
cowtowncoder May 18, 2024
d7a5519
Minor tweaking
cowtowncoder May 18, 2024
791e8fa
Last comment tweaks
cowtowncoder May 18, 2024
2b52cd7
Renaming
cowtowncoder May 18, 2024
0a054e0
Remove unused method
cowtowncoder May 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import com.fasterxml.jackson.annotation.JsonFormat;
import com.fasterxml.jackson.databind.*;
import com.fasterxml.jackson.databind.cfg.ConstructorDetector;
import com.fasterxml.jackson.databind.cfg.HandlerInstantiator;
import com.fasterxml.jackson.databind.cfg.MapperConfig;
import com.fasterxml.jackson.databind.jdk14.JDK14Util;
Expand Down Expand Up @@ -88,7 +89,7 @@ public class POJOPropertiesCollector
*/
protected LinkedHashMap<String, POJOPropertyBuilder> _properties;

protected LinkedList<POJOPropertyBuilder> _creatorProperties;
protected List<POJOPropertyBuilder> _creatorProperties;

/**
* A set of "field renamings" that have been discovered, indicating
Expand Down Expand Up @@ -430,7 +431,7 @@ protected void collectAll()
// inner classes, see [databind#1502]
// 13-May-2023, PJ: Need to avoid adding creators for Records when serializing [databind#3925]
if (!_classDef.isNonStaticInnerClass() && !(_forSerialization && isRecord)) {
_addCreators(props);
_addPotentialCreators(props);
}

// Remove ignored properties, first; this MUST precede annotation merging
Expand Down Expand Up @@ -614,9 +615,162 @@ protected void _addFields(Map<String, POJOPropertyBuilder> props)
}
}

// @since 2.18
protected void _addPotentialCreators(Map<String, POJOPropertyBuilder> props)
{
_creatorProperties = new ArrayList<>();

// First, resolve explicit annotations:
List<PotentialCreator> ctors = _collectCreators(_classDef.getConstructors());
List<PotentialCreator> factories = _collectCreators(_classDef.getFactoryMethods());

final PotentialCreators collector = new PotentialCreators(ctors, factories);

// and use them to find highest precedence Creators
if (_useAnnotations) { // can't have explicit ones without Annotation introspection
// Start with Constructors as they have higher precedence:
_addExplicitCreators(collector, collector.constructors, props, false);
// followed by Factory methods (lower precedence)
_addExplicitCreators(collector, collector.factories, props,
collector.propertiesBased != null);
}

final boolean hasExplicit = collector.hasParametersBasedOrDelegating();

// Find canonical (record) Constructor if no explicitly marked creators:
if (!hasExplicit) {
// !!! TODO
}

PotentialCreator primary = collector.propertiesBased;
if (primary != null) {
_addExplicitCreatorParams(props, primary);
}
}

private List<PotentialCreator> _collectCreators(List<? extends AnnotatedWithParams> ctors)
{
List<PotentialCreator> result = null;
for (AnnotatedWithParams ctor : ctors) {
JsonCreator.Mode creatorMode = _useAnnotations
? _annotationIntrospector.findCreatorAnnotation(_config, ctor) : null;
// explicitly prevented? Remove
if (creatorMode == JsonCreator.Mode.DISABLED) {
continue;
}
if (result == null) {
result = new ArrayList<>();
}
result.add(new PotentialCreator(ctor, creatorMode));
}
return (result == null) ? Collections.emptyList() : result;
}

private void _addExplicitCreators(PotentialCreators collector, List<PotentialCreator> ctors,
Map<String, POJOPropertyBuilder> props,
boolean skipPropsBased)
{
final ConstructorDetector ctorDetector = _config.getConstructorDetector();
Iterator<PotentialCreator> it = ctors.iterator();
while (it.hasNext()) {
PotentialCreator ctor = it.next();

final int paramCount = ctor.paramCount();
if (paramCount == 0) {
it.remove();
collector.addDefault(ctor.creator);
continue;
}

if (ctor.creatorMode == null) {
continue;
}
it.remove();

Boolean propsBased = null;

switch (ctor.creatorMode) {
case DELEGATING:
propsBased = false;
break;
case PROPERTIES:
propsBased = true;
break;
case DEFAULT:
default:
// First things first: if not single-arg Creator, must be Properties-based
// !!! Or does it? What if there's @JacksonInject etc?
if (paramCount != 1) {
propsBased = true;
}
}

// Must be 1-arg case:
if (propsBased == null) {
// Is ambiguity/heuristics allowed?
if (ctorDetector.requireCtorAnnotation()) {
throw new IllegalArgumentException(String.format(
"Ambiguous 1-argument Creator; `ConstructorDetector` requires specifying `mode`: %s",
ctor));
}

// First things first: if explicit names found, is Properties-based
ctor.introspectParamNames(_config);
propsBased = ctor.hasExplicitNames()
|| ctorDetector.singleArgCreatorDefaultsToProperties();
// One more possibility: implicit name that maps to implied
// property based on Field/Getter/Setter
if (!propsBased) {
String implName = ctor.implicitNameSimple(0);
propsBased = (implName != null) && props.containsKey(implName);
}
}

if (propsBased) {
// Skipping done if we already got higher-precedence Creator
if (!skipPropsBased) {
collector.addPropertiesBased(_config, ctor, "explicit");
}
} else {
collector.addDelegating(ctor);
}
}
}

private void _addExplicitCreatorParams(Map<String, POJOPropertyBuilder> props,
PotentialCreator ctor)
{
for (int i = 0, len = ctor.paramCount(); i < len; ++i) {
final AnnotatedParameter param = ctor.param(i);
final PropertyName explName = ctor.explicitName(i);
PropertyName implName = ctor.implicitName(i);
final boolean hasExplicit = (explName != null);

if (!hasExplicit) {
Copy link
Member

@JooHyukKim JooHyukKim May 18, 2024

Choose a reason for hiding this comment

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

This may be personal preference.

From here to line 766 seems like we can just end everything with...

if  explicit do explcit handling
else do implicit 

And not mix up two different handling. So that is easier to make changes per case

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I am sure there are ways to clean things up: at first I am trying to replicate existing logic. So once things pass will focus more on clean up.

Exciting thing is that right now I am 1 test fail away from passing all Java 8 tests -- then need to tackle record handling.

And once that happens, then start making use of Creators detected: right now all I have changed is finding properties-based Creators to link properties in POJOPropertiesCollector, but BasicDeserializerFactory then re-scans everything. But that won't be necessary for long.

... and need to figure out at what point to merge things to master. Probably half-way through, to keep diff manageable.

But at least I am FINALLY making progress!

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to mention that this part of code will definitely be rewritten as right now it is strictly duplicating existing handling, but once everything works as well as it used to, can and will start simplifying & cleaning up.

if (implName == null) {
// Important: if neither implicit nor explicit name, cannot make use of
// this creator parameter -- may or may not be a problem, verified at a later point.
continue;
}
}

// 27-Dec-2019, tatu: [databind#2527] may need to rename according to field
if (implName != null) {
String n = _checkRenameByField(implName.getSimpleName());
implName = PropertyName.construct(n);
}

POJOPropertyBuilder prop = (implName == null)
? _property(props, explName) : _property(props, implName);
prop.addCtor(param, hasExplicit ? explName : implName, hasExplicit, true, false);
_creatorProperties.add(prop);
}
}

/**
* Method for collecting basic information on constructor(s) found
*/
@Deprecated
protected void _addCreators(Map<String, POJOPropertyBuilder> props)
{
// can be null if annotation processing is disabled...
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
package com.fasterxml.jackson.databind.introspect;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.databind.AnnotationIntrospector;
import com.fasterxml.jackson.databind.PropertyName;
import com.fasterxml.jackson.databind.cfg.MapperConfig;

/**
* Information about a single Creator (constructor or factory method),
* kept during property introspection.
*
* @since 2.18
*/
public class PotentialCreator
{
private static final PropertyName[] NO_NAMES = new PropertyName[0];

public final AnnotatedWithParams creator;

public final JsonCreator.Mode creatorMode;

private PropertyName[] implicitParamNames;

private PropertyName[] explicitParamNames;

public PotentialCreator(AnnotatedWithParams cr,
JsonCreator.Mode cm)
{
creator = cr;
creatorMode = cm;
}

/*
/**********************************************************************
/* Mutators
/**********************************************************************
*/

public PotentialCreator introspectParamNames(MapperConfig<?> config)
{
if (implicitParamNames != null) {
return this;
}

final AnnotationIntrospector intr = config.getAnnotationIntrospector();
final int paramCount = creator.getParameterCount();

if (paramCount == 0) {
implicitParamNames = explicitParamNames = NO_NAMES;
return this;
}

explicitParamNames = new PropertyName[paramCount];
implicitParamNames = new PropertyName[paramCount];

for (int i = 0; i < paramCount; ++i) {
AnnotatedParameter param = creator.getParameter(i);

String rawImplName = intr.findImplicitPropertyName(param);
if (rawImplName != null && !rawImplName.isEmpty()) {
implicitParamNames[i] = PropertyName.construct(rawImplName);
}
PropertyName explName = intr.findNameForDeserialization(param);
if (explName != null && !explName.isEmpty()) {
explicitParamNames[i] = explName;
}
}
return this;
}

/*
/**********************************************************************
/* Accessors
/**********************************************************************
*/

public int paramCount() {
return creator.getParameterCount();
}

public AnnotatedParameter param(int ix) {
return creator.getParameter(ix);
}

public boolean hasExplicitNames() {
for (int i = 0, end = explicitParamNames.length; i < end; ++i) {
if (explicitParamNames[i] != null) {
return true;
}
}
return false;
}

public PropertyName explicitName(int ix) {
return explicitParamNames[ix];
}

public PropertyName implicitName(int ix) {
return implicitParamNames[ix];
}

public String implicitNameSimple(int ix) {
PropertyName pn = implicitParamNames[ix];
return (pn == null) ? null : pn.getSimpleName();
}

/*
/**********************************************************************
/* Misc other
/**********************************************************************
*/

// For troubleshooting
@Override
public String toString() {
return "(mode="+creatorMode+")"+creator;
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package com.fasterxml.jackson.databind.introspect;

import java.util.*;

import com.fasterxml.jackson.databind.cfg.MapperConfig;

public class PotentialCreators
{
public final List<PotentialCreator> constructors;

public final List<PotentialCreator> factories;

/**
* Property-based Creator found, if any
*/
public PotentialCreator propertiesBased;

public AnnotatedWithParams defaultCreator;

public final List<PotentialCreator> delegating = new ArrayList<>();

public PotentialCreators(List<PotentialCreator> constructors,
List<PotentialCreator> factories)
{
this.constructors = constructors;
this.factories = factories;
}

/*
/**********************************************************************
/* Accumulating candidates
/**********************************************************************
*/

// desc -> "explicit", "implicit" etc
public void addPropertiesBased(MapperConfig<?> config, PotentialCreator ctor, String mode)
{
if (propertiesBased != null) {
throw new IllegalArgumentException(String.format(
"Conflicting property-based creators: already had %s creator %s, encountered another: %s",
mode, propertiesBased.creator, ctor.creator));
}
propertiesBased = ctor.introspectParamNames(config);
}

public void addDelegating(PotentialCreator ctor)
{
delegating.add(ctor);
}

public void addDefault(AnnotatedWithParams ctor)
{
defaultCreator = ctor;
}

/*
/**********************************************************************
/* Accessors
/**********************************************************************
*/

public boolean hasParametersBasedOrDelegating() {
return (propertiesBased != null) || !delegating.isEmpty();
}
}
Loading
Loading