rmcdouga commented on a change in pull request #6: Added support for simulating
HTML forms (including file uploads)…
URL:
https://github.com/apache/sling-org-apache-sling-servlet-helpers/pull/6#discussion_r288810038
##########
File path:
src/main/java/org/apache/sling/servlethelpers/MockSlingHttpServletRequest.java
##########
@@ -82,7 +83,7 @@
private final ResourceResolver resourceResolver;
private final RequestPathInfo requestPathInfo;
private Map<String, Object> attributeMap = new HashMap<String, Object>();
- private Map<String, String[]> parameterMap = new LinkedHashMap<String,
String[]>();
+ private Map<String, MockRequestParameter[]> parameterMap = new
LinkedHashMap<>();
Review comment:
So, I had a look at this and I’m not sure if this change is worth it. Let
me explain:
MockRequestParameterMap is only used in one place, getRequestParameterMap().
That method creates and returns a new copy each time it is called.
If we were to use this class for the parameter storage in
MockSlingHttpRequest, then we would need to move the addRequestParameter()
methods into the MockRequestParameterClass *and* retain delegate methods in the
MockSlingHttpRequest class.
In my opinion, those delegate methods are the ones that people will want to
call. There aren’t any circumstances that I can think of where someone would
want to create a map separately and then add parameters to it. So the only
class that will call the addRequestParameter methods will be the
MockSlingRequest. We will be adding a level of indirection that provides no
benefit.
Also, if we use MockRequestParameterMap for storage, I don’t think we should
return it directly from getRequestParameterMap() without making a defensive
copy, so we’re not really saving anything by using it for storage.
Basically, we would be adding a bunch more code (all the delegate methods)
with no real benefit. I think I’d prefer to keep the MockRequestParameterMap
strictly as a mock object returned by getRequestParameterMap() and not try and
use for the internal storage mechanism too.
Let me know if you feel differently and we can discuss further.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
