Add ‘method’ attribute to ‘request-map’ elements

[Mathieu Lirzin]

Hello,

Following revert of rev 1834917, I want to “properly” propose adding a
‘method’ attribute to ‘request-map’ elements.

I am currently working on adding REST based Web APIs to OFBiz [1].  In
order to do that, it is important for the HTTP controller to handle
various HTTP methods differently, meaning allowing a different request
handler to be called depending on the method of the HTTP request.
Syntactically I am proposing the following:

   <request-map uri="examples" method="get">
      <security https="true" auth="true"/>
      <event type="java" path="ExamplesHandlers" invoke="getExamples"/>
      <response name="success" type="view" value="..."/>
      <response name="error" type="view" value="..."/>
   </request-map>

   <request-map uri="examples" method="post">
      <security https="true" auth="true"/>
      <event type="java" path="ExamplesHandlers" invoke="postExamples"/>
      <response name="success" type="view" value="..."/>
      <response name="error" type="view" value="..."/>
   </request-map>

Here ‘method’ is an optional attribute which defaults to "all" and/or
"".  Currently the patch supports both but I would be in favour of
handling only one for the sake of simplicity.  The semantic of that
default is that it mimics the current behavior, meaning it handles every
methods.

Attached you will find a serie of 3 patches (meant to be applied in
order) implementing the described change:

- The first one handles the parsing of the XML (the change in the xsd
  file has not been reverted).

- The second is a simple refactoring to agglomerate the read of the XML
  file in one place.  This is done to ease the implementation and tests
  in the third patch.

- The third patch implements the segregation of ‘request-map’ elements
  based on the actual request method.  In order to associate a request
  to an event handler we now need to dispatch both on the URI and on the
  method.  To achieve that, I have decided to keep using the URI as a
  key but associate it to a List of ‘request-map’ (one for each method)
  instead of a single one.  In term of implementation a ‘MultivaluedMap’
  has been used.  The list of candidates for a particular URI is then
  filtered to keep only the best matching one.  The ‘request-map’ Map
  concrete type was a MapContext which is a stack of Maps (to handle
  includes), The ‘MultivaluedMapContext’ implementation is an adaptation
  of MapContext to MultivaluedMaps.  Since
  ‘ControllerConfig::getRequestMapMap’ is used at multiple places I have
  kept it temporarily and replaced its code with an adapter to the new
  ‘ControllerConfig::getRequestMapMultiMap’ method.



Comments, questions and/or reviews welcome.

[1] https://issues.apache.org/jira/browse/OFBIZ-4274

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

[Jacques Le Roux]

Thanks Mathieu,

FYI: You were lucky that your attached patches passed, it's not usually.

In general better to reference patches in some place. An existing Jira, like OFBIZ-4274 or OFBIZ-10438 here, fits here

Jacques


Le 03/07/2018 à 18:05, Mathieu Lirzin a écrit :

