Hello Daniel,
Thank you for taking a look at the PR.
I'vea read your eamil and I believe we are aligned on the direction we
are going.
** A solution
What we can do now that we know the/a destination is to create the empty
gradle projects in OFbiz and slowly move in the classes from my PR to
those projects, in smaller batches.
I don't know how well this will work (see bellow for why) but it's worth
a shot if you don't want to merge a big PR.
What do you think about this approach?
** More rambling/justification :)
I will try to answer some of the concerns you expressed in the email.
There are a large number or touched files, but there are minimal / no
behavior changes.
All I did was to move classes to another source root.
If you check the changed files you will probably find very few package
renames / changes (less than 10, maybe 5 such changes).
Those where needed to break cyclic dependency (introduce the layered
approach you mentioned in your email).
The code foramtting was required because some classes I introduced (e.g
UtilPropertiesRuntime) made the lines too big and checkstyle complained.
After my experience, the uitl, crypto and datafile parts are the crack
where we need to work to open OFBiz up.
If you work on remaking the PR, you might reach the conclusion that you
can't split just one of them.
There are dependencies between util and crypto so you need to extract
both to have something working.
You might make the PR simpler and leave datafile out, but for me
datafile was the thread I pulled on to unreavel the other 2 libraries.
It was the goal of that motivated my work: to be able to import / export
data in OFbiz from CLI to DB or via a web service and do some processing
as well.
You can try to make smaller changes, but it is hard without a goal to
guide you.
For me the goal was datafile and I have entity-engine next since this
will allow me to interact with OFbiz and build import/export and
processing tooling.
By exploring how to extract entity-engine you will see all of the
dependencies that break the API boundaries: Cache, config, Script
engine, entity-engine.
Those will definitly require more collaboration and buy-in from OFbiz
contributors since they will require behavior changes.
For those I would definitly use the "add, deprecate old, then remove"
approach.
Looking forward for more feedback.
I am open to participate in calls and work to get things moving.
Eugen
La 27.02.2024 12:45, Daniel Watford a scris:
Hello all,
I haven't been able to fully review the PR yet - it is quite long! :) I
feel I have read enough of it, along with Eugen's comments in this thread,
to appreciate its aims. That doesn't mean I'm ready to merge it.....yet!
I'm broadly supportive of efforts to remove cyclic dependencies and to
identify project components that could be treated as standalone libraries.
In the long run I think this could help simplify project code by further
defining 'layers' of software such as (ordered lower- to higher-level):
- 1. Standalone functional (e.g. cryptography functions, utilities)
- 2. Technology services (e.g. persistence/entity, properties, service
despatch, caching, content, user, authentication/authorisation, ui/widget
rendering, multi-server clustering, REST API, logging)
- 3. Technology applications (e.g. webtools, backup/restore, cache control,
user management, cluster management, metrics, log management)
- 4. Business services (e.g. Parties, Orders, GL Transactions)
- 5. Business applications (e.g Party Manager, Order Manager)
- 6. Plugins
-- Plugins which add new behaviour (e.g. webpos)
-- Plugins which override services and/or screen and form rendering (e.g.
BIRT reporting)
Software in one layer may only reference software in the same or a lower
layer.
From previous jira tickets and wiki pages (
https://cwiki.apache.org/confluence/display/OFBIZ/Component+and+Component+Set+Dependencies)
I believe the project is also broadly in support of these aims. At least, I
haven't noticed any comments suggesting otherwise. (There are comments
reminding us to be mindful of the impact to existing
customisations/integrations which I have tried to address below - re
deprecation)
We can already point to one beneficiary if we were to extract the
cryptography functions into a standalone library. In docker-entrypoint.sh,
the code at the following link is used to encrypt the admin user's password
as part of spinning up an OFBiz container instance. If we change how user
passwords are persisted in OFBiz, then we will likely need to rework
docker-entrypoint.sh to reflect those changes. Being able to utilise an
ofbiz-crypto library to encrypt the password, or even a slightly higher
level ofbiz-user library in concert with an ofbiz-entity library to prepare
and persist the admin user's data, could help keep docker-entrypoint.sh in
sync with the ofbiz-framework.
https://github.com/apache/ofbiz-framework/blob/6ad1c20518ad75773fca02ceac3c1d116f097f77/docker/docker-entrypoint.sh#L228C4-L241C62
I'm also broadly supportive of Eugens suggestion to only apply
autodiscovery of components to Plugins (layer 6). This would remove a level
of indirection that developers need to follow when understanding how OFBiz
is loaded at runtime. I think it might also benefit tools in navigating
project structure.
Although I think most developers would be happy to see some refinements of
ofbiz's layered architecture, we must still consider the impact on those
existing ofbiz deployments, providing them an upgrade path to newer major
releases. Since we have no sight of those ofbiz-deployments and cannot
easily quantify the impact of any changes, I think we need to take the
approach of deprecating, rather than breaking, APIs.
That being said, I don't think the project should be shackled to the
long-term maintenance of APIs that do not serve our development goals. I
therefore think an aggressive deprecation policy is acceptable where we
require an API item to be marked as deprecated for the entire life of only
a single release before it is removed in a subsequent release. I am happy
to consider even more aggressive policies where an API item could be
deprecated at any point during a major release's life and then removed in
the next, but that might have the consequence of deprecating something in a
release such as 18.12.13 and then removing it is the (theoretical) 24.4
release.
Since we do not yet use Java Modules in ofbiz-framework, essentially every
class is a potential API item. I think we can take a pragmatic view here
and determine whether any given class should be considered part of an API
or an internal implementation detail. Internal implementation details can
be changed/removed without regard to deprecation policy. Other API items
include service definitions and widget screens/forms/grids since these can
all be referred to by other components. This may mean we need to extend
service and widget definitions to incorporate a deprecation flag which is
logged when the API item is used.
A justification for the above deprecation policy proposal is that we have
limited resources available to work on the project and maintaining APIs not
helpful for our development goals is a significant drain on those resources.
How does the above affect Eugen's PR?
I think we should follow the spirit of the PR's aims, but not necessarily
the implementation as is currently committed.
By this I mean we should absolutely look to create base
components/libraries (layers 1 and 2) on which the rest of the ofbiz
framework and applications can depend. We should also do whatever we can to
remove cyclic dependencies.
But these things must be done in compliance with a deprecation policy. (I
appreciate that we haven't formally defined a deprecation policy -
apologies if there is one that I have missed)
Therefore, if Eugen is willing, I ask that the PR is reimplemented such
that no classes which might be considered an API are moved or have their
method signatures altered. Instead, those API classes and/or the methods
they contain should be marked as deprecated. The deprecated classes can
then delegate to the 'real' implementation which has been moved to a
package/component/library at an appropriate layer.
The above restriction should allow us to refactor implementation code to
untangle the cyclic dependencies without breaking existing customisations
and integrations.
Some other requests for PR authors when undertaking significant code
refactoring:
- Turn off any code formatting your tooling may automatically apply. In
the PR there were several instances of formatting code changes that did not
affect the code's behaviour or structure. The formatting changes still
needed to be reviewed and added to the cognitive load of reviers.
- Keep PRs as small as possible whilst still bringing about a worthwhile
change. I think this will be easier to achieve if following a deprecation
policy since the need to amend every single calling point for a refactored
piece of code is avoided. Small refactoring PRs with no formatting changes
will be easier to review and are more likely to get merged before going
stale.
- Large refactoring efforts should be broken into smaller tasks that are
considered part of a larger restructuring programme. Such programmes should
have a Wiki page to coordinate efforts and gather ideas of what changes are
considered worthwhile. These programmes are long running and need some
top-level visibility to track movement, deprecation and removal of code.
If we want to restructure, when should we start?
As soon as possible, and preferably before we prepare a new release branch.
We run the risk that we keep putting off cutting our new release branch,
but if we refactor a significant portion of the code base after starting
the new release branch, porting backfixes between the release branch and
trunk will be more difficult.
This is another reason why we need the top-level wiki page to coordinate
refactoring efforts since that will help inform what refactoring tasks we
might want to see completed before creating the new release branch.
I have other refactoring topics I would like to discuss at some point -
such as considering whether we should bring plugins maintained by the
project into the ofbiz-framework repository, or whether we should set
cookies at site root to authenticate a user across all applications at once
- but the above seems enough to grapple with for the time being.
Thanks,
Dan.
On Sat, 17 Feb 2024 at 23:15, Eugen Stan <eugen.s...@netdava.com> wrote:
Hi,
Did anyone have time to check out the PR?
I did rebase it on current trunk.
https://github.com/apache/ofbiz-framework/pull/678
Thanks,
Eugen
La 02.01.2024 14:50, Eugen Stan a scris:
Hello,
Happy new year!
I hope you had a great 2023.
Has anyone had time to look over the exploratory work?
If you have, please don't be shy, share your findings, good or bad.
It would be great if we can work on concrete examples.
I am still available to answer any questions and improve on the PR's to
make the changes part of OFBiz moving forward.
Let's make parts of OFBiz available as libraries so they can be used in
other projects (ofbiz tooling) / other ways of building OFBiz.
Regards,
Eugen
La 09.12.2023 01:04, Eugen Stan a scris:
Hi,
I pushed some changes that significantly reduce the number of lines
changed and also the possibility of breakage:
- merged back UtilValidateDateTime and UtilValidateEmpty into
UtilValidate
- moved some methods from *Runtime -> former class.
If you test the changes and find some issues please let me know.
I am willing to work to fix those and make the PR mergeable.
I also did an exploratory work to see what would it take to have
entity engine as a library see
https://github.com/ieugen/ofbiz-framework/pull/3
On top of this PR, it adds only 670 lines and remove 370.
Very small price to pay IMO.
I have partial integration tests running.
I have added some comments related to the blockers (EntityEca, Tennant)
They should be doable, but I need more info / help.
To reiterate, please let me know of any issue you find and I will work
to fix it.
Regards,
Eugen
La 07.12.2023 20:55, Eugen Stan a scris:
Hi Michael,
Thank you for taking a look.
I will reply inline.
I did a demo project to support my case
https://github.com/ieugen/ofbiz-tooling-demo .
La 07.12.2023 12:53, Michael Brohl a scris:
Hi Eugen,
can you give us some explanations/examples why this is a useful
change?
What are the downsides?
I don't see any downsides.
There might be some breaking changes on upgrades.
We could work to mitigate them - will explain bellow.
I see a lot of changes in the base API like UtilValidate,
UtilProperties etc. which will brake almost every custom project out
there.
Have you tried it in your projects?
Do you know how many fail?
All the integrations tests pass.
Also we could do some work to mitigate those braking changes:
- copy those methods back to the classes they came from
- implement delegation: add a method with same signature that calls
on the class from base/util (UtilValidate calls UtilValidateEmpty).
IMO this should be a transition period until the code is cleaned up.
I did not do the mitigations because this is intendend as a POC to be
prepared once we decide it's a good direction.
Also - I don't have access to 'all the projects out there' to test.
The changes also need explanation. For example: why the split of
UtilValidate to UtilValidateEmpty etc.
Because of the cyclic dependency between classes.
There is a LOT of cyclic dependencies which IMO is a code smell.
UtilValidate depens on UtilProperties which depens on UtilValidate.
There are numerous cases for this.
If you run Intellij IDEA Analyze -> Analyze Cyclic dependencies you
will see them.
( I posted an image on gh issue)
https://github.com/apache/ofbiz-framework/pull/678#issuecomment-1845903198
I think we need to have very strong advantages to make those changes
and currently I do not see them.
We open OFBiz to be used as libraries.
Then developers can use things like entity-engine, datafile, bits to
build tooling as CLI or services that integrate with OFBiz.
- It opens a path forward to inovation
- It will make life of integrators MUCH more easy as it will provide
java API's for them to do integrations
- It exposes the dependencies by making them explicit (as gradle deps)
- Developers can focus on a smaller part of the code base: component
loading, XML parsing, entity engine without worrying about the rest
of the code.
- It will allow users to build tools and ofbiz components and package
them as jar files (component/datafile is a component as a jar file -
it depends on base/util and base/crypto - but nothing else from OFBiz).
- It will facilitate the developement of proper java API's - now
OFBiz is an implementation only - the API is the implementation.
I would like to build some import tooling that uses the OFBiz code
(so I don't reinvent the weel).
Currently I don't see a way on how to do that.
See the https://github.com/ieugen/ofbiz-tooling-demo I just did for
Crypto. It can help people migrate data from/to OFBiz by using the
same crypto code and the same xml data processing code.
I do not need to bring whole OFBiz if all I want is to push / pull
data from DB or to some files.
To my knowledge that is not possible with OFBiz right now (unlesss
you reimplement the code). It will be possible once we publish
libraries.
There is much more that this can help with but we will see if OFBiz
to accepts some changes.
And again, my changes don't break ANY integration test from the 780?!
tests.
I am very excited of finding this way of breaking OFBiz into smaller
parts without breaking OFBiz runtime.
There will be work to be done to streamline the code as libs but I am
optimistic.
Regards,
--
Eugen Stan
+40770 941 271 / https://www.netdava.com
--
Eugen Stan
+40770 941 271 / https://www.netdava.com
