Skip to content
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

@Meta annotation not working properly with MongoRepository and custom repositories #4852

Open
malaquf opened this issue Dec 12, 2024 · 8 comments · May be fixed by #4854
Open

@Meta annotation not working properly with MongoRepository and custom repositories #4852

malaquf opened this issue Dec 12, 2024 · 8 comments · May be fixed by #4854
Assignees
Labels
type: documentation A documentation update

Comments

@malaquf
Copy link

malaquf commented Dec 12, 2024

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.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 12, 2024
@christophstrobl
Copy link
Member

@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!

@malaquf
Copy link
Author

malaquf commented Dec 12, 2024

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

@mp911de mp911de added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 13, 2024
@mp911de
Copy link
Member

mp911de commented Dec 13, 2024

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.

@mp911de mp911de changed the title @Meta annotation not working properly with ReactiveMongoRepository and custom repositories @Meta annotation not working properly with MongoRepository and custom repositories Dec 13, 2024
@mp911de mp911de changed the title @Meta annotation not working properly with MongoRepository and custom repositories @Meta annotation not working properly with MongoRepository and custom repositories Dec 13, 2024
@mp911de mp911de added type: documentation A documentation update and removed type: bug A general bug labels Dec 13, 2024
@mp911de mp911de self-assigned this Dec 13, 2024
@mp911de mp911de linked a pull request Dec 13, 2024 that will close this issue
@malaquf
Copy link
Author

malaquf commented Dec 13, 2024

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?

@malaquf
Copy link
Author

malaquf commented Dec 13, 2024

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)
		}
	}
}

@malaquf
Copy link
Author

malaquf commented Dec 13, 2024

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 :/

@malaquf
Copy link
Author

malaquf commented Dec 13, 2024

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.

@malaquf
Copy link
Author

malaquf commented Dec 17, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation A documentation update
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants