micronaut-projects / micronaut-data

Ahead of Time Data Repositories
Apache License 2.0
466 stars 197 forks source link

DATA-JDBC / Postgres / ONE_TO_MANY Invalid Mapping #669

Closed emmanuj closed 3 years ago

emmanuj commented 4 years ago

I have a model that looks like the following:

@MappedEntity("answers")
public class CategoryAnswer implements Serializable {
    @Id
    @AutoPopulated
    private UUID id;
    private String category;
    private String answer;

    // ...

    @Relation(value = Relation.Kind.ONE_TO_MANY, mappedBy = "answer")
    Set<AnswerVote> votes = new HashSet<>();
}
@MappedEntity("votes")
public class AnswerVote {
    @Id
    @AutoPopulated
    UUID id;

    @Relation(value = Relation.Kind.MANY_TO_ONE, cascade = Relation.Cascade.ALL)
    @MappedProperty(value = "answer_id")
    CategoryAnswer answer;

    int vote;
}
@JdbcRepository(dialect = Dialect.POSTGRES)
public abstract class CategoryAnswerRepository implements CrudRepository<Answer, UUID> {
    @NonNull
    @Join("votes")
    public abstract List<CategoryAnswer> findAll();
}

Then given the following data:

answer 1 => vote1, vote2 answer 2 => vote3, vote4 answer 3 => vote5, vote6 answer 4 => vote7, vote8

Expected Behaviour

Calling categoryAnswersRepository.findAll() should have returned:

CategoryAnswer(id=answer1, votes=[AnswerVote(id=vote1, vote=accept), AnswerVote(id=vote2, vote=accept)])
CategoryAnswer(id=answer2, votes=[AnswerVote(id=vote3, vote=accept), AnswerVote(id=vote4 vote=accept)])
CategoryAnswer(id=answer3, votes=[AnswerVote(id=vote5, vote=accept), AnswerVote(id=vote6, vote=accept)])
CategoryAnswer(id=answer4, votes=[AnswerVote(id=vote7, vote=accept), AnswerVote id=vote8,vote=accept)])

Actual Behaviour

What I got instead:

CategoryAnswer(id=answer1, votes=[AnswerVote(id=vote1, vote=accept)])
CategoryAnswer(id=answer2, votes=[AnswerVote(id=vote3, vote=accept), AnswerVote(id=vote4 vote=accept)])
CategoryAnswer(id=answer1, votes=[AnswerVote(id=vote2, vote=accept)])
CategoryAnswer(id=answer3, votes=[AnswerVote(id=vote5, vote=accept)])
CategoryAnswer(id=answer4, votes=[AnswerVote(id=vote7, vote=accept)])
CategoryAnswer(id=answer3, votes=[AnswerVote(id=vote6, vote=accept)])
CategoryAnswer(id=answer4, votes=[AnswerVote id=vote8,vote=accept)])

This issue is very similar to https://github.com/micronaut-projects/micronaut-data/issues/192#issue-502140146

It only happens when you run a query that returns a list. I'm not seeing the issue with findById

Environment Information

Workaround

Seem to work fine if I include ordering in the SQL. e.g. findByOrderById

graemerocher commented 4 years ago

Could you provide an example in a github repo?

issmo commented 4 years ago

I do not face the issue as described but still there is a tricky issue here. The same as #760

What I have came with is the fact the method used is findAll which is declared in both the repository and the CrudRepository interface. The data-processor generate two implementations (which is actually invalid) for this method:

  1. findAll from the repository class taking into account @Join
  2. findAll from the interface which does not declare a @Join

Unfortunately the method being called is the one from the interface. In any case, this is invalid and should be avoided. I do not think that this should be resolved in micronaut-data as it is more linked to micronaut-core.

I have spotted the issue in the inject-java module and can submit a PR for it to discuss.

issmo commented 3 years ago

This actually has been already solved by this commit https://github.com/micronaut-projects/micronaut-core/commit/1ff3b52f4a6e3b4420d32f60117f7986d4091596#diff-b1a225b0a4c862cc096dac401c3daae1e9ffa5be5c6974142e8b1b63f3471600

master branch is good, however there is a regression in branches 2.1.x, 2.2.x of micronaut-core related to this file at least.

emmanuj commented 3 years ago

It's been a while but I'm going to see if I can still reproduce and report back. I'll share a repo if I'm able to.

emmanuj commented 3 years ago

