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

Fix for ReportQuery not fetching relationships with BatchFetch.IN #2303

Merged
merged 6 commits into from
Nov 22, 2024

Conversation

Sheikah45
Copy link
Contributor

@Sheikah45 Sheikah45 commented Nov 18, 2024

This PR includes the fix for #2011.

A ReportQuery was not populating the DataResults for it's batchFetchPolicy. This prevented the mapping from knowing what keys to use for the in query and always returned null instead.

For now it is just targeted to master, but I believe it can be backported as you see fit.

@Sheikah45 Sheikah45 force-pushed the batchfetchin branch 4 times, most recently from 48f6aaa to 911118d Compare November 18, 2024 10:26
@Sheikah45
Copy link
Contributor Author

After some further testing I ran into the issue where the relationship was not populated when it was selected as not the first element in the Report Query and where the number of rows returned by the original query was greater than the batch size. This caused the foreignReferenceMapping to attempt to do an indexOf with the parent rows to find the starting index. However, the parent rows were the full row returned by the query but ReportQueryResult trimmed the database row that was passed to the ForeignReferenceMapping. Due to this the trimmed row didn't exist in the parent rows and so null was returned instead. For this reason I made it so that ReportQueryResult populates the trimmed rows as the parent row for the mappings specific to that item. But it could very well be the case that this is not the best approach and any input is welcome.

It may also be a possibility that the an untrimmed row could be used as it looked like fields were used directly by ReportQueryResult to get the necessary data out of the DatabaseRecord but I was unsure if this was always the case and it wasn't clear why the trimmed row was being used in the first place.

@Sheikah45
Copy link
Contributor Author

Additionally if there is a better place for the tests rather than their own app, let me know and I can move them to the appropriate location.

@rfelcman
Copy link
Contributor

Additionally if there is a better place for the tests rather than their own app, let me know and I can move them to the appropriate location.

No new Maven sub-project is fine.

Copy link
Contributor

@rfelcman rfelcman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rfelcman rfelcman merged commit d7114a0 into eclipse-ee4j:master Nov 22, 2024
6 checks passed
@rfelcman
Copy link
Contributor

Thank You
It will be available in the next 5.0.0-SNAPSHOT when nightly build pass from https://jakarta.oss.sonatype.org/content/groups/staging/org/eclipse/persistence/eclipselink/5.0.0-SNAPSHOT/ .

@Sheikah45
Copy link
Contributor Author

Sheikah45 commented Nov 22, 2024

Thanks, and just for information do you think this should be back ported?
If so I can open PRe against the other branches.

@rfelcman
Copy link
Contributor

Thanks, and just for information do you think this should be back ported? If so I can open PRe against the other branches.

Please open PR against other branches.

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