Skip to content

Breaking change from 4.2.7-SNAPSHOT to 4.3.1 for repositories with abstract or interface entities #1315

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.

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

Closed
JesusTheHun opened this issue Feb 1, 2022 · 9 comments · Fixed by #1319
Labels
type: regression A regression from a previous release

Comments

@JesusTheHun
Copy link

This repository method causes an issue with 4.3.1 but not with 4.2.7-SNAPSHOT :

@Query("#{#n1ql.selectEntity} USE KEYS $1")
List<BaseManageableEntity> findAllByIdIn(List<String> ids);

This method can return a variety of types of documents all extending the BaseManageableEntity abstract class.
With 4.2.7-SNAPSHOT the returned objects have the type of their inner class, A or B both extending BaseManageableEntity, the list carries the lower bound and is of type List<BaseManageableEntity>.
With 4.3.1 the returned method tries to deserialize the documents as instances of BaseManageableEntity causing an error, since BaseManageableEntity is an abstract class.

The behavior of 4.2.7-SNAPSHOT is the correct behavior imo, since it's easier and more natural to go in this direction than the other one.

@mikereiche
Copy link
Collaborator

There was a change to create objects that match the return type instead of the entity type of the repository - to handle methods that use projections to create other types of objects. Looking...

@mikereiche
Copy link
Collaborator

Hi - Can you provide an example of this?

@JesusTheHun
Copy link
Author

This is a nice improvement but it should not break current behavior in this major release branch.

One solution could be to use the _class attribute if present and then cast into the return type, and if not present then use the return type.
This way you can avoid breaking both features.

@mikereiche
Copy link
Collaborator

mikereiche commented Feb 1, 2022

use the _class attribute if present and then cast into the return type, and if not present then use the return type.

It's supposed to do that. That's the bug. In the change for matching the return type, projecting the _class (or typeKey) property was dropped. So it always fell back to the entity of the repository. The fix is simple. But obviously I need to add some test cases.

As a hack, adding a _class property to the BaseManageableEntity class will result in the _class property of the document being projected, and the converter being able to figure out what to instantiate.

protected String _class;

@mikereiche mikereiche added type: regression A regression from a previous release and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 2, 2022
@JesusTheHun
Copy link
Author

JesusTheHun commented Feb 2, 2022

Hi - Can you provide an example of this?

Here is a close reproduction of my case :

Entities
public abstract class AuditedEntity implements Serializable {

    @Field("is_audited_entity")
    protected boolean isAuditedEntity = true; // we use this as a class hierarchy root identifier.

    @Id
    @GeneratedValue(strategy = UNIQUE, delimiter = ID_DELIMITER)
    protected String id;

    @Version
    @Field("revision")
    protected Long revision;

    @CreatedBy
    @Field("created_by")
    protected String createdById;

    @CreatedDate
    @Field("created_date")
    protected Instant createdDate = Instant.now();

    @LastModifiedBy
    @Field("last_modified_by")
    protected String lastModifiedById;

    @LastModifiedDate
    @Field("last_modified_date")
    protected Instant lastModifiedDate = Instant.now();

    public String getId() {
        return id;
    }

    public void setId(String id) {
        this.id = id;
    }

    public String getCreatedById() {
        return createdById;
    }

    public void setCreatedBy(Customer customer) {
        this.createdById = customer.getId();
    }

    public void setCreatedById(String createdBy) {
        this.createdById = createdBy;
    }

    public Instant getCreatedDate() {
        return createdDate;
    }

    public void setCreatedDate(Instant createdDate) {
        this.createdDate = createdDate;
    }

    public String getLastModifiedById() {
        return lastModifiedById;
    }

    public void setLastModifiedBy(Customer lastModifiedBy) {
        this.lastModifiedById = lastModifiedBy.getId();
    }

    public void setLastModifiedById(String lastModifiedById) {
        this.lastModifiedById = lastModifiedById;
    }

    public Instant getLastModifiedDate() {
        return lastModifiedDate;
    }

    public void setLastModifiedDate(Instant lastModifiedDate) {
        this.lastModifiedDate = lastModifiedDate;
    }

    public Long getRevision() {
        return revision;
    }

    public void setRevision(long revision) {
        this.revision = revision;
    }
}

@Document
@Data
class EntityA extends AuditedEntity {
    String specificPropOne;
}

@Document
@Data
class EntityB extends AuditedEntity {
    String specificPropTwo;
}
Repositories
public interface EntityARepository extends CouchbaseRepository<String, EntityA> {

}

public interface EntityBRepository extends CouchbaseRepository<String, EntityB> {

}

public interface AuditedEntityRepository extends Repository<String, AuditedEntity> {
    @Query("SELECT * FROM #{#n1ql.bucket} WHERE is_audited_entity = true")
    List<AuditedEntity> findAll();
}

Then you should be able to retrieve all AuditedEntity with the repo method. All objects being either EntityA or EntityB.
Does that respond to your request ?

@mikereiche
Copy link
Collaborator

That's good. I actually was able to reproduce with the Foo/BaseManageableEntity from issue 1312. The fix is to change this is the addition of typeField on the entity projection in StringBasedN1qlQueryParser. As soon as I make some test cases I'll merge the fix to both main and 4.3.x

String entity = "META(" + i(b) + ").id AS " + SELECT_ID + ", META(" + i(b) + ").cas AS " + SELECT_CAS + ", "+typeField;

mikereiche added a commit that referenced this issue Feb 2, 2022
Restore projection of class in queries. This is necessary for abstract repositories.

Closes #1315.
mikereiche added a commit that referenced this issue Feb 2, 2022
Restore projection of class in queries. This is necessary for abstract repositories.

Closes #1315.
@mikereiche
Copy link
Collaborator

btw - derived queries (those without an @query definition) on a repository for an abstract entity class and the default type (i.e. no @typealias) will never select any rows that have been saved by that repository because the _class = predicate will use the abstract class name, while the documents will have been saved with their concrete class name.

mikereiche added a commit that referenced this issue Feb 3, 2022
Restore projection of class in queries. This is necessary for abstract repositories.

Closes #1315.
@mikereiche mikereiche added this to the 4.3.2 (2021.1.2) milestone Feb 3, 2022
@JesusTheHun
Copy link
Author

Is there an ETA for 4.3.2 ? In the meanwhile, what snapshot version can we use ?

mikereiche added a commit that referenced this issue Feb 4, 2022
Restore projection of class in queries. This is necessary for abstract repositories.

Closes #1315.
@mikereiche
Copy link
Collaborator

February 18. https://calendar.spring.io/ (2021.1.x -> 4.3.x)
I've updated 4.3x just now, the next 4.3.2-SNAPSHOT will have the change.

mikereiche added a commit that referenced this issue Feb 8, 2022
Restore projection of class in queries. This is necessary for abstract repositories.

Closes #1315.
@mikereiche mikereiche changed the title Breaking change from 4.2.7-SNAPSHOT to 4.3.1 Breaking change from 4.2.7-SNAPSHOT to 4.3.1 for repositories with abstract or interface entities Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression A regression from a previous release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants