spring-projects/spring-data-mongodb

`@Meta` annotation not working properly with `MongoRepository` and custom repositories

malaquf opened this issue · 8 comments

Environment:
Kotlin with coroutines 2.1.0
Spring Boot 3.3.1

Problem
When adding a @meta annotation to my queries in a given repository, I get empty lists as result.

Unfortunately, I could not find the root cause yet, but I hope the following description helps to reproduce it.

CrudMethodMetadataPopulatingMethodInterceptor checks if a method is a query using isQueryMethod(). Because @meta adds @QueryAnnotation, the method is not added to Set<Method> implementations.

Surprisingly, query methods defined in repositories by default are added to Set<Method> implementations from CrudMethodMetadataPopulatingMethodInterceptor, and everything works fine when not adding the @meta annotation.

The difference I see so far is in the execution path for the query in the following method:

@Override
		public Object invoke(MethodInvocation invocation) throws Throwable {

			Method method = invocation.getMethod();

			if (!implementations.contains(method)) {
				return invocation.proceed();
			}

			MethodInvocation oldInvocation = currentInvocation.get();
			currentInvocation.set(invocation);

			try {

				CrudMethodMetadata metadata = (CrudMethodMetadata) TransactionSynchronizationManager.getResource(method);

				if (metadata != null) {
					return invocation.proceed();
				}

				CrudMethodMetadata methodMetadata = metadataCache.get(method);

				if (methodMetadata == null) {

					methodMetadata = new DefaultCrudMethodMetadata(repositoryInformation.getRepositoryInterface(), method);
					CrudMethodMetadata tmp = metadataCache.putIfAbsent(method, methodMetadata);

					if (tmp != null) {
						methodMetadata = tmp;
					}
				}

				TransactionSynchronizationManager.bindResource(method, methodMetadata);

				try {
					return invocation.proceed();
				} finally {
					TransactionSynchronizationManager.unbindResource(method);
				}
			} finally {
				currentInvocation.set(oldInvocation);
			}
		}
	}

When adding @meta annotation, my query is executed through invocation.proceed() within if (!implementations.contains(method)) block. If @meta is not present, the execution is done by the rest of the code.

Repository example:

interface MyRepository : ReactiveMongoRepository<MyDocument, MyKey> {

	@Meta(maxExecutionTimeMs = 30000)
	override fun findAllById(ids: Iterable<MyKey>): Flux<MyDocument>

	@Meta(maxExecutionTimeMs = 30000)
	override fun findById(id: MyKey): Mono<MyDocument>
}

I'll update the issue if I have more information.

By the way, all I want in this case is to define maxExecutionTimeMs for my queries and I believe there should be a better way to customise it (and specially set a default) without having to define it for each method in the repository. Like having an interceptor for easily extending the Query options, for example.

Thank you for looking into it.

@malaquf a complete minimal sample (something that we can unzip or git clone, build, and deploy) that reproduces the problem would help a lot. Thank you!

@christophstrobl thank you for the very fast answer. Sorry for that, here it goes: https://github.com/malaquf/spring-data-mongodb-issue-4852
Thanks!

There are a couple of aspects and the fix isn't trivial. @Meta wasn't intended for implementation method usage and it isn't applicable for each operation (e.g. our save method doesn't even consider meta settings). A key problem is that @Meta isn't documented properly and therefore, there's no documentation about the usage scope.

When using @Meta, your method is considered a query method using query derivation. When using a single id and when your identifier is annotated with @MongoId instead of @Id, then the resulting query is valid and will return a result.

Going forward, there are a few things we need to consider:

  1. Document the annotation properly (that's what we're going to do with this ticket)
  2. Consider where @Meta could be used. Likely, we can only leverage a few find methods. Query by Example or Querydsl methods won't work really because of different infrastructure.

Interesting! Thank you for the info. I did some quick research and it seems I could simply replace @Id by @MongoId without any breaking changes. Is it really the case or there are some caveats?

Do you see a better/simpler approach to intercept the queries and append a default maxTimeMS?

Ok, I think this should be good for my use cases:

@Configuration
class MongoConfig {
	@Value("\${spring.data.mongodb.maxTimeMs}")
	private var maxTimeMs: Long = 7000

	@Bean
	fun mongoDBDefaultSettings(credential: MongoCredential): MongoClientSettingsBuilderCustomizer {
		return MongoClientSettingsBuilderCustomizer { builder: MongoClientSettings.Builder ->
			builder.addCommandListener(MaxTimeMsCommandListener(maxTimeMs))
		}
	}
}

class MaxTimeMsCommandListener(private val maxTimeMs: Long) : CommandListener {
	private val commandsWithMaxTimeMs = setOf(
		"find",
		"aggregate",
		"count",
		"distinct",
		"mapReduce",
		"group"
	)

	override fun commandStarted(event: CommandStartedEvent) {
		if (event.commandName in commandsWithMaxTimeMs) {
			event.command["maxTimeMS"] = BsonInt64(maxTimeMs)
		}
	}
}

Unfortunately, it is not that easy, as it turned out, command is immutable.
I see no way to extend spring data for that, unless I use Mongo Templates instead :/

It looks like org.springframework.data.mongodb.core.FindPublisherPreparer#prepare would be an ideal place to extend, but unfortunately, it's an internal class. I'd like to ask you if you could provide us a way to set maxTimeMS, as this is a very important configuration.

I'm also aware of this new mongodb driver feature, and would also like to ask you to consider supporting it soon: https://jira.mongodb.org/browse/JAVA-3828

Thank you.

Hello @mp911de,
I've seen the changes in this PR but I honestly, with all respect, don't know how this solves any problem.
I use @query without any problems on methods like the one I sent in my example, also with no need for a @mongoid annotated document. The documentation added is still not clear regarding the limitations mentioned in your comment in my opinion, and it is still not possible to set the maxTimeMS property in repository interfaces.