I'm not able to reproduce this issue anymore. A few notes for anyone else who runs into it.

  1. Make sure you've defined your associations correctly
  2. FindAll uses INNER JOIN by default so if your entity is designed similar to the above, findAll() will return empty.
  3. You can define the fetch type on the repository instance: e.g:
 @Join(value="votes", alias="v_", type = Join.Type.LEFT_FETCH)
    public abstract List<CategoryAnswer> findAllOrderById();

See https://github.com/emmanuj/micronaut-data-one-to-many-demo for example with tests

Closing as not reproducible.

graemerocher commented 3 years ago

@issmo Do you have an example that reproduces the regression?

PiotrBaczkowski commented 3 years ago

Hello, i also noticed that it happens depending of order of items, for example when sql returns a.1 b.2 b.3 a.2 a.3 is more likely to return bugged result than a.1 a.2 a.3 b.1 b.2 also the more items the more likely it is to happen

I will upload my repo later tonight, i am able to reproduce this issue, micronaut data 2.1.1 micronaut core 2.1.3

issmo commented 3 years ago

@issmo Do you have an example that reproduces the regression?

@graemerocher here it is https://github.com/issmo/overriden-methods in gradle.properties change micronautVersion from 2.0.2 to 2.0.3 to fail the test (regression)

Please notice in the test case that the repository CategoryAnswerRepository contains an override of findAll from CrudRepository with different return type. This is key to reproduce the failure CategoryAnswerRepository: List findAll() CrudRepository: Iterable findAll()

The issue is in the SuperclassAwareTypeVisitor (micronaut-core/inject-java) where the processed methods are cached with a key containing the return type. In this case, the find method is implemented twice (once from the repository with @Join definition and once from the superclass without @Join)

in this original commit the implementation is correct https://github.com/micronaut-projects/micronaut-core/commit/1ff3b52f4a6e3b4420d32f60117f7986d4091596#diff-b1a225b0a4c862cc096dac401c3daae1e9ffa5be5c6974142e8b1b63f3471600

Below a comparison between v2.0.2 (good), v2.1.1 and v2.1.3 (both failing)

Commit from v2.1.3 tag (Failing - qualifiedName includes the return type) https://github.com/micronaut-projects/micronaut-core/blob/fd260da80d7b051bacf49d7d13edc293d388bd57/inject-java/src/main/java/io/micronaut/annotation/processing/SuperclassAwareTypeVisitor.java#L194

Commit from v2.1.1 tag (Failing - qualifiedName includes the return type) https://github.com/micronaut-projects/micronaut-core/blob/6d1d6dcfa2ee289f2d51f5cb6a5a6d93db0620f6/inject-java/src/main/java/io/micronaut/annotation/processing/SuperclassAwareTypeVisitor.java#L73

And commit from v2.0.2 tag (Good) https://github.com/micronaut-projects/micronaut-core/blob/5c424faab8131d28c41a1ccd85575bed6b208dfb/inject-java/src/main/java/io/micronaut/annotation/processing/SuperclassAwareTypeVisitor.java#L65-L71

I hope this is clearer. Let me know if it is not.

emmanuj commented 3 years ago

Sorry for closing it prematurely. Sounds like this is still an issue. Feel free to close it if that's not the case.

jameskleeh commented 3 years ago

@emmanuj Just FYI this only worked in 2.0.1 and 2.0.2. There was a change made for 2.0.1 that allowed this code to work but it had to be reverted because it caused a regression. This issue in itself is not a regression, but it is a bug we should fix. I think the issue is due to how Groovy decides to dispatch methods. I don't think the issue would happen if executed from Java code.

PiotrBaczkowski commented 3 years ago

@graemerocher Hello, I was busy with work but found some free time to post repo now

https://github.com/PiotrBaczkowski/micronautdataissue/tree/one_to_many

GET /categories to see that the issue is still present at microanut 2.1.3 and micronaut data 2.2.0

PiotrBaczkowski commented 3 years ago

Do we have any ETA for this? I think that it might be scary for new Microanut users to see such bugs.

graemerocher commented 3 years ago

Not yet no, but PRs welcome.

Also you always have JPA as an alternative and I know of no other tool or framework that supports joining in the same way as Micronaut Data so it is not like you are missing out on anything in other technology stacks

dstepanov commented 3 years ago

@PiotrBaczkowski I think this is fixed in 2.2.0 snapshot, you can try:

