Skip to content

Support initialisation of ToMany collections using List.of() etc #220

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

Merged
rbygrave merged 4 commits into from
Mar 8, 2025
Merged

Support initialisation of ToMany collections using List.of() etc #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
edd262a
Support initialisation of ToMany collections using List.of() etc
rbygrave Mar 3, 2025
3377e64
Tidy up test dependencies
rbygrave Mar 4, 2025
20b1bc4
Add EnhancementException to throw when unsupported OneToMany initiali…
rbygrave Mar 4, 2025
af1a203
Add back support for List.of() and Collections.emptyList() as initial…
rbygrave Mar 4, 2025
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
22 changes: 4 additions & 18 deletions ebean-agent/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
<dependency>
<groupId>io.avaje</groupId>
<artifactId>junit</artifactId>
<version>1.1</version>
<version>1.5</version>
<scope>test</scope>
</dependency>

Expand Down Expand Up @@ -92,23 +92,9 @@
</dependency>

<dependency>
<groupId>org.avaje.composite</groupId>
<artifactId>logback</artifactId>
<version>1.1</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>3.11.2</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.avaje.composite</groupId>
<artifactId>composite-testing</artifactId>
<version>3.1</version>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
<version>1.5.17</version>
<scope>test</scope>
</dependency>

Expand Down
12 changes: 12 additions & 0 deletions ebean-agent/src/main/java/io/ebean/enhance/EnhancementException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package io.ebean.enhance;

