Fix orderBy for cached many requests - v2#3486
Fix orderBy for cached many requests - v2#3486nPraml wants to merge 3 commits intoebean-orm:masterfrom
Conversation
| } else { | ||
| query.select(many.targetIdProperty()); | ||
| } | ||
| } |
There was a problem hiding this comment.
CHECKME: Is ist ok, if the onlyIds code path is in newLoadManyQuery or may there be cases, where configureQuery overwrites the select?
| query.where().idEq(parentId); | ||
| query.setBeanCacheMode(CacheMode.OFF); | ||
| query.setMode(Mode.LAZYLOAD_MANY); | ||
| query.setLazyLoadManyPath(many.name()); |
There was a problem hiding this comment.
CHECKME: what do we need here?
- I think we've lost the
CacheMode.OFFfor refresh? - see: https://github.com/ebean-orm/ebean/pull/3486/files#diff-5ce08d7970ceac437b60d46bbe9a40b0a4893f2211045684f0c29342c3322541R105 - Do we need
Mode.LAZYLOAD_MANY
|
Would be good if we have some feedback from Rob, because I am unsure about the change ;) |
|
Hmmm, when we say "refactored solution" it can mean different things to different people. Generally a code reviewer wants to know what code was changed and why, and what code was simply moved but not actually changed [and maybe why it was moved / why include it in the PR?]. Looking to distill the "changes to the logic" and confirm there were no "Accidental changes" or "Changes with unknown impacts". The easiest PRs only change what they absolutely need to and nothing more. In this PR I see some changes that don't look the same but I don't know if that was actually the intention or something else? Can you state what the changes here are and why they were maed? For example, why change findOne into findList? Why was that done etc. You can do this by putting your own comments into the PR - 1 per change. Like "This code was moved over to other location with no other changes", "findOne changed to findList because ...". Add those comments in and then I can review this again. |
|
This PR can be closed now right? as we merged in the other alternative PR. |
|
Hello @rbygrave, sorry I was on vacation, hence the late reply. I probably wouldn't have time to work on this PR for the next few weeks. Is it ok for you? |
3 participants
Hello @rbygrave,
this is the refactored solution, the easy one is in #3485
Please take a look and give us feedback.
Cheers
Noemi
@rPraml FYI