Use ContentManagerSession in GetMany

OrchardCMS / Orchard

Orchard is a free, open source, community-focused Content Management System built on the ASP.NET MVC platform.

https://orchardproject.net

BSD 3-Clause "New" or "Revised" License

2.37k stars 1.12k forks source link

Use ContentManagerSession in GetMany #8705

Open MatteoPiovanelli-Laser opened 1 year ago

MatteoPiovanelli-Laser commented 1 year ago

This is the first hacky attempt. I'm not satisfied with the code: I look at it and it feels like it's doing some stuff too many times where it shouldn't. Anyway, it does reduce the number of repeated calls to the database. In some setups, that may become significant.

I haven't had a chance to properly test this, or measure the timing performance, to make sure that we aren't losing time in the work we do to avoid calls to the db.

To summarize, my concerns with my code in this PR right now:

it looks hacky to me.
I'm not 100% confident it will always 100% of the time in every condition return the exact same results as the live code, even though I can't think of a case where it wouldn't.
I have tested the timing of the methods I changed.

MatteoPiovanelli-Laser commented 1 year ago

@BenedekFarkas forgot to tag you in the main post

BenedekFarkas commented 1 year ago

I'll need a few days to get down to testing it properly, but it looks good on first glance!

MatteoPiovanelli-Laser commented 10 months ago

@BenedekFarkas I just realized this is still open. My tests still look good, But I'm still not too fond of the code I wrote.

BenedekFarkas commented 10 months ago

Well, DefaultContentManager is very old, so there's probably a lot of stuff we could refactor (which sort of affects what you can achieve there). I wouldn't merge it without unit/Spec testing completing on it first but being able to run tests is still blocked by me (https://github.com/OrchardCMS/Orchard/issues/8686). We could also add some specific tests to verify this change, if necessary.

MatteoPiovanelli-Laser commented 10 months ago

this is targeted at 1.10.x, so I think specflows should be able to run... I'll get back to you on this.

MatteoPiovanelli-Laser commented 10 months ago

I merged the latest 1.10.x into the branch for this PR and I was able to run all specflows.

MatteoPiovanelli-Laser commented 4 months ago

I'm still not 100% on this solution. Results look consistent, and I see fewer queries in the logs, but no great impact on final timings (in quick page load tests I did). Perhaps doing a conditional and short circuiting the case when there is a single Id being queried would be useful, as that is an actual common case I see in our tenants, in ContentPickerFields and such