Add support for factory methods to instantiate entities

[mp911de]

We now support static factory methods to instantiate entities. Factory methods can be disambiguated by annotating these with @Factory.

// using factory method
class FactoryPerson {

	private final String firstname, lastname;

	private FactoryPerson(String firstname, String lastname) {
		this.firstname = firstname;
		this.lastname = lastname;
	}

	public static FactoryPerson of(String firstname, String lastname) {
		return new FactoryPerson(firstname, lastname);
	}
}

// using annotated factory method
class FactoryMethodsPerson {

	private final String firstname, lastname;

	private FactoryMethodsPerson(String firstname, String lastname) {
		this.firstname = firstname;
		this.lastname = lastname;
	}

	public static FactoryMethodsPerson of(String firstname) {
		return new FactoryMethodsPerson(firstname, "unknown");
	}

	@Factory
	public static FactoryMethodsPerson of(String firstname, String lastname) {
		return new FactoryMethodsPerson(firstname, lastname);
	}
}

// using annotated constructor
class ConstructorPerson {

	private final String firstname, lastname;

	@PersistenceConstructor
	private ConstructorPerson(String firstname, String lastname) {
		this.firstname = firstname;
		this.lastname = lastname;
	}

	public static ConstructorPerson of(String firstname, String lastname) {
		return new ConstructorPerson(firstname, lastname);
	}
}

Closes #2476

[odrotbohm]

A couple of inline comments. I am not entirely sure about the naming of @Factory as that collides with the concept of a factory type (as in: the pattern).

[odrotbohm]

Could this be a method on EntityCreator with the different ways of looking this up living in the implementations?

[mp911de]

I considered the future arrangements where we might have no reflection access to constructors/methods (e.g. AOT/Graal Native that come with a different creator mechanism for entities). So I'd leave the reflective bits in the calling code.

[odrotbohm]

Wouldn't this be a different implementation of EntityInstantiator that could simply not call these methods? Or a decorator in between EntityCreator and the actual implementation instance?

[mp911de]

Likely it would be a different EntityInstantiator that is implemented using generated code (AOT) per entity type. In our AOT Build-time POC a single class was implementing TypeInformation, EntityInstantiator and a few other interfaces to keep all concerns regarding a single class in a single place.

[odrotbohm]

Could this be moved to a callback on EntityCreator with the default just returning false?

[mp911de]

Hum, enclosing class is only applicable in the context of constructors and non-static inner classes. We have a single call to that method so it doesn't seem to be a good fit for the general API.

[odrotbohm]

I thought about broadening the name of the method to something like isParentParameter() and have that notion of "enclosing class parameter" move into the PersistenceConstructor implementation. Methods actually have something similar to that synthetic parameter, called "receiver parameters" that don't appear on the call side of the invocation: https://www.logicbig.com/tutorials/core-java-tutorial/java-8-enhancements/explicit_receiver_parameters.html. Not saying we should support those, but it's not as conceptually odd as it seems.

[mp911de]

thought about broadening the name of the method to something like isParentParameter()

That makes sense. Let me handle that as a separate commit.

[odrotbohm]

Again, I think this should be on the EntityCreator interface to avoid the instance of checks and pulling out implementation details.

[odrotbohm]

Wonder if we could get as far as avoiding having to expose these types (just as PreferredConstructor) publically, now that EntityCreator is available.

[mp911de]

Depends on the design really. A FactoryMethod/PreferredConstructor makes only sense when there's reflective access. With our separation where we externalize EntityInstantiators we always leak implementation details. EntityCreator reduces the concept of entity creation to the actual description of input parameters without actually defining the approach (reflection, constructor/factory method, a provided lambda) how entities are being created. Exposing Executable would already assume that we use reflection information in some sense to create entities.

[odrotbohm]

I get that, but most places in which the reflection API is used are not for the actual (reflective) invocation of those methods, but the inspection and decisions triggered by the outcome of that. Even in an AOT scenario, we would need that, wouldn't we? It feels pretty brittle to see all those instance of checks in different places, as they stop working as soon as you wrap the original instance in some decorator. The entire detection process leading to the creation of FactoryMethod/PreferredConstructor is reflection-based. If we avoid reflection happening in front of those instances, it's much easier to potentially replace the entire detection process and instances. For example, by an instance of EntityCreator that will just obtain that metadata from a different place without using any runtime reflection at all.

[odrotbohm]

Introducing this flag seems to be related but not strictly required, is it?

[mp911de]

This is for testing purposes only. Tests were green during development although class-generated entity creation was broken and the creation fell back to reflective instance creation.

[mp911de]

@Factory was the initial name. Now that we have a bit of code to back up the discussion, we can become creative about naming. @FactoryMethod would express as well what's meant.

[odrotbohm]

Thanks for all the work implementing this, Mark. As a final step, I suggest a bit of polishing in the annotation model. We should expose a canonical annotation to declare both constructors and factory methods as "creator". Long term, it doesn't really make sense to go with @PersistenceConstructor and @FactoryMethod as they're not really symmetric names and there's no real need for two different annotations.

Proposals:

• Promote @EntityCreatorAnnotation to be the primary user-facing annotation, renaming it to @PersistenceCreator. The name makes obvious that the annotated element is used to create instances of that type by the persistence framework. I thought about @ObjectCreator, too, but that seems too generic.
• Remove @FactoryMethod entirely, as it's obsolete if we want to move to a canonical annotation.
• Deprecate @PersistenceConstructor in favor of @PersistenceCreator. Potential removal candidate versions are 3.0 (assuming we introduce this in 2.7) or 3.1.

Feel free to chime in if you'd like to veto those steps. Unless I get any until Monday, I'll proceed with the changes as proposed.

[mp911de]

Sounds good to have @PersistenceCreator as top-level annotation and move towards Persistence… instead of Entity….

[odrotbohm]

That's applied to 2.7 and the 3.0 branch.