> Hello,
>
> Following revert of rev 1834917, I want to “properly” propose adding a
> ‘method’ attribute to ‘request-map’ elements.
>
> I am currently working on adding REST based Web APIs to OFBiz [1].  In
> order to do that, it is important for the HTTP controller to handle
> various HTTP methods differently, meaning allowing a different request
> handler to be called depending on the method of the HTTP request.
> Syntactically I am proposing the following:
>
>     <request-map uri="examples" method="get">
>        <security https="true" auth="true"/>
>        <event type="java" path="ExamplesHandlers" invoke="getExamples"/>
>        <response name="success" type="view" value="..."/>
>        <response name="error" type="view" value="..."/>
>     </request-map>
>
>     <request-map uri="examples" method="post">
>        <security https="true" auth="true"/>
>        <event type="java" path="ExamplesHandlers" invoke="postExamples"/>
>        <response name="success" type="view" value="..."/>
>        <response name="error" type="view" value="..."/>
>     </request-map>
>
> Here ‘method’ is an optional attribute which defaults to "all" and/or
> "".  Currently the patch supports both but I would be in favour of
> handling only one for the sake of simplicity.  The semantic of that
> default is that it mimics the current behavior, meaning it handles every
> methods.
>
> Attached you will find a serie of 3 patches (meant to be applied in
> order) implementing the described change:
>
> - The first one handles the parsing of the XML (the change in the xsd
>    file has not been reverted).
>
> - The second is a simple refactoring to agglomerate the read of the XML
>    file in one place.  This is done to ease the implementation and tests
>    in the third patch.
>
> - The third patch implements the segregation of ‘request-map’ elements
>    based on the actual request method.  In order to associate a request
>    to an event handler we now need to dispatch both on the URI and on the
>    method.  To achieve that, I have decided to keep using the URI as a
>    key but associate it to a List of ‘request-map’ (one for each method)
>    instead of a single one.  In term of implementation a ‘MultivaluedMap’
>    has been used.  The list of candidates for a particular URI is then
>    filtered to keep only the best matching one.  The ‘request-map’ Map
>    concrete type was a MapContext which is a stack of Maps (to handle
>    includes), The ‘MultivaluedMapContext’ implementation is an adaptation
>    of MapContext to MultivaluedMaps.  Since
>    ‘ControllerConfig::getRequestMapMap’ is used at multiple places I have
>    kept it temporarily and replaced its code with an adapter to the new
>    ‘ControllerConfig::getRequestMapMultiMap’ method.
>
>
>
> Comments, questions and/or reviews welcome.
>
> [1] https://issues.apache.org/jira/browse/OFBIZ-4274
>

[Shi Jinghai-3]

Thank you Mathieu!

Yeah, I forgot the site-conf.xsd, now I reverted it in rev. 1835043.

-----邮件原件-----
发件人: Mathieu Lirzin [mailto:[hidden email]]
发送时间: 2018年7月4日 0:06
收件人: [hidden email]
主题: Add ‘method’ attribute to ‘request-map’ elements

Hello,

Following revert of rev 1834917, I want to “properly” propose adding a ‘method’ attribute to ‘request-map’ elements.

I am currently working on adding REST based Web APIs to OFBiz [1].  In order to do that, it is important for the HTTP controller to handle various HTTP methods differently, meaning allowing a different request handler to be called depending on the method of the HTTP request.
Syntactically I am proposing the following:

   <request-map uri="examples" method="get">
      <security https="true" auth="true"/>
      <event type="java" path="ExamplesHandlers" invoke="getExamples"/>
      <response name="success" type="view" value="..."/>
      <response name="error" type="view" value="..."/>
   </request-map>

   <request-map uri="examples" method="post">
      <security https="true" auth="true"/>
      <event type="java" path="ExamplesHandlers" invoke="postExamples"/>
      <response name="success" type="view" value="..."/>
      <response name="error" type="view" value="..."/>
   </request-map>

Here ‘method’ is an optional attribute which defaults to "all" and/or "".  Currently the patch supports both but I would be in favour of handling only one for the sake of simplicity.  The semantic of that default is that it mimics the current behavior, meaning it handles every methods.

Attached you will find a serie of 3 patches (meant to be applied in
order) implementing the described change:

- The first one handles the parsing of the XML (the change in the xsd
  file has not been reverted).

- The second is a simple refactoring to agglomerate the read of the XML
  file in one place.  This is done to ease the implementation and tests
  in the third patch.

- The third patch implements the segregation of ‘request-map’ elements
  based on the actual request method.  In order to associate a request
  to an event handler we now need to dispatch both on the URI and on the
  method.  To achieve that, I have decided to keep using the URI as a
  key but associate it to a List of ‘request-map’ (one for each method)
  instead of a single one.  In term of implementation a ‘MultivaluedMap’
  has been used.  The list of candidates for a particular URI is then
  filtered to keep only the best matching one.  The ‘request-map’ Map
  concrete type was a MapContext which is a stack of Maps (to handle
  includes), The ‘MultivaluedMapContext’ implementation is an adaptation
  of MapContext to MultivaluedMaps.  Since
  ‘ControllerConfig::getRequestMapMap’ is used at multiple places I have
  kept it temporarily and replaced its code with an adapter to the new
  ‘ControllerConfig::getRequestMapMultiMap’ method.

