Cannot sort by a @CompositeProperty

[ellebrecht]

When using @Nodes containing a @CompositeProperty, I'd expect to be able to use the mapped Neo4j node property in Neo4jRepository.findAll(Sort.by("composite.property"))

Affected Versions
SDN 7.2.4 with Spring Boot 3.2.4

Reproduce
Here's a simple class to reproduce (see https://github.com/ellebrecht/sdnCompositeIssueReproducer for a complete repo)

@SpringBootApplication
@Slf4j
public class SdnCompositeIssueReproducerApplication implements CommandLineRunner {

    @Autowired
    private Repo repo;

    public static void main(String[] args) {
        log.info("Starting");
        SpringApplication.run(SdnCompositeIssueReproducerApplication.class, args);
    }

    @Override
    public void run(String... args) {
        log.info("Saving");
        repo.save(Entity.builder().name("test1").composite(Map.of("c1", true, "c2", 1)).build());
        repo.save(Entity.builder().name("test2").composite(Map.of("c1", true, "c2", 2)).build());
        repo.save(Entity.builder().name("test3").composite(Map.of("c1", false, "c2", 3)).build());
        log.info("Finding");
        List<Entity> found = repo.findAll(Sort.by("composite.c2"));
        log.info("Found {}", found);
    }

}

@Node
@Data
@Builder
class Entity {

    @Id
    @GeneratedValue(generatorClass = UUIDStringGenerator.class)
    private String uuid;

    private String name;

    @CompositeProperty
    private Map<String, Object> composite;

}

@Repository
interface Repo extends Neo4jRepository<Entity, String> {
}

Expected behaviour
I expected the repo.findAll(Sort.by("composite.c2")) to work like sorting by any other property, e.g. repo.findAll(Sort.by("name"))

Actual behaviour

