Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvement: Extract datafile component, base/util, base/crypt projects #678

Closed
wants to merge 1 commit into from

Conversation

ieugen
Copy link
Member

@ieugen ieugen commented Dec 6, 2023

  • There are a LOT of circular dependencies !!!
  • ~99% it is code shuffling - no behavior change
  • I had to split some files to be able to shuffle code around: UtilValidateEmpty, UtilPropertiesRuntime, etc.
  • A lot of code changes are related to CheckStyle
  • We should consider spotbugs for code formatting via gradle

We can publish datafile component as for example:
org.apache.ofbiz/component-datafile/18.12.10

Appllications can consume this via reuglar maven dependency.
The dependency could be specified as having runtime / provided scope so when it gets pulled in OFBiz it will use the version available there (patch versions).

Also each component jar can be a Java 9 module - to encapsulate it's dependencies and avoid jar hell.

I have added a sample project that uses the crypto code from OFBiz lib to do crypto.
I could do a datafile example tool but that would take more time.
Fill in the blanks: ofbiz functionality in other apps and services.
Ease the integration at java level.
Open the borders of OFBiz to the outside developer world.
https://github.com/ieugen/ofbiz-tooling-demo

Also as exploratory work, making enity engine as a library (on top of this PR) takes ~ 670 additions and ~ 370 deletions. Work is not done yet, but close. See ieugen#3 .

@ieugen
Copy link
Member Author

ieugen commented Dec 6, 2023

@ieugen
Copy link
Member Author

ieugen commented Dec 7, 2023

A glimpse of the cyclic dependencies in OFBiz

image

@ieugen ieugen force-pushed the OFBIZ-3500-split-crypt-datafile branch 4 times, most recently from c308eb9 to 1ca2a41 Compare December 8, 2023 22:34
@ieugen ieugen force-pushed the OFBIZ-3500-split-crypt-datafile branch from 1ca2a41 to b5c4c7a Compare December 19, 2023 10:25
Copy link

sonarcloud bot commented Dec 19, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

…BIZ-3500)

* There are a LOT of circular dependencies !!!
* I had to split some files to be able to shuffle code around:
  UtilPropertiesRuntime, etc.
* A lot of code changes are related to CheckStyle
* We should consider spotbugs for code formatting via gradle
@ieugen ieugen force-pushed the OFBIZ-3500-split-crypt-datafile branch from b5c4c7a to d010816 Compare February 17, 2024 22:27
Copy link

sonarcloud bot commented Feb 17, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@JacquesLeRoux
Copy link
Contributor

We can neglet the CodeQl analysis. It reports 208 "RandomStringUtils.random(SECURE_RANDOM" and it's safe because of SECURE_RANDOM. Sincerely the tool is clumsy. I have also dismissed a lot (300+ ?) in the OOTB code.

@ieugen
Copy link
Member Author

ieugen commented Feb 23, 2024

We can neglet the CodeQl analysis. It reports 208 "RandomStringUtils.random(SECURE_RANDOM" and it's safe because of SECURE_RANDOM. Sincerely the tool is clumsy. I have also dismissed a lot (300+ ?) in the OOTB code.

Other than this, did you manage to take a look at the code?

@JacquesLeRoux
Copy link
Contributor

Hi Ieugen,

Not yet, I'm currently focusing on the new codeQL implementation.

I tried it few years ago for Java but it was not able to handle OFBiz, only JavaScript. It now works but still, like for JavaScript in the past, has a number of false alerts, like this one. I remember now, exactly 271 same cases. Fortunately GH is able to ease dismissing them, still 553 remaining, a lot of duplicate.

@JacquesLeRoux
Copy link
Contributor

BTW, this is about whole OOTB OFBiz code. There is no other than this one in your PR.

@JacquesLeRoux
Copy link
Contributor

JacquesLeRoux commented Feb 24, 2024

For now I had only a cursory look at your work. But, as I said, I intend to have a deeper look in 2024.

@ieugen
Copy link
Member Author

ieugen commented Feb 25, 2024

Thank you @JacquesLeRoux , It would be great.
Right now I paused my work on OFBiz.
IMO it does not worth investing more until I have a path forward.
I believe this PR opens the door for that path forward: more modular ofbiz, ability to develop tooling outside the codebase that can use parts of OFBiz.

Looking forward to your review and hopefully merge of this PR (in this form or another).
After that I will continue working on OFBiz.

@JacquesLeRoux
Copy link
Contributor

Hi @ieugen,

While working on upgrading Apache Commons IO because of https://www.cve.org/CVERecord?id=CVE-2024-47554 (though we don't use org.apache.commons.io.input.XmlStreamReader so not a big deal) I was surprised to not find any Apache Commons IO Gradle dependency.

Then looking for "commons-io" I stumbled upon this PR. There is much conflicts and some recent ones. For instance dependencies are now in dependencies.gradle, and they have evolved with https://issues.apache.org/jira/browse/OFBIZ-13123.

Nevertheless, I have a question about Commons IO. I see that you used api 'commons-io:commons-io:2.15.1', why ?

Also are you still interested about pushing this PR (w/o conflicts of course)?

TIA

@ieugen
Copy link
Member Author

ieugen commented Oct 15, 2024

Hi @JacquesLeRoux ,

I was surprised to not find any Apache Commons IO Gradle dependency.

The dependencies are brought in by transitive dependencies, that is why they are not "visible".
OFBiz does use the classes directly, see

I see that you used api 'commons-io:commons-io:2.15.1', why ?

I think that was the release available at that time https://commons.apache.org/proper/commons-io/changes-report.html

I am open to fixing and merging this PR.
I did start a process to try and merge it piece by piece here: #837

It's more slow and forces us to discuss and tackle issues one by one.
There are 2 options:

While not as clean, merging this PR would push things forward faster :) .

@mbrohl
Copy link
Contributor

mbrohl commented Oct 15, 2024

It's more slow and forces us to discuss and tackle issues one by one. There are 2 options:

* fix this PR and merge it and then try to clean up the code later

* close this PR and continue with discussions on [Implemented: Introduced base/util module (OFBIZ-12308) #837](https://github.com/apache/ofbiz-framework/pull/837) and future PR's .

While not as clean, merging this PR would push things forward faster :) .

I recommend to go with option 2. There is no need to hurry.

@JacquesLeRoux
Copy link
Contributor

Thanks Eugen,

I agree with Michael, so close here.

I neglect the commons io update for now as there is no risk, trunk uses commons-io-2.16.1. Last one is commons-io-2.17.0 from 15 Sept. It's safe since 2.14.0;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants