Hello Taher, Taher Alkhateeb <[email protected]> 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
