SONARJAVA-5324 @PathVariable must have path template placeholder #5016
Conversation
erwan-serandour
requested review from
leonardo-pilastri-sonarsource
and removed request for
leonardo-pilastri-sonarsource
| .withCheck(new MissingPathVariableAnnotationCheck()) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| @Test | ||
| void test_without_semantic(){ |
There was a problem hiding this comment.
Since we are adding a new method, it might be the good time to extract the filename and the check instance into 2 constants
| .collect(Collectors.toSet()); | ||
| pathVariables.stream() | ||
| .filter(v -> !allUriParameters.contains(v.value())) | ||
| .filter(v -> !v.parameter().symbol().type().is(MAP)) |
There was a problem hiding this comment.
Maybe this is material for another rule, but my observation is that whatever type of Map you provide as a @PathVariable (Map<Long, Long>, Map<Double,Exception>), spring will always convert it to a Map<String, String> and map all the path variables inside. Maybe this rule is not the right place for this, but I wanted to mention this (we could have a dedicated rule about specifying Map<String, String>, as it's confusing otherwise)
| pathVariables.stream() | ||
| .filter(v -> !allUriParameters.contains(v.value())) | ||
| .filter(v -> !v.parameter().symbol().type().is(MAP)) | ||
| .forEach(v -> reportIssue(v.parameter(), String.format("Bind path variable \"%s\" to a path parameter.", v.value()))); |
There was a problem hiding this comment.
I think the messages we are attaching to issues are getting a little bit confusing because of the 2-ways relation between method parameters and URI template variables.
From the spring docs it appears to me that we should refer to something like the id in /path/{id} as a URI template variable, maybe template variable for simplicity, and to the corresponding @PathVariable String id simply as the method parameter, or maybe the PathVariable argument?
So I suggest we refactor the issue messages following this naming convention, because I see for instance:
Bind path variable "id" to a method parameter. and in the other case:
Bind path variable "aVar" to a path parameter. and I think it can be confusing.
|
SONARJAVA-5324