Skip to content

Add LocalVariableNameFactory. #3271

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: 4.0.x
Choose a base branch
from
Open

Add LocalVariableNameFactory. #3271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0612b25
Prepare issue branch.
christophstrobl Apr 16, 2025
dacd58a
Add LocalVariableNameFactory.
christophstrobl Apr 16, 2025
cb93ce2
Use plain string instead of wrapper
christophstrobl May 2, 2025
7b5f476
Reduce visibility & add documentation (well a little at least)
christophstrobl May 2, 2025
4a48edc
Move method parameter initialization to MethodMetadata.
christophstrobl May 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-commons</artifactId>
<version>4.0.0-SNAPSHOT</version>
<version>4.0.x-GH-3270-SNAPSHOT</version>

<name>Spring Data Core</name>
<description>Core Spring concepts underpinning every Spring Data module.</description>
Expand Down
37 changes: 15 additions & 22 deletions ...ava/org/springframework/data/repository/aot/generate/AotQueryMethodGenerationContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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);
}

Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest refining the way we approach the problem. We generally hold details in the context and can look these up. I would suggest that we keep a map around with parameter name mappings and use this method as getLocalVariableName(…) or localVariable(String logicalName). One could then call the method with the same name twice. On the first call, we would ensure there is no clash, the second call would return the mapped name. Would also alleviate the need to track the variable name in the calling code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I don't get that comment. Why would I want to call the same method twice?
you'll have to keep track of variable name in the calling code since you might have to apply it in different code blocks or have to pass it around between query creation and execution so you refer to right thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll have to keep track of variable name in the calling code since you might have to apply it in different code blocks or have to pass it around between query creation and execution so you refer to right thing.

Calling code would then not be required to track the actual variable name but could refer to the context so the context provides the information.

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
Expand Down Expand Up @@ -227,7 +237,7 @@ public List<String> getBindableParameterNames() {
List<String> result = new ArrayList<>();

for (Parameter parameter : queryMethod.getParameters().getBindableParameters()) {
parameter.getName().map(result::add);
getParameterName(parameter.getIndex());
}

return result;
Expand All @@ -237,14 +247,7 @@ public List<String> getBindableParameterNames() {
* @return list of all parameter names (including non-bindable special parameters).
*/
public List<String> getAllParameterNames() {

List<String> 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) {
Expand All @@ -269,17 +272,7 @@ public String getParameterNameOf(Class<?> type) {
}

public @Nullable String getParameterName(int position) {

if (0 > position) {
return null;
}

List<Entry<String, ParameterSpec>> 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) {
Expand Down
24 changes: 0 additions & 24 deletions ...ain/java/org/springframework/data/repository/aot/generate/AotRepositoryMethodBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,15 @@
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;
import java.util.stream.Collectors;

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;
Expand All @@ -50,25 +44,7 @@ class AotRepositoryMethodBuilder {
private BiConsumer<AotQueryMethodGenerationContext, MethodSpec.Builder> 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());
}
}

/**
Expand Down
87 changes: 87 additions & 0 deletions src/main/java/org/springframework/data/repository/aot/generate/LocalVariableNameFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<String, String> 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<String> predefinedVariables) {
return new LocalVariableNameFactory(predefinedVariables);
}

LocalVariableNameFactory(Iterable<String> predefinedVariableNames) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simplification of this could be:

	LocalVariableNameFactory(Iterable<String> predefinedVariableNames) {
		predefinedVariableNames.forEach((paramName) -> variables.put(paramName, 0L));
	}

Wdyt?


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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to update the original intendedVariableName?

This code is fine but concurrency aside, I still think something like the following that would replace line62-86 would simplify things:

@Override
public String generateName(String suggestedName) {
	var counter = variables.compute(suggestedName,
			(__, currentCount) -> currentCount == null ? 0 : currentCount.longValue() + 1);
	return counter == 0 ? suggestedName : "%s_%s".formatted(suggestedName, counter));
}

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);
}
}
48 changes: 42 additions & 6 deletions src/main/java/org/springframework/data/repository/aot/generate/MethodMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -31,17 +35,19 @@
* Metadata about an AOT Repository method.
*
* @author Christoph Strobl
* @since 4.0
*/
class MethodMetadata {

private final Map<String, ParameterSpec> methodArguments = new LinkedHashMap<>();
private final ResolvableType actualReturnType;
private final ResolvableType returnType;

public MethodMetadata(RepositoryInformation repositoryInformation, Method method) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I liked the visibility modifiers to indicate which methods are desired API and to differentiate between those ones, that are package-private for testing purposes.

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
Expand All @@ -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<String, ParameterSpec> getMethodArguments() {
Map<String, ParameterSpec> getMethodArguments() {
return methodArguments;
}

@Nullable
String getParameterName(int position) {

if (0 > position) {
return null;
}

List<Entry<String, ParameterSpec>> 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());
}
}
}
38 changes: 38 additions & 0 deletions src/main/java/org/springframework/data/repository/aot/generate/VariableNameFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);

}
Loading