Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support concrete Map subtypes for @RequestParam arguments #22094

Closed
spring-issuemaster opened this issue Dec 4, 2018 · 11 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Dec 4, 2018

yl-yue opened SPR-17562 and commented

Problem description

Github problem notes

Additional description

expect

@RequestParam annotation Subclasses of map are supported, Not just HashMap.

implementation

As github described it.

Or you can override the default Argument Resolvers by customizing the Argument Resolvers, as in the comments below.


Affects: 5.0.10

Reference URL: spring-projects/spring-boot#15377

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 5, 2018

yl-yue commented

Perhaps you could provide a custom Argument Resolvers override the default Argument Resolvers.

But!!!This will also involve an Argument Resolvers priority problem.

If you can control the order well, this will also be a good solution, and can meet more requirements at the same time.

 

package org.springframework.web.servlet.mvc.method.annotation;
public class RequestMappingHandlerAdapter extends AbstractHandlerMethodAdapter
		implements BeanFactoryAware, InitializingBean {
	/**
	 * Return the list of argument resolvers to use including built-in resolvers
	 * and custom resolvers provided via {@link #setCustomArgumentResolvers}.
	 */
	private List<HandlerMethodArgumentResolver> getDefaultArgumentResolvers() {
		List<HandlerMethodArgumentResolver> resolvers = new ArrayList<>();

		// Annotation-based argument resolution
		resolvers.add(new RequestParamMethodArgumentResolver(getBeanFactory(), false));
		resolvers.add(new RequestParamMapMethodArgumentResolver());
		resolvers.add(new PathVariableMethodArgumentResolver());
		resolvers.add(new PathVariableMapMethodArgumentResolver());
		resolvers.add(new MatrixVariableMethodArgumentResolver());
		resolvers.add(new MatrixVariableMapMethodArgumentResolver());
		resolvers.add(new ServletModelAttributeMethodProcessor(false));
		resolvers.add(new RequestResponseBodyMethodProcessor(getMessageConverters(), this.requestResponseBodyAdvice));
		resolvers.add(new RequestPartMethodArgumentResolver(getMessageConverters(), this.requestResponseBodyAdvice));
		resolvers.add(new RequestHeaderMethodArgumentResolver(getBeanFactory()));
		resolvers.add(new RequestHeaderMapMethodArgumentResolver());
		resolvers.add(new ServletCookieValueMethodArgumentResolver(getBeanFactory()));
		resolvers.add(new ExpressionValueMethodArgumentResolver(getBeanFactory()));
		resolvers.add(new SessionAttributeMethodArgumentResolver());
		resolvers.add(new RequestAttributeMethodArgumentResolver());

		// Type-based argument resolution
		resolvers.add(new ServletRequestMethodArgumentResolver());
		resolvers.add(new ServletResponseMethodArgumentResolver());
		resolvers.add(new HttpEntityMethodProcessor(getMessageConverters(), this.requestResponseBodyAdvice));
		resolvers.add(new RedirectAttributesMethodArgumentResolver());
		resolvers.add(new ModelMethodProcessor());
		resolvers.add(new MapMethodProcessor());
		resolvers.add(new ErrorsMethodArgumentResolver());
		resolvers.add(new SessionStatusMethodArgumentResolver());
		resolvers.add(new UriComponentsBuilderMethodArgumentResolver());

		// Custom arguments
		if (getCustomArgumentResolvers() != null) {
			resolvers.addAll(getCustomArgumentResolvers());
		}

		// Catch-all
		resolvers.add(new RequestParamMethodArgumentResolver(getBeanFactory(), true));
		resolvers.add(new ServletModelAttributeMethodProcessor(true));

		return resolvers;
	}
}
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 6, 2018

Rossen Stoyanchev commented

Thanks for the report. I read the original ticket filed in Spring Boot, but did not understand what the request is about. Can you fill out the description field above and explain what the problem is? If you cannot edit the description, just add a new comment, but preferably fill out the description.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 7, 2018

yl-yue commented

Additional description: spring-projects/spring-boot#15377

@yl-yue

This comment has been minimized.

Copy link

@yl-yue yl-yue commented Apr 19, 2019

@spring-issuemaster Hello, has this enhancement been implemented?

@snicoll

This comment has been minimized.

Copy link
Member

@snicoll snicoll commented Apr 19, 2019

