DATACOUCH-550 - Refactored the usage of CouchbasePersistentEntityIndexCreator

[aaronjwhiteside]

pom.xml

• updated CouchbaseMock dependency to the official release on maven central, which now includes the required commit.

CouchbaseTemplate.java

• remove creation of CouchbasePersistentEntityIndexCreator
• there can only be a single CouchbaseMappingContext and CouchbasePersistentEntityIndexCreator, not sure why there was code dealing with multiples of each, and there was no corresponding tests for these assumptions.
• no longer does the job of a @Configuation class by gluing beans together with their dependencies.

CouchbaseMappingContext.java

• removed references to auto index creation, that functionality should be a completely separate concern.

CouchbasePersistentEntityIndexCreator.java

• as well as implementing ApplicationListener, it now also implements InitializingBean
• when afterPropertiesSet() is called it uses CouchbaseMappingContext#getPersistentEntities to retrieve the list of entity candidates for index creation. This will handle the creation of indices for entities known at @configuration time.
• for entities not in the initialEntitySet of the MappingContext, the ApplicationListener will handle those entities at runtime.
• this is needed because the AbstractMappingContext will emit events when it is initialized via InitializingBean, and any beans that depend on MappingContext and also implement ApplicationListener expecting to receive those events won't.
• in our case because Spring will have fully initialised the CouchbaseMappingContext instance before passing it to the CouchbasePersistentEntityIndexCreator instance we won't get those events.
• added a boolean flag to indicate if automatic index creation is to be enabled or now, passed in by the factory method that creates this bean.
• minor logging cleanup, use property placeholders where appropriate

AbstractCouchbaseConfiguration.java

• added a new factory method for the CouchbasePersistentEntityIndexCreator bean.
• CouchbaseMappingContext no longer has anything to do with index creation, other than supplying a candidate list of entities that may need indices.

Prompted by:
spring-projects/spring-boot#24525
and the discussion on #295

[mikereiche]

Any persistent entities added to a mapping context after application initialization (including creation of a new mapping context) will not be processed for autoindex generation.

[aaronjwhiteside]

So what are the actual use cases for these? I didn't see any tests for these use-cases/assumptions.

How would entities be added to the running application dynamically?

And for multiple MappingContext's they would need to create a corresponding new CouchbasePersistentEntityIndexCreator instance. Lots of places in the code make the assumption that there is only a single MappingContext.. for example the CouchbaseTemplate by way of CouchbaseConverter itself, so this isn't without precedent.

[mikereiche]

If UserX is outside the @EnableCouchbaseRepositories package, its indexes won't be generated.
I don't know who would do this - but there's nothing preventing it.

    @Autowired private ApplicationContext applicationContext; // added
    @Autowired private CouchbaseClientFactory couchbaseClientFactory; // added
    
        MappingContext mc= new CouchbaseMappingContext();
        ((CouchbaseMappingContext)mc).setApplicationContext(applicationContext); // added - second edit
        CouchbaseConverter cvtr = new MappingCouchbaseConverter(mc);
        CouchbaseTemplate tmpl  = new CouchbaseTemplate(couchbaseClientFactory, cvtr);
        tmpl.setApplicationContext(applicationContext); // added.  This was missing from initial post.
        UserX u3 = tmpl.findById(UserX.class).one(someId);

[aaronjwhiteside]

So it seems to me that this depends entirely on how the MappingContext was constructed.. it's typically the responsibility of the @Configuration bean to find and supply the context with potential entities..

In the vanilla Spring Data Couchbase, AbstractCoucbbaseConfiguration#getInitialEntitySet() uses org.springframework.context.annotation.ClassPathScanningCandidateComponentProvider scanning from the base package of the concrete class that extends AbstractCoucbbaseConfiguration.

In Spring Boot org.springframework.boot.autoconfigure.data.couchbase.CouchbaseDataConfiguration uses org.springframework.boot.autoconfigure.domain.EntityScanner which has its own logic for resolving potential entities.

And finally if someone is manually constructing beans as in your example, and if they forget to call setInitialEntitySet() then that's incorrect usage/a bug on their part? In your example there won't ever be any MappingContextEvent's for the indexer to consume because no one called setInitialEntitySet() on the MappingContext.

I don't think it's ever been the concern of the MappingContext to know how entities are found/resolved, that's always been something that's been externalised? Separation of concern..

Without test cases for the assumptions, it's really hard to ever know what the original intent was, and even harder to make sure we don't break it when making changes. But working through it logically I can't devine any meaningful reason that it needs to stick around?

Especially since if you're going through the effort of creating multiple MappingContext's and associated beans that depend on said MappingContext's, the functionality already exists to make it work as is.. as long as you glue things together correctly and call setInitialEntitySet() on the bespoke MappingContext instance?

@daschl's name is on the git blame for those original lines, perhaps he might have some more insight/context?

[mikereiche]

"So it seems to me that this depends entirely on how the MappingContext was constructed."

It shouldn't and it doesn't. But changing CouchbasePersistentIndexCreator to a configuration bean would make it so.

"And finally if someone is manually constructing beans as in your example, and if they forget to call setInitialEntitySet() then that's incorrect usage/a bug on their part?"

No. Entities can be added afterwards, just as in the example I provided. The applicationListener allows that.

"I don't believe that it's ever been the concern of the MappingContext to know how entities are found/resolved, that's always been something that's been externalised? "

