+1
A much better explanation of my thoughts than I could have put into words.
Thanks,
Michael
Am 02.05.23 um 11:43 schrieb Daniel Watford:
Hi Nicholas,
I can see value in the approach I think you are proposing (please correct
me if I am wrong) which I would summarise as:
- All scripts are considered service implementations and will handle
parameters and return types accordingly, and
- GroovyEventHandler should invoke an event handler as if it was a service
implementation and convert the result (i.e. a Map) in a way
similar to that in the GroovyEventHandler code that you quoted.
GroovyEventHandler effectively becomes an adapter between service calling
conventions and event-handler calling conventions.
The problem I see with the above approach is that an event handler may need
to access the Request object, something that service implementations cannot
do. If we force all groovy event handlers to be implemented as service
implementations which are later 'adapted' to the event handler calling
conventions, then we are making it difficult for the developer to
understand what interface (calling conventions) they are working with.
Taking my previous suggestion further, we could implement logic around the
following test in GroovyEventHandler and GroovyEngine respectively:
script
.getClass().getDeclaredMethod(event.getInvoke()).getReturnType().isAssignableFrom(EventResponse.
class)
script
.getClass().getDeclaredMethod(modelService.getInvoke()).getReturnType().isAssignableFrom(ServiceResponse.
class)
If the methods to be called by GroovyEventHandler and GroovyEngine do not
declare the expected return type, then a warning can be logged.
For the simple 'adapter' cases, we could implement helper methods in
GroovyBaseScript to convert between EventResponse and ServiceResponse
objects, but for more complicated cases, the method author is free to
implement an adapter appropriate to their business rules.
A principle that drives the above, in my opinion at least, is that a method
author MUST know whether they are implementing a service or an event
handler since there may be differences in context that they depend on. By
incorporating the EventReponse and ServiceReponse types the author can make
it clear to the reader (and to OFBiz) which context the code was written
for.
Thanks,
Dan.
On Fri, 28 Apr 2023 at 16:53, Nicolas Malin
wrote:
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(...) {...}