[Mathieu Lirzin]

Jacques Le Roux <[hidden email]> writes:

> FYI: You were lucky that your attached patches passed, it's not usually.
>
> In general better to reference patches in some place. An existing
> Jira, like OFBIZ-4274 or OFBIZ-10438 here, fits here

OK, I have included those patches in OFBIZ-10438.

Thank you for the explanation.

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

[Mathieu Lirzin]

Shi Jinghai <[hidden email]> writes:

> Yeah, I forgot the site-conf.xsd, now I reverted it in rev. 1835043.

Thanks, I have updated the patches from OFBIZ-10438 to re-add those :-)

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

[taher]

Hi Mathieu,

Great ideas, thank you for your contributions so far.

Before we dig into the details, may I first understand _why_ are you
adding that functionality in the first place? In other words, you can
decide on the HTTP protocol from the calling page right? If it is a
form, it's a POST, if it is a URL, it's a GET, or you can override
that in certain areas using the OFBiz DSL. So what is the purpose here
of adding this functionality in the controller? If we implement a
RESTFUL API, wouldn't we still decide on the HTTP protocol from the
client side? Or am I missing something here?

On Tue, Jul 3, 2018 at 7:05 PM, Mathieu Lirzin
<[hidden email]> wrote:

> Hello,
>
> Following revert of rev 1834917, I want to “properly” propose adding a
> ‘method’ attribute to ‘request-map’ elements.
>
> I am currently working on adding REST based Web APIs to OFBiz [1].  In
> order to do that, it is important for the HTTP controller to handle
> various HTTP methods differently, meaning allowing a different request
> handler to be called depending on the method of the HTTP request.
> Syntactically I am proposing the following:
>
>    <request-map uri="examples" method="get">
>       <security https="true" auth="true"/>
>       <event type="java" path="ExamplesHandlers" invoke="getExamples"/>
>       <response name="success" type="view" value="..."/>
>       <response name="error" type="view" value="..."/>
>    </request-map>
>
>    <request-map uri="examples" method="post">
>       <security https="true" auth="true"/>
>       <event type="java" path="ExamplesHandlers" invoke="postExamples"/>
>       <response name="success" type="view" value="..."/>
>       <response name="error" type="view" value="..."/>
>    </request-map>
>
> Here ‘method’ is an optional attribute which defaults to "all" and/or
> "".  Currently the patch supports both but I would be in favour of
> handling only one for the sake of simplicity.  The semantic of that
> default is that it mimics the current behavior, meaning it handles every
> methods.
>
> Attached you will find a serie of 3 patches (meant to be applied in
> order) implementing the described change:
>
> - The first one handles the parsing of the XML (the change in the xsd
>   file has not been reverted).
>
> - The second is a simple refactoring to agglomerate the read of the XML
>   file in one place.  This is done to ease the implementation and tests
>   in the third patch.
>
> - The third patch implements the segregation of ‘request-map’ elements
>   based on the actual request method.  In order to associate a request
>   to an event handler we now need to dispatch both on the URI and on the
>   method.  To achieve that, I have decided to keep using the URI as a
>   key but associate it to a List of ‘request-map’ (one for each method)
>   instead of a single one.  In term of implementation a ‘MultivaluedMap’
>   has been used.  The list of candidates for a particular URI is then
>   filtered to keep only the best matching one.  The ‘request-map’ Map
>   concrete type was a MapContext which is a stack of Maps (to handle
>   includes), The ‘MultivaluedMapContext’ implementation is an adaptation
>   of MapContext to MultivaluedMaps.  Since
>   ‘ControllerConfig::getRequestMapMap’ is used at multiple places I have
>   kept it temporarily and replaced its code with an adapter to the new
>   ‘ControllerConfig::getRequestMapMultiMap’ method.
>
>
>
> Comments, questions and/or reviews welcome.
>
> [1] https://issues.apache.org/jira/browse/OFBIZ-4274
>
> --
> Mathieu Lirzin
> GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37
>