It's (Abstract)MappingContext that resolves entity classes producing PersistentEntity objects. Maybe I misunderstood something.

[aaronjwhiteside]

I edited the example to add the missing setApplicationContext() call.

Actually it's needed on all the instances, both MappingCouchbaseConverter and CouchbaseMappingContext implement ApplicationContextAware. But at this point I feel the example is a little contrived, even more than you probably intended.

Suffice to say I can now appreciate that not all entities will be discovered at @Configuration time, and some can come at runtime. Whether that is a bug or not can be argued about, but since AbstractMappingContext explicitly deals with it, it makes sense we conform as well.

I'm not convinced that adding a second mechanism for auto-indexing is simpler than fixing the original mechanism.

The code/design in its current form is complex, it introduces unnecessary cognitive load for something that I've shown can be much simpler.

There is no need to pollute the CouchbaseTemplate class with concerns of glueing things together for the purpose of indexing.

The same goes for CouchbaseMappingContext class, it does not need to cross the boundary into the functionality of indexing.

This doesn't need to be a special case:
https://github.com/spring-projects/spring-data-couchbase/blob/master/src/main/java/org/springframework/data/couchbase/core/mapping/CouchbaseMappingContext.java#L141-L147

There doesn't need to be a questionable cyclic dependency between CouchbaseMappingContext and CouchbasePersistentEntityIndexCreator.

Keeping things simple means having a single place that deals with indexing, and that seems to be the CouchbasePersistentEntityIndexCreator, it's right there in the name..

CouchbasePersistentEntityIndexCreator does not need to leak details of its internal implementation, and require logic in CouchbaseMappingContext and CouchbaseTemplate to make it work correctly.

I don't claim this is the simplest it can be, but it IS much simpler than the current design.

127 lines removed and 44 added.

Indexing logic in a single self-contained location.

Clean separation of concerns.

[mikereiche]

Last commit causes this failure:

Error starting ApplicationContext. To display the conditions report re-run your application with 'debug' enabled.
2020-12-18 11:01:36.663 ERROR 17469 --- [           main] o.s.b.d.LoggingFailureAnalysisReporter   : 

***************************
APPLICATION FAILED TO START
***************************

Description:

The dependencies of some of the beans in the application context form a cycle:

   cmdRunner (field private com.test.UserRepository com.test.CmdRunner.userRepository)
      ↓
   couchbaseRepositoryOperationsMapping defined in class path resource [com/test/CouchbaseConfig.class]
┌─────┐
|  couchbaseTemplate defined in class path resource [com/test/CouchbaseConfig.class]
↑     ↓
|  couchbaseMappingConverter defined in class path resource [com/test/CouchbaseConfig.class]
↑     ↓
|  couchbaseMappingContext defined in class path resource [com/test/CouchbaseConfig.class]
↑     ↓
|  couchbasePersistentEntityIndexCreator defined in class path resource [com/test/CouchbaseConfig.class]
└─────┘

[mikereiche]

After fixing the cyclic dependency, classes found later do not get processed as the mappingContext they are emitted from is different from the the mappingContext of the CouchbasePersistentEntityIndexCreator. This seems to be the reason that the indexCreator is tied to the template.

onApplicationEvent()
if (!enabled || !event.wasEmittedBy(mappingContext))

[aaronjwhiteside]

@mikereiche crap, that'll serve me right for pushing a commit without running the build first.

fixed the cyclic dependency, and pushed.

I'm not seeing any more test failures now though

[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 118, Failures: 0, Errors: 0, Skipped: 0
....
[INFO] 
[INFO] Tests run: 37, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
....
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  41.636 s
[INFO] Finished at: 2020-12-19T10:04:56+11:00
[INFO] ------------------------------------------------------------------------

I also tested this in a Spring Boot (2.4.1) application we have, and other than having to manually create the CouchbasePersistentEntityIndexCreator bean because it's not in the spring boot auto configuration yet, I didn't have any issues.

Can you share a sample project or code that I can use to reproduce this issue? I can then also add test cases to this repo to ensure it doesn't get broken again..

[aaronjwhiteside]

emitted from is different from the the mappingContext of the CouchbasePersistentEntityIndexCreator. This seems to be the reason that the indexCreator is tied to the template.

So I've been thinking about this, I think I said it before but maybe not as succinctly. If you're explicitly creating a new MappingContext, then just like the need to explicitly create a dedicated CouchbaseConverter and CouchbaseTemplate that are bound to the new MappingContext, you should also create a dedicated CouchbasePersistentEntityIndexCreator. This seems to be the established pattern.

It shouldn't be seen as a burden, for most people they won't even know about it since it's hidden away in a @Configuration class. And for those that need to customize, then it becomes another implementation detail they have to deal with.. like all the others.

But thinking a little bit more, we could however modify the CouchbasePersistentEntityIndexCreator to take a List<MappingContext> in the constructor instead of a single MappingContext, but I feel this goes against the already established pattern?

And it won't work if you create things outside Spring, but I'm not very concerned with that. Because this is after all Spring Data Couchbase, so the expectation is that Spring is always involved to work it's magic, for better or worse.

[mikereiche]

Please use the standard format for the first line of the commit message. i.e.

DATACOUCH-550 - Refactored the usage of CouchbasePersistentEntityIndexCreator.

[aaronjwhiteside]

Please use the standard format for the first line of the commit message. i.e.

DATACOUCH-550 - Refactored the usage of CouchbasePersistentEntityIndexCreator.

Done