AttributeBytecodeGenerator throws Exceptions if called twice

[drekbour]

Found during testing, where facilities are being initialised many times and with types that the SUT code doesn't know about.

Simplest reproduction:

Class<IndexedCollection> clazz = IndexedCollection.class;
createAttributes(clazz, GETTER_METHODS_ONLY);
createAttributes(clazz, GETTER_METHODS_ONLY);

gives: Caused by: java.lang.ClassFormatError: loader (instance of sun/misc/Launcher$AppClassLoader): attempted duplicate class definition for name: "com/googlecode/cqengine/IndexedCollection$$CQEngine_SimpleNullableAttribute_getMetadataEngine"

Workaround:

static final Map<Class<?>, Collection<? extends Attribute<?, ?>>> CACHE = new ConcurrentHashMap<>();
Collection<? extends Attribute<?, ?>> a = CACHE.computeIfAbsent(clazz, c -> createAttributes(c).values());
return parser.registerAttributes((Collection<? extends Attribute<O, ?>>) a);

Seems reasonable to expect this generator to be idempotent by checking if the target classname exists and using that if it does (e.g. com/googlecode/cqengine/IndexedCollection$$CQEngine_SimpleNullableAttribute_getMetadataEngine)

If it's not considered a bug, then I think it would be nice to update the class Javadoc.

[npgall]

This issue has come up before in https://github.com/npgall/cqengine/issues/141 and I posted some thoughts there.

Basically, I don't know of a better way to handle this than the current situation, which is to throw an exception. It's because CQEngine has no way of knowing if the application really wants the same attribute that was generated previously to be returned, or if the application is inadvertently trying to reimplement/redefine an existing attribute to fetch data in a different way; for which case throwing an exception is the safest thing to do.

However, you are totally right that this should be documented!! I'll make a note to update the JavaDoc, and will keep this issue open as a reminder until it's updated.

[drekbour]

That workaround using a cache is absolutely not suitable for inclusion in the library itself. The Exception however is because a very specifically named class is already loaded. How else could that class be in existence except via a previous call to the same method with the same params?

Given this, Principle of Least Surprise says AttributeBytecodeGenerator should use the existing class; the specification of the createAttributes method is to return new Attribute instances meeting the requirements. This can be done with either a new or pre-existing class definition.

inadvertently trying to reimplement/redefine an existing attribute

In the context of the above, this seems either foolish or brave but, either way, is well off the path and I would say those users should be ones figuring out why things are not working - not the vanilla ones.