[Mathieu Lirzin]

Hello Taher,

Taher Alkhateeb <[hidden email]> writes:

> Before we dig into the details, may I first understand _why_ are you
> adding that functionality in the first place? In other words, you can
> decide on the HTTP protocol from the calling page right? If it is a
> form, it's a POST, if it is a URL, it's a GET, or you can override
> that in certain areas using the OFBiz DSL. So what is the purpose here
> of adding this functionality in the controller? If we implement a
> RESTFUL API, wouldn't we still decide on the HTTP protocol from the
> client side? Or am I missing something here?

I guess by “HTTP protocol” you actually meant “HTTP method”.  The reason
I want to add a ‘method’ attribute is that I want the controller to be
able to provide a RESTful API, where each HTTP method has semantics
attached to it [1].

The controller is where requests are dispatched to there corresponding
handler (event, service, view).  Currently it is written in a RPC way
where URI refers to *actions* instead of *resources*. For example take
the Geo management from the webtools controller:

    <request-map uri="LookupGeo">...</request-map>
    <request-map uri="createGeo">...</request-map>
    <request-map uri="updateGeo">...</request-map>
    <request-map uri="deleteGeo">...</request-map>

In order to implement a RESTful API with the same capability.  The idea
is to define a “geos” resouce which is a container of geo resources and
implement the different *actions* available on those resources via the
various HTTP methods available.  So this would convert into something
like this:

    <request-map uri="geos" method="get">...</request-map>
    <request-map uri="geos" method="post">...</request-map>
    <request-map uri="geos/{id}" method="update">...</request-map>
    <request-map uri="geos/{id}" method="delete">...</request-map>

The last two examples requires additional changes that are not
implemented in the patches from OFBIZ-10458:

    - Handling URI templates
    - Replacing the overridden view URI feature with an alternate solution.

I am currently working on this in parallel of the JSON view handler.

Does this additional explanation and example help?

Thanks for the feedback.

[1] https://www.restapitutorial.com/lessons/httpmethods.html

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

[taher]

Thank you Mathieu, this makes things clearer and yes I stand corrected
on using the word protocol instead of method.

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.

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?
- 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?
- What is the purpose of the new Controller object?
- 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?
- 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?
- 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?

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

On Sat, Jul 7, 2018 at 3:36 PM, Mathieu Lirzin
<[hidden email]> wrote:

> Hello Taher,
>
> Taher Alkhateeb <[hidden email]> writes:
>
>> Before we dig into the details, may I first understand _why_ are you
>> adding that functionality in the first place? In other words, you can
>> decide on the HTTP protocol from the calling page right? If it is a
>> form, it's a POST, if it is a URL, it's a GET, or you can override
>> that in certain areas using the OFBiz DSL. So what is the purpose here
>> of adding this functionality in the controller? If we implement a
>> RESTFUL API, wouldn't we still decide on the HTTP protocol from the
>> client side? Or am I missing something here?
>
> I guess by “HTTP protocol” you actually meant “HTTP method”.  The reason
> I want to add a ‘method’ attribute is that I want the controller to be
> able to provide a RESTful API, where each HTTP method has semantics
> attached to it [1].
>
> The controller is where requests are dispatched to there corresponding
> handler (event, service, view).  Currently it is written in a RPC way
> where URI refers to *actions* instead of *resources*. For example take
> the Geo management from the webtools controller:
>
>     <request-map uri="LookupGeo">...</request-map>
>     <request-map uri="createGeo">...</request-map>
>     <request-map uri="updateGeo">...</request-map>
>     <request-map uri="deleteGeo">...</request-map>
>
> In order to implement a RESTful API with the same capability.  The idea
> is to define a “geos” resouce which is a container of geo resources and
> implement the different *actions* available on those resources via the
> various HTTP methods available.  So this would convert into something
> like this:
>
>     <request-map uri="geos" method="get">...</request-map>
>     <request-map uri="geos" method="post">...</request-map>
>     <request-map uri="geos/{id}" method="update">...</request-map>
>     <request-map uri="geos/{id}" method="delete">...</request-map>
>
> The last two examples requires additional changes that are not
> implemented in the patches from OFBIZ-10458:
>
>     - Handling URI templates
>     - Replacing the overridden view URI feature with an alternate solution.
>
> I am currently working on this in parallel of the JSON view handler.
>
> Does this additional explanation and example help?
>
> Thanks for the feedback.
>
> [1] https://www.restapitutorial.com/lessons/httpmethods.html
>
> --
> Mathieu Lirzin
> GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Mathieu Lirzin]

