After upgrading to SonarQube 10.4, I have a false positive on java:S6856 when using a @ModelAttribute to store a @PathVariable resolved item for all @RequestMapping methods in the controller.
Here is a reproducer :
@ModelAttribute("viewCfg")
public ViewConfig getViewConfiguration(@PathVariable("view") final String viewName)
{
return viewConfigService.resolveByName(viewName);
}
@GetMapping("/{view}") // FP here : "Bind path variable "view" to a method parameter"
public String list(@ModelAttribute("viewCfg") final ViewConfig viewConfig)
{
return "";
}
Hey @alec,
Thanks again for pushing the new rule in its corner
Unfortunately for this case, I am unable to reproduce the issue with the snippet you shared. In addition, from reading the implementation, we explicitly avoid raising on path variables that rely on the annotation @ModelAttribute for the value to be injected.
But it happens that when covering different annotation set we failed to collect some. A few questions that might help us sort this out:
Which version of Spring are you using?
Is @org.springframework.web.bind.annotation.ModelAttribute the annotation you are referring to?
Sorry, I simplified it a bit too much and forgot there is inheritance involved.
class A {
@ModelAttribute("viewCfg")
public ViewConfig getViewConfiguration(@PathVariable("view") final String viewName) {
return viewConfigService.resolveByName(viewName);
}
}
class B extends A {
@GetMapping("/{view}") // FP here : "Bind path variable "view" to a method parameter"
public String list(@ModelAttribute("viewCfg") final ViewConfig viewConfig) {
return "";
}
}
I suppose the rule is only checking for ModelAttributes in the current class and not in the class hierarchy.
Spring framework/mvc version 6.1.3
I’m effectively using this annotation.
I also suppose the rule can have the same problem with a ModelAttribute populated through a ControllerAdvice but this case seems harder than the inheritance one and a rare use case.
Hi @alec,
Thank you for coming back to this topic with an updated example. I can now reproduce the issue with the inheritance.
I created a ticket to track the issue. Let me know if you see missing information there that could be helpful when we start working on a fix.