ebean-orm
Fix orderBy for cached many requests - v2
Hello @rbygrave,
this is the refactored solution, the easy one is in https://github.com/ebean-orm/ebean/pull/3485
Please take a look and give us feedback.
Cheers Noemi
@rPraml FYI
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. I would still leave the PR open because we noticed in https://github.com/ebean-orm/ebean/pull/3485/files#r1783081211 that the queries that should actually look the same are different (difference: the master bean is in the cache or not). For this reason, we started refactoring so that queries are only built in a central location.
Is it ok for you?