Taher Alkhateeb <[hidden email]> 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

[Mathieu Lirzin]

Hello,

Taher Alkhateeb <[hidden email]> writes:

> Examining your patch I have a few comments / questions:

FYI I have updated the third patch on OFBIZ-10438 [1] to improve the
tests for the ‘resolveMethod’ static method.

[1] https://issues.apache.org/jira/browse/OFBIZ-10438

Thanks.

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

[taher]

Hello Mathieu,

First of all, I would like to say I really enjoy looking at your code.
I can tell that it was done with lots of love :)

I took a deeper look at the code and I have a few more questions /
comments / suggestions:
- First of all and to stay consistent with the rest of the OFBiz
architecture, you can perhaps call your controller object
"ControllerConfig" just like we have "ContainerConfig"
"ComponentConfig" and so on.
- What do you have a temporary converter from Map to MultiMap? That
piece of code is really sour on the eyes with all those overrides
- I still don't understand your answer on the new data structure
"MultiValuedMapContext". This is essentially a whole new data
structure designed by you and it overrides nearly all standard
methods. Having such a custom and un-tested data structure might be
worrying. Why aren't you going with a standard and tested solution?
What methods exactly do we need that requires that level of
customization? Is it the getRequestMapMap and getRequestMultiMap? Why
not simply create two custom logic methods in a Util class to handle
that instead of that mesh of object inheritance? I find it a little
worrying to strongly tie core framework classes with external
libraries. We should perhaps attempt to do that in a de-coupled
fashion. For example, take a look at Start.java where it has a
function to parse online arguments. This function completely hides the
libraries away from the framework. Maybe we should attempt something
similar?
- I am assuming from the comment in ConfigXMLReader that you intend to
refactor the converter right? How will this conversion happen, when
and how?

Thank you for your patience and follow through :)

On Thu, Jul 12, 2018 at 4:37 PM, Mathieu Lirzin
<[hidden email]> wrote:

> Hello,
>
> Taher Alkhateeb <[hidden email]> writes:
>
>> Examining your patch I have a few comments / questions:
>
> FYI I have updated the third patch on OFBIZ-10438 [1] to improve the
> tests for the ‘resolveMethod’ static method.
>
> [1] https://issues.apache.org/jira/browse/OFBIZ-10438
>
> Thanks.
>
> --
> Mathieu Lirzin
> GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[taher]

Oh and for the rest of the folks in our community, this is a very
critical piece of work, I think more than one pair of eyes should be
on it, so please take a look at it if you can.

On Sun, Jul 15, 2018 at 7:47 PM, Taher Alkhateeb
<[hidden email]> wrote:

