[BLOSSOM-304] DialogDefinitionRegistry decoration context missing Created: 27/Jun/22  Updated: 26/Sep/22

Status: Open
Project: Blossom
Component/s: None
Affects Version/s: 3.5.1
Fix Version/s: None

Type: Bug Priority: Critical
Reporter: Björn Eschle Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:
Epic Link: Support
Team: DeveloperX

 Description   

When using blossom with liveCopy, the required blossom dialog definition decoration is not executed, because the instantiation of AppController is throwing NoSuchComponentException (

DialogCreatorDefinitionDecorator.CompatibilityAppsPredicate).

As a result, a legacy UI 5 ConfiguredFormDialogDefinition is returned.



 Comments   
Comment by Björn Eschle [ 28/Jun/22 ]

I had a look at this issue with @Dominic. The root cause seems to be that there is no MgnlContext present in the dialog action command, which isrequired to build the dialogDefinition in blossom.

Comment by Björn Eschle [ 28/Jun/22 ]

Found the cause:

PropagateMasterContentChangesActionDefinition is configured as an asynchronous definition in the liveCopy page decoration. The MgnlContext is not set in the executing thread! Redecorating it to asynchronous: false works, but setting the context on the thread would be a nicer solution.

Maybe @roman.kovarik@magnolia-cms.com can give some insights.

 

Comment by Roman Kovařík [ 28/Jun/22 ]

Hey Björn,  

looks like the blossom dialog definition creation and form creation is bound together.

This works fine when opening the dialog from UI (in the thread with WebContext, request/response and AppController) but livecopy probably accesses definitions to get the field names etc. asynchronously (maybe from a command?).

I can think of this workaround (not tested):

DialogCreationContext newContext = new DialogCreationContext();
if (MgnlContext.isWebContext()) {
    newContext.setRequest(MgnlContext.getWebContext().getRequest());
    newContext.setResponse(MgnlContext.getWebContext().getResponse());

    Components.getComponent(AppController.class).getCurrentAppContext()
            .map(AppContext::getActiveSubAppContext)
            .map(activeSubAppContext -> SessionStore.access().getBeanStore(UiContextReference.ofSubApp(activeSubAppContext)))
            .map((Function<BeanStore, ValueContext<?>>) beanStore -> {
                ValueContext<?> valueContext = beanStore.get(ValueContext.class);
                newContext.setValueContext(valueContext);
                newContext.setParentView(beanStore.get(UIComponent.class));
                return valueContext;
            })
            .flatMap(ValueContext::getSingle)
            .filter(Node.class::isInstance)
            .map(Node.class::cast)
            .ifPresent(node -> {
                newContext.setContentNode(node);
                newContext.setContentPath(NodeUtil.getPathIfPossible(node));
            });
}
DialogCreationContextHolder.set(newContext);

Looks like the request/response are not even used.

Not sure about the node/path, you might know better than me, but I'd expect it's required only  when dialogs are opened.

The proper solution would be most likely moving the DialogCreationContext of out the registry mechanism as I can be accessed outside of UI.

Best regards
Roman
 

Comment by Björn Eschle [ 28/Jun/22 ]

Hey Roman,

thanks for your feedback. I tried this as well. Unfortunately the MgnlContext (ServletContext) is required to build the DialogDefinition ( see UI6DialogCreator ).

Is it possible to set the MgnlContext for the thread used in the async command?

Kind regards,

Björn

Comment by Roman Kovařík [ 29/Jun/22 ]

Hi,

I wasn't sure about the steps to reproduce so I've tried: 

  1. Click Push master changes 
  2. Select a page
  3. Push and Publish
  4. Push & Publish now

As PropagateMasterContentChangesAction has asynchronous=true, WebContext/AppController are not present in the executing thread.

  1. I've edited http://localhost:8080/magnoliaAuthor/.magnolia/admincentral#app:resources:edit;/livecopy/decorations/pages-app/apps/pages-app.subApps.browser.actions.yaml:edit
  2. Replaced asynchronous=true with false (the actions might be slow with huge pages)
  3. I haven't found any other asynchronous actions (at least from the live copy configs https://git.magnolia-cms.com/plugins/servlet/search?q=project%3AADDON%20repo%3Alive-copy%20asynchronous
  4. Now WebContext/AppController are available (at least for this use case). 

 

