Question: generated entityview class does not use inherited fields

jwgmeligmeyling commented 6 years ago

Given a POJO from a library code base:

class Pojo {
    private String id;
    public String getId() { return id; }
    public void setId(String id) { this.id = id; }
}

I would like to fill this POJO using entity views, so that I do not have to write verbose code to copy properties from my entity view to the libraries Pojo. So I came up with the following:

@EntityView(MyEntity.class)
abstract class PojoView extends Pojo {
     @Override @IdMapping public abstract getId();
}

To my own enthusiasm this works perfectly fine, but there is one thing that bothers me: the id field is recreated within the Pojo class. So in debuggers etc. the field lists up twice, once with a null value.

Is it perhaps an idea to use existing fields if available for entity view classes that extend abstract classes?

Description

Expected behavior

Actual behavior

Steps to reproduce

Environment

Version:
JPA-Provider:
DBMS:
Application Server:

beikov commented 6 years ago

Usually, the integration with existing models is done by using constructor injection like

@EntityView(MyEntity.class)
abstract class PojoView extends Pojo {
  PojoView(@IdMapping String id) {
    setId(id);
  }
}

The problem with reusing the existing field is when it comes to updatable entity views, as we aren't in control of the write access anymore if we use a foreign field. I'm not sure if it would actually be possible to use the field in this case. It is private and since the getter that exposes the field is shadowed by the abstract getter, I don't think it's possible to implement proper field access. Or do you have an idea?

jwgmeligmeyling commented 6 years ago

I don't think it is possible indeed. I remember calling the super of overridden default methods on interfaces, but it was quite a big hack and I don't think it'll work for classes.

Does the constructor mapping approach work for updatable entities? Do you somehow figure out the property name based on the parameter name (which should not work prior to Java 8)? Or are these fields simply not included in the changes tracked?

The constructor approach works for me, but I find it just slightly hard to see what mapping belongs to which field, especially with long annotations such as correlation mappings.

Alternatives that I can think of, but by no means are considered to be "better":

Allow non-abstract methods in entity view classes, that are expected to be bean property accessors (i.e. call super method). Obviously though, we can't really enforce/check that behaviour so possibly quite some weird stuff can happen here.
Introduction of an annotation that allows to add mappings on properties on a type level (somewhat similar to AttributeOverride in JPA I suppose). Getting this right may not be really trivial, because Mapping, IdMapping and other mapping annotations do not share a common type.
Accessing the field through reflection with the setAccessible(true) hack, but I think it would not really be appropriate to bypass the accessors, and I am not sure whether this use of reflection will remain supported.
Only use the existing property for (package) protected or public fields, but this would seem fairly inconsistent.

beikov commented 6 years ago

No, the constructor mapping I presented doesn't work for updatable entity views. There is an issue for supporting field injection in general(#474) but especially for updatable entity views, this will be very hard to implement as it requires instrumentation/weaving. I thought a little about what the Java Agent implementation for this could look like, but I think it's not so easy, especially with Javassist. So if we decide to do this, it might be worth trying to do it with Bytebuddy/ASM or at least do a PoC with Javassist first before trying to implement all features.

Supporting injection into POJO entity views by setting fields or invoking a setter shouldn't be too hard to implement. The main question we have to answer is how we would like to configure it properly.

Essentially, it must be possible on a subclass to

Define the default access strategy for a super class(Or maybe just configure that also concrete fields should be considered?)
Exclude or mark fields as transient
Override the mappings for the fields of a super class

I'd love to see some suggestions if you have time to think about this :) For me, this doesn't have a big priority right now as the main use case "make use of existing POJO models" can be solved in a different manner.

Blazebit / blaze-persistence