Remove use of java.util.Properties for URL query parsing utility

[tfmorris]

As part of the epic #6391 to minimize our use of java.util.Properties we want our URL parameter parsing to not use this as a return type.

Proposed solution

• deprecate all methods in ParsingUtilities which return java.util.Properties`
• create a replacement for static public Properties parseUrlParameters(HttpServletRequest request) with the signature static public Map<String, String> parseParameters(HttpServletRequest request) which uses available functionality for its implementation like request.getParameterMap()
• modify all uses of parseUrlParameters to use the new method and adjust all necessary code to accommodate the new return type. This primarily affects our bundled importers.
• be careful to preserve any public APIs (ie you can add new methods, but not remove or modify existing ones). I think that only ImportingUtilities has public APIs which are affected, but double check.

Alternatives considered

We could use request.getParameterMap directly, but since it returns Map<String, String[]> instead of Map<String, String> it would require more extensive modifications to the existing code and none of our current parameters can be repeated, so the array support is unnecessary.

[EliasStihl]

Hi, how do you recommend we change ImportingUtilities to accommodate the new changes?

If we are unable to modify the existing methods through refactoring, we could potentially face significant code duplication issues. For instance, in the case of the retrieveContentFromPostRequest method, it appears that only a single line of code needs to be modified within a method spanning 227 lines.

Any suggestions?

[tfmorris]

A standard mechanism for dealing with code duplication is to extract a common method which can be called in the multiple places where it's needed. Would that work here? If you are using an IDE, it should have an "Extract Method" refactoring function that can help with this.

…

Message ID: ***@***.***>

[EliasStihl]

Okay, I think the problem is that the code that needs to be changed is nested very deep inside retrieveContentFromPostRequest, so I don't think that a refactoring of it would help that much to avoid code duplication.

Is it acceptable to modify the public method retrieveContentFromPostRequest as long as it doesn't change the signature and functionality of the method?

If so, my solution would be create a new method that takes Map<String,String> and from the original method simply convert Properties parameters to a Map<String, String> and call the new method. Like this:

// Original method
static public void retrieveContentFromPostRequest(
            HttpServletRequest request,
            Properties parameters,
            File rawDataDir,
            ObjectNode retrievalRecord,
            final Progress progress) throws IOException, FileUploadException {

        Map<String, String> parametersMap = new HashMap<>();
        for (String key : parameters.stringPropertyNames()) {
            String value = parameters.getProperty(key);
            parametersMap.put(key, value);
        }
        retrieveContentFromPostRequest(request, parametersMap, rawDataDir, retrievalRecord, progress);
}

// New method which takes map instead of properties
static public void retrieveContentFromPostRequest(
            HttpServletRequest request,
            Map<String, String> parameters,
            File rawDataDir,
            ObjectNode retrievalRecord,
            final Progress progress) throws IOException, FileUploadException {

            // Same code as before

}

Is that a viable solution?

[tfmorris]

Yes, exactly. I suspect you'll need to do that for multiple methods, so you might want to extract your property remapping code into a separate utility method that you can reuse. Pleased also add an @Deprecated annotation to the original method so that people know not to use it for new code.

[tfmorris]

p.s. you can use Java 8 Streams to make your remapping a little more concise:

   private static Map<String, String> mapProps(Properties properties) {
       return properties.entrySet().stream().collect(Collectors.toMap(e -> (String) e.getKey(), e -> (String) e.getValue()));
   }

[EliasStihl]

Great, thanks for your tips, will do that!