micronautVersion=2.2.0.BUILD-SNAPSHOT
repositories {
    jcenter()
    maven { url "https://oss.jfrog.org/oss-snapshot-local" }
}
PiotrBaczkowski commented 3 years ago

Still doesn't work. response example:

[ { "id": 1, "name": "Category #1", "iconUrl": "https://google.com", "visible": false, "productList": [ { "id": 1, "name": "Product#1", "price": 10, "description": "description", "visible": true, "categoryId": 1, "imageUrl": "https://google.pl", "createDate": [ 2020, 11, 18, 0, 17, 31, 601033000 ] } ], "createDate": [ 2020, 11, 18, 0, 17, 31, 601033000 ] }, { "id": 2, "name": "Category #2", "iconUrl": "https://google.com", "visible": false, "productList": [ { "id": 2, "name": "Product#2", "price": 10, "description": "description", "visible": true, "categoryId": 2, "imageUrl": "https://google.pl", "createDate": [ 2020, 11, 18, 0, 17, 31, 638586000 ] } ], "createDate": [ 2020, 11, 18, 0, 17, 31, 601033000 ] }, { "id": 1, "name": "Category #1", "iconUrl": "https://google.com", "visible": false, "productList": [ { "id": 3, "name": "Product#2", "price": 10, "description": "description", "visible": true, "categoryId": 1, "imageUrl": "https://google.pl", "createDate": [ 2020, 11, 18, 0, 17, 31, 638586000 ] } ], "createDate": [ 2020, 11, 18, 0, 17, 31, 601033000 ] }, { "id": 2, "name": "Category #2", "iconUrl": "https://google.com", "visible": false, "productList": [ { "id": 4, "name": "Product#2", "price": 10, "description": "description", "visible": true, "categoryId": 2, "imageUrl": "https://google.pl", "createDate": [ 2020, 11, 18, 0, 17, 31, 638586000 ] }, { "id": 5, "name": "Product#2", "price": 10, "description": "description", "visible": true, "categoryId": 2, "imageUrl": "https://google.pl", "createDate": [ 2020, 11, 18, 0, 17, 31, 638586000 ] } ], "createDate": [ 2020, 11, 18, 0, 17, 31, 601033000 ] }, { "id": 1, "name": "Category #1", "iconUrl": "https://google.com", "visible": false, "productList": [ { "id": 6, "name": "Product#2", "price": 10, "description": "description", "visible": true, "categoryId": 1, "imageUrl": "https://google.pl", "createDate": [ 2020, 11, 18, 0, 17, 31, 638586000 ] } ], "createDate": [ 2020, 11, 18, 0, 17, 31, 601033000 ] } ]

dstepanov commented 3 years ago

Looks like there are a few issues mixed up together: @issmo reported problem https://github.com/micronaut-projects/micronaut-core/issues/4447 and it's working in 2.2.0 @PiotrBaczkowski Can you add tests to your project that shows the problem?

PiotrBaczkowski commented 3 years ago

@dstepanov pushed test

issmo commented 3 years ago

@dstepanov I do confirm that this was impacted by two issues. One related to the multiple implementations of repository methods overrides which prevented the method decorated with Join to be executed. The second, related to the way data-jdbc collects the Join associated entities which do work only if the dataset is ordered by id. This issue is now easily reproducible as far as the database returns a non ordered dataset.

the issue is linked to the Stream support which somehow imposes to collect one entity (and its associated entities) at a time. https://github.com/micronaut-projects/micronaut-data/blob/6be157a327ae3dea268c7101ec16274edc4cf980/data-jdbc/src/main/java/io/micronaut/data/jdbc/operations/DefaultJdbcRepositoryOperations.java#L373-L375

In this scenario, the SqlResultEntityTypeMapper loops through the records until it finds a different Id which will only work if we have ordered the results by Id.

https://github.com/micronaut-projects/micronaut-data/blob/268e52c8775f9d5d7705112f275f34f0c59302bb/data-runtime/src/main/java/io/micronaut/data/runtime/mapper/sql/SqlResultEntityTypeMapper.java#L457-L480

I think this is the exact issue here. I see two possibilities here: Join should either collect all the resultset records, order or hash them and only then stream them, or add an ORDER BY "ID" clause to the query.

What do you think?

graemerocher commented 3 years ago

Adding ORDER BY ID makes sense to me

dstepanov commented 3 years ago

This is fixed in 2.3.0. Thanks for reporting and analysis @issmo @PiotrBaczkowski