> Hello Mathieu,
>
> First of all, I would like to say I really enjoy looking at your code.
> I can tell that it was done with lots of love :)
>
> I took a deeper look at the code and I have a few more questions /
> comments / suggestions:
> - First of all and to stay consistent with the rest of the OFBiz
> architecture, you can perhaps call your controller object
> "ControllerConfig" just like we have "ContainerConfig"
> "ComponentConfig" and so on.
> - What do you have a temporary converter from Map to MultiMap? That
> piece of code is really sour on the eyes with all those overrides
> - I still don't understand your answer on the new data structure
> "MultiValuedMapContext". This is essentially a whole new data
> structure designed by you and it overrides nearly all standard
> methods. Having such a custom and un-tested data structure might be
> worrying. Why aren't you going with a standard and tested solution?
> What methods exactly do we need that requires that level of
> customization? Is it the getRequestMapMap and getRequestMultiMap? Why
> not simply create two custom logic methods in a Util class to handle
> that instead of that mesh of object inheritance? I find it a little
> worrying to strongly tie core framework classes with external
> libraries. We should perhaps attempt to do that in a de-coupled
> fashion. For example, take a look at Start.java where it has a
> function to parse online arguments. This function completely hides the
> libraries away from the framework. Maybe we should attempt something
> similar?
> - I am assuming from the comment in ConfigXMLReader that you intend to
> refactor the converter right? How will this conversion happen, when
> and how?
>
> Thank you for your patience and follow through :)
>
> On Thu, Jul 12, 2018 at 4:37 PM, Mathieu Lirzin
> <[hidden email]> wrote:
>> Hello,
>>
>> Taher Alkhateeb <[hidden email]> writes:
>>
>>> Examining your patch I have a few comments / questions:
>>
>> FYI I have updated the third patch on OFBIZ-10438 [1] to improve the
>> tests for the ‘resolveMethod’ static method.
>>
>> [1] https://issues.apache.org/jira/browse/OFBIZ-10438
>>
>> Thanks.
>>
>> --
>> Mathieu Lirzin
>> GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Mathieu Lirzin]

Hello Taher,

Taher Alkhateeb <[hidden email]> writes:

> First of all, I would like to say I really enjoy looking at your code.
> I can tell that it was done with lots of love :)

Thanks for your kind words. :-)

> I took a deeper look at the code and I have a few more questions /
> comments / suggestions:
> - First of all and to stay consistent with the rest of the OFBiz
> architecture, you can perhaps call your controller object
> "ControllerConfig" just like we have "ContainerConfig"
> "ComponentConfig" and so on.

OK, I will do that.

> - Why do you have a temporary converter from Map to MultiMap? That
> piece of code is really sour on the eyes with all those overrides

‘ConfigXMLReader.ControllerConfig::getRequestMapMap’ is called 12 times
in the framework.  I wasn't sure about the backward compatibility
requirements of this method. Additionally I didn't want to spread my
modifications across a lot of files in order to help the review process.
As a consequence I have prefered to create an ad-hoc temporary wrapper.

> - I am assuming from the comment in ConfigXMLReader that you intend to
> refactor the converter right? How will this conversion happen, when
> and how?

The idea is to simply remove it and adapt all its callers to use
‘getRequestMultiMap’ instead.  However if there is some backward
compatibility requirement for this method it is possible to keep it and
mark it as deprecated.  Both solutions can be done at anytime even
during this patch serie if it is prefered.

> - I still don't understand your answer on the new data structure
> "MultiValuedMapContext". This is essentially a whole new data
> structure designed by you and it overrides nearly all standard
> methods. Having such a custom and un-tested data structure might be
> worrying. Why aren't you going with a standard and tested solution?

All the methods are overridden because they implements the
‘MultivaluedMap’ interface.  :-)

‘MultiValuedMapContext’ is a clone of ‘MapContext’ but adapted to
MultivaluedMap.  The idea of ‘MapContext’ and ‘MultiValuedMapContext’ is
to provide a Map/MultivaluedMap view of a collection of configuration
files which are included from each other without any cycle (i.e. it's a
DAG of files).  Does it help?  I agree that there is room for
improvements in terms of documentation, refactoring, and tests.