/**
* Some unexpected bytecode that can't be supported.
* <p>
* For example, some unsupported OneToMany collection initialisation.
*/
public class EnhancementException extends RuntimeException {
public EnhancementException(String message) {
super(message);
}
}
3 changes: 3 additions & 0 deletions ebean-agent/src/main/java/io/ebean/enhance/Transformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ public byte[] transform(ClassLoader loader, String className, Class<?> classBein
// the class is an interface
log(8, className, "No Enhancement required " + e.getMessage());
return null;
} catch (EnhancementException e) {
enhanceContext.log(className, "Transform error " + e.getMessage());
throw e;
} catch (IllegalArgumentException | IllegalStateException e) {
log(2, className, "No enhancement on class due to " + e);
return null;
Expand Down
28 changes: 14 additions & 14 deletions ebean-agent/src/main/java/io/ebean/enhance/ant/OfflineFileTransform.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.ebean.enhance.ant;

import io.ebean.enhance.EnhancementException;
import io.ebean.enhance.Transformer;
import io.ebean.enhance.common.InputStreamTransform;

Expand Down Expand Up @@ -60,7 +61,6 @@ private String trimSlash(String dir) {
* Process the packageNames as comma delimited string.
*/
public void process(String packageNames) {

if (packageNames == null) {
// just process all directories
processPackage("");
Expand All @@ -69,7 +69,6 @@ public void process(String packageNames) {

Set<String> pkgNames = new LinkedHashSet<>();
Collections.addAll(pkgNames, packageNames.split(","));

process(pkgNames);
}

Expand All @@ -81,7 +80,6 @@ public void process(String packageNames) {
* </p>
*/
public void process(Set<String> packageNames) {

if (packageNames == null || packageNames.isEmpty()) {
// just process all directories
inputStreamTransform.log(2, "processing all directories (as no explicit packages)");
Expand All @@ -90,17 +88,14 @@ public void process(Set<String> packageNames) {
}

for (String pkgName : packageNames) {

String pkg = pkgName.trim().replace('.', '/');

if (pkg.endsWith("**")) {
pkg = pkg.substring(0, pkg.length() - 2);
} else if (pkg.endsWith("*")) {
pkg = pkg.substring(0, pkg.length() - 1);
}

pkg = trimSlash(pkg);

processPackage(pkg);
}
}
Expand Down Expand Up @@ -142,15 +137,20 @@ private void processPackage(String dir) {
}

private void transformFile(File file) throws IOException, IllegalClassFormatException {

String className = getClassName(file);

byte[] result = inputStreamTransform.transform(className, file);

if (result != null) {
InputStreamTransform.writeBytes(result, file);
if (listener != null && logLevel > 0) {
listener.logEvent("Enhanced " + file);
try {
byte[] result = inputStreamTransform.transform(className, file);
if (result != null) {
InputStreamTransform.writeBytes(result, file);
if (listener != null && logLevel > 0) {
listener.logEvent("Enhanced " + file);
}
}
} catch (EnhancementException e) {
if (listener != null) {
listener.logError("Error enhancing class " + className + " " + e.getMessage());
} else {
throw e;
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions ebean-agent/src/main/java/io/ebean/enhance/common/AgentManifest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public final class AgentManifest {
private boolean transientInternalFields;
private boolean transientInit;
private boolean transientInitThrowError;
private boolean unsupportedInitThrowError = true;
private boolean checkNullManyFields = true;
private boolean enableProfileLocation = true;
private boolean enableEntityFieldAccess;
Expand Down Expand Up @@ -151,6 +152,10 @@ public boolean isTransientInitThrowError() {
return transientInitThrowError;
}

public boolean isUnsupportedInitThrowError() {
return unsupportedInitThrowError;
}

/**
* Return true if we should use transient internal fields.
*/
Expand Down Expand Up @@ -315,6 +320,7 @@ private void readOptions(Attributes attributes) {
transientInternalFields = bool("transient-internal-fields", transientInternalFields, attributes);
checkNullManyFields = bool("check-null-many-fields", checkNullManyFields, attributes);
allowNullableDbArray = bool("allow-nullable-dbarray", allowNullableDbArray, attributes);
unsupportedInitThrowError = bool("unsupported-init-error", unsupportedInitThrowError, attributes);
}

private boolean bool(String key, boolean defaultValue, Attributes attributes) {
Expand Down
15 changes: 15 additions & 0 deletions ebean-agent/src/main/java/io/ebean/enhance/common/ClassMeta.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public class ClassMeta {
/**
* If enhancement is adding a default constructor - only default constructors are supported initialising transient fields.
*/
private final Set<String> unsupportedInitMany = new LinkedHashSet<>();
private final Set<String> unsupportedTransientInitialisation = new LinkedHashSet<>();
private final Map<String, CapturedInitCode> transientInitCode = new LinkedHashMap<>();
private final LinkedHashMap<String, FieldMeta> fields = new LinkedHashMap<>();
Expand Down Expand Up @@ -447,6 +448,20 @@ public Collection<CapturedInitCode> transientInit() {
return transientInitCode.values();
}

public void addUnsupportedInitMany(String name) {
unsupportedInitMany.add(name);
}

public boolean hasUnsupportedInitMany() {
return !unsupportedInitMany.isEmpty();
}

public String initFieldErrorMessage() {
return "ERROR: Unsupported initialisation of @OneToMany or @ManyToMany on: "
+ className + " fields: " + unsupportedInitMany
+ " Refer: https://ebean.io/docs/trouble-shooting#initialisation-error";
}

public void addUnsupportedTransientInit(String name) {
unsupportedTransientInitialisation.add(name);
}
Expand Down
4 changes: 4 additions & 0 deletions ebean-agent/src/main/java/io/ebean/enhance/common/EnhanceContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,10 @@ public boolean isTransientInitThrowError() {
return manifest.isTransientInitThrowError();
}

public boolean isUnsupportedInitThrowError() {
return manifest.isUnsupportedInitThrowError();
}

/**
* Return true if internal ebean fields in entity classes should be transient.
*/
Expand Down
9 changes: 9 additions & 0 deletions ebean-agent/src/main/java/io/ebean/enhance/entity/ClassAdapterEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.ebean.enhance.entity;

import io.ebean.enhance.EnhancementException;
import io.ebean.enhance.asm.AnnotationVisitor;
import io.ebean.enhance.asm.ClassVisitor;
import io.ebean.enhance.asm.FieldVisitor;
Expand Down Expand Up @@ -205,6 +206,14 @@ public void visitEnd() {
if (!classMeta.isEntityEnhancementRequired()) {
throw new NoEnhancementRequiredException();
}
if (classMeta.hasUnsupportedInitMany()) {
if (classMeta.context().isUnsupportedInitThrowError()) {
throw new EnhancementException(classMeta.initFieldErrorMessage());
} else {
// the default constructor being added will leave some transient fields uninitialised (null, 0, false etc)
System.err.println(classMeta.initFieldErrorMessage());
}
}
if (!classMeta.hasStaticInit()) {
IndexFieldWeaver.addPropertiesInit(cv, classMeta);
}
Expand Down
60 changes: 50 additions & 10 deletions ebean-agent/src/main/java/io/ebean/enhance/entity/ConstructorDeferredCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ enum State {
INVOKE_SPECIAL,
KT_CHECKCAST, // optional kotlin state
KT_LABEL, // optional kotlin state
KT_EMPTYLIST
EMPTY,
MAYBE_UNSUPPORTED
}

private static final ALoad ALOAD_INSTRUCTION = new ALoad();
Expand Down Expand Up @@ -130,20 +131,53 @@ boolean deferVisitMethodInsn(int opcode, String owner, String name, String desc,
stateInitialiseType = owner;
return true;
}
if (opcode == INVOKESTATIC && stateAload() && kotlinEmptyList(owner, name, desc)) {
codes.add(new NoArgInit(opcode, owner, name, desc, itf));
state = State.KT_EMPTYLIST;
stateInitialiseType = "java/util/ArrayList";
return true;
if (opcode == INVOKESTATIC && stateAload()) {
if (kotlinEmptyList(owner, name, desc) || emptyList(owner, name, desc)) {
codes.add(new NoArgInit(opcode, owner, name, desc, itf));
state = State.EMPTY;
stateInitialiseType = "java/util/ArrayList";
return true;
}
if (emptySet(owner, name, desc)) {
codes.add(new NoArgInit(opcode, owner, name, desc, itf));
state = State.EMPTY;
stateInitialiseType = "java/util/LinkedHashSet";
return true;
}
if (emptyMap(owner, name, desc)) {
codes.add(new NoArgInit(opcode, owner, name, desc, itf));
state = State.EMPTY;
stateInitialiseType = "java/util/LinkedHashMap";
return true;
}
}
flush();
state = State.MAYBE_UNSUPPORTED;
return false;
}

private boolean isNoArgInit(String name, String desc) {
return name.equals(INIT) && desc.equals(NOARG_VOID);
}

private boolean emptyList(String owner, String name, String desc) {
return desc.equals("()Ljava/util/List;")
&& ((owner.equals("java/util/List") && name.equals("of"))
|| (owner.equals("java/util/Collections") && name.equals("emptyList")));
}

private boolean emptySet(String owner, String name, String desc) {
return desc.equals("()Ljava/util/Set;")
&& ((owner.equals("java/util/Set") && name.equals("of"))
|| (owner.equals("java/util/Collections") && name.equals("emptySet")));
}

private boolean emptyMap(String owner, String name, String desc) {
return desc.equals("()Ljava/util/Map;")
&& ((owner.equals("java/util/Map") && name.equals("of"))
|| (owner.equals("java/util/Collections") && name.equals("emptyMap")));
}

private boolean kotlinEmptyList(String owner, String name, String desc) {
return owner.equals("kotlin/collections/CollectionsKt")
&& name.equals("emptyList")
Expand All @@ -155,6 +189,12 @@ private boolean kotlinEmptyList(String owner, String name, String desc) {
*/
boolean consumeVisitFieldInsn(int opcode, String owner, String name, String desc) {
if (opcode == PUTFIELD) {
if (state == State.MAYBE_UNSUPPORTED && meta.isConsumeInitMany(name)) {
// a OneToMany/ManyToMany is initialised in an unsupported manor
meta.addUnsupportedInitMany(name);
flush();
return false;
}
if (stateConsumeDeferred()) {
if (meta.isConsumeInitMany(name) && isConsumeManyType()) {
if (meta.isLog(3)) {
Expand Down Expand Up @@ -232,17 +272,17 @@ private boolean stateInvokeSpecial() {
}

private boolean stateConsumeDeferred() {
return state == State.INVOKE_SPECIAL || state == State.KT_CHECKCAST || state == State.KT_EMPTYLIST;
return state == State.INVOKE_SPECIAL || state == State.KT_CHECKCAST || state == State.EMPTY;
}

/**
* Return true if the type being initialised is valid for auto initialisation of ToMany or DbArray.
*/
private boolean isConsumeManyType() {
return ("java/util/ArrayList".equals(stateInitialiseType)
return "java/util/ArrayList".equals(stateInitialiseType)
|| "java/util/LinkedHashSet".equals(stateInitialiseType)
|| "java/util/HashSet".equals(stateInitialiseType));
//|| "java/util/LinkedHashMap".equals(stateInitialiseType)
|| "java/util/HashSet".equals(stateInitialiseType)
|| "java/util/LinkedHashMap".equals(stateInitialiseType);
//|| "java/util/HashMap".equals(stateInitialiseType));
}

Expand Down
1 change: 1 addition & 0 deletions test/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

<properties>
<ebean.version>13.4.0</ebean.version>
<java.version>11</java.version>
</properties>

<dependencies>
Expand Down
40 changes: 40 additions & 0 deletions test/src/test/java/test/enhancement/WithInitialisedCollections2Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package test.enhancement;

import io.ebean.common.BeanList;
import io.ebean.common.BeanMap;
import io.ebean.common.BeanSet;
import org.junit.jupiter.api.Test;
import test.model.Contact;
import test.model.WithInitialisedCollections2;

import static org.assertj.core.api.Assertions.assertThat;

class WithInitialisedCollections2Test extends BaseTest {

@Test
void oneToMany_initialisationCode_expect_removed() {
WithInitialisedCollections2 bean = new WithInitialisedCollections2();

assertThat(bean.listOf()).isInstanceOf(BeanList.class);
assertThat(bean.listCollEmpty()).isInstanceOf(BeanList.class);
assertThat(bean.setOf()).isInstanceOf(BeanSet.class);
assertThat(bean.setCollEmpty()).isInstanceOf(BeanSet.class);
assertThat(bean.mapOf()).isInstanceOf(BeanMap.class);
assertThat(bean.mapCollEmpty()).isInstanceOf(BeanMap.class);


assertThat(bean.transientList()).isNotInstanceOf(BeanList.class);
assertThat(bean.transientList2()).isNotInstanceOf(BeanList.class);
assertThat(bean.transientSet()).isNotInstanceOf(BeanSet.class);
assertThat(bean.transientSet2()).isNotInstanceOf(BeanSet.class);
assertThat(bean.transientMap()).isNotInstanceOf(BeanMap.class);
assertThat(bean.transientMap2()).isNotInstanceOf(BeanMap.class);


// these methods work because the underlying collection is a BeanCollection
bean.listOf().add(new Contact("junk"));
Copy link
Contributor

@rPraml rPraml Mar 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My expectation is, that this should fail.
When initializing with List.of etc. the collection is immutable.
Can we transfer this information to the BeanCollection? (if agent detects immutable initialization, the replaced collection is also immutable from the perspective of the user - not for lazy loading)

BTW. There are other ways to initialize immutable lists:

  • Collections.EMTY_LIST
  • Collections.unmodifiableList(new ArrsyList())
  • Arrays.asList(...)

I think, the agent should fail or at least warn, if an unknown initialization is detectef

Copy link
Member Author

@rbygrave rbygrave Mar 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My expectation is, that this should fail.

That is reasonable. My expectation was that people wouldn't use List.of() at all ...

Can we transfer this information to the BeanCollection?

The initialisation for OneToMany and ManyToMany is removed. So for example, if we detect new ArrayList<>() initialisation for a OneToMany - that bytecode is removed so it is as if it wasn't there at all.

So this PR was done such that initialisation via List.of() ... was detected and removed as if it wasn't there at all.

That is, the only reasons why we'd choose to initialise OneToMany collections in such a way is so that (A) the fields can be final and (B) it gives the impression to people reading the code that the collection will not be null.
... noting the for (B) this is only an impression because ebean will make sure the collection is not null regardless [well except for the new immutable read only stuff that isn't in yet and will throw an exception ... so also there no application code will see a null OneToMany collection].

So that all said, I've pushed another PR just now that detects when initialisation code for OneToMany or ManyToMany isn't supported and throws a new EnhancementException [and for maven we will fail the build at enhancement due when this is detected].

I think, the agent should fail or at least warn, if an unknown initialisation is detect

Yes. I think I've just managed to do that with the latest commit pushed here plus a change to the maven plugin to detect and fail the build for that case.

The outstanding question I think is, should we support List.of() and Collections.emptyList() [where support means that this bytecode is detected and removed just like new ArrayList<>() is].

Copy link
Contributor

@rPraml rPraml Mar 4, 2025

Choose a reason for hiding this comment

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

That is reasonable. My expectation was that people wouldn't use List.of() at all ...

I would never have thought of that ;)

We would need the following use cases:

  • null / new ArrayList() for normal lists
  • a distinction between unsorted sets and sorted sets/maps (we initialize them with LinkedHashSet/LinkedHashMap)

The outstanding question I think is, should we support List.of() and Collections.emptyList()

if we decide to do, I would expect, that the BeanList/BeanSet etc. is also readonly. So BeanList needs a readOnly flag and the agent must call the new constructor new BeanList(underlyingList, true)

I can imagine use cases, where I want to protect the modification of that list by someone else.

but this is more theoretical, so it would be also OK to throw an error in the enhancer

Copy link
Member Author

@rbygrave rbygrave Mar 4, 2025

Choose a reason for hiding this comment

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

a distinction between unsorted sets and sorted sets/maps

I have no intention of ebean supporting unsorted sets/maps. I don't think that is a need / requirement.

I would expect, that the BeanList/BeanSet etc. is also readonly.

I don't think we can easily do that at this point. I don't want to try to get that clever unless we have to.

My suspicion is that I don't think people are thinking deeply about this. I suspect the use of List.of() was more just a bit of an accident.

Copy link
Member Author

@rbygrave rbygrave Mar 4, 2025

Choose a reason for hiding this comment

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

Reviewing the JPA spec, the initialisation of OneToMany and ManyToMany is vague:

"Collection-valued persistent fields and properties must be defined in terms of one of the following collection-valued interfaces regardless of whether the entity class otherwise adheres to the JavaBeans method conventions noted above and whether field or property access is used: java.util.Collection, java.util.Set, java.util.List [3], java.util.Map. The collection implementation type may be used by the application to initialize fields or properties before the entity is made persistent."

Noting that stack overflow has - https://stackoverflow.com/questions/20715143/to-initialize-or-not-initialize-jpa-relationship-mappings

At this stage, it feels like the safest option would be to not support List.of() and Collections.emptyList() etc as long as we can explicitly fail the enhancement. Noting that there might have "holes" in this detection like an application that ONLY using javaagent style enhancement could miss an error going to system out [but the entity would not be enhanced so it would not be useable as an entity - it's just the error might get misleading].

bean.setOf().add(new Contact("junk"));
bean.listCollEmpty().add(new Contact("junk"));
bean.setCollEmpty().add(new Contact("junk"));
}
}
Loading