java.lang.IllegalStateException: Cannot order by the unknown graph property: 'composite.c2'
	at org.springframework.data.neo4j.repository.query.CypherAdapterUtils.lambda$sortAdapterFor$0(CypherAdapterUtils.java:79) ~[spring-data-neo4j-7.2.4.jar:7.2.4]
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) ~[na:na]
	at java.base/java.util.ArrayList$Itr.forEachRemaining(ArrayList.java:1085) ~[na:na]
	at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1939) ~[na:na]
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509) ~[na:na]
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) ~[na:na]
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921) ~[na:na]
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[na:na]
	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682) ~[na:na]
	at org.springframework.data.neo4j.repository.query.CypherAdapterUtils.toSortItems(CypherAdapterUtils.java:177) ~[spring-data-neo4j-7.2.4.jar:7.2.4]
	at org.springframework.data.neo4j.repository.query.QueryFragmentsAndParameters.getQueryFragmentsAndParameters(QueryFragmentsAndParameters.java:348) ~[spring-data-neo4j-7.2.4.jar:7.2.4]
	at org.springframework.data.neo4j.repository.query.QueryFragmentsAndParameters.forPageableAndSort(QueryFragmentsAndParameters.java:181) ~[spring-data-neo4j-7.2.4.jar:7.2.4]
	at org.springframework.data.neo4j.repository.support.SimpleNeo4jRepository.findAll(SimpleNeo4jRepository.java:90) ~[spring-data-neo4j-7.2.4.jar:7.2.4]
	at org.springframework.data.neo4j.repository.support.SimpleNeo4jRepository.findAll(SimpleNeo4jRepository.java:50) ~[spring-data-neo4j-7.2.4.jar:7.2.4]
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) ~[na:na]
	at java.base/java.lang.reflect.Method.invoke(Method.java:580) ~[na:na]
	at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:351) ~[spring-aop-6.1.5.jar:6.1.5]
	at org.springframework.data.repository.core.support.RepositoryMethodInvoker$RepositoryFragmentMethodInvoker.lambda$new$0(RepositoryMethodInvoker.java:277) ~[spring-data-commons-3.2.4.jar:3.2.4]
	at org.springframework.data.repository.core.support.RepositoryMethodInvoker.doInvoke(RepositoryMethodInvoker.java:170) ~[spring-data-commons-3.2.4.jar:3.2.4]
	at org.springframework.data.repository.core.support.RepositoryMethodInvoker.invoke(RepositoryMethodInvoker.java:158) ~[spring-data-commons-3.2.4.jar:3.2.4]
	at org.springframework.data.repository.core.support.RepositoryComposition$RepositoryFragments.invoke(RepositoryComposition.java:516) ~[spring-data-commons-3.2.4.jar:3.2.4]
	at org.springframework.data.repository.core.support.RepositoryComposition.invoke(RepositoryComposition.java:285) ~[spring-data-commons-3.2.4.jar:3.2.4]
	at org.springframework.data.repository.core.support.RepositoryFactorySupport$ImplementationMethodExecutionInterceptor.invoke(RepositoryFactorySupport.java:628) ~[spring-data-commons-3.2.4.jar:3.2.4]
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:184) ~[spring-aop-6.1.5.jar:6.1.5]
	at org.springframework.data.repository.core.support.QueryExecutorMethodInterceptor.doInvoke(QueryExecutorMethodInterceptor.java:168) ~[spring-data-commons-3.2.4.jar:3.2.4]
	at org.springframework.data.repository.core.support.QueryExecutorMethodInterceptor.invoke(QueryExecutorMethodInterceptor.java:143) ~[spring-data-commons-3.2.4.jar:3.2.4]
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:184) ~[spring-aop-6.1.5.jar:6.1.5]
	at org.springframework.data.projection.DefaultMethodInvokingMethodInterceptor.invoke(DefaultMethodInvokingMethodInterceptor.java:70) ~[spring-data-commons-3.2.4.jar:3.2.4]
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:184) ~[spring-aop-6.1.5.jar:6.1.5]
	at org.springframework.transaction.interceptor.TransactionInterceptor$1.proceedWithInvocation(TransactionInterceptor.java:123) ~[spring-tx-6.1.5.jar:6.1.5]
	at org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(TransactionAspectSupport.java:392) ~[spring-tx-6.1.5.jar:6.1.5]
	at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:119) ~[spring-tx-6.1.5.jar:6.1.5]
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:184) ~[spring-aop-6.1.5.jar:6.1.5]
	at org.springframework.dao.support.PersistenceExceptionTranslationInterceptor.invoke(PersistenceExceptionTranslationInterceptor.java:137) ~[spring-tx-6.1.5.jar:6.1.5]
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:184) ~[spring-aop-6.1.5.jar:6.1.5]
	at org.springframework.aop.interceptor.ExposeInvocationInterceptor.invoke(ExposeInvocationInterceptor.java:97) ~[spring-aop-6.1.5.jar:6.1.5]
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:184) ~[spring-aop-6.1.5.jar:6.1.5]
	at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:220) ~[spring-aop-6.1.5.jar:6.1.5]
	at com.ellebrecht.sdnCompositeIssueReproducer.$Proxy62.findAll(Unknown Source) ~[na:na]
	at com.ellebrecht.sdnCompositeIssueReproducer.SdnCompositeIssueReproducerApplication.run(SdnCompositeIssueReproducerApplication.java:41) ~[main/:na]
	at org.springframework.boot.SpringApplication.lambda$callRunner$5(SpringApplication.java:790) ~[spring-boot-3.2.4.jar:3.2.4]
	at org.springframework.util.function.ThrowingConsumer$1.acceptWithException(ThrowingConsumer.java:83) ~[spring-core-6.1.5.jar:6.1.5]
	at org.springframework.util.function.ThrowingConsumer.accept(ThrowingConsumer.java:60) ~[spring-core-6.1.5.jar:6.1.5]
	at org.springframework.util.function.ThrowingConsumer$1.accept(ThrowingConsumer.java:88) ~[spring-core-6.1.5.jar:6.1.5]
	at org.springframework.boot.SpringApplication.callRunner(SpringApplication.java:798) ~[spring-boot-3.2.4.jar:3.2.4]
	at org.springframework.boot.SpringApplication.callRunner(SpringApplication.java:789) ~[spring-boot-3.2.4.jar:3.2.4]
	at org.springframework.boot.SpringApplication.lambda$callRunners$3(SpringApplication.java:774) ~[spring-boot-3.2.4.jar:3.2.4]
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184) ~[na:na]
	at java.base/java.util.stream.SortedOps$SizedRefSortingSink.end(SortedOps.java:357) ~[na:na]
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:510) ~[na:na]
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) ~[na:na]
	at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151) ~[na:na]
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174) ~[na:na]
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[na:na]
	at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596) ~[na:na]
	at org.springframework.boot.SpringApplication.callRunners(SpringApplication.java:774) ~[spring-boot-3.2.4.jar:3.2.4]
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:341) ~[spring-boot-3.2.4.jar:3.2.4]
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:1354) ~[spring-boot-3.2.4.jar:3.2.4]
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:1343) ~[spring-boot-3.2.4.jar:3.2.4]
	at com.ellebrecht.sdnCompositeIssueReproducer.SdnCompositeIssueReproducerApplication.main(SdnCompositeIssueReproducerApplication.java:31) ~[main/:na]