> What methods exactly do we need that requires that level of
> customization? Is it the getRequestMapMap and getRequestMultiMap?

Mainly those two methods + the ‘MapStack’ which extends ‘MapContext’ and
is used at some other places.

> Why not simply create two custom logic methods in a Util class to
> handle that instead of that mesh of object inheritance?

This concerns design choices that were already made in ‘MapContext’ and
‘MapStack’.  That could be reworked but given the non-trivial amount of
work it represents this should be done outside of this review IMO.

> I find it a little worrying to strongly tie core framework classes
> with external libraries. We should perhaps attempt to do that in a
> de-coupled fashion. For example, take a look at Start.java where it
> has a function to parse online arguments. This function completely
> hides the libraries away from the framework. Maybe we should attempt
> something similar?

AIUI this is a different context, the command line parsing has a
specific role which can easily be encapsulated in one class, however a
MultivaluedMap is a generic container which is intended to be passed
around.

If we don't want to depend on ‘javax.ws.rs.core.MultivaluedMap’ it would
be better to reimplement it ourselves.  This is possible, however this
would force us to not use ‘org.apache.cxf.jaxrs.model.URITemplate’ which
depends on ‘javax.ws.rs.core.MultivaluedMap’ and that I intend to use in
order to add support URI templates.

‘javax.ws.rs.core.MultivaluedMap’ is part of the JAX-RS specification
which is a core component of JavaEE REST support, so it feels like a
reasonable core dependency to have.

Regarding ‘org.apache.cxf.jaxrs.model.URITemplate’ it can be
encapsulated like what is done for ‘org.apache.commons.cli.*’ in
‘StartupCommandUtil’.  Not depending on it is still a viable option
however it will mean having to maintain a non-negligeable amount of
non-trivial code in OFBiz.  WDYT?

Thank you for your insightful review.

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

[Mathieu Lirzin]

Hello,

Just wanted to correct a mistake I have made.

Mathieu Lirzin <[hidden email]> writes:

> 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.

‘MultivaluedMap::put’ is destructive like ‘Map::put’ meaning that it
replaces the previously added values which makes sense since
‘MultivaluedMap’ extends ’Map’ so should have the same semantics.  What
provides the non-destructive semantics is the ‘MultivaluedMap::add’
method.  Sorry for the confusion.

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

[Mathieu Lirzin]

Mathieu Lirzin <[hidden email]> writes:

> Taher Alkhateeb <[hidden email]> writes:
>
>> - I still don't understand your answer on the new data structure
>> "MultiValuedMapContext". This is essentially a whole new data
>> structure designed by you and it overrides nearly all standard
>> methods. Having such a custom and un-tested data structure might be
>> worrying. Why aren't you going with a standard and tested solution?
>
> All the methods are overridden because they implements the
> ‘MultivaluedMap’ interface.  :-)
>
> ‘MultiValuedMapContext’ is a clone of ‘MapContext’ but adapted to
> MultivaluedMap.  The idea of ‘MapContext’ and ‘MultiValuedMapContext’ is
> to provide a Map/MultivaluedMap view of a collection of configuration
> files which are included from each other without any cycle (i.e. it's a
> DAG of files).  Does it help?  I agree that there is room for
> improvements in terms of documentation, refactoring, and tests.
>
>> What methods exactly do we need that requires that level of
>> customization? Is it the getRequestMapMap and getRequestMultiMap?
>
> Mainly those two methods + the ‘MapStack’ which extends ‘MapContext’ and
> is used at some other places.
>
>> Why not simply create two custom logic methods in a Util class to
>> handle that instead of that mesh of object inheritance?
>
> This concerns design choices that were already made in ‘MapContext’ and
> ‘MapStack’.  That could be reworked but given the non-trivial amount of
> work it represents this should be done outside of this review IMO.

For the record, I will start refactoring MapContext/MapStack in parallel
of this review process.

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