@yl-yue that's a bot, there's no need to mention it. Also, you don't need to ask either: this issue is opened so, no, it's not implemented yet.

@sbrannen sbrannen removed the in: core label Apr 21, 2019
@sbrannen

This comment has been minimized.

Copy link
Member

@sbrannen sbrannen commented Apr 21, 2019

From the class-level Javadoc for @RequestParam, we see:

If the method parameter is Map<String, String> or MultiValueMap<String, String> and a parameter name is not specified, then the map parameter is populated with all request parameter names and values.

@yl-yue, if I understood your proposal, you would like to be able to declare a @RequestParam parameter with a concrete subtype of Map -- for example, net.minidev.json.JSONObject which extends HashMap<String, Object>.

Is that correct?

@sbrannen sbrannen changed the title AOP @RequestParam invalid, AOP RequestParamMapMethodArgumentResolver Class invalid [SPR-17562] Support concrete Map subtypes for @RequestParam arguments Apr 21, 2019
@yl-yue

This comment has been minimized.

Copy link

@yl-yue yl-yue commented Apr 22, 2019

@sbrannen Yes, expect @RequestParam this annotation to support automatic parsing of HashMap<String, Object> extension classes

@rstoyanchev

This comment has been minimized.

Copy link
Contributor

@rstoyanchev rstoyanchev commented May 1, 2019

@yl-yue I think we could make a change so any Map type (with a default constructor) can be instantiated. However we might have to apply similar changes to other argument resolvers (e.g. for consistency) and it's important to understand what this is needed for and therefore which resolvers it might apply to. So could you explain why isn't Map sufficient?

Looking at net.minidev.json.JSONObject that you mention, it has static methods that accept Map so couldn't you do something like.:

public void handle(@RequestParam Map<String, ?> params) {
    String json = JSONObject.toJSONString(params);
}
@yl-yue

This comment has been minimized.

Copy link

@yl-yue yl-yue commented May 2, 2019

@rstoyanchev fastjson

<dependency>
	<groupId>com.alibaba</groupId>
	<artifactId>fastjson</artifactId>
	<version>1.2.57</version>
</dependency>
public void name(@RequestParam Map<String, Object> paramMap) {
	/**
	 * This way, if you want to get the value of the validated type
	 * , you also need to convert it through a utility class, such as MapUtils
	 */
	paramMap.get("id");
	
	// Map subclass, an enhanced Map
	JSONObject paramJson = new JSONObject(paramMap);
	paramJson.getLong("id");
	paramJson.getString("id");
	paramJson.getDouble("id");
	paramJson.getDouble("id");
	paramJson.getJSONObject("id");
	paramJson.getObject("id", Long.class);// More methods ...
	
	// As you mentioned above, the toJSONString() dot method
	paramJson.toJSONString();
	
	// JSONObject default constructor
	new JSONObject();
}

expect

public void name(@RequestParam JSONObject paramJson) {
	// ...
}
@yl-yue

This comment has been minimized.

Copy link

@yl-yue yl-yue commented Aug 1, 2019

com.alibaba.fastjson.JSONObject Will it be supported

@rstoyanchev rstoyanchev self-assigned this Aug 1, 2019
@rstoyanchev rstoyanchev added this to the 5.2 RC1 milestone Aug 1, 2019
@rstoyanchev

This comment has been minimized.

Copy link
Contributor

@rstoyanchev rstoyanchev commented Aug 1, 2019

@yl-yue I considered this but I'm not sure our own RequestParamMapMethodArgumentResolver is a good place for this, as it has to create the sub-map via reflection. Also that resolver supports quite a number of variations of maps (single value, multi-value, with MultipartFile or Part as values).

I think it would be much better if this was handled as a custom argument resolver which understands what map needs to be created and can do a simple instantiation. I would recommend creating a custom type, something like:

public final class JsonParams {

    private final JSONObject jsonObject;

    public JsonParams(Map<String, String> params) {
        this.jsonObject = new JSONObject(params);
    }

    public JSONObject json() {
        return this.jsonObject.
    }
}

Then a custom argument resolver could support a controller method like below based on the target type, without the need for adding @RequestParam which in the case of a Map does not add any value:

public void name(JsonParams params) {
    // ...
}
@rstoyanchev rstoyanchev closed this Aug 1, 2019
@rstoyanchev rstoyanchev removed this from the 5.2 RC1 milestone Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.