Skip to content
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

Tests only - new test on multiple many joins #3581

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

apflieger
Copy link

This test reproduce the problem of #3573
The field initialization = List.of(); prevents eager secondary query.

This test reproduce the problem of ebean-orm#3573
The field initialization `= List.of();` prevents eager secondary query.
rbygrave added 2 commits March 3, 2025 21:34
The test was using field access??? when that isn't valid unless we specifically turn on enhancement to support that.

Using proper method access though stops the test from failing.
@rbygrave
Copy link
Member

rbygrave commented Mar 3, 2025

While this test is Ok'ish in that it shows the issue with List.of(), it does actually rely incorrectly on field access ... which you can't do unless you explicitly enable a special enhancement flag entity-field-access.

That is the assert:

    assertThat(list2.get(0).orderInvoices).hasSize(2);

... is relying on the GETFIELD call for orderInvoices, and if that changes to an expected method call like getOrderInvoices() then the test fails to reproduce an error (because lazy loading is invoked and replaces the empty collection with a populated one via lazy loading).

So that means I'm concerned that your application code is incorrectly using field access? [Or why did the test case get written using field access? Did you just get lucky?]

@apflieger
Copy link
Author

Heh, yes I was aware of that. Our production code doesn't access fields directly. I was trying to reproduce the case in a simple way and I had in mind that this test wouldn't get merged anyway, so I judge it to be good enough for a disposable test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants