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

Allow @PathVariable to be optional [SPR-14646] #19212

Closed
spring-projects-issues opened this issue Aug 31, 2016 · 8 comments
Closed

Allow @PathVariable to be optional [SPR-14646] #19212

spring-projects-issues opened this issue Aug 31, 2016 · 8 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Aug 31, 2016

Ulrich Hecht opened SPR-14646 and commented

@PathVariable's named-value-method-argument-resolver (PathVariableMethodArgumentResolver) defines it always as 'required'. In general this makes sense, because if you use @PathVariable then the path variable is expected to be available in the @RequestMapping's path value.

But consider the usage of @PathVariable in @ModelAttribute annotated methods. Those methods are called for all handlers and not all handlers provide the same path variables.

In my case I have a controller in which 4 of 5 methods provide a specific path variable. In order to avoid duplicate code I collect view variables in @ModelAttribute-methods.

This is, how it would look like if @PathVariable had a "required"-attribute:

@RequestMapping("/activity-recordings")
public String getActivityRecordingsAction(/*...*/) { /* This method doesn't have and need path variables */ }

@RequestMapping("/activity-recordings/{purchasePositionNumber}-{index}")
public String getActivityRecordingAction(/*...*/) { /* ... */ }

@PostMapping("/activity-recordings/{purchasePositionNumber}-{index}")
public String updateActivityRecordingAction(/*...*/) { /* ... */ }

@RequestMapping("/activity-recordings/{purchasePositionNumber}-new")
public String newActivityRecordingAction(/*...*/) { /* ... */ }

@PostMapping("/activity-recordings/{purchasePositionNumber}-new")
public String addActivityRecordingAction(/*...*/) { /* ... */ }

@ModelAttribute("activityRecordingPriceLimit")
public Double getActivityRecordingPriceLimit(@PathVariable(name = "purchasePositionNumber", required = "false") String purchasePositionNumber) {
   if (purchasePositionNumber == null) return null;
   /* Some code calculating the value. I don't want to duplicate it 4 times */
}

/* Lots of other @ModelAttribute methods. If I cannot use them I will need to have a block of multiple "modelMap.put(...)" lines that I need to duplicate in the last 4 request handlers */

At the moment, this code will throw a MissingPathVariableException when the first handler is called. My workaround is to define the @ModelAttribute method's argument as Map:

@ModelAttribute("activityRecordingPriceLimit")
public Double getActivityRecordingPriceLimit(@PathVariable Map<String, String> pathVariableMap) {
/* Look up required path variable in the map */
}

However, I don't like this solution.


Affects: 4.3.2

Issue Links:

  • #16171 Provide @ModelAttribute(required="false") for session attributes
  • #18468 Convenient access to session and request attributes in controller methods
  • #10172 Using PathVariable in ModelAttribute method
@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 31, 2016

Juergen Hoeller commented

I've added basic support for a required flag and verified that our java.util.Optional support works for @PathVariable as well. This is in sync with 4.3's @RequestAttribute and @SessionAttribute now, both of which provide required support but no defaultValue.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 31, 2016

Ulrich Hecht commented

Wow, that was fast (y) Thank you! :)

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 31, 2016

Ulrich Hecht commented

This seems to be related: #10172

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 31, 2016

Juergen Hoeller commented

BTW, it would be great if you could give this an early try against the latest 4.3.3.BUILD-SNAPSHOT...

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 31, 2016

Ulrich Hecht commented

Built my application against the snapshot 4.3.3 from today. Works perfectly (y)

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 31, 2016

Juergen Hoeller commented

Great to hear! Thanks for the immediate feedback.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 1, 2016

Rossen Stoyanchev commented

This seems a straight-forward case for @ModelAttribute methods.

For @RequestMapping the required flag won't ever make a difference. In other words for anyone looking at the flag it might seem like "/foo/{bar}" might match "/foo". Perhaps worth clearing this up more explicitly in the Javadoc what this flag is intended for and that it has no impact whatsoever on @RequestMapping methods which is the case as far as I can see.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 1, 2016

Juergen Hoeller commented

I guess it could make a difference if @RequestMapping specifies several paths to match to, e.g. one with the path variable and one without? Generally agreed that it won't make a difference for typical canonical REST mappings...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants