Implemented: Introduced base/util module (OFBIZ-12308)

[ieugen]

• Movend lang package to util gradle project
• Moved some classes
• First road block: Debug needs UtilValidation, UtilProperties -> UtilCache -> Entity ...

Explanation:

The goal is to have a set of common classes in a library (gradle project) that we can use in the other components.
To minimize friction, classes are moved as is.

First road block: org.apache.ofbiz.base.util.Debug depends on UtilValidation and UtilProperties that bring a LOT of stuff in and start the dependency cycle.

I fixed this in my previous PR by splitting some methods into a Util*Runtime class.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[OmarAbdullwahhab]

Hi Eugen,
You need to add more explanation about your goals,
What I understood is that you want to create a separate ( Shared Module/Library)
For utility classes that can even be used out-side OFBiz framework entirely, Right ?

[ieugen]

Hi @OmarAbdullwahhab ,

Thanks for taking an interest.
There are several goals achieved by splitting OFBiz into modules:

• make modules usable outside OFBiz - not just utility classes but components: data file component, entity engine
• improve visibility in project dependencies - each module will depend on the exact libraries. Right now some dependencies are not visible
• improve maintainability / upgrades (now project is distributed as source code in an all or nothing approach). This means people need to upgrade the full source tree instead of upgrading just the entity engine or maybe just a specific app.

One of the blockers for moving things forward are the circular dependencies between classes.
Debug uses UtilValidation that uses Debug (maybe not directly but through another chain).
IMO code should be built in layers.
Top layers should be built on lower layers.
Lower layers should not depend on top layers.

[OmarAbdullwahhab]

Thank you @ieugen
Now its very clear,
Yes actually the Debug class name is misleading,
It should be renamed to a name close to Logging/LogManager/OfbizLogging or a similar name.
And it uses UtilProperties and UtilValidate in only one place for each,
UtilValidate can be easily replaced with an internal check of the null or empty string.
So, a good thing is to isolate Debug and implement its auxiliary methods internally, whatever the redundancy
This is not a big deal at least currently.

[ieugen]

Hi @OmarAbdullwahhab ,

Thanks for the suggestions.
The case for Debug is an example.
The same issues - circular dependencies are systemic to OFBiz.
Look at UtilProperties and UtilCache relationship .
Look for uses of Entity* classes in some of the utility classes - Entity being high level layer and Utility being a low level layer (IMO) .

There is a pattern where some classes (UtilProperties + UtilCache is one example) initialize state as static fields.

private static final UtilCache<String, Properties> URL_CACHE = UtilCache.createUtilCache("properties.UtilPropertiesUrlCache");

These initialization bind the two classes in a circular dependency : UtilProperties depends on UtilCache that depends on Debug that depends on UtilProperties.

In some cases we can solve this and avoid code duplication if we pull out the initialization and move it into Main class for example.
So when the app starts, we can create the cache and assign it to UtilProperties.

This is a solution that should be less invasive.

The pattern here is the one described by Dependency Injection / Inversion of Control.
OFBiz does not use any dependency injection framework/ library.
Using such a dependency injection framework would make these kind of issues more visible.
We can do without an IoC framework, but it requires a bit of discipline and an eye for the patterns.

[OmarAbdullwahhab]

Yes I searched and found the following classes uses a ( Higher Level ) modules,
For example the following classes are using Entity Engine modules,

UtilFormatOut
UtilHttp
UtilMisc
UtilProperties
UtilValidate

So my opinion is now lets do

1. Determine the modules used in ofbiz base module which should not.
2. Classify the ofbiz base module classes according to its purpose ( http, xml, widget, logging, html, text ...etc).
3. Determine which ones should reside in the base module, or remain in the framework
4. move the classes of the framework that needs to be in another modules ( webapp, webtools, security, ...etc)

These are my ideas
give me your plan

[ieugen]

One solution to break the dependency in static initializations like :

private static final UtilCache<String, Properties> URL_CACHE = UtilCache.createUtilCache("properties.UtilPropertiesUrlCache");

is to move initialization in the app entry point: https://github.com/apache/ofbiz-framework/blob/trunk/framework/start/src/main/java/org/apache/ofbiz/base/start/Start.java

with code like this (provided we use an interface):

