[WW-5173] - Attempt to fix DI behaviour for custom cache factories

[JCgH4164838Gh792C124B5]

Hello Apache Struts Team.

This is an initial attempt to fix the dependency-injection behaviour for the custom expression and BeanInfo cache factory mechanism introduced in 6.0.0 (was 2.6). It took a fair amount of debugging and playing with configuration values to arrive at a small number of changes.

With these changes, and a custom implementation, it seems that Container getInstance(type, name) will now return the custom implementation. That is an improvement, but the default container instance construction of OgnlUtil still seems to use the default implementations (so it seems something is still missing at this point).

Please advise if anyone can see additional changes that might resolve the remaining issue(s) for custom cache factory assignment.

[coveralls]

Coverage increased (+0.03%) to 50.663% when pulling 15bbf0ef1d21ae75f6eec243c613527fe9e90f79 on JCgH4164838Gh792C124B5:localS2_WW5173_Upd1 into 64d054221f959397f47f6335dca25d7817f245c0 on apache:master.

[yasserzamani]

Thank you so much @JCgH4164838Gh792C124B5 ! I remember that I also had some same difficulties when I was working on #167 . Finally I simply workaround it by injecting Container itself and getting required bean instead of injecting the bean itself. This isn't a good practice albeit, as Lukasz commented there. But this is what I could and able to do that time to make it working. Even with a lot debugging.

I'm trying to say that your case is same but more complex, i.e. with constructor injection with multiple overloaded constructors. But you were able to make it work with no hack. So I think now you know DI better than any of us and your changes are best possible solution :)

BTW I found all usages of @Inject in constructors parameters by IntelliJ IDEA. There are similar cases already implemented in Struts. I assume they're working. So the problem is not because of multiple overloaded constructors I think.

And could you please explain a bit more about:

but the default container instance construction of OgnlUtil still seems to use the default implementations

Thanks again!

[JCgH4164838Gh792C124B5]

Hello @yasserzamani . Thanks for taking a look and providing feedback. 😄

I will attempt to respond to your question for more detail on the container construction still using the default implementations. The information is based on stepping through breakpoints using a sample application with a custom expression cache factory configured.

The Dispatcher init() method container/configuration loading seems to happen in two phases. Both phases invoke DefaultConfiguration methods, the first pass-through calling createBootstrapContainer(), and then a second pass-through calling reloadConatiner(). For the "bootstrap" container, the OgnlUtil instance created has the default cache factory instances injected, but those factories do not have any configuration parameters set (just the default state, without injection) at that point. Next, for the "reloaded" container, the OgnlUtil instance created has the default factory instances injected, but this time with the parameters that were set in configuration (size, LRU mode).

In neither case is the custom cache factory dependency recognized as being present to be injected. In the test application, when container.getInstance(ExpressionCacheFactory.class, container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_FACTORY)) is called within an action, it then returns the custom cache factory ... but the container has to construct the instance at that point (being the first call generating the custom factory instance).

So, the custom factory instance appears to be wired-up correctly, but something is still missing. Maybe a ContainerProvider of some sort is needed to allow for "early-binding" of something like this, or maybe something else, but so far nothing I have tried has allowed for a custom cache factory (expression, beaninfo) to be injected into the OgnlUtil singleton instance.

I am not sure what else to try at this point, something in my understanding of the mechanism seems to be missing ... 😞

[yasserzamani]

Thank you so much @JCgH4164838Gh792C124B5 for the explanation! Yes I remember too in past I was observing multiple hits on a breakpoint during injections, provided I expected one hit. Could you please somehow share that sample app including breakpoints places? I can take a look when I find some free cycles :) thanks again for your works!

[JCgH4164838Gh792C124B5]

Hello @yasserzamani . Thanks for the update.

I attached a sample application project (S2_StarterApp_1.zip) to the WW-5173 JIRA as a .zip file (it is just a struts2-archetype-starter Maven Archetype project with an attempt to wire in a custom cache factory implementation). Unfortunately, my IDE does not seem to have a breakpoint export facility, so I put a comment in the CustomOECFactory class indicating where I had set the major breakpoints in general. Hopefully that will be enough to work with (if you have spare cycles to take a look), but if not, please let me know.

One other note, I think the recent naming changes for the DTD from 2.6 to 6.0 was causing issues, as I had to adjust the sample application configuration to use 6.0.0 (rather than 6.1.0-SNAPSHOT) in order to successfully deploy it for interactive debugging.

[yasserzamani]