Let me know if this workaround works for you or share steps to reproduce other use cases so I can test locally or schedule a call.

Comment by Björn Eschle [ 29/Jun/22 ]

Hey @Roman,

decorating the pages app action with async=false works, which is what we already did as a workaround.

However I don't think it is the nicest solution as this can take quite some time if there are a lot of child pages.

If you have some time I would like to have a quick meeting to brainstorm a sustainable solution.

Regards,

Björn

Comment by Björn Eschle [ 29/Jun/22 ]

Call with @Roman:

Setting the context in the async executor thread can have unwanted side effects, so we should adjust blossom to be able to create the definitions without requiring a context.

I'll have another look with @Mika once he is back from his vacation.

Idea for ArgumentResolverParameterResolver: Use ApplicationContextAware

Comment by Mikaël Geljić [ 14/Jul/22 ]

Thank you both for digging and for the comments. I had a quick look yesterday, until then the dependency between Blossom and Live Copy was never quite clear nor anticipated when we developed either modules.

On the one hand, Live Copy uses dialogs as the model to collect properties and sync changes between master and copies. On the other hand, Blossom allows for that model to be dynamic based on a number of context attributes.
For the record, the DialogCreator invocations were originally filtered because of several lifecycles where the "node" or other such DialogCreationContext attributes were not available, and we did not need to build an effective dialog.

Meanwhile, if you use Blossom to produce mostly static definitions, it could make sense for us to try to invoke the dialog/tab-factories, except we don't know in advance if a parameter's gonna be missing for injection.
One way to circumvent that would be to replace/complement direct injection of Node, WebContext, etc. with lazy indirect Providers. Or indeed skipping them might be sufficient for your use cases, as rkovarik suggested.

Still the decorator fails on the #appliesTo predicate before that. If we support async invocation, I'd like to have an explicit condition to identify it as a legit caller.

As a result, a legacy UI 5 ConfiguredFormDialogDefinition is returned.

As far as I see, the NoSuchComponentException suggests the dialog def should be empty/unprocessed, unclear where the legacy def might come from.

Re: ApplicationContextAware, what do you have in mind?

Comment by Mikaël Geljić [ 22/Jul/22 ]

beschle Can you please elaborate on steps to reproduce in description? Following same steps as rkovarik, I do observe a few warnings in logs

2022-07-22 17:37:55,151 WARN  le.blossom.dialog.DialogCreatorDefinitionDecorator: No AppController available while invoking Blossom dialog with id custom-text-dialog. Not invoking dialog controller.

However, content is successfully pushed/copied.

Comment by Björn Eschle [ 08/Aug/22 ]

Hey @Mika,

sry, for my late response, there was a lot on my plate before my vacation...

Re: ApplicationContextAware, what do you have in mind?

The main reason we need the context is to get the AutowireCapableBeanFactory in the DialogCreator, used in BeanParameterResolver to resolve the spring beans.

We could implement ApplicationContextAware to get the applicationContext, which than can be used to resolve the beans: (Untested/-compiled)

 

public class BeanParameterResolver extends ParameterResolver implements ApplicationContextAware {
    private ApplicationContext applicationContext;

    public BeanParameterResolver(ParameterResolver parent) {
        super(parent);
    }

    @Override
    public Object resolveParameter(Class<?> parameterType) {
        return Optional
                .ofNullable(applicationContext)
                .map(context -> (Object)context.getBean(parameterType))
                .orElseGet(() -> super.resolveParameter(parameterType));
    }

    @Override
    public void setApplicationContext(final ApplicationContext applicationContext) throws BeansException {
        this.applicationContext = applicationContext;
    }
}

 

 

As far as I see, the NoSuchComponentException suggests the dialog def should be empty/unprocessed, unclear where the legacy def might come from.

Currently the incomplete legacy dialogDefinition is created in the BlossomDialogDefinitionProvider and then gets 'decorated' (if predicate applies) in the DialogCreatorDefinitionDecorator (Although in my opinion it would be nicer to move the 'decoration' to the dialogDefinitionProvider).

Kind regards,

Björn

Generated at Sun Feb 11 23:32:09 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.