diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index e5c8ba0afd1..6afdcaf134c 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -49,7 +49,7 @@ jobs: maven-args: > --define maven.test.skip=true --define sonar.skip=true - --projects !java-checks-test-sources/aws,!java-checks-test-sources/default,!java-checks-test-sources/java-17,!java-checks-test-sources/spring-3.2,!java-checks-test-sources/spring-web-4.0 + --projects !java-checks-test-sources/aws,!java-checks-test-sources/default,!java-checks-test-sources/java-17,!java-checks-test-sources/spring-3.2,!java-checks-test-sources/spring-web-4.0,!java-checks-test-sources/spring-web-7.0 ruling-qa: strategy: diff --git a/its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java b/its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java index bfae131a57a..d6a8b189195 100644 --- a/its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java +++ b/its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java @@ -123,7 +123,7 @@ public void javaCheckTestSources() throws Exception { .setProjectName(PROJECT_NAME) .setProjectVersion("0.1.0-SNAPSHOT") .setSourceEncoding("UTF-8") - .setSourceDirs("aws/src/main/java/,default/src/main/java/,java-17/src/main/java/,spring-3.2/src/main/java/,spring-web-4.0/src/main/java/") + .setSourceDirs("aws/src/main/java/,default/src/main/java/,java-17/src/main/java/,spring-3.2/src/main/java/,spring-web-4.0/src/main/java/,spring-web-7.0/src/main/java/") .setTestDirs("default/src/test/java/,java-17/src/test/java/,test-classpath-reader/src/test/java") .setProperty("sonar.java.source", "26") // common properties diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S6856.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6856.json index 39ef299c909..c6304799015 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S6856.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6856.json @@ -1,6 +1,6 @@ { "ruleKey": "S6856", "hasTruePositives": false, - "falseNegatives": 61, + "falseNegatives": 65, "falsePositives": 0 } diff --git a/java-checks-test-sources/README.md b/java-checks-test-sources/README.md index 794750834fe..b6cf8edb0ce 100644 --- a/java-checks-test-sources/README.md +++ b/java-checks-test-sources/README.md @@ -15,7 +15,7 @@ This is the right place when a sample: * needs the classpath or dependencies prepared by `java-checks-test-sources` * should be compiled as part of one of the dedicated test-source modules * is intentionally non-compiling and should live under `src/main/files/non-compiling` -* targets a specific dedicated module such as `default`, `java-17`, `spring-3.2`, or `spring-web-4.0` +* targets a specific dedicated module such as `default`, `java-17`, `spring-3.2`, `spring-web-4.0`, or `spring-web-7.0` Prefer `java-checks/src/test/files` only for fixtures that are not supposed to belong to one of these Maven modules, for example parser-only inputs or other ad hoc verifier fixtures. diff --git a/java-checks-test-sources/pom.xml b/java-checks-test-sources/pom.xml index 3700b3c2919..9be5e5bbda1 100644 --- a/java-checks-test-sources/pom.xml +++ b/java-checks-test-sources/pom.xml @@ -22,6 +22,7 @@ java-17 spring-3.2 spring-web-4.0 + spring-web-7.0 test-classpath-reader diff --git a/java-checks-test-sources/spring-web-7.0/pom.xml b/java-checks-test-sources/spring-web-7.0/pom.xml new file mode 100644 index 00000000000..5b38e23d4de --- /dev/null +++ b/java-checks-test-sources/spring-web-7.0/pom.xml @@ -0,0 +1,126 @@ + + + 4.0.0 + + + org.sonarsource.java + java-checks-test-sources + 8.35.0-SNAPSHOT + + + spring-web-7.0 + SonarQube Java :: Checks Test Sources :: Spring Web 7.0 + + + 7.0.8 + true + true + true + true + + + + + org.springframework + spring-web + ${spring-framework.version} + provided + + + org.springframework + spring-webmvc + ${spring-framework.version} + provided + + + org.springframework + spring-expression + ${spring-framework.version} + provided + + + + + + analyze-tests + + false + + + + + + + + org.apache.maven.plugins + maven-compiler-plugin + + 17 + + + + org.simplify4u.plugins + sign-maven-plugin + + + sign-artifacts + none + + + + + maven-jar-plugin + + + default-jar + none + + + + + maven-source-plugin + + + attach-sources + none + + + + + maven-javadoc-plugin + + + attach-javadocs + none + + + + + maven-install-plugin + + + default-install + none + + + + + com.mycila + license-maven-plugin + + + + + src/main/java/** + src/test/java/** + + + + + + + + + diff --git a/java-checks-test-sources/spring-web-7.0/src/main/java/checks/spring/s6856/ApiVersionPathConfiguration.java b/java-checks-test-sources/spring-web-7.0/src/main/java/checks/spring/s6856/ApiVersionPathConfiguration.java new file mode 100644 index 00000000000..991c7930037 --- /dev/null +++ b/java-checks-test-sources/spring-web-7.0/src/main/java/checks/spring/s6856/ApiVersionPathConfiguration.java @@ -0,0 +1,20 @@ +package checks.spring.s6856; + +import java.util.function.Predicate; +import org.springframework.http.server.RequestPath; +import org.springframework.web.servlet.config.annotation.ApiVersionConfigurer; +import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; + +class ApiVersionPathConfiguration implements WebMvcConfigurer { + + @Override + public void configureApiVersioning(ApiVersionConfigurer configurer) { + Predicate versionedPaths = this::isVersionedPath; + configurer.usePathSegment(1, versionedPaths); + } + + private boolean isVersionedPath(RequestPath requestPath) { + String path = requestPath.pathWithinApplication().value(); + return path.startsWith("/api/"); + } +} diff --git a/java-checks-test-sources/spring-web-7.0/src/main/java/checks/spring/s6856/ApiVersionPathUnconditionalConfiguration.java b/java-checks-test-sources/spring-web-7.0/src/main/java/checks/spring/s6856/ApiVersionPathUnconditionalConfiguration.java new file mode 100644 index 00000000000..b5e919cc352 --- /dev/null +++ b/java-checks-test-sources/spring-web-7.0/src/main/java/checks/spring/s6856/ApiVersionPathUnconditionalConfiguration.java @@ -0,0 +1,12 @@ +package checks.spring.s6856; + +import org.springframework.web.servlet.config.annotation.ApiVersionConfigurer; +import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; + +class ApiVersionPathUnconditionalConfiguration implements WebMvcConfigurer { + + @Override + public void configureApiVersioning(ApiVersionConfigurer configurer) { + configurer.usePathSegment(1); + } +} diff --git a/java-checks-test-sources/spring-web-7.0/src/main/java/checks/spring/s6856/MissingPathVariableAnnotationCheck_ApiVersionPath.java b/java-checks-test-sources/spring-web-7.0/src/main/java/checks/spring/s6856/MissingPathVariableAnnotationCheck_ApiVersionPath.java new file mode 100644 index 00000000000..50c4244a7c9 --- /dev/null +++ b/java-checks-test-sources/spring-web-7.0/src/main/java/checks/spring/s6856/MissingPathVariableAnnotationCheck_ApiVersionPath.java @@ -0,0 +1,51 @@ +package checks.spring.s6856; + +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.RequestMapping; + +public class MissingPathVariableAnnotationCheck_ApiVersionPath { + + static class VersionedController { + + @GetMapping(value = "/api/v{apiVersion}/resource/{id}", version = "2") + String valid(@PathVariable String id) { + return id; + } + + @GetMapping(value = "/api/v{apiVersion}/resource/{id}", version = "2") // Noncompliant {{Bind template variable "id" to a method parameter.}} + String missingId() { + return ""; + } + + @GetMapping(value = "/api/v{major}.{minor}/resource", version = "2") + String multipleVariablesInVersionSegment() { + return ""; + } + + @GetMapping("/api/v{apiVersion}/unversioned") // Noncompliant {{Bind template variable "apiVersion" to a method parameter.}} + String versionlessMapping() { + return ""; + } + + @GetMapping(value = "/api/{*apiVersion}", version = "2") // Noncompliant {{Bind template variable "apiVersion" to a method parameter.}} + String catchAllVariable() { + return ""; + } + + @GetMapping(value = {"/api/v{apiVersion}/alternative", "/{apiVersion}/alternative"}, version = "2") // Noncompliant {{Bind template variable "apiVersion" to a method parameter.}} + String variableAtDifferentIndices() { + return ""; + } + } + + @RequestMapping("/api") + static class TypeLevelPathController { + + @GetMapping(value = "/v{apiVersion}/resource/{id}", version = "2") + String valid(@PathVariable String id) { + return id; + } + } + +} diff --git a/java-checks-test-sources/test-classpath-reader/src/main/java/org/sonar/java/test/classpath/TestClasspathUtils.java b/java-checks-test-sources/test-classpath-reader/src/main/java/org/sonar/java/test/classpath/TestClasspathUtils.java index b07799d8816..5735f0b4451 100644 --- a/java-checks-test-sources/test-classpath-reader/src/main/java/org/sonar/java/test/classpath/TestClasspathUtils.java +++ b/java-checks-test-sources/test-classpath-reader/src/main/java/org/sonar/java/test/classpath/TestClasspathUtils.java @@ -48,6 +48,7 @@ public final class TestClasspathUtils { public static final Module JAVA_17_MODULE = new Module("java-checks-test-sources/java-17"); public static final Module SPRING_32_MODULE = new Module("java-checks-test-sources/spring-3.2"); public static final Module SPRING_WEB_40_MODULE = new Module("java-checks-test-sources/spring-web-4.0"); + public static final Module SPRING_WEB_70_MODULE = new Module("java-checks-test-sources/spring-web-7.0"); private static final Path testJarsPath = Path.of("java-checks-test-sources/target/test-jars"); public static List getTestJars(List jars) { diff --git a/java-checks-test-sources/test-classpath-reader/src/test/java/org/sonar/java/test/classpath/TestClasspathUtilsTest.java b/java-checks-test-sources/test-classpath-reader/src/test/java/org/sonar/java/test/classpath/TestClasspathUtilsTest.java index 837c8f1a520..ecfc6662cd3 100644 --- a/java-checks-test-sources/test-classpath-reader/src/test/java/org/sonar/java/test/classpath/TestClasspathUtilsTest.java +++ b/java-checks-test-sources/test-classpath-reader/src/test/java/org/sonar/java/test/classpath/TestClasspathUtilsTest.java @@ -43,6 +43,7 @@ void test_modules_path() { assertThat(Path.of(TestClasspathUtils.JAVA_17_MODULE.getPath())).exists(); assertThat(Path.of(TestClasspathUtils.SPRING_32_MODULE.getPath())).exists(); assertThat(Path.of(TestClasspathUtils.SPRING_WEB_40_MODULE.getPath())).exists(); + assertThat(Path.of(TestClasspathUtils.SPRING_WEB_70_MODULE.getPath())).exists(); TestClasspathUtils.Module unknowModule = new TestClasspathUtils.Module("unknow"); assertThat(unknowModule.getPath()).isNull(); @@ -60,6 +61,7 @@ void test_modules_classpath() { assertThat(TestClasspathUtils.JAVA_17_MODULE.getClassPath()).hasSizeGreaterThan(5); assertThat(TestClasspathUtils.SPRING_32_MODULE.getClassPath()).hasSizeGreaterThan(15); assertThat(TestClasspathUtils.SPRING_WEB_40_MODULE.getClassPath()).hasSizeGreaterThan(5); + assertThat(TestClasspathUtils.SPRING_WEB_70_MODULE.getClassPath()).hasSizeGreaterThan(5); TestClasspathUtils.Module unknowModule = new TestClasspathUtils.Module("unknow"); assertThat(unknowModule.getClassPath()).isEmpty(); diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheck.java index 31f90cfd176..1144f4e4397 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheck.java @@ -27,16 +27,28 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import javax.annotation.Nullable; +import org.sonar.api.batch.fs.InputComponent; import org.sonar.check.Rule; import org.sonar.java.annotations.VisibleForTesting; +import org.sonar.java.model.DefaultModuleScannerContext; +import org.sonar.java.model.ExpressionUtils; +import org.sonar.java.reporting.AnalyzerMessage; import org.sonar.plugins.java.api.DependencyVersionAware; +import org.sonar.plugins.java.api.InputFileScannerContext; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.ModuleScannerContext; import org.sonar.plugins.java.api.Version; +import org.sonar.plugins.java.api.internal.EndOfAnalysis; import org.sonar.plugins.java.api.semantic.Symbol; import org.sonar.plugins.java.api.semantic.SymbolMetadata; import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.AnnotationTree; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.VariableTree; @@ -45,13 +57,23 @@ import static org.sonar.java.checks.helpers.MethodTreeUtils.isSetterLike; @Rule(key = "S6856") -public class MissingPathVariableAnnotationCheck extends IssuableSubscriptionVisitor implements DependencyVersionAware { +public class MissingPathVariableAnnotationCheck extends IssuableSubscriptionVisitor implements DependencyVersionAware, EndOfAnalysis { private static final String PATH_VARIABLE_ANNOTATION = "org.springframework.web.bind.annotation.PathVariable"; private static final String MAP = "java.util.Map"; private static final String MODEL_ATTRIBUTE_ANNOTATION = "org.springframework.web.bind.annotation.ModelAttribute"; private static final String REQUEST_MAPPING_ANNOTATION = "org.springframework.web.bind.annotation.RequestMapping"; + private static final String MVC_API_VERSION_CONFIGURER = "org.springframework.web.servlet.config.annotation.ApiVersionConfigurer"; + private static final String WEB_MVC_CONFIGURER = "org.springframework.web.servlet.config.annotation.WebMvcConfigurer"; + private static final String WEB_MVC_CONFIGURATION_SUPPORT = "org.springframework.web.servlet.config.annotation.WebMvcConfigurationSupport"; private static final String PROPERTY_PLACEHOLDER_PATTERN = "\\$\\{[^{}]*\\}"; + private static final Set AMBIGUOUS_API_VERSION_CONFIGURATION_METHODS = Set.of( + "useRequestHeader", + "useQueryParam", + "useMediaTypeParameter", + "useVersionResolver", + "setDefaultVersion"); + private static final Set MAPPING_ANNOTATIONS = Set.of( REQUEST_MAPPING_ANNOTATION, "org.springframework.web.bind.annotation.GetMapping", @@ -67,6 +89,10 @@ public class MissingPathVariableAnnotationCheck extends IssuableSubscriptionVisi private static final String BIND_PARAM_ANNOTATION = "org.springframework.web.bind.annotation.BindParam"; private SpringWebVersion springWebVersion; + private final Set apiVersionPathSegments = new HashSet<>(); + private boolean hasAmbiguousApiVersionConfiguration; + private final List pendingIssues = new ArrayList<>(); + private final List pendingMappingIssues = new ArrayList<>(); @Override public List nodesToVisit() { @@ -82,12 +108,14 @@ public void visitNode(Tree tree) { .map(MethodTree.class::cast) .toList(); + collectApiVersionPathSegments(methods); + // request @RequestMapping can be put on top of a class, path template inside it will affect all the class methods var requestMappingArguments = clazzTree.symbol().metadata().valuesForAnnotation(REQUEST_MAPPING_ANNOTATION); - Set requestMappingTemplateVariables = new HashSet<>(); + MappingInfo classMapping = MappingInfo.empty(); if (requestMappingArguments != null) { try { - requestMappingTemplateVariables = templateVariablesFromMapping(requestMappingArguments); + classMapping = mappingInfoFromMapping(requestMappingArguments); } catch (DoNotReport ignored) { return; } @@ -96,7 +124,94 @@ public void visitNode(Tree tree) { Set modelAttributeMethodParameter = extractModelAttributeMethodParameter(methods); modelAttributeMethodParameter.addAll(addInheritedModelAttributeMethodParameter(modelAttributeMethodParameter, clazzTree)); - checkParametersAndPathTemplate(methods, modelAttributeMethodParameter, requestMappingTemplateVariables); + checkParametersAndPathTemplate(methods, modelAttributeMethodParameter, classMapping); + } + + private void collectApiVersionPathSegments(List methods) { + if (springWebVersion != SpringWebVersion.START_FROM_7_0) { + return; + } + + methods.stream() + .filter(MissingPathVariableAnnotationCheck::isApiVersionConfigurationMethod) + .forEach(method -> { + Symbol configurerParameter = method.parameters().get(0).symbol(); + method.accept(new BaseTreeVisitor() { + @Override + public void visitIdentifier(IdentifierTree identifier) { + if (configurerParameter.equals(identifier.symbol()) + && !isConfigurerParameterDeclaration(identifier) + && !isDirectConfigurerReceiver(identifier)) { + hasAmbiguousApiVersionConfiguration = true; + } + super.visitIdentifier(identifier); + } + + @Override + public void visitMethodInvocation(MethodInvocationTree invocation) { + collectApiVersionConfiguration(invocation, configurerParameter); + super.visitMethodInvocation(invocation); + } + }); + }); + } + + private static boolean isApiVersionConfigurationMethod(MethodTree method) { + if (!"configureApiVersioning".equals(method.simpleName().name()) || method.parameters().size() != 1 + || !method.parameters().get(0).type().symbolType().is(MVC_API_VERSION_CONFIGURER)) { + return false; + } + + Type ownerType = method.symbol().owner().type(); + return ownerType.isSubtypeOf(WEB_MVC_CONFIGURER) || ownerType.isSubtypeOf(WEB_MVC_CONFIGURATION_SUPPORT); + } + + private void collectApiVersionConfiguration(MethodInvocationTree invocation, Symbol configurerParameter) { + Symbol.MethodSymbol method = invocation.methodSymbol(); + if (!method.owner().type().is(MVC_API_VERSION_CONFIGURER) || !isInvokedOnParameter(invocation, configurerParameter)) { + return; + } + + if ("usePathSegment".equals(method.name()) && !invocation.arguments().isEmpty()) { + Optional index = invocation.arguments().get(0).asConstant(Integer.class).filter(value -> value >= 0); + if (index.isPresent()) { + apiVersionPathSegments.add(index.get()); + } else { + hasAmbiguousApiVersionConfiguration = true; + } + } else if (AMBIGUOUS_API_VERSION_CONFIGURATION_METHODS.contains(method.name()) + || isPossiblyOptionalVersionConfiguration(invocation, method)) { + hasAmbiguousApiVersionConfiguration = true; + } + } + + private static boolean isInvokedOnParameter(MethodInvocationTree invocation, Symbol configurerParameter) { + if (!(invocation.methodSelect() instanceof MemberSelectExpressionTree memberSelect)) { + return false; + } + ExpressionTree receiver = ExpressionUtils.skipParentheses(memberSelect.expression()); + while (receiver instanceof MethodInvocationTree receiverInvocation + && receiverInvocation.methodSelect() instanceof MemberSelectExpressionTree receiverMethodSelect) { + receiver = ExpressionUtils.skipParentheses(receiverMethodSelect.expression()); + } + return receiver instanceof IdentifierTree identifier && configurerParameter.equals(identifier.symbol()); + } + + private static boolean isConfigurerParameterDeclaration(IdentifierTree identifier) { + return identifier.parent() instanceof VariableTree variable && variable.simpleName() == identifier; + } + + private static boolean isDirectConfigurerReceiver(IdentifierTree identifier) { + return identifier.parent() instanceof MemberSelectExpressionTree memberSelect + && memberSelect.expression() == identifier + && memberSelect.parent() instanceof MethodInvocationTree invocation + && invocation.methodSelect() == memberSelect + && invocation.methodSymbol().owner().type().is(MVC_API_VERSION_CONFIGURER); + } + + private static boolean isPossiblyOptionalVersionConfiguration(MethodInvocationTree invocation, Symbol.MethodSymbol method) { + return "setVersionRequired".equals(method.name()) + && (invocation.arguments().isEmpty() || !invocation.arguments().get(0).asConstant(Boolean.class).orElse(false)); } private static Set extractModelAttributeMethodParameter(List methods){ @@ -133,11 +248,11 @@ private static Set addInheritedModelAttributeMethodParameter(Set return modelAttributeMethodParameters; } - private void checkParametersAndPathTemplate(List methods, Set modelAttributeMethodParameter, Set requestMappingTemplateVariables) { + private void checkParametersAndPathTemplate(List methods, Set modelAttributeMethodParameter, MappingInfo classMapping) { for (var method : methods) { if (!method.symbol().metadata().isAnnotatedWith(MODEL_ATTRIBUTE_ANNOTATION)) { try { - checkParametersAndPathTemplate(method, modelAttributeMethodParameter, requestMappingTemplateVariables); + checkParametersAndPathTemplate(method, modelAttributeMethodParameter, classMapping); } catch (DoNotReport ignored) { // We don't want to report when semantics is broken or we were unable to parse the path template } @@ -145,7 +260,7 @@ private void checkParametersAndPathTemplate(List methods, Set modelAttributeMethodParameters, Set requestMappingTemplateVars) { + private void checkParametersAndPathTemplate(MethodTree method, Set modelAttributeMethodParameters, MappingInfo classMapping) { // we find path variable annotations and extract the name // example find : @PathVariable() String id and extract id List methodParameters = new ArrayList<>(); @@ -165,7 +280,7 @@ private void checkParametersAndPathTemplate(MethodTree method, Set model // we find mapping annotation and extract path // example find @GetMapping("/{id}") and extract "/{id}" - List>> templateVariables = new ArrayList<>(); + List templateVariables = new ArrayList<>(); for (var ann : method.modifiers().annotations()) { if (ann.symbolType().isUnknown()) { throw new DoNotReport(); @@ -177,19 +292,19 @@ private void checkParametersAndPathTemplate(MethodTree method, Set model continue; } - templateVariables.add(new UriInfo<>(ann, templateVariablesFromMapping(values))); + templateVariables.add(new UriInfo(ann, mappingInfoFromMapping(values))); } // we handle the case where a path variable doesn't match to uri parameter (/{aParam}/) Set allTemplateVariables = templateVariables.stream() - .flatMap(uri -> uri.value().stream()) + .flatMap(uri -> uri.mapping().templateVariables().stream()) .collect(Collectors.toSet()); - allTemplateVariables.addAll(requestMappingTemplateVars); + allTemplateVariables.addAll(classMapping.templateVariables()); methodParameters.stream() .filter(v -> !allTemplateVariables.contains(v.value())) .filter(v -> !v.parameter().symbol().type().is(MAP)) - .forEach(v -> reportIssue(v.parameter(), String.format("Bind method parameter \"%s\" to a template variable.", v.value()))); + .forEach(v -> addPendingIssue(v.parameter(), String.format("Bind method parameter \"%s\" to a template variable.", v.value()))); if (containsTypeMapAsParameter(method)) { /* @@ -209,17 +324,143 @@ private void checkParametersAndPathTemplate(MethodTree method, Set model allPathVariables.addAll(extractClassAndRecordProperties(method)); templateVariables.stream() - .filter(uri -> !allPathVariables.containsAll(uri.value())) + .filter(uri -> !allPathVariables.containsAll(uri.mapping().templateVariables())) .forEach(uri -> { - Set unbind = new HashSet<>(uri.value()); + Set unbind = new HashSet<>(uri.mapping().templateVariables()); unbind.removeAll(allPathVariables); - reportIssue( - uri.request(), - "Bind template variable \"" + String.join("\", \"", unbind) + "\" to a method parameter."); + reportOrDeferMissingTemplateVariables(uri, classMapping, unbind); }); } + private void reportOrDeferMissingTemplateVariables(UriInfo uri, MappingInfo classMapping, Set unboundVariables) { + if (springWebVersion != SpringWebVersion.START_FROM_7_0) { + addPendingIssue(uri.request(), missingTemplateVariablesMessage(unboundVariables)); + return; + } + + pendingMappingIssues.add(new PendingMappingIssue( + context.getInputFile(), + AnalyzerMessage.textSpanFor(uri.request()), + Set.copyOf(unboundVariables), + combinePaths(classMapping.paths(), uri.mapping().paths()), + classMapping.versioned() || uri.mapping().versioned())); + } + + private void addPendingIssue(Tree tree, String message) { + pendingIssues.add(new AnalyzerMessage(this, context.getInputFile(), AnalyzerMessage.textSpanFor(tree), message, 0)); + } + + private static String missingTemplateVariablesMessage(Set variables) { + return "Bind template variable \"" + String.join("\", \"", variables) + "\" to a method parameter."; + } + + private static List combinePaths(List classPaths, List methodPaths) { + List combinedPaths = new ArrayList<>(); + for (String classPath : classPaths) { + for (String methodPath : methodPaths) { + combinedPaths.add(combinePath(classPath, methodPath)); + } + } + return combinedPaths; + } + + private static String combinePath(String classPath, String methodPath) { + String left = classPath.replaceAll("/+$", ""); + String right = methodPath.replaceAll("^/+", ""); + if (left.isEmpty()) { + return right.isEmpty() ? "" : "/" + right; + } + return right.isEmpty() ? left : left + "/" + right; + } + + @Override + public boolean scanWithoutParsing(InputFileScannerContext inputFileScannerContext) { + // Cross-file configuration discovery is only needed for Spring Web 7 and later. + return springWebVersion != SpringWebVersion.START_FROM_7_0; + } + + @Override + public void endOfAnalysis(ModuleScannerContext moduleContext) { + DefaultModuleScannerContext defaultContext = (DefaultModuleScannerContext) moduleContext; + pendingIssues.forEach(defaultContext::reportIssue); + for (PendingMappingIssue pendingIssue : pendingMappingIssues) { + Set unboundVariables = new HashSet<>(pendingIssue.unboundVariables()); + if (pendingIssue.versioned() && !hasAmbiguousApiVersionConfiguration && apiVersionPathSegments.size() == 1) { + unboundVariables.removeIf(variable -> isReadByPathVersionResolver(variable, pendingIssue.paths(), apiVersionPathSegments)); + } + if (!unboundVariables.isEmpty()) { + defaultContext.reportIssue(new AnalyzerMessage( + this, + pendingIssue.inputComponent(), + pendingIssue.textSpan(), + missingTemplateVariablesMessage(unboundVariables), + 0)); + } + } + + apiVersionPathSegments.clear(); + hasAmbiguousApiVersionConfiguration = false; + pendingIssues.clear(); + pendingMappingIssues.clear(); + } + + private static boolean isReadByPathVersionResolver(String variable, List paths, Set configuredSegments) { + if (configuredSegments.isEmpty()) { + return false; + } + + boolean found = false; + for (String path : paths) { + if (path.contains("${")) { + return false; + } + List segments = pathSegments(path); + for (int index = 0; index < segments.size(); index++) { + String segment = segments.get(index); + if (PathPatternParser.parsePathVariables(segment).contains(variable)) { + found = true; + if (!configuredSegments.contains(index) || segment.contains("{*" + variable + "}")) { + return false; + } + } + } + } + return found; + } + + private static List pathSegments(String path) { + List segments = new ArrayList<>(); + int segmentStart = 0; + int braces = 0; + boolean escaped = false; + for (int i = 0; i < path.length(); i++) { + char current = path.charAt(i); + if (escaped) { + escaped = false; + continue; + } + if (current == '\\') { + escaped = true; + } else if (current == '{') { + braces++; + } else if (current == '}' && braces > 0) { + braces--; + } else if (current == '/' && braces == 0) { + addPathSegment(path, segmentStart, i, segments); + segmentStart = i + 1; + } + } + addPathSegment(path, segmentStart, path.length(), segments); + return segments; + } + + private static void addPathSegment(String path, int start, int end, List segments) { + if (start < end) { + segments.add(path.substring(start, end)); + } + } + private static boolean containsTypeMapAsParameter(MethodTree method) { return method.parameters().stream() .filter(parameter -> parameter.symbol().metadata().isAnnotatedWith(PATH_VARIABLE_ANNOTATION)) @@ -229,22 +470,23 @@ private static boolean containsTypeMapAsParameter(MethodTree method) { }); } - private static Set templateVariablesFromMapping(List values) { + private static MappingInfo mappingInfoFromMapping(List values) { Map nameToValue = values.stream() .collect(Collectors.toMap(SymbolMetadata.AnnotationValue::name, SymbolMetadata.AnnotationValue::value)); List path = arrayOrString(nameToValue.get("path")); List value = arrayOrString(nameToValue.get("value")); - - if (path != null || value != null) { - List paths = path != null ? path : value; - return paths.stream() - .map(MissingPathVariableAnnotationCheck::removePropertyPlaceholder) - .map(PathPatternParser::parsePathVariables) - .flatMap(Collection::stream) - .collect(Collectors.toSet()); - } else { - return Set.of(); + List paths = path != null ? path : value; + if (paths == null || paths.isEmpty()) { + paths = List.of(""); } + + Set templateVariables = paths.stream() + .map(MissingPathVariableAnnotationCheck::removePropertyPlaceholder) + .map(PathPatternParser::parsePathVariables) + .flatMap(Collection::stream) + .collect(Collectors.toSet()); + boolean versioned = nameToValue.get("version") instanceof String version && !version.isBlank(); + return new MappingInfo(List.copyOf(paths), templateVariables, versioned); } private static ParameterInfo extractPathMethodParameters(VariableTree parameter, List arguments) { @@ -279,7 +521,22 @@ static class DoNotReport extends RuntimeException { private record ParameterInfo(VariableTree parameter, String value) { } - private record UriInfo(AnnotationTree request, A value) { + + private record UriInfo(AnnotationTree request, MappingInfo mapping) { + } + + private record MappingInfo(List paths, Set templateVariables, boolean versioned) { + private static MappingInfo empty() { + return new MappingInfo(List.of(""), Set.of(), false); + } + } + + private record PendingMappingIssue( + InputComponent inputComponent, + AnalyzerMessage.TextSpan textSpan, + Set unboundVariables, + List paths, + boolean versioned) { } private static String removePropertyPlaceholder(String path){ @@ -530,11 +787,15 @@ public boolean isCompatibleWithDependencies(Function> } private static SpringWebVersion getSpringWebVersion(Version springWebVersion) { - return (springWebVersion.isLowerThan("5.3") ? SpringWebVersion.LESS_THAN_5_3 : SpringWebVersion.START_FROM_5_3); + if (springWebVersion.isLowerThan("5.3")) { + return SpringWebVersion.LESS_THAN_5_3; + } + return springWebVersion.isLowerThan("7.0") ? SpringWebVersion.FROM_5_3_TO_6_X : SpringWebVersion.START_FROM_7_0; } private enum SpringWebVersion { LESS_THAN_5_3, - START_FROM_5_3; + FROM_5_3_TO_6_X, + START_FROM_7_0; } } diff --git a/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_DynamicIndex.java b/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_DynamicIndex.java new file mode 100644 index 00000000000..01f21e7d2de --- /dev/null +++ b/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_DynamicIndex.java @@ -0,0 +1,29 @@ +package files.checks.spring; + +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.servlet.config.annotation.ApiVersionConfigurer; +import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; + +public class MissingPathVariableAnnotationCheck_DynamicIndex { + + static class VersionedController { + + @GetMapping(value = "/api/v{apiVersion}/resource", version = "2") // Noncompliant + String ambiguousVersionSegment() { + return ""; + } + } + + static class ApiVersionConfiguration implements WebMvcConfigurer { + + @Override + public void configureApiVersioning(ApiVersionConfigurer configurer) { + configurer.usePathSegment(1); + configurer.usePathSegment(dynamicIndex()); + } + + private static int dynamicIndex() { + return 2; + } + } +} diff --git a/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_EscapingConfiguration.java b/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_EscapingConfiguration.java new file mode 100644 index 00000000000..0ea1e25e023 --- /dev/null +++ b/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_EscapingConfiguration.java @@ -0,0 +1,29 @@ +package files.checks.spring; + +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.servlet.config.annotation.ApiVersionConfigurer; +import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; + +public class MissingPathVariableAnnotationCheck_EscapingConfiguration { + + static class VersionedController { + + @GetMapping(value = "/api/{ordinaryVariable}/resource", version = "2") // Noncompliant + String missingOrdinaryVariable() { + return ""; + } + } + + static class ApiVersionConfiguration implements WebMvcConfigurer { + + @Override + public void configureApiVersioning(ApiVersionConfigurer configurer) { + configurer.usePathSegment(1, path -> path.value().startsWith("/versioned/")); + configureFallback(configurer); + } + + private static void configureFallback(ApiVersionConfigurer configurer) { + configurer.useRequestHeader("API-Version"); + } + } +} diff --git a/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_InactiveConfiguration.java b/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_InactiveConfiguration.java new file mode 100644 index 00000000000..bcc26be884e --- /dev/null +++ b/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_InactiveConfiguration.java @@ -0,0 +1,23 @@ +package files.checks.spring; + +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.servlet.config.annotation.ApiVersionConfigurer; +import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; + +public class MissingPathVariableAnnotationCheck_InactiveConfiguration { + + static class VersionedController { + + @GetMapping(value = "/api/{ordinaryVariable}/resource", version = "2") // Noncompliant + String missingOrdinaryVariable() { + return ""; + } + } + + static class ApiVersionConfiguration implements WebMvcConfigurer { + + void unusedHelper(ApiVersionConfigurer configurer) { + configurer.usePathSegment(1); + } + } +} diff --git a/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_MultipleIndices.java b/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_MultipleIndices.java new file mode 100644 index 00000000000..766fb584715 --- /dev/null +++ b/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_MultipleIndices.java @@ -0,0 +1,25 @@ +package files.checks.spring; + +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.servlet.config.annotation.ApiVersionConfigurer; +import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; + +public class MissingPathVariableAnnotationCheck_MultipleIndices { + + static class VersionedController { + + @GetMapping(value = "/api/v{apiVersion}/resource", version = "2") // Noncompliant + String ambiguousVersionSegment() { + return ""; + } + } + + static class ApiVersionConfiguration implements WebMvcConfigurer { + + @Override + public void configureApiVersioning(ApiVersionConfigurer configurer) { + configurer.usePathSegment(0); + configurer.usePathSegment(1); + } + } +} diff --git a/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_NonPathApiVersion.java b/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_NonPathApiVersion.java new file mode 100644 index 00000000000..4424eab3637 --- /dev/null +++ b/java-checks/src/test/files/checks/spring/MissingPathVariableAnnotationCheck_NonPathApiVersion.java @@ -0,0 +1,26 @@ +package files.checks.spring; + +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.servlet.config.annotation.ApiVersionConfigurer; +import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; + +public class MissingPathVariableAnnotationCheck_NonPathApiVersion { + + static class VersionedController { + + @GetMapping(value = "/api/{ordinaryVariable}/resource/{id}", version = "2") // Noncompliant {{Bind template variable "ordinaryVariable" to a method parameter.}} + String missingOrdinaryVariable(@PathVariable String id) { + return id; + } + } + + static class ApiVersionConfiguration implements WebMvcConfigurer { + + @Override + public void configureApiVersioning(ApiVersionConfigurer configurer) { + configurer.usePathSegment(1, path -> path.value().startsWith("/versioned/")); + configurer.useRequestHeader("API-Version"); + } + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheckTest.java index d40f8fdba75..84b7d03ba82 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheckTest.java @@ -17,6 +17,7 @@ package org.sonar.java.checks.spring; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.stream.Stream; @@ -38,12 +39,29 @@ import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPathInModule; import static org.sonar.java.test.classpath.TestClasspathUtils.SPRING_32_MODULE; +import static org.sonar.java.test.classpath.TestClasspathUtils.SPRING_WEB_70_MODULE; class MissingPathVariableAnnotationCheckTest { private static final String TEST_SOURCE_FILE = mainCodeSourcesPath("checks/spring/s6856/MissingPathVariableAnnotationCheck_PathVariable.java"); private static final String TEST_SOURCE_FILE_MODEL_ATTRIBUTE = mainCodeSourcesPath("checks/spring/s6856/MissingPathVariableAnnotationCheck_ModelAttribute.java"); private static final String SETTER_PROPERTIES_TEST_FILE = mainCodeSourcesPath("checks/spring/s6856/ExtractSetterPropertiesTestData.java"); private static final String RECORD_COMPONENTS_TEST_FILE = mainCodeSourcesPathInModule(SPRING_32_MODULE, "checks/ExtractRecordPropertiesTestData.java"); + private static final String API_VERSION_PATH_TEST_FILE = mainCodeSourcesPathInModule( + SPRING_WEB_70_MODULE, "checks/spring/s6856/MissingPathVariableAnnotationCheck_ApiVersionPath.java"); + private static final String API_VERSION_PATH_CONFIGURATION_FILE = mainCodeSourcesPathInModule( + SPRING_WEB_70_MODULE, "checks/spring/s6856/ApiVersionPathConfiguration.java"); + private static final String API_VERSION_PATH_UNCONDITIONAL_CONFIGURATION_FILE = mainCodeSourcesPathInModule( + SPRING_WEB_70_MODULE, "checks/spring/s6856/ApiVersionPathUnconditionalConfiguration.java"); + private static final String NON_PATH_API_VERSION_TEST_FILE = + "src/test/files/checks/spring/MissingPathVariableAnnotationCheck_NonPathApiVersion.java"; + private static final String INACTIVE_API_VERSION_CONFIGURATION_TEST_FILE = + "src/test/files/checks/spring/MissingPathVariableAnnotationCheck_InactiveConfiguration.java"; + private static final String MULTIPLE_API_VERSION_INDICES_TEST_FILE = + "src/test/files/checks/spring/MissingPathVariableAnnotationCheck_MultipleIndices.java"; + private static final String ESCAPING_API_VERSION_CONFIGURATION_TEST_FILE = + "src/test/files/checks/spring/MissingPathVariableAnnotationCheck_EscapingConfiguration.java"; + private static final String DYNAMIC_API_VERSION_INDEX_TEST_FILE = + "src/test/files/checks/spring/MissingPathVariableAnnotationCheck_DynamicIndex.java"; private static final JavaFileScanner check = new MissingPathVariableAnnotationCheck(); private static final Map testTypes = new HashMap<>(); @@ -123,6 +141,69 @@ void test_classes_and_records() { .verifyIssues(); } + @Test + void test_spring_7_path_api_version() { + CheckVerifier.newVerifier() + .onFiles(List.of(API_VERSION_PATH_TEST_FILE, API_VERSION_PATH_CONFIGURATION_FILE)) + .withCheck(new MissingPathVariableAnnotationCheck()) + .withClassPath(SPRING_WEB_70_MODULE.getClassPath()) + .verifyIssues(); + } + + @Test + void test_spring_7_unconditional_path_api_version() { + CheckVerifier.newVerifier() + .onFiles(List.of(API_VERSION_PATH_TEST_FILE, API_VERSION_PATH_UNCONDITIONAL_CONFIGURATION_FILE)) + .withCheck(new MissingPathVariableAnnotationCheck()) + .withClassPath(SPRING_WEB_70_MODULE.getClassPath()) + .verifyIssues(); + } + + @Test + void test_spring_7_path_api_version_with_alternative_resolver() { + CheckVerifier.newVerifier() + .onFile(NON_PATH_API_VERSION_TEST_FILE) + .withCheck(new MissingPathVariableAnnotationCheck()) + .withClassPath(SPRING_WEB_70_MODULE.getClassPath()) + .verifyIssues(); + } + + @Test + void test_spring_7_ignores_inactive_api_version_configuration() { + CheckVerifier.newVerifier() + .onFile(INACTIVE_API_VERSION_CONFIGURATION_TEST_FILE) + .withCheck(new MissingPathVariableAnnotationCheck()) + .withClassPath(SPRING_WEB_70_MODULE.getClassPath()) + .verifyIssues(); + } + + @Test + void test_spring_7_multiple_path_api_version_indices_are_ambiguous() { + CheckVerifier.newVerifier() + .onFile(MULTIPLE_API_VERSION_INDICES_TEST_FILE) + .withCheck(new MissingPathVariableAnnotationCheck()) + .withClassPath(SPRING_WEB_70_MODULE.getClassPath()) + .verifyIssues(); + } + + @Test + void test_spring_7_configurer_escape_is_ambiguous() { + CheckVerifier.newVerifier() + .onFile(ESCAPING_API_VERSION_CONFIGURATION_TEST_FILE) + .withCheck(new MissingPathVariableAnnotationCheck()) + .withClassPath(SPRING_WEB_70_MODULE.getClassPath()) + .verifyIssues(); + } + + @Test + void test_spring_7_dynamic_path_api_version_index_is_ambiguous() { + CheckVerifier.newVerifier() + .onFile(DYNAMIC_API_VERSION_INDEX_TEST_FILE) + .withCheck(new MissingPathVariableAnnotationCheck()) + .withClassPath(SPRING_WEB_70_MODULE.getClassPath()) + .verifyIssues(); + } + @Test void test_without_semantic() { CheckVerifier.newVerifier() diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6856.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6856.html index 8351dde38ea..cd893b08202 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6856.html +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6856.html @@ -11,8 +11,12 @@

Why is this an issue?

variables to properties that have setter methods (including Lombok-generated setters). For records (Spring Web 5.3+), Spring binds path variables to record components through their accessor methods. The @BindParam annotation can be used on record components to specify custom binding names.

-

This rule will raise an issue if a method has a path template with a placeholder, but no corresponding @PathVariable, matching class -property, or matching record component, or vice-versa.

+

Spring Framework 7 also supports resolving an API version from a path segment. When +ApiVersionConfigurer.usePathSegment(...) is configured, the API version resolver reads the raw request path segment independently of +controller argument binding. A placeholder in an explicitly versioned mapping can therefore be structural and does not need to be exposed as a +handler method parameter.

+

Except for a placeholder recognized as an API-version path segment, this rule will raise an issue if a method has a path template with a +placeholder, but no corresponding @PathVariable, matching class property, or matching record component, or vice-versa.

How to fix it

Code examples

Noncompliant code example

@@ -133,9 +137,9 @@

Documentation

Framework API - PutMapping
  • Spring Framework API - DeleteMapping
  • +
  • Spring Framework - API Version
  • Articles & blog posts

    -