public static void main(String[] args) { 
  // .... 
  UtilCache<String, Properties> URL_CACHE =  UtilCache.createUtilCache("properties.UtilPropertiesUrlCache"); 
  UtilProperties.setUrlCache(URL_CACHE);   
  // ....
} 

A similar initialization pattern can be used for Debug with UtilProperties.
Where properties are loaded in Start.
Then UtilProperties LEVEL_ON_CACHE is initialized in Start.
After reading the code better, this particular case should be easy to implement for Debug.
A bit more work for UtilCache - since we need an interface there.

cc @mbrohl , @JacquesLeRoux : is the above solution ok for you?
I am asking because I would like to get the buy in of people in the community as soon as possible.

[danwatford]

Hi @ieugen ,

I'm generally on-board with the IoC approach, but it is safer when dealing with instantiated objects rather than static fields in classes. If we had an IoC framework, such as Spring, I think this would be an easy pattern to implement.

Alas, we are don't have such a framework, and we shouldn't let 'perfect be the enemy of good', so should carefully implement our own IoC patterns per your suggestion.

Personally, I'm happy for classes at the same level or in the same module to reference each other, as long as those classes do not require any configuration. In the particular case of UtilCache/UtilProperties/Debug, it seems to me that the configuration is actually in the static initialiser of Debug where we find a reference to UtilProperties.

My preference would be to break the link in Debug by removing reference to UtilProperties. Instead there should be a static method in Debug to apply the configuration that is currently being read from debug.properties. Some other class (such as Start) should be responsible for reading debug.properties and determining the debug configuration.

Debug currently has two jobs: reading its configuration from a properties file; carry out logging. The above proposed changes would reduce Debug's responsibility and break the dependency cycle between UtilCache, UtilProperties and Debug.

Summary: I'm broadly supportive of the pattern, but IoC with static initialisers and methods can be risky. Focussing on parts of the dependency cycle where we can reduce the responsibilities a class has might yield a similar result.

If you decide to go with your original proposal of
UtilCache<String, Properties> URL_CACHE = UtilCache.createUtilCache("properties.UtilPropertiesUrlCache"); UtilProperties.setUrlCache(URL_CACHE);

please consider adding a null check in the places where URL_CACHE is used and throw a suitable exception to explain the cause, rather than letting a NPE bubble up the stack.

[ieugen]

hi @danwatford ,

Thanks for taking the time.

so should carefully implement our own IoC patterns per your suggestion.

I think this is the sensible way to move forward.

Personally, I'm happy for classes at the same level or in the same module to reference each other

The issue is with circular dependencies. It often leads to situations where it is very hard to refactor - like our case now.
Especially if lower lever Util classes depend on "higher" level classes like Entity .

In the particular case of UtilCache/UtilProperties/Debug, it seems to me that the configuration is actually in the static initialiser of Debug

Yes, this one should be easy to fix via the IoC pattern.

My preference would be to break the link in Debug by removing reference to UtilProperties. Instead there should be a static method in Debug to apply the configuration that is currently being read from debug.properties. Some other class (such as Start) should be responsible for reading debug.properties and determining the debug configuration.

Totally agree.

I'm broadly supportive of the pattern, but IoC with static initialisers and methods can be risky. Focussing on parts of the dependency cycle where we can reduce the responsibilities a class has might yield a similar result.

Agree. We are where we are and we can do what we can do.
Baby steps forward is ok for me.
At some point we might have the IoC library/framework discussion.

please consider adding a null check in the places where URL_CACHE is used and throw a suitable exception

Sounds reasonable.

[grozadanut]

I have a quick note on the IoC container idea. I also used to believe we need one in Ofbiz(for example Spring) in order to help with testability, but recently I discovered an article from James Shore where he explains the testing approach he takes.

So now I don't think we really need an IoC container, neither do we need to write one ourselves. We can just use constructor based injection. This would work for a normal non-static scenario, not sure about the static case you have here.

Also, in order to not instantiate the whole dependency tree in unit tests, see the Parameterless Instantiation section in the article.

Here is the article in question: https://www.jamesshore.com/v2/projects/nullables/testing-without-mocks

[OmarAbdullwahhab]

Regarding the cyclic dependency .
I found this nice video deserves an hour
https://youtu.be/dNN8yTnA4fI
( Java's Cyclic Object Graphs Challenges )