Thank you very much @JCgH4164838Gh792C124B5 ! I've been working on it today. You're right, doesn't work as expected, provided it's added similarly like ActionProxyFactory. I'll work on it in next days :) thanks again!

[yasserzamani]

Hi @JCgH4164838Gh792C124B5 sorry I was wrong yesterday. Your sample app works, however you need to apply a few fixes on your previous work which I introduced by #581 - I learnt them from already working extension points like ObjectFactory and ActionFactory.

Please feel free to either merge my PR and rebase your PR based on it, or simply redo same fixes on your PR and close my PR please.

Thanks again for your works! and sorry for my late answer.

BTW regarding DTD problem, please apply following patch on your sample app

Index: src/main/resources/struts.xml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/main/resources/struts.xml	(date 1659109456598)
+++ src/main/resources/struts.xml	(date 1659109456598)
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8" ?>
 <!DOCTYPE struts PUBLIC
-        "-//Apache Software Foundation//DTD Struts Configuration 2.6//EN"
-        "http://struts.apache.org/dtds/struts-2.6.dtd">
+        "-//Apache Software Foundation//DTD Struts Configuration 6.0//EN"
+        "http://struts.apache.org/dtds/struts-6.0.dtd">
 
 <struts>
 

[JCgH4164838Gh792C124B5]

Hi @yasserzamani . Thank you so much for taking a look, determining what was failing, and creating PR #581 to show a working solution. 😄 I was not certain I could do a merge/rebase of your PR correctly, so I just implemented the equivalent changes in this PR (with some slight modifications). When built locally, and debugging with the sample app, the custom factory appears to get instantiated properly, so the changes you proposed/provided look like they work to me. 👍 The subtleties for how the naming/associations for the DI configuration change the behaviour are still a bit mysterious to me, but comparing what worked and did not in this PR history might be helpful to others in the future. Thank you so much, and let me know if the PR looks OK now. If anyone else would like to comment, please feel free as well.

[yasserzamani]

Thank you @JCgH4164838Gh792C124B5 for testing bits!

The subtleties for how the naming/associations for the DI configuration change the behaviour are still a bit mysterious to me

For me too. Today I also tried a lot to make it working as you planned in previous version. Finally instead of I fixing Struts, it fixed me :)) and taught me that looks like that when you want to introduce an extension point, then you shouldn't use a concrete name in .factory( and let Struts use its default name. Otherwise we cannot inject it by a dynamic name. At least I couldn't. Instead, we should define aliases like what's done in .alias(.

At bottom this LGTM and will merge. Just one thing. A few tests which are from past and not related, are deleted, I think by mistake, right? If so, then could you revert them. thanks!

[lukaszlenart]

I added an example app using custom cache implementations and it works with this PR 💯 https://github.com/apache/struts-examples/commit/50013f1beb1b159d5e8fffc962d1bb1f929fbc0d

[JCgH4164838Gh792C124B5]

Hi @yasserzamani.

Thanks for the review, and more background on the DI and alias behaviour. 😄

At bottom this LGTM and will merge. Just one thing. A few tests which are from past and not related, are deleted, I think by mistake, right? If so, then could you revert them. thanks!

The testXXXAlternateConstructor() tests were intentionally removed with the recent set of changes, as they no longer made sense to me once the changes you made to the parameterized OgnlUtil constructor were applied to this PR. Those deleted tests were originally code coverage "duplicate" tests of existing tests to cover OgnlUtil(null, null) calls, but now using null parameters is invalid and will result in IllegalArgumentException , so those tests no longer seemed to make sense. The general test for OgnlUtil with null parameters should now be covered by testDefaultOgnlUtilAlternateConstructorArguments() , and there are still the testXXXAlternateConstructorPopulated() tests covering non-null parameters.

Please let me know if the above makes sense, and if you agree the test changes are valid ?

If not, I can restore the deleted tests and make changes to pattern them like testDefaultOgnlUtilAlternateConstructorArguments(). Please let me know what you think.

[DeepakLalchandani]

I have been told struts is obsolete. Is it true ? Then how come developers are adding new features ?

Give me a insight on this

[yasserzamani]

@JCgH4164838Gh792C124B5 thanks for clarification! LGTM :+1:

[yasserzamani]

@DeepakLalchandani thanks for asking! But please ask in Struts user mailing list instead where you will reach very more people (and more related people) than here.

[lukaszlenart]

@DeepakLalchandani you probably heard rumour about Apache Struts 1 EOL - for more info please ask your question on the mailing list as suggested by Yasser.