search

OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.43k stars 2.4k forks source link

Widget Display Driver being called multiple times in Admin #3250

Open marlon-tucker opened 5 years ago

marlon-tucker commented 5 years ago

Using 1.0.0-beta3-70622

In our Orchard project, we have a fairly large amount of Widgets, most of which have to make external API calls to retrieve the data they need. Both when they are in their edit state as well as their display state.

When creating or editing a page, Orchard calls EditAsync on every DisplayDriver on every Widget that is currently active in the system. For us, this means all the external API calls happen if a page is created or edited (regardless if they are used on the page).

I assumed Orchard was caching these for when the user added the widget to the page, however, the EditAsync method is called again when that happens.

I realise our situation is unique, and Orchard was designed to have these methods read data from its own database, however it would be fantastic if this can be mitigated somehow. Either by removing the initial call to EditAsync as it does not seem like it is required? Or when that initial call happens, can a flag be present which can be checked, so we don't make external API calls unless required.

sebastienros commented 5 years ago

The widgets are all rendered when a page is edited, if this page contains a FlowPart or a BagPart (anything that can create widgets dynamically). It's done so that the scripts/css files that the widget editor requires are inserted in the rendered page, before the widget itself is actually used (dynamically). These css/scripts will be rendered from the view, so we need to execute the view, which is driven by the Edit method. So we need to call the Edit method. We could use a flag like an http header, to let you handle that case specifically, so you don't have to call the APIs.

marlon-tucker commented 5 years ago

Thank you for explaining, that makes sense.

Having a flag would be useful as a way to avoid expensive calls during that process.

sebastienros commented 5 years ago

Please implement it if that can help you get it more quickly and do a PR.

marlon-tucker commented 5 years ago

What initiates that initial call?

sebastienros commented 5 years ago

Search for the text // Render a mock widget so that its resources are included in the page I found three locations.

marlon-tucker commented 5 years ago

ok thank you, I'll take a look

marlon-tucker commented 5 years ago

Hi @sebastienros

When you get a chance, can you take a look at this diff please?

https://github.com/OrchardCMS/OrchardCore/compare/dev...marlon-tucker:MockContentItemFlag

I want to make sure I'm going about this in an acceptable way before I tidy up and add some comments.

I have chosen a generic solution which allows Orchard Modules to set arbitrary flags on the current HttpContext, I'm not completely happy with the naming strategy but I couldn't think of a better name to use. The benefit of this solution is it will allow future scenarios where modules need to set a particular context flag, and other modules having to detect that flag's presence.

The downside is it is not particularly discoverable to developers.

I did think about adding a IsMock property to the BuildEditorContext class as that would fix the discovery problem, but it's a question of whether you want the mocking feature to be that deeply embedded into Orchard or not. If it is, it would be logical to then have a isMock parameter to the BuildEditorAsync method, which already has a fairly large amount of parameters.

Anyway, am open to any ideas you may have.

sebastienros commented 5 years ago

You are still calling BuildEditorAsync, how is that fixing your issue?

marlon-tucker commented 5 years ago

When a module wants to render a widget in a "mock" way, it is surrounded in a using statement, which comes from an injectable service IHttpContextFlags.

That same service can be injected into a display driver and can be queried as to what flags are currently set. Flags are in the form of static readonly objects, for example in this instance the flag is OrchardCore.ContentManagement.Display.HttpContextFlags.RenderingMockContentItem

The alternative way would be to change how BuildEditorAsync is called, ideally by having a parameter object which has defaults, so callers only have to set the properties that they are interested in. However I realise that is quite the breaking change.

ns8482e commented 4 years ago

Related #5149