Skip to content

Align AOT and reflective repository behavior. #5038

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.

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

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-mongodb-parent</artifactId>
<version>5.0.0-SNAPSHOT</version>
<version>5.0.x-GH-5027-SNAPSHOT</version>
<packaging>pom</packaging>

<name>Spring Data MongoDB</name>
Expand Down
2 changes: 1 addition & 1 deletion spring-data-mongodb-distribution/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-mongodb-parent</artifactId>
<version>5.0.0-SNAPSHOT</version>
<version>5.0.x-GH-5027-SNAPSHOT</version>
<relativePath>../pom.xml</relativePath>
</parent>

Expand Down
2 changes: 1 addition & 1 deletion spring-data-mongodb/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-mongodb-parent</artifactId>
<version>5.0.0-SNAPSHOT</version>
<version>5.0.x-GH-5027-SNAPSHOT</version>
<relativePath>../pom.xml</relativePath>
</parent>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,11 @@
import org.springframework.data.mongodb.core.timeseries.Granularity;
import org.springframework.data.mongodb.core.validation.Validator;
import org.springframework.data.projection.EntityProjection;
import org.springframework.data.projection.ProjectionFactory;
import org.springframework.data.util.CloseableIterator;
import org.springframework.data.util.Lazy;
import org.springframework.data.util.Optionals;
import org.springframework.data.util.TypeInformation;
import org.springframework.lang.Contract;
import org.springframework.util.Assert;
import org.springframework.util.ClassUtils;
Expand Down Expand Up @@ -2272,8 +2274,17 @@ protected <O> AggregationResults<O> doAggregate(Aggregation aggregation, String
<T, O> AggregationResults<O> doAggregate(Aggregation aggregation, String collectionName, Class<T> outputType,
QueryResultConverter<? super T, ? extends O> resultConverter, AggregationOperationContext context) {

DocumentCallback<O> callback = new QueryResultConverterCallback<>(resultConverter,
final DocumentCallback<O> callback;
if(aggregation instanceof TypedAggregation<?> ta && outputType.isInterface()) {
EntityProjection<T, ?> projection = operations.introspectProjection(outputType, ta.getInputType());
ProjectingReadCallback cb = new ProjectingReadCallback(mongoConverter, projection, collectionName);
callback = new QueryResultConverterCallback<>(resultConverter,
cb);
} else {

callback = new QueryResultConverterCallback<>(resultConverter,
new ReadDocumentCallback<>(mongoConverter, outputType, collectionName));
}

AggregationOptions options = aggregation.getOptions();
AggregationUtil aggregationUtil = new AggregationUtil(queryMapper, mappingContext);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,9 @@ Document getMappedFields(@Nullable MongoPersistentEntity<?> entity, EntityProjec
mappedFields = queryMapper.getMappedFields(fields, entity);
} else {
mappedFields = propertyOperations.computeMappedFieldsForProjection(projection, fields);
if(projection.getMappedType().getType().isInterface()) {
mappedFields = queryMapper.getMappedFields(mappedFields, entity);
}
mappedFields = queryMapper.addMetaAttributes(mappedFields, entity);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@

import org.bson.Document;
import org.jspecify.annotations.NullUnmarked;

import org.springframework.core.ResolvableType;
import org.springframework.core.annotation.MergedAnnotation;
import org.springframework.data.domain.SliceImpl;
import org.springframework.data.domain.Sort.Order;
import org.springframework.data.mongodb.core.MongoOperations;
import org.springframework.data.mongodb.core.aggregation.Aggregation;
import org.springframework.data.mongodb.core.aggregation.AggregationOperation;
import org.springframework.data.mongodb.core.aggregation.AggregationOptions;
import org.springframework.data.mongodb.core.aggregation.AggregationPipeline;
import org.springframework.data.mongodb.core.aggregation.AggregationResults;
Expand Down Expand Up @@ -80,12 +80,7 @@ CodeBlock build() {

builder.add("\n");

Class<?> outputType = queryMethod.getReturnedObjectType();
if (MongoSimpleTypes.HOLDER.isSimpleType(outputType)) {
outputType = Document.class;
} else if (ClassUtils.isAssignable(AggregationResults.class, outputType)) {
outputType = queryMethod.getReturnType().getComponentType().getType();
}
Class<?> outputType = getOutputType(queryMethod);

if (ReflectionUtils.isVoid(queryMethod.getReturnedObjectType())) {
builder.addStatement("$L.aggregate($L, $T.class)", mongoOpsRef, aggregationVariableName, outputType);
Expand Down Expand Up @@ -146,7 +141,6 @@ CodeBlock build() {
builder.addStatement("return $L.aggregateStream($L, $T.class)", mongoOpsRef, aggregationVariableName,
outputType);
} else {

builder.addStatement("return $L.aggregate($L, $T.class).getMappedResults()", mongoOpsRef,
aggregationVariableName, outputType);
}
Expand All @@ -155,6 +149,17 @@ CodeBlock build() {

return builder.build();
}

}

private static Class<?> getOutputType(MongoQueryMethod queryMethod) {
Class<?> outputType = queryMethod.getReturnedObjectType();
if (MongoSimpleTypes.HOLDER.isSimpleType(outputType)) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's use SimpleTypeHolder from MongoRepositoryContributor.

outputType = Document.class;
} else if (ClassUtils.isAssignable(AggregationResults.class, outputType) && queryMethod.getReturnType().getComponentType() != null) {
outputType = queryMethod.getReturnType().getComponentType().getType();
}
return outputType;
}

@NullUnmarked
Expand All @@ -173,13 +178,7 @@ static class AggregationCodeBlockBuilder {

this.context = context;
this.queryMethod = queryMethod;
String parameterNames = StringUtils.collectionToDelimitedString(context.getAllParameterNames(), ", ");

if (StringUtils.hasText(parameterNames)) {
this.parameterNames = ", " + parameterNames;
} else {
this.parameterNames = "";
}
this.parameterNames = StringUtils.collectionToDelimitedString(context.getAllParameterNames(), ", ");
}

AggregationCodeBlockBuilder stages(AggregationInteraction aggregation) {
Expand Down Expand Up @@ -231,7 +230,8 @@ private CodeBlock pipeline(String pipelineVariableName) {
builder.add(aggregationStages(context.localVariable("stages"), source.stages()));

if (StringUtils.hasText(sortParameter)) {
builder.add(sortingStage(sortParameter));
Class<?> outputType = getOutputType(queryMethod);
builder.add(sortingStage(sortParameter, outputType));
}

if (StringUtils.hasText(limitParameter)) {
Expand All @@ -244,6 +244,7 @@ private CodeBlock pipeline(String pipelineVariableName) {

builder.addStatement("$T $L = createPipeline($L)", AggregationPipeline.class, pipelineVariableName,
context.localVariable("stages"));

return builder.build();
}

Expand Down Expand Up @@ -303,7 +304,7 @@ private CodeBlock aggregationStages(String stageListVariableName, Collection<Str

VariableSnippet stageSnippet = Snippet.declare(builder)
.variable(Document.class, context.localVariable("stage_%s".formatted(stageCounter)))
.of(MongoCodeBlocks.asDocument(stage, parameterNames));
.of(MongoCodeBlocks.asDocument(context.getExpressionMarker(), stage, parameterNames));
builder.addStatement("$L.add($L)", stageListVariableName, stageSnippet.getVariableName());

stageCounter++;
Expand All @@ -312,7 +313,7 @@ private CodeBlock aggregationStages(String stageListVariableName, Collection<Str
return builder.build();
}

private CodeBlock sortingStage(String sortProvider) {
private CodeBlock sortingStage(String sortProvider, Class<?> outputType) {

Builder builder = CodeBlock.builder();

Expand All @@ -322,8 +323,17 @@ private CodeBlock sortingStage(String sortProvider) {
builder.addStatement("$1L.append($2L.getProperty(), $2L.isAscending() ? 1 : -1);",
context.localVariable("sortDocument"), context.localVariable("order"));
builder.endControlFlow();
builder.addStatement("stages.add(new $T($S, $L))", Document.class, "$sort",
context.localVariable("sortDocument"));

if (outputType == Document.class || MongoSimpleTypes.HOLDER.isSimpleType(outputType)
Copy link
Member

Choose a reason for hiding this comment

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

MongoRepositoryContributor uses a MongoMappingContext. Ideally, we use its SimpleTypeHolder for further introspection to use a consistent configuration source.

|| ClassUtils.isAssignable(context.getRepositoryInformation().getDomainType(), outputType)) {
builder.addStatement("$L.add(new $T($S, $L))", context.localVariable("stages"), Document.class, "$sort",
context.localVariable("sortDocument"));
} else {
builder.addStatement("$L.add(($T) _ctx -> new $T($S, _ctx.getMappedObject($L, $T.class)))",
context.localVariable("stages"), AggregationOperation.class, Document.class, "$sort",
context.localVariable("sortDocument"), outputType);
}

builder.endControlFlow();

return builder.build();
Expand All @@ -333,7 +343,7 @@ private CodeBlock pagingStage(String pageableProvider, boolean slice) {

Builder builder = CodeBlock.builder();

builder.add(sortingStage(pageableProvider + ".getSort()"));
builder.add(sortingStage(pageableProvider + ".getSort()", getOutputType(queryMethod)));

builder.beginControlFlow("if ($L.isPaged())", pageableProvider);
builder.beginControlFlow("if ($L.getOffset() > 0)", pageableProvider);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import java.util.List;

import org.jspecify.annotations.Nullable;
import org.springframework.data.geo.Box;
import org.springframework.data.geo.Circle;
import org.springframework.data.geo.Distance;
Expand Down Expand Up @@ -52,7 +53,7 @@ static Placeholder indexed(int position) {
* @param type
* @return
*/
public static Shape geoJson(int index, String type) {
static Shape geoJson(int index, String type) {
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I liked to keep public in package-protected types to indicate a method is intended to be used as sort-of a public API (entrypoint) for other callers outside of the class. In any case, just a thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather have them aligned with the type scope in such cases.

return new GeoJsonPlaceholder(index, type);
}

Expand All @@ -62,7 +63,7 @@ public static Shape geoJson(int index, String type) {
* @param index zero-based index referring to the bindable method parameter.
* @return
*/
public static Point point(int index) {
static Point point(int index) {
return new PointPlaceholder(index);
}

Expand All @@ -72,7 +73,7 @@ public static Point point(int index) {
* @param index zero-based index referring to the bindable method parameter.
* @return
*/
public static Shape circle(int index) {
static Shape circle(int index) {
return new CirclePlaceholder(index);
}

Expand All @@ -82,7 +83,7 @@ public static Shape circle(int index) {
* @param index zero-based index referring to the bindable method parameter.
* @return
*/
public static Shape box(int index) {
static Shape box(int index) {
return new BoxPlaceholder(index);
}

Expand All @@ -92,7 +93,7 @@ public static Shape box(int index) {
* @param index zero-based index referring to the bindable method parameter.
* @return
*/
public static Shape sphere(int index) {
static Shape sphere(int index) {
return new SpherePlaceholder(index);
}

Expand All @@ -102,20 +103,23 @@ public static Shape sphere(int index) {
* @param index zero-based index referring to the bindable method parameter.
* @return
*/
public static Shape polygon(int index) {
static Shape polygon(int index) {
return new PolygonPlaceholder(index);
}

static RegexPlaceholder regex(int index, @Nullable String options) {
return new RegexPlaceholder(index, options);
}

/**
* A placeholder expression used when rending queries to JSON.
*
* @since 5.0
* @author Christoph Strobl
*/
public interface Placeholder {
interface Placeholder {

String getValue();

}

/**
Expand All @@ -139,7 +143,7 @@ private static class PointPlaceholder extends Point implements Placeholder {

private final int index;

public PointPlaceholder(int index) {
PointPlaceholder(int index) {
super(Double.NaN, Double.NaN);
this.index = index;
}
Expand Down Expand Up @@ -184,7 +188,7 @@ private static class CirclePlaceholder extends Circle implements Placeholder {

private final int index;

public CirclePlaceholder(int index) {
CirclePlaceholder(int index) {
super(new PointPlaceholder(index), Distance.of(1, Metrics.NEUTRAL)); //
this.index = index;
}
Expand All @@ -205,7 +209,7 @@ private static class BoxPlaceholder extends Box implements Placeholder {

private final int index;

public BoxPlaceholder(int index) {
BoxPlaceholder(int index) {
super(new PointPlaceholder(index), new PointPlaceholder(index));
this.index = index;
}
Expand All @@ -226,7 +230,7 @@ private static class SpherePlaceholder extends Sphere implements Placeholder {

private final int index;

public SpherePlaceholder(int index) {
SpherePlaceholder(int index) {
super(new PointPlaceholder(index), Distance.of(1, Metrics.NEUTRAL)); //
this.index = index;
}
Expand All @@ -247,7 +251,7 @@ private static class PolygonPlaceholder extends Polygon implements Placeholder {

private final int index;

public PolygonPlaceholder(int index) {
PolygonPlaceholder(int index) {
super(new PointPlaceholder(index), new PointPlaceholder(index), new PointPlaceholder(index),
new PointPlaceholder(index));
this.index = index;
Expand All @@ -265,4 +269,29 @@ public String toString() {

}

static class RegexPlaceholder implements Placeholder {

private final int index;
private final @Nullable String options;

RegexPlaceholder(int index, @Nullable String options) {
this.index = index;
this.options = options;
}

@Nullable String regexOptions() {
return options;
}

@Override
public String getValue() {
return "?" + index;
}

@Override
public String toString() {
return getValue();
}
}

}
Loading