Additional info
CypherAdapterUtils.sortAdapterFor():49 tests for the presence of a dot to determine whether the property is qualified with a name. This doesn't work with @CompositeProperty because Java Map keys are mapped to Neo4j node property keys using a dot as well.

[meistermeier]

Thanks for raising this limitation. I am currently trying a few things around this to allow this access to a "unknown" (e.g. key value of a map) Java property while keeping the safety guards for the rest alive. Needs some rearrangement in the code to check for the (composite) property of the entity earlier.
Would be a nice addition if it does not affect the existing code paths.

[meistermeier]

There is now a 7.2.5-GH-2884-SNAPSHOT available in Spring's maven repository.
If you want to give it a try, you should add the spring-data-neo4j dependency explicitly and define the snapshot/milestone repositories.

implementation("org.springframework.data:spring-data-neo4j:7.2.5-GH-2884-SNAPSHOT")

repositories {
  mavenCentral()
  maven { url 'https://repo.spring.io/milestone' }
  maven { url 'https://repo.spring.io/snapshot' }
}

[ellebrecht]

Thanks @meistermeier for the update.

I tested it (see updated https://github.com/ellebrecht/sdnCompositeIssueReproducer) and it does not yet work.

The Cypher query generated is
MATCH (entity:`Entity`) RETURN entity{.name, __allProperties__: entity{.*}, __nodeLabels__: labels(entity), __internalNeo4jId__: id(entity), __elementId__: id(entity)} ORDER BY entity.`composite.c2`, entity.name

This seems to simply yield the order in which the nodes were inserted.

However, the order seems correct if I prepend the composite property key like this
MATCH (entity:`Entity`) RETURN entity{.`composite.c2`, .name, __allProperties__: entity{.*}, __nodeLabels__: labels(entity), __internalNeo4jId__: id(entity), __elementId__: id(entity)} ORDER BY entity.`composite.c2`, entity.name

[meistermeier]

Thanks for the feedback. You are completely right. I got it working because the test data set "accidentally" had the right order matching my property and inverting the order, inverted the result correctly.

[ellebrecht]

Glad to hear you could verify it. Will you update your branch accordingly? Just checking if this still fits with our current sprint :)

[meistermeier]

Trying to get this in the next patch release (Friday April, 12th). The problem you showed in your updated branch/feedback is not only affecting the composite properties, as far as I can tell yet. Need some more investigation on
a) does it really not work for everything that enters this code path
b) if a) is true, why are our tests still coloured in happy green

[meistermeier]

I pushed an update to the branch. (~15 minutes and it will be available)
Your updated reproducer works with this version. Would be good to get your feedback on this again.
It is not covered in your reproducer, but I need to adjust a few things more for other return patterns that are not map projections.

[meistermeier]

There was a problem with a test case. The updated snapshot should be available in a few minutes.

[ellebrecht]

Works great, thanks! 👍
Can you predict which SDN release this will make it into?

[meistermeier]

Good to hear, thanks for your feedback. It will be in the 7.1.11 patch release and all newer versions.