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

Implicit /** mapping on type-level @RequestMapping [SPR-5631] #10302

Closed
spring-projects-issues opened this issue Mar 29, 2009 · 11 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Mar 29, 2009

Arjen Poutsma opened SPR-5631 and commented

When a @Controller has a class-level @RequestMapping annotation, with further sub-paths mappings defined on the methods, it makes sense to implicitely add /** to the type-level annotation.

Since method mappings can just have a RequestMethod (GET or POST)on it, with no further path information, we have to be a little clever about this.


Issue Links:

Referenced from: commits 6121da9, 65afc80, acc8492

@spring-projects-issues
Copy link
Collaborator Author

Grzegorz Borkowski commented

As this discussion was started here: http://grzegorzborkowski.blogspot.com/2009/03/test-drive-of-spring-30-m2-rest-support.html, I copy my comments from there to here. They are perhaps arguable, but I believe it is better to keep all arguments in one place:

"...the fact that you always needed to end class-level request mappings with ** bothered me in the past..." The point is that according to JavaDoc, it is actually not required. See JavaDoc:
"In a Servlet environment: the path mapping URIs (e.g. "/myPath.do"). Ant-style path patterns are also supported (e.g. "/myPath/*.do"). At the method level, relative paths (e.g. "edit.do") are supported within the primary mapping expressed at the type level. Supported at the type level as well as at the method level! When used at the type level, all method-level mappings inherit this primary mapping, narrowing it for a specific handler method."
So if I understand English correctly, this doesn't say "method-level mappings are relative to type-level only if type-level ends with wildcard". Quite opposite: it says method-level mappings are always relative to type-level, when type-level is present. This is why I said that changing the actual behavior should be treated as bug fix, because Spring 2.5 never worked properly against its JavaDoc. BTW the documented behavior is reasonable - I cannot imagine why it was implemented differently. Perhaps it was really the bug in implementation? I can raise the bug against 2.5 for this in JIRAk, if it helps :) Still there can be some problem with backward compatibility, if people coded against buggy interface to make it work (though I think the number of actual cases that can break will be very very small). You know, this is like a case of Microsoft problem with IE8 standards-compatibility :)

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Mar 30, 2009

Grzegorz Borkowski commented

Some additional comments:

  1. I don't believe many people used the configuration with type-level mapping not ending with wildcard, and separate method level annotation, with 2.5. In such case, type level and method level annotations are completely independent. In fact, it's hard to me to think about any logical application of such setup. If they are completely independent, then I would put it into separate controllers or I'd put in one controller on different methods to keep it on the same level. Using type-level and method-level annotations logically implies some hierarchy (master-slave setup). This obviously is not a proof that nobody used such configuration, but I guess it would be a very rare case.

  2. The one issue is not clear looking at Javadoc, and should be perhaps also fixed in javadoc: what if we have type-level annotation with wildcard ending (either explicit or say implicit in 3.0), and the method-level annotation starting with "/" then should be treated as relative to type level or not? This part of Javadoc: "At the method level, relative paths (e.g. "edit.do") are supported within the primary mapping expressed at the type level" implies that only relative paths on method level are concatenated with type-level. On the other hand, this part: "When used at the type level, all method-level mappings inherit this primary mapping, narrowing it for a specific handler method." Implies that is is always concatenated, even if starts with "/".

  3. I think we may also look at JAX-RS documentation. Aligning Spring's logic or @RequestMapping with JAX-RS @Path would be beneficial in long run I think, and perhaps it could even be argument for minor backward-incomatibilities. For example, in relation to previous point, JAX-RS always treats method-level annotation as relative to type-level, see https://jsr311.dev.java.net/nonav/releases/1.0/javax/ws/rs/Path.html: "For the purposes of absolutizing a path against the base URI , a leading '/' in a path is ignored and base URIs are treated as if they ended in '/'"

  4. See also @RequestMapping matching should be insensitive to trailing slashes [SPR-5636] #10307 for related issue.

@spring-projects-issues
Copy link
Collaborator Author

Arjen Poutsma commented

People do use @RequestMapping with a wildcard. We even blogged about it: http://blog.springsource.com/2008/03/23/using-a-hybrid-annotations-xml-approach-for-request-mapping-in-spring-mvc/. Then there are cases like these:

@Controller
@RequestMapping("/*.do")
private static class MyAdaptedController2 {

	@RequestMapping
	public void myHandle(HttpServletRequest request, HttpServletResponse response) throws IOException {
		response.getWriter().write("test");
	}

	@RequestMapping("/myPath2.do")
	public void myHandle(@RequestParam("param1") String p1,
			int param2,
			HttpServletResponse response,
			@RequestHeader("header1") String h1,
			@CookieValue("cookie1") String c1) throws IOException {
		response.getWriter().write("test-" + p1 + "-" + param2 + "-" + h1 + "-" + c1);
	}

	@RequestMapping("/myPath3")
	public void myHandle(TestBean tb, HttpServletResponse response) throws IOException {
		response.getWriter().write("test-" + tb.getName() + "-" + tb.getAge());
	}

	@RequestMapping("/myPath4.*")
	public void myHandle(TestBean tb, Errors errors, HttpServletResponse response) throws IOException {
		response.getWriter().write("test-" + tb.getName() + "-" + errors.getFieldError("age").getCode());
	}
}

This one of our unit test, so I could just change it to reflect any new behavior, but that would defy the point of a unit test, wouldn't it?

So, I came up with the following rules:

  1. If a type-level @RequestMapping does not start with /, we prepend it.
  2. If a type-level @RequestMapping contains a *, we leave it as in Spring 2.5
  3. If a type-level @RequestMapping does not contain a *, there are two possibilities:
    • If there are further path mappings on the methods, we append the method-level mappings to the type-level mapping, inserting a / between the two if necessary.
    • If there are only HTTP methods defined on the methods, we leave things as in Spring 2.5

I believe this algorithm does not break backwards compatibility, while also allowing for hierarchical mappings.

@spring-projects-issues
Copy link
Collaborator Author

Grzegorz Borkowski commented

I'm not sure if you understood me correctly. I didn't say people don't use @RequestMapping with a wildcard. For sure they do. I said I don't think many people used class-level mapping without wildcard, and method-level mapping at the same time. I've tried to use it in the past, assuming that class-level wildcard will be implicitly added by the framework. But it didn't work. Simply I wasn't able to make it work in any logical way, because (as I remember) the method-level mappings looked like completely unrelated to type-level. So, if they are unrelated, what's the point of using it at the same time in one class? That's why I think it wasn't popular setup.
But perhaps I'm wrong, maybe someone has used it for some strange things.

The rules you describe sounds ok. I'm not sure though if there will be difference for /x/.do and /x/ mappings on type level. From the point of view of REST services, the /x/.do is of less importance. So for me the /x/ is much more important. However it would be good to have consistent rules for both wildcard mapping types.

Your rule 3(a) means that my point #2 from previous comment is solved in favor of "method-level mappings are always relative to type-level, even if they start from / character". That's ok for me.

@spring-projects-issues
Copy link
Collaborator Author

Grzegorz Borkowski commented

Oups, liks like Jira treats * characters as bold-markers... Not good in this case :)
Let's try again with blanks between characters:
I meant two types of mapping:
/ x / * .do and / x / *
Only the second will be used in REST mappings, probably.

@spring-projects-issues
Copy link
Collaborator Author

Grzegorz Borkowski commented

From my point of view, target behavior (to be used in tests, for example) for the RESTful controller like this:

@Controller
@RequestMapping("/projects")
public class ProjectEndpoint {
 
  @RequestMapping(method=GET)
  public HttpResponse list() {...}

  @RequestMapping(method=POST)
  public HttpResponse create() {...}
 
  @RequestMapping(method=GET, value="/{projectId}")
  public HttpResponse show(@PathVariable long projectId) {...}
 
  @RequestMapping(method=PUT,value="/{projectId}")
  public HttpResponse createOrUpdate(@PathVariable long projectId) {...}
 
  @RequestMapping(method=DELETE, value="/{projectId}")
  public HttpResponse remove(@PathVariable long projectId) {...}
}

should be following:

request URL selected method
GET /projects list
GET /projects/ list
POST /projects create
POST /projects/ create
GET /projects/12 show
PUT /projects/14 createOrUpdate
DELETE /projects/14 remove
PUT /projects/ probably "405 Method not allowed" or "404 Not found" response
PUT /projects/aaa "400 Bad request" response

@spring-projects-issues
Copy link
Collaborator Author

Arjen Poutsma commented

I have a testcase like that in place. Making it work while not breaking backwards compatibility is not trivial, however. Working on that now.

@spring-projects-issues
Copy link
Collaborator Author

Arjen Poutsma commented

It took a while, but this is fixed in SVN, and will be part of M3.

@spring-projects-issues
Copy link
Collaborator Author

Grzegorz Borkowski commented

I'm testing M3 now. Looks like mappings work properly with the following setup:

@Controller
@RequestMapping("/projects")
public class ProjectEndpoint {

   @RequestMapping(method=GET)
   public HttpResponse list() {...}
   
   @RequestMapping(method=GET, value="{projectName}")
   public HttpResponse get(@PathVariable String projectName) {...}
}

Request for URLs: /projects, /projects/, /projects/myproject, /projects/myproject/ are dispatched properly.

However, if you add leading slash at method level, like this:

@Controller
@RequestMapping("/projects")
public class ProjectEndpoint {

   @RequestMapping(method=GET)
   public HttpResponse list() {...}
   
   @RequestMapping(method=GET, value="/{projectName}")
   public HttpResponse get(@PathVariable String projectName) {...}
}

then for request /projects and /projects/ the exception is thrown:
java.lang.IllegalStateException: Could not find @PathVariable [projectName] in @RequestMapping
Looks like the request is dispatched to the get(), instead of list(), method. A bug?
It touches point #2 in my comments above: what if method-level starts with slash - is it relative to type level then, or not? Both make sense.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented May 7, 2009

Arjen Poutsma commented

This seems to be the same as the issue filed by Keith: #10396. Will fix asap.

As for the semantics for method-level annotations starting with a slash: it is supposed to be relative to the type level. So in your example, the full URL handled is /projects/{projectName}. To me, that seemed to be the most natural solution.

@spring-projects-issues
Copy link
Collaborator Author

Grzegorz Borkowski commented

Yes, I would also prefer it be solved this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants