Re: [OFBIZ-12801] "Error at CommunicationEventServices.groovy:489"
Hi,
My preference would be to the simplest for developer and keep the three
word (error, failure and success). For that, we can force the return of
each function as Map.
After delegate the problematic to each handler. By the way, the
GroovyEventHandler that call GroovyBaseScript already support an output
as Map or as String :
// GroovyEventHandler.java:117 [1]
// check the result
if (result instanceof Map) {
Map resultMap = UtilGenerics.cast(result);
String successMessage = (String)
resultMap.get("_event_message_");
if (successMessage != null) {
request.setAttribute("_EVENT_MESSAGE_",
successMessage);
}
String errorMessage = (String)
resultMap.get("_error_message_");
if (errorMessage != null) {
request.setAttribute("_ERROR_MESSAGE_", errorMessage);
}
return (String) resultMap.get("_response_code_");
}
if (result != null && !(result instanceof String)) {
throw new EventHandlerException("Event did not return a
String result, it returned a " + result.getClass().getName());
}
return (String) result;
If I understand well the problematic, move the return to Map, and
improve GroovyEventHandler to support the groovy script return (better
than what it did currently) would be cover all case ?
Cheers,
Nicolas
[1]
https://github.com/apache/ofbiz-framework/blob/64d012d2c20d76200cedd3e1861b720d55a61398/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/GroovyEventHandler.java#L117
On 28/04/2023 16:07, Daniel Watford wrote:
Hi Gil,
I don't think we need to go as far as creating a new GroovyBaseClass.
Deprecating GroovyBaseScript:success is still preferred in my opinion,
replacing it with serviceSuccess and scriptSuccess. These two methods could
return separate types which extend from Map and help make it clear whether
the method in the groovyScript is intended by the author to implement a
service or an event handler.
In the rare cases of a current groovy method being used to implement both a
service and an event handler, I think we could do something similar to this
contrived example:
/
// GroovyBaseScript.groovy
interface ServiceResponse extends Map
interface EventResponse extends Map
ServiceResponse serviceSuccess() { ... }
ServiceResponse serviceFail(...) {...}
ServiceResponse serviceError(...) {...}
EventResponse eventSuccess(...) {...}
EventResponse eventFail(...) {...}
EventResponse eventError(...) {...}
//
// ExampleServiceAndEventImpl.groovy
// Here is the main implementation, initially implemented as a service
handler
ServiceResponse countProducts () {
...
...
return serviceSuccess([stock: 42])
// Here is the event handler implementation, leveraging the service
implementation as an adapter.
EventResponse countProductsEventHandler () {
ServiceResponse sr = countProducts();
return eventSuccess("Found ${sr.stock} products");
}
The return type of the above methods help identify whether we are dealing
with a service or event handler. If conversion is needed from one type to
the other, an adapter specific to the business logic can decide how to map
between EventResponses and ServiceResponses.
Thanks,
Dan.
On Fri, 28 Apr 2023 at 14:27, Gil Portenseigne
wrote:
Hello I got a quick look about it, having two separate class means
having two distinct groovy DSL and need lot of modification that IMO are
not worth the complexity for this subject.
I now only wonder, is it that bad too keep that one exception (about
untyped return) for GroovyBaseScript::success ?
Gil
Le 26/04/2023 à 09:49, Gil Portenseigne a écrit :
Hello,
I like the idea that the developer do not have to sync about which
method to use.
If I understand well what Michael envision, i.e. to use for event a
new GroovyBaseEvent class, and for services/scripts a GroovyBaseScript
class, that both extends a common class for the common code, should be
one way to allow this usage.
But I don't know about IDE integration behavior of such a solution...
Do you think that is worth a look ?
I will just add that there is a chance that project implementation are
using groovy script as the event target (I know some ;) )
Thanks,
Gil
Le 20/04/2023 à 17:13, Michael Brohl a écrit :
To have it even more clear, I would separate logic for events and
services.
The GroovyBaseScript in the service engine package should only be
used for services and there should be another one for events, if
really needed. Mixing both together is bad practice IMO. There seem
to be only 7 controller entries using a groovy script as the event
target.
Best regards,
Michael Brohl
ecomify GmbH -www.ecomify.de
Am 20.04.23 um 16:49 schrieb Jacques Le Roux:
Hi Daniel,
I dont think there is a knowledge about methods being both services
Re: OFBiz 22.01 - Eclipse - Issues on setting up a debugging environment.
Hi everyone, I have created OFBIZ-12813 to coordinate the refactoring work. Best regards, Wiebke Am 27.04.23 um 19:18 schrieb Jacques Le Roux: Hi Michael, Actually, as you may have seen with recent Daniel's work, lazy consensus is de facto if nobody oppose :) Cheers Jacques Le 27/04/2023 à 18:49, Michael Brohl a écrit : Hi everyone, what do you think about this refactoring suggestion? We would organize to do this work but won't start until the community decides to do so. It will be some amount of work so it should definetely be backed by the community. Can we apply lazy consensus here or do we need some kind of voting? Best regards, Michael Brohl ecomify GmbH - www.ecomify.de Am 26.04.23 um 13:21 schrieb Michael Brohl: Hi, I suggest to start with a new ticket to coordinate the refactoring work (will you take this into your hands, Wiebke?). OFBIZ-10226 has another intention which will not solve the overall problem Wiebke described. Does the community agree that we'll have to do this work? Even more, do we agree that it has to be done before we can ship a first 22.01 release based on JDK 17? Best regards, Michael Brohl ecomify GmbH - www.ecomify.de Am 25.04.23 um 18:30 schrieb Jacques Le Roux: Thanks Wiebke, I know that for a while (https://s.apache.org/kewrn) but was desperately trying to avoid what you propose. It's indeed the right solution. So I think we can go on with OFBIZ-10226. At the bottom there is a link to a related discussion with some opinions from then. Or do you prefer to start anew for the sake of clarity? Thanks again for your work, I was not aware of this Groovy page. It definitely confirms what Mathieu said then. Jacques Le 25/04/2023 à 16:09, Wiebke Pätzold a écrit : Hi everyone, I did a bit of research regarding the groovy debugging. After some research I found this: “The Java Platform Module System requires that classes in distinct modules have distinct package names. Groovy has its own "modules" but these haven’t historically been structured according to the above requirement. For this reason, Groovy 2.x and 3.0 should be added to the classpath not module path when using JDK9+. This places Groovy’s classes into the unnamed module where the split package naming requirement is not enforced.“ http://groovy-lang.org/releasenotes/groovy-3.0.html#Groovy3.0releasenotes-Splitpackages For testing I used the service "sendEmailDated" in CommunicationEventServices.groovy. I can confirm the described behavior of Jacques, without a package declaration the service does not stop at my breakpoint. If I add the package declaration for the class, the service stops at my breakpoint. From my point of view it would make sense for the project not only to add the package declaration in all groovy classes, but also to ensure a consistent folder structure. For example, under framework -> base -> src there is a distinction between main and test. Within the test folder there is again a distinction between groovy and Java. Therefore I would suggest to introduce this structure everywhere, which means that there would be a src folder which in turn contains main, test, ... within these folders there is again a distinction between groovy and java. Example: applications -> product -> src -> main -> groovy -> org -> apache -> ofbiz ->... -> java -> org -> apache -> ofbiz ->... -> test -> groovy -> org -> apache -> ofbiz ->... -> java -> org -> apache -> ofbiz ->... What do you think about this idea? Best regards, Wiebke ecomify GmbH - www.ecomify.de Am 20.04.23 um 11:46 schrieb Michael Brohl: We have a working solution with all tests passing for release22.01 and trunk, I have created a Jira issue to track the effort. https://issues.apache.org/jira/browse/OFBIZ-12808 Best regards, Michael Brohl ecomify GmbH - www.ecomify.de Am 19.04.23 um 15:52 schrieb Michael Brohl: Hi everyone, it seems that we have a solution for the Eclipse Java build and runtime problems we faced with JDK 17 (not speaking of the Groovy build problems). We removed some (transitive) dependencies in the build file and updated some of them so that libraries are used from the JDK instead of external libaries. This avoids the compiler from finding duplicate classes which seem to be ignored and therefore not being found also at runtime. We are checking the changes with a project now and provide a pull request when we are sure everything works fine. Best regards, Michael Brohl ecomify GmbH - www.ecomify.de Am 14.04.23 um 21:53 schrieb Michael Brohl: Thanks Jacques! for me, org.eclipse.jdt.core.compiler.ignoreUnnamedModuleForSplitPackage only works as far as the build problems get away but there are still packages and classes not found by Eclipse at runtime so not really working for debugging here. Additionally, I realized that the settings file is (re)generated by
[GitHub] [ofbiz-tools] danwatford merged pull request #8: Added: RAT exclusions for buildSrc output and npm package.json files (OFBIZ-12789)
danwatford merged PR #8: URL: https://github.com/apache/ofbiz-tools/pull/8 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@ofbiz.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: OFBiz 22.01 - Eclipse - Issues on setting up a debugging environment.
Hello Wiebke, Please could you confirm that this work only relates to groovy code that is expected to be compiled to class files and will not alter the structure of the various groovyScripts directories in OFBiz components? The reason for checking is that groovyScripts are loaded as independent scripts and compiled at runtime by OFBiz (See GroovyUtil#getScriptClassFromLocation), rather than being loaded from a pre-compiled JAR. OFBIZ-12149 (https://issues.apache.org/jira/browse/OFBIZ-12149) made changes to the build to have those script files included from source set but excluded from compilation. If those groovy script files are moved to sit alongside groovy classes then we may need to rework OFBIZ-12149. Thanks, Dan. On Fri, 28 Apr 2023 at 10:09, Wiebke Pätzold wrote: > Hi everyone, > > I have created OFBIZ-12813 to coordinate the refactoring work. > > Best regards, > > Wiebke > > > Am 27.04.23 um 19:18 schrieb Jacques Le Roux: > > Hi Michael, > > > > Actually, as you may have seen with recent Daniel's work, lazy > > consensus is de facto if nobody oppose :) > > > > Cheers > > > > Jacques > > > > Le 27/04/2023 à 18:49, Michael Brohl a écrit : > >> Hi everyone, > >> > >> what do you think about this refactoring suggestion? > >> > >> We would organize to do this work but won't start until the community > >> decides to do so. It will be some amount of work so it should > >> definetely be backed by the community. > >> > >> Can we apply lazy consensus here or do we need some kind of voting? > >> > >> Best regards, > >> > >> Michael Brohl > >> > >> ecomify GmbH - www.ecomify.de > >> > >> > >> Am 26.04.23 um 13:21 schrieb Michael Brohl: > >>> Hi, > >>> > >>> I suggest to start with a new ticket to coordinate the refactoring > >>> work (will you take this into your hands, Wiebke?). > >>> > >>> OFBIZ-10226 has another intention which will not solve the overall > >>> problem Wiebke described. > >>> > >>> Does the community agree that we'll have to do this work? > >>> > >>> Even more, do we agree that it has to be done before we can ship a > >>> first 22.01 release based on JDK 17? > >>> > >>> Best regards, > >>> > >>> Michael Brohl > >>> > >>> ecomify GmbH - www.ecomify.de > >>> > >>> > >>> Am 25.04.23 um 18:30 schrieb Jacques Le Roux: > Thanks Wiebke, > > I know that for a while (https://s.apache.org/kewrn) but was > desperately trying to avoid what you propose. It's indeed the right > solution. > > So I think we can go on with OFBIZ-10226. At the bottom there is a > link to a related discussion with some opinions from then. Or do > you prefer to start anew for the sake of clarity? > > Thanks again for your work, I was not aware of this Groovy page. It > definitely confirms what Mathieu said then. > > Jacques > > Le 25/04/2023 à 16:09, Wiebke Pätzold a écrit : > > Hi everyone, > > > > I did a bit of research regarding the groovy debugging. > > After some research I found this: > > > > “The Java Platform Module System requires that classes in distinct > > modules have distinct package names. Groovy has its own "modules" > > but these haven’t historically been structured according to the > > above requirement. For this reason, Groovy 2.x and 3.0 should be > > added to the classpath not module path when using JDK9+. This > > places Groovy’s classes into the unnamed module where the split > > package naming requirement is not enforced.“ > > > http://groovy-lang.org/releasenotes/groovy-3.0.html#Groovy3.0releasenotes-Splitpackages > > > > > > For testing I used the service "sendEmailDated" in > > CommunicationEventServices.groovy. I can confirm the described > > behavior of Jacques, without a package declaration the service > > does not stop at my breakpoint. If I add the package declaration > > for the class, the service stops at my breakpoint. > > > > From my point of view it would make sense for the project not only > > to add the package declaration in all groovy classes, but also to > > ensure a consistent folder structure. > > > > For example, under framework -> base -> src there is a distinction > > between main and test. Within the test folder there is again a > > distinction between groovy and Java. > > > > Therefore I would suggest to introduce this structure everywhere, > > which means that there would be a src folder which in turn > > contains main, test, ... within these folders there is again a > > distinction between groovy and java. > > > > Example: > > applications -> product -> src -> main -> groovy -> org -> apache > > -> ofbiz ->... > > -> java -> org -> apache -> ofbiz ->... > > -> test -> > > groovy -> org -> apache -> ofbiz ->... > > -> java -> org -> apache -> ofbiz ->... > > > >
Re: [OFBIZ-12801] "Error at CommunicationEventServices.groovy:489"
Hello I got a quick look about it, having two separate class means having two distinct groovy DSL and need lot of modification that IMO are not worth the complexity for this subject. I now only wonder, is it that bad too keep that one exception (about untyped return) for GroovyBaseScript::success ? Gil Le 26/04/2023 à 09:49, Gil Portenseigne a écrit : Hello, I like the idea that the developer do not have to sync about which method to use. If I understand well what Michael envision, i.e. to use for event a new GroovyBaseEvent class, and for services/scripts a GroovyBaseScript class, that both extends a common class for the common code, should be one way to allow this usage. But I don't know about IDE integration behavior of such a solution... Do you think that is worth a look ? I will just add that there is a chance that project implementation are using groovy script as the event target (I know some ;) ) Thanks, Gil Le 20/04/2023 à 17:13, Michael Brohl a écrit : To have it even more clear, I would separate logic for events and services. The GroovyBaseScript in the service engine package should only be used for services and there should be another one for events, if really needed. Mixing both together is bad practice IMO. There seem to be only 7 controller entries using a groovy script as the event target. Best regards, Michael Brohl ecomify GmbH - www.ecomify.de Am 20.04.23 um 16:49 schrieb Jacques Le Roux: Hi Daniel, I dont think there is a knowledge about methods being both services and events. I think there are not (much?) such cases. Being acquainted to OFBiz logs I did not check the trunk demo log content (now in Docker); so I wonder if there are such other cases than CommunicationEventServices::sendEmail (colon notation is available in Groovy 3) that bots and demo uses could have generated. I tend to agree about having GroovyBaseScript::success deprecated and replaced with methods GroovyBaseScript::scriptSuccess GroovyBaseScript::serviceSuccess and GroovyCaseScript::eventSuccess I'm not yet acquainted with Codernarc rules, but the changes in GroovyBaseScript seem straightforward. And (hopefully) this should not be a big deal to change accordingly in scripts methods with the help of Codenarc, right ? My 2 cts Jacques Le 19/04/2023 à 18:37, Daniel Watford a écrit : Hello, In my opinion, the semantics of calling an event handler vs a service implementation are different, albeit similar enough that most handler/implementation authors wouldn't necessarily care how the code was called. The untyped nature of Groovy had allowed a certain degree of flexibility in code that GroovyBaseScript#success could be relied upon to prepare a response appropriate to the calling conventions of an event handler or service implementation. However over the last decade, possibly driven by increased use of linters/static analysers, we have seen a push back towards explicit typing, particularly on public methods. If we continue to adopt the guidance from static analysers and apply explicit typing to public methods in our groovy code, then we need to avoid the black box approach of GroovyBaseScript#success figuring out what calling conventions (i.e. event or service) are in play and, instead, a groovy method should be intentionally written as either a service or event handler. If we have cases where a groovy method is used to provide implementations for both a service and an event handler, then we can employ a thin adapter layer to convert the result type between the two calling conventions. Do we know if many such cases currently exist in OFBiz? My preference would be to see GroovyBaseScript#success deprecated and replaced with methods along the lines of GroovyBaseScript#scriptSuccess and GroovyCaseScript#eventSuccess that return a Map and String respectively. Thanks, Dan. On Wed, 19 Apr 2023 at 16:44, Jacques Le Roux wrote: Hi All, At OFBIZ-12801, we had a discussion with Daniel and Gil about the described issue (last comments there) We are unsure of the best solution, so this thread to discuss and decide. As Gil reported, Jacopo's comment of the related commit* contains <> What would be your opinion about a best solution? TIA Jacques *http://svn.apache.org/viewvc?view=revision=1298908
Re: [OFBIZ-12801] "Error at CommunicationEventServices.groovy:489"
Hi Gil,
I don't think we need to go as far as creating a new GroovyBaseClass.
Deprecating GroovyBaseScript:success is still preferred in my opinion,
replacing it with serviceSuccess and scriptSuccess. These two methods could
return separate types which extend from Map and help make it clear whether
the method in the groovyScript is intended by the author to implement a
service or an event handler.
In the rare cases of a current groovy method being used to implement both a
service and an event handler, I think we could do something similar to this
contrived example:
/
// GroovyBaseScript.groovy
interface ServiceResponse extends Map
interface EventResponse extends Map
ServiceResponse serviceSuccess() { ... }
ServiceResponse serviceFail(...) {...}
ServiceResponse serviceError(...) {...}
EventResponse eventSuccess(...) {...}
EventResponse eventFail(...) {...}
EventResponse eventError(...) {...}
//
// ExampleServiceAndEventImpl.groovy
// Here is the main implementation, initially implemented as a service
handler
ServiceResponse countProducts () {
...
...
return serviceSuccess([stock: 42])
// Here is the event handler implementation, leveraging the service
implementation as an adapter.
EventResponse countProductsEventHandler () {
ServiceResponse sr = countProducts();
return eventSuccess("Found ${sr.stock} products");
}
The return type of the above methods help identify whether we are dealing
with a service or event handler. If conversion is needed from one type to
the other, an adapter specific to the business logic can decide how to map
between EventResponses and ServiceResponses.
Thanks,
Dan.
On Fri, 28 Apr 2023 at 14:27, Gil Portenseigne
wrote:
> Hello I got a quick look about it, having two separate class means
> having two distinct groovy DSL and need lot of modification that IMO are
> not worth the complexity for this subject.
>
> I now only wonder, is it that bad too keep that one exception (about
> untyped return) for GroovyBaseScript::success ?
>
> Gil
>
> Le 26/04/2023 à 09:49, Gil Portenseigne a écrit :
> > Hello,
> >
> > I like the idea that the developer do not have to sync about which
> > method to use.
> >
> > If I understand well what Michael envision, i.e. to use for event a
> > new GroovyBaseEvent class, and for services/scripts a GroovyBaseScript
> > class, that both extends a common class for the common code, should be
> > one way to allow this usage.
> >
> > But I don't know about IDE integration behavior of such a solution...
> >
> > Do you think that is worth a look ?
> >
> > I will just add that there is a chance that project implementation are
> > using groovy script as the event target (I know some ;) )
> >
> > Thanks,
> >
> > Gil
> >
> > Le 20/04/2023 à 17:13, Michael Brohl a écrit :
> >> To have it even more clear, I would separate logic for events and
> >> services.
> >>
> >> The GroovyBaseScript in the service engine package should only be
> >> used for services and there should be another one for events, if
> >> really needed. Mixing both together is bad practice IMO. There seem
> >> to be only 7 controller entries using a groovy script as the event
> >> target.
> >>
> >> Best regards,
> >>
> >> Michael Brohl
> >>
> >> ecomify GmbH - www.ecomify.de
> >>
> >>
> >> Am 20.04.23 um 16:49 schrieb Jacques Le Roux:
> >>> Hi Daniel,
> >>>
> >>> I dont think there is a knowledge about methods being both services
> >>> and events. I think there are not (much?) such cases.
> >>> Being acquainted to OFBiz logs I did not check the trunk demo log
> >>> content (now in Docker);
> >>> so I wonder if there are such other cases than
> >>> CommunicationEventServices::sendEmail (colon notation is available
> >>> in Groovy 3)
> >>> that bots and demo uses could have generated.
> >>>
> >>> I tend to agree about having GroovyBaseScript::success deprecated
> >>> and replaced with methods GroovyBaseScript::scriptSuccess
> >>> GroovyBaseScript::serviceSuccess and GroovyCaseScript::eventSuccess
> >>>
> >>> I'm not yet acquainted with Codernarc rules, but the changes in
> >>> GroovyBaseScript seem straightforward.
> >>> And (hopefully) this should not be a big deal to change accordingly
> >>> in scripts methods with the help of Codenarc, right ?
> >>>
> >>> My 2 cts
> >>>
> >>> Jacques
> >>>
> >>> Le 19/04/2023 à 18:37, Daniel Watford a écrit :
> Hello,
>
> In my opinion, the semantics of calling an event handler vs a service
> implementation are different, albeit similar enough that most
> handler/implementation authors wouldn't necessarily care how the
> code was
> called.
>
> The untyped nature of Groovy had allowed a certain degree of
> flexibility in
> code that GroovyBaseScript#success could be relied upon to prepare a
> response appropriate to the calling conventions of an event handler or
> service implementation. However over the last decade, possibly
> driven
