ofbiz-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mathieu Lirzin <mathieu.lir...@nereide.fr>
Subject Re: Add ‘method’ attribute to ‘request-map’ elements
Date Sun, 08 Jul 2018 00:15:31 GMT
Taher Alkhateeb <slidingfilaments@gmail.com> writes:

> I appreciate your patience and help in easing my way to your code, and
> please excuse any dumb questions from my side :) it's a bug chunk of
> code indeed.

No problem. :-)

> Examining your patch I have a few comments / questions:
>
> - Your explanation of the second and third patch is very brief,
> perhaps some elaboration could help? Can you describe the architecture
> and design and what you're trying to do?

As you have pointed below the ‘RequestHandler’ is hard to understand
(and to change) and more particularly the infamous
‘RequestHandler::doRequest’ method.  The second patch is a minimal
refactoring that I have done to properly implement the ‘resolveURI’ and
‘resolveMethod’ in the third patch.  The idea is that instead of
dispatching the XML parsing of the ContollerConfig and handling
WebAppConfigurationException at multiple places it is more cleaner to do
it in one step to avoid the extra noise.

The third patch is about modifying the ‘doRequest’ method to handle
multiple methods.  However in the reducing the mess this has been done
in separated methods so that they can be relatively easily unit tested.
The architecture choice I have made is that the ‘resolveURI’ and
‘resolveMethod’ static methods are *pure* functions which helps to
reason about them.  As a consequence the error handling is kept at their
boundary (in ‘doRequest’).

> - The RequestHandler.java is really painful to look at. 1291 (jumping
> to 1355) lines of code, giving a sign of design issues. Maybe while
> working on implementing this feature we need to first refactor this
> file. It's HUGE and almost impossible to navigate. What do you think
> about refactoring it as a first step?

I deeply agree on the need of refactoring.  However given that these set
of patches is aleady participating in that effort, I would prefer doing
more refactoring only after settling on the discussion about adding
‘method’ attribute.

In fact there are already some pending refactoring patches in
OFBIZ-1045O, OFBIZ-10451, and OFBIZ-10452.

> - What is the purpose of the new Controller object?

It is an instantiation of the controller config, meaning a snapshot of
the controller XML config.  I initially gathered the parsing of all the
variables in a single try catch block without introducing a class.
However to emphasize that the variables are mainly read-only with the
except of ‘statusCodeString’ I have gathered them in a sort of immutable
class which allows to be explicit about the “read-onlyness”.
Additionally It was convenient for testing purpose too to reduce the
number of arguments to pass to ‘resolveURI’.

I am not entirely satisfied with the “Controller” name, feel free to
suggest better ones.

> - What is the purpose of adding the new data structures from
> javax.ws.rs? What features do we need that are not covered by the
> standard collection APIs?

I needed a MultiMap meaning a map which associates a key to a List of
values not to a scalar.  What is nice is that “MultivaluedMap::put” is
not destructive, it just appends the new value to the already associated
ones.

I could have worked around with a Map<String, List<String>> but for the
future step of implementing URI templates like ‘/foo/bar/{baz}’ I have
been using org.apache.cxf.jaxrs.model.URITemplate [1] which makes use of
javax.ws.rs.MultivaluedMap too.  So it was a good fit.

> - Can you elaborate on the connection between the architecture you're
> trying to apply with the resource example provided by Scott in [1].
> Specifically, do we intend to include resource definitions in the DSL?
> How will they relate to the controller as patched in your work?

In the work I am doing a resource is simply something that is identified
by a URI (Uniform Resource Identifier), so there is no direct link with
the “resource” example that Scott presented.  IIUC what Scott is
providing is a DSL for generating the representations of a resource and
the action that it supports.  This could potentially be plugged in the
‘request-map’ change I am proposing but I see this as a complementary if
not as an alternate work.

> - Can you provide a full explanation of the targeted architecture? It
> seems you're saying you have patches that you want to apply so that
> you can prepare other patches. What is the combined result of all of
> those? What exactly do you want to provide as the final architecture?
> Can you give us an idea of the full envisioned workflow from
> submitting a REST URL throughout the whole cycle?

Yes I have already been working on supporting URI templates.

I don't have much to say about the architecture.  Initially I thought I
would provide a REST controller which would be separate from the
“controller.xml” however having acknowledged that “controller.xml” is
already an HTTP request handler with both User and API handlers it fits
better to adapt/extend this controller to allow RESTful HTTP
interactions by following Richardson Maturity Model [2].  This
adaptation doesn't prevent to move things in a separate controller
later.  Basically the architecture I am seeking is to not commit to any
hard to change design decision in order to preserve my agility as long
as possible.  ;-)

> Thank you in advance for your feedback, and keep up the momentum, It's
> exciting stuff!
>
> [1] https://issues.apache.org/jira/browse/OFBIZ-4274

Thanks.

[1] https://cxf.apache.org/javadoc/latest/org/apache/cxf/jaxrs/model/URITemplate.html
[2] https://www.martinfowler.com/articles/richardsonMaturityModel.html

-- 
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

Mime
View raw message