diff --git a/pom.xml b/pom.xml index a6dc167a03..1ba673449d 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-commons - 4.0.0-SNAPSHOT + 4.0.x-GH-3270-SNAPSHOT Spring Data Core Core Spring concepts underpinning every Spring Data module. diff --git a/src/main/java/org/springframework/data/repository/aot/generate/AotQueryMethodGenerationContext.java b/src/main/java/org/springframework/data/repository/aot/generate/AotQueryMethodGenerationContext.java index 0dd0806637..abe49df347 100644 --- a/src/main/java/org/springframework/data/repository/aot/generate/AotQueryMethodGenerationContext.java +++ b/src/main/java/org/springframework/data/repository/aot/generate/AotQueryMethodGenerationContext.java @@ -19,12 +19,10 @@ import java.lang.reflect.Method; import java.util.ArrayList; import java.util.List; -import java.util.Map.Entry; import javax.lang.model.element.Modifier; import org.jspecify.annotations.Nullable; - import org.springframework.core.ResolvableType; import org.springframework.core.annotation.MergedAnnotation; import org.springframework.core.annotation.MergedAnnotationSelectors; @@ -54,6 +52,7 @@ public class AotQueryMethodGenerationContext { private final AotRepositoryFragmentMetadata targetTypeMetadata; private final MethodMetadata targetMethodMetadata; private final CodeBlocks codeBlocks; + private final VariableNameFactory variableNameFactory; AotQueryMethodGenerationContext(RepositoryInformation repositoryInformation, Method method, QueryMethod queryMethod, AotRepositoryFragmentMetadata targetTypeMetadata) { @@ -64,6 +63,7 @@ public class AotQueryMethodGenerationContext { this.repositoryInformation = repositoryInformation; this.targetTypeMetadata = targetTypeMetadata; this.targetMethodMetadata = new MethodMetadata(repositoryInformation, method); + this.variableNameFactory = LocalVariableNameFactory.forMethod(targetMethodMetadata); this.codeBlocks = new CodeBlocks(targetTypeMetadata); } @@ -127,6 +127,16 @@ public TypeName getReturnTypeName() { return TypeName.get(getReturnType().getType()); } + /** + * Suggest naming clash free variant for the given intended variable name within the local method context. + * + * @param variableName the intended variable name. + * @return the suggested VariableName + */ + public String suggestLocalVariableName(String variableName) { + return variableNameFactory.generateName(variableName); + } + /** * Returns the required parameter name for the {@link Parameter#isBindable() bindable parameter} at the given * {@code parameterIndex} or throws {@link IllegalArgumentException} if the parameter cannot be determined by its @@ -227,7 +237,7 @@ public List getBindableParameterNames() { List result = new ArrayList<>(); for (Parameter parameter : queryMethod.getParameters().getBindableParameters()) { - parameter.getName().map(result::add); + getParameterName(parameter.getIndex()); } return result; @@ -237,14 +247,7 @@ public List getBindableParameterNames() { * @return list of all parameter names (including non-bindable special parameters). */ public List getAllParameterNames() { - - List result = new ArrayList<>(); - - for (Parameter parameter : queryMethod.getParameters()) { - parameter.getName().map(result::add); - } - - return result; + return targetMethodMetadata.getMethodArguments().keySet().stream().toList(); } public boolean hasField(String fieldName) { @@ -269,17 +272,7 @@ public String getParameterNameOf(Class type) { } public @Nullable String getParameterName(int position) { - - if (0 > position) { - return null; - } - - List> entries = new ArrayList<>( - targetMethodMetadata.getMethodArguments().entrySet()); - if (position < entries.size()) { - return entries.get(position).getKey(); - } - return null; + return targetMethodMetadata.getParameterName(position); } public void addParameter(ParameterSpec parameter) { diff --git a/src/main/java/org/springframework/data/repository/aot/generate/AotRepositoryMethodBuilder.java b/src/main/java/org/springframework/data/repository/aot/generate/AotRepositoryMethodBuilder.java index fdea9bf60a..0994f234cd 100644 --- a/src/main/java/org/springframework/data/repository/aot/generate/AotRepositoryMethodBuilder.java +++ b/src/main/java/org/springframework/data/repository/aot/generate/AotRepositoryMethodBuilder.java @@ -16,7 +16,6 @@ package org.springframework.data.repository.aot.generate; import java.lang.reflect.Method; -import java.lang.reflect.Parameter; import java.lang.reflect.TypeVariable; import java.util.function.BiConsumer; import java.util.function.Function; @@ -24,13 +23,8 @@ import javax.lang.model.element.Modifier; -import org.springframework.core.DefaultParameterNameDiscoverer; -import org.springframework.core.MethodParameter; -import org.springframework.core.ResolvableType; -import org.springframework.data.repository.core.RepositoryInformation; import org.springframework.javapoet.CodeBlock; import org.springframework.javapoet.MethodSpec; -import org.springframework.javapoet.ParameterSpec; import org.springframework.javapoet.TypeName; import org.springframework.javapoet.TypeVariableName; import org.springframework.util.StringUtils; @@ -50,25 +44,7 @@ class AotRepositoryMethodBuilder { private BiConsumer customizer = (context, body) -> {}; AotRepositoryMethodBuilder(AotQueryMethodGenerationContext context) { - this.context = context; - initParameters(context.getMethod(), context.getRepositoryInformation()); - } - - private void initParameters(Method method, RepositoryInformation repositoryInformation) { - - ResolvableType repositoryInterface = ResolvableType.forClass(repositoryInformation.getRepositoryInterface()); - - for (Parameter parameter : method.getParameters()) { - - MethodParameter methodParameter = MethodParameter.forParameter(parameter); - methodParameter.initParameterNameDiscovery(new DefaultParameterNameDiscoverer()); - ResolvableType resolvableParameterType = ResolvableType.forMethodParameter(methodParameter, repositoryInterface); - - TypeName parameterType = TypeName.get(resolvableParameterType.getType()); - - this.context.addParameter(ParameterSpec.builder(parameterType, methodParameter.getParameterName()).build()); - } } /** diff --git a/src/main/java/org/springframework/data/repository/aot/generate/LocalVariableNameFactory.java b/src/main/java/org/springframework/data/repository/aot/generate/LocalVariableNameFactory.java new file mode 100644 index 0000000000..419d63b74d --- /dev/null +++ b/src/main/java/org/springframework/data/repository/aot/generate/LocalVariableNameFactory.java @@ -0,0 +1,87 @@ +/* + * Copyright 2025 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.repository.aot.generate; + +import java.util.Set; + +import org.springframework.util.LinkedMultiValueMap; +import org.springframework.util.MultiValueMap; + +/** + * Non thread safe {@link VariableNameFactory} implementation keeping track of defined names resolving name clashes + * using internal counters appending {@code _%d} to a suggested name in case of a clash. + * + * @author Christoph Strobl + * @since 4.0 + */ +class LocalVariableNameFactory implements VariableNameFactory { + + private final MultiValueMap variables; + + /** + * Create a new {@link LocalVariableNameFactory} considering available {@link MethodMetadata#getMethodArguments() + * method arguments}. + * + * @param methodMetadata source metadata + * @return new instance of {@link LocalVariableNameFactory}. + */ + static LocalVariableNameFactory forMethod(MethodMetadata methodMetadata) { + return of(methodMetadata.getMethodArguments().keySet()); + } + + /** + * Create a new {@link LocalVariableNameFactory} with a predefined set of initial variable names. + * + * @param predefinedVariables variables already known to be used in the given context. + * @return new instance of {@link LocalVariableNameFactory}. + */ + static LocalVariableNameFactory of(Set predefinedVariables) { + return new LocalVariableNameFactory(predefinedVariables); + } + + LocalVariableNameFactory(Iterable predefinedVariableNames) { + + variables = new LinkedMultiValueMap<>(); + predefinedVariableNames.forEach(varName -> variables.add(varName, varName)); + } + + @Override + public String generateName(String intendedVariableName) { + + if (!variables.containsKey(intendedVariableName)) { + variables.add(intendedVariableName, intendedVariableName); + return intendedVariableName; + } + + String targetName = suggestTargetName(intendedVariableName); + variables.add(intendedVariableName, targetName); + variables.add(targetName, targetName); + return targetName; + } + + String suggestTargetName(String suggested) { + return suggestTargetName(suggested, 1); + } + + String suggestTargetName(String suggested, int counter) { + + String targetName = "%s_%s".formatted(suggested, counter); + if (!variables.containsKey(targetName)) { + return targetName; + } + return suggestTargetName(suggested, counter + 1); + } +} diff --git a/src/main/java/org/springframework/data/repository/aot/generate/MethodMetadata.java b/src/main/java/org/springframework/data/repository/aot/generate/MethodMetadata.java index a69ad48aed..a9a65a300d 100644 --- a/src/main/java/org/springframework/data/repository/aot/generate/MethodMetadata.java +++ b/src/main/java/org/springframework/data/repository/aot/generate/MethodMetadata.java @@ -16,12 +16,16 @@ package org.springframework.data.repository.aot.generate; import java.lang.reflect.Method; +import java.util.ArrayList; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; import java.util.Map.Entry; import org.jspecify.annotations.Nullable; - +import org.springframework.core.DefaultParameterNameDiscoverer; +import org.springframework.core.MethodParameter; +import org.springframework.core.ParameterNameDiscoverer; import org.springframework.core.ResolvableType; import org.springframework.data.repository.core.RepositoryInformation; import org.springframework.javapoet.ParameterSpec; @@ -31,6 +35,7 @@ * Metadata about an AOT Repository method. * * @author Christoph Strobl + * @since 4.0 */ class MethodMetadata { @@ -38,10 +43,11 @@ class MethodMetadata { private final ResolvableType actualReturnType; private final ResolvableType returnType; - public MethodMetadata(RepositoryInformation repositoryInformation, Method method) { + MethodMetadata(RepositoryInformation repositoryInformation, Method method) { this.returnType = repositoryInformation.getReturnType(method).toResolvableType(); this.actualReturnType = ResolvableType.forType(repositoryInformation.getReturnedDomainClass(method)); + this.initParameters(repositoryInformation, method, new DefaultParameterNameDiscoverer()); } @Nullable @@ -54,20 +60,50 @@ public String getParameterNameOf(Class type) { return null; } - public ResolvableType getReturnType() { + ResolvableType getReturnType() { return returnType; } - public ResolvableType getActualReturnType() { + ResolvableType getActualReturnType() { return actualReturnType; } - public void addParameter(ParameterSpec parameterSpec) { + void addParameter(ParameterSpec parameterSpec) { this.methodArguments.put(parameterSpec.name, parameterSpec); } - public Map getMethodArguments() { + Map getMethodArguments() { return methodArguments; } + @Nullable + String getParameterName(int position) { + + if (0 > position) { + return null; + } + + List> entries = new ArrayList<>(methodArguments.entrySet()); + if (position < entries.size()) { + return entries.get(position).getKey(); + } + return null; + } + + private void initParameters(RepositoryInformation repositoryInformation, Method method, + ParameterNameDiscoverer nameDiscoverer) { + + ResolvableType repositoryInterface = ResolvableType.forClass(repositoryInformation.getRepositoryInterface()); + + for (java.lang.reflect.Parameter parameter : method.getParameters()) { + + MethodParameter methodParameter = MethodParameter.forParameter(parameter); + methodParameter.initParameterNameDiscovery(nameDiscoverer); + ResolvableType resolvableParameterType = ResolvableType.forMethodParameter(methodParameter, repositoryInterface); + + TypeName parameterType = TypeName.get(resolvableParameterType.getType()); + + addParameter(ParameterSpec.builder(parameterType, methodParameter.getParameterName()).build()); + } + } } diff --git a/src/main/java/org/springframework/data/repository/aot/generate/VariableNameFactory.java b/src/main/java/org/springframework/data/repository/aot/generate/VariableNameFactory.java new file mode 100644 index 0000000000..c5e4047fce --- /dev/null +++ b/src/main/java/org/springframework/data/repository/aot/generate/VariableNameFactory.java @@ -0,0 +1,38 @@ +/* + * Copyright 2025 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.repository.aot.generate; + +import org.springframework.lang.CheckReturnValue; + +/** + * Name factory for generating clash free variable names checking an intended name against predefined and already used + * ones. + * + * @author Christoph Strobl + * @since 4.0 + */ +interface VariableNameFactory { + + /** + * Compare and potentially generate a new name for the given intended variable name. + * + * @param intendedVariableName must not be {@literal null}. + * @return the {@literal intendedVariableName} if no naming clash detected or a clash free generated name. + */ + @CheckReturnValue + String generateName(String intendedVariableName); + +} diff --git a/src/test/java/org/springframework/data/repository/aot/generate/AotQueryMethodGenerationContextUnitTests.java b/src/test/java/org/springframework/data/repository/aot/generate/AotQueryMethodGenerationContextUnitTests.java new file mode 100644 index 0000000000..e76187303e --- /dev/null +++ b/src/test/java/org/springframework/data/repository/aot/generate/AotQueryMethodGenerationContextUnitTests.java @@ -0,0 +1,70 @@ +/* + * Copyright 2025 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.repository.aot.generate; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.eq; + +import java.lang.reflect.Method; + +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; +import org.springframework.data.domain.Pageable; +import org.springframework.data.repository.core.RepositoryInformation; +import org.springframework.data.repository.query.QueryMethod; +import org.springframework.data.util.TypeInformation; + +/** + * Tests targeting {@link AotQueryMethodGenerationContext}. + * + * @author Christoph Strobl + */ +class AotQueryMethodGenerationContextUnitTests { + + @Test // GH-3270 + void suggestLocalVariableNameConsidersMethodArguments() throws NoSuchMethodException { + + AotQueryMethodGenerationContext ctx = ctxFor("reservedParameterMethod"); + + assertThat(ctx.suggestLocalVariableName("foo")).isEqualTo("foo"); + assertThat(ctx.suggestLocalVariableName("arg0")).isNotIn("arg0", "arg1", "arg2"); + } + + AotQueryMethodGenerationContext ctxFor(String methodName) throws NoSuchMethodException { + + Method target = null; + for (Method m : DummyRepo.class.getMethods()) { + if (m.getName().equals(methodName)) { + target = m; + break; + } + } + + if (target == null) { + throw new NoSuchMethodException(methodName); + } + + RepositoryInformation ri = Mockito.mock(RepositoryInformation.class); + Mockito.doReturn(TypeInformation.of(target.getReturnType())).when(ri).getReturnType(eq(target)); + + return new AotQueryMethodGenerationContext(ri, target, Mockito.mock(QueryMethod.class), + Mockito.mock(AotRepositoryFragmentMetadata.class)); + } + + private interface DummyRepo { + String reservedParameterMethod(Object arg0, Pageable arg1, Object arg2); + } +} diff --git a/src/test/java/org/springframework/data/repository/aot/generate/LocalVariableNameFactoryUnitTests.java b/src/test/java/org/springframework/data/repository/aot/generate/LocalVariableNameFactoryUnitTests.java new file mode 100644 index 0000000000..3ca63aaa58 --- /dev/null +++ b/src/test/java/org/springframework/data/repository/aot/generate/LocalVariableNameFactoryUnitTests.java @@ -0,0 +1,72 @@ +/* + * Copyright 2025 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.repository.aot.generate; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.Set; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * @author Christoph Strobl + */ +class LocalVariableNameFactoryUnitTests { + + LocalVariableNameFactory variableNameFactory; + + @BeforeEach + void beforeEach() { + variableNameFactory = LocalVariableNameFactory.of(Set.of("firstname", "lastname", "sort")); + } + + @Test // GH-3270 + void resolvesNameClashesInNames() { + + assertThat(variableNameFactory.generateName("name")).isEqualTo("name"); + assertThat(variableNameFactory.generateName("name")).isEqualTo("name_1"); + assertThat(variableNameFactory.generateName("name")).isEqualTo("name_2"); + assertThat(variableNameFactory.generateName("name1")).isEqualTo("name1"); + assertThat(variableNameFactory.generateName("name3")).isEqualTo("name3"); + assertThat(variableNameFactory.generateName("name3")).isEqualTo("name3_1"); + assertThat(variableNameFactory.generateName("name4_1")).isEqualTo("name4_1"); + assertThat(variableNameFactory.generateName("name4")).isEqualTo("name4"); + assertThat(variableNameFactory.generateName("name4_1_1")).isEqualTo("name4_1_1"); + assertThat(variableNameFactory.generateName("name4_1")).isEqualTo("name4_1_2"); + assertThat(variableNameFactory.generateName("name4_1")).isEqualTo("name4_1_3"); + } + + @Test // GH-3270 + void worksWithVariablesContainingUnderscores() { + + assertThat(variableNameFactory.generateName("first_name")).isEqualTo("first_name"); + assertThat(variableNameFactory.generateName("first_name")).isEqualTo("first_name_1"); + assertThat(variableNameFactory.generateName("first_name")).isEqualTo("first_name_2"); + assertThat(variableNameFactory.generateName("first_name_3")).isEqualTo("first_name_3"); + assertThat(variableNameFactory.generateName("first_name")).isEqualTo("first_name_4"); + } + + @Test // GH-3270 + void considersPredefinedNames() { + assertThat(variableNameFactory.generateName("firstname")).isEqualTo("firstname_1"); + } + + @Test // GH-3270 + void considersCase() { + assertThat(variableNameFactory.generateName("firstName")).isEqualTo("firstName"); + } +} diff --git a/src/test/java/org/springframework/data/repository/aot/generate/MethodMetadataUnitTests.java b/src/test/java/org/springframework/data/repository/aot/generate/MethodMetadataUnitTests.java new file mode 100644 index 0000000000..8cc981251a --- /dev/null +++ b/src/test/java/org/springframework/data/repository/aot/generate/MethodMetadataUnitTests.java @@ -0,0 +1,85 @@ +/* + * Copyright 2025 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.repository.aot.generate; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.eq; + +import java.lang.reflect.Method; + +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; +import org.springframework.data.domain.Pageable; +import org.springframework.data.repository.core.RepositoryInformation; +import org.springframework.data.util.TypeInformation; + +/** + * Unit tests for {@link MethodMetadata}. + * + * @author Christoph Strobl + */ +class MethodMetadataUnitTests { + + @Test // GH-3270 + void getParameterNameByIndex() throws NoSuchMethodException { + + MethodMetadata metadata = methodMetadataFor("threeArgsMethod"); + + assertThat(metadata.getParameterName(0)).isEqualTo("arg0"); + assertThat(metadata.getParameterName(1)).isEqualTo("arg1"); + assertThat(metadata.getParameterName(2)).isEqualTo("arg2"); + } + + @Test // GH-3270 + void getParameterNameByNonExistingIndex() throws NoSuchMethodException { + + MethodMetadata metadata = methodMetadataFor("threeArgsMethod"); + + assertThat(metadata.getParameterName(-1)).isNull(); + assertThat(metadata.getParameterName(3)).isNull(); + } + + @Test // GH-3270 + void getParameterNameForNoArgsMethod() throws NoSuchMethodException { + assertThat(methodMetadataFor("noArgsMethod").getParameterName(0)).isNull(); + } + + static MethodMetadata methodMetadataFor(String methodName) throws NoSuchMethodException { + + Method target = null; + for (Method m : DummyRepo.class.getMethods()) { + if (m.getName().equals(methodName)) { + target = m; + break; + } + } + + if (target == null) { + throw new NoSuchMethodException(methodName); + } + + RepositoryInformation ri = Mockito.mock(RepositoryInformation.class); + Mockito.doReturn(TypeInformation.of(target.getReturnType())).when(ri).getReturnType(eq(target)); + return new MethodMetadata(ri, target); + } + + private interface DummyRepo { + + String noArgsMethod(); + + String threeArgsMethod(Object arg0, Pageable arg1, Object arg2); + } +}