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 > > -- Daniel Watford
