Inconsistent implementation of MutablePersistable

[th3n3rd]

The current implementation of PersistableImplementor generates byte-code that a typical compiler probably would not.

Let's take one of the sample classes part of the test suite:

@Getter
public class SampleAggregate implements AggregateRoot<SampleAggregate, SampleAggregateIdentifier> {

    private final SampleAggregateIdentifier id;

        // ... other non-relevant stuff

    public SampleAggregate(SampleAggregateIdentifier id) {
        this.id = id;
    }

        // ... other non-relevant stuff
}

The above will generate byte-code with the following problems:

1. It will incorrectly implement MutablePersistable, lacking the implementation for the __jMolecules__markNotNew method
2. it will have an additional default constructor despite the final fields that need initialisation, in the original code

Suggestions:

The first problem might be fixable by always generating the missing implementation, instead of being conditional to a given callbackInterface.

e.g.

JMoleculesType implementPersistable(JMoleculesType type) {
    // ...
    return type.findIdField().map(field -> {
        // ...
        return type
            .implement(MutablePersistable.class, type.getTypeDescription(), field.getType())
            .mapBuilder(this::generateCallbacks)
            // fix (1): added the following instruction to generate the method regardless
            .mapBuilder(it -> !it.hasMethod(named(MARK_NOT_NEW_METHOD)), this::generateMarkNotNewMethod)
            .mapBuilder(it -> !it.hasField(named(IS_NEW_FIELD)), this::generateIsNewField)
            .mapBuilder(it -> !it.hasMethod(isEquals()), it -> generateEquals(it, isIdField))
            .mapBuilder(it -> !it.hasMethod(isHashCode()), it -> generateHashCode(it, isIdField))
            .mapBuilder(it -> !it.hasMethod(hasMethodName(IS_NEW_METHOD)), this::generateIsNewMethod);
        }).orElse(type);
}

// ...

private Builder<?> generateCallbacks(Builder<?> builder) {
    if (options.callbackInterface != null) {
        // fix (1): simplified as the method generation moved earlier
        builder = builder.require(createCallbackComponent(builder.toTypeDescription()));
    }

    // ...

    return builder;
}

Not sure how to fix (2) though.

[odrotbohm]

It will incorrectly implement MutablePersistable, lacking the implementation for the jMoleculesmarkNotNew method

Can you elaborate on that? I don't see how you come to that, as the code you show below checks for the presence of the method, and, if not present, generates it.

it will have an additional default constructor despite the final fields that need initialisation, in the original code

That's correct but mandated by JPA. None of the code generated here will be callable from user code, though.

[th3n3rd]

Can you elaborate on that? I don't see how you come to that, as the code you show below checks for the presence of the method, and, if not present, generates it.

The code that I pasted in there is a possible fix, the current version doesn't look like that.

The latest version available here generates the method conditionally on whether callbackInterface has been provided or not. In cases where the callbackInterface is not provided the method is not getting generated, hence the interface not implemented properly.