Multiple repositories per domain class lead to ambiguous conversion [DATACMNS-1142]

[spring-projects-issues]

Burkhard Graves opened DATACMNS-1142 and commented

If there are two instances of a CrudRepository for a given domain class, one unsecured and one secured by using @PreAuthorize, it's totally ambiguous which repository is used if request parameters or path variables are converted to instances of the domain class (which is done by DomainClassConverter respectively its inner class ToEntityConverter).

If the unsecured repository is used everything works fine, if the secured one is used - bang.

This is related to DATAREST-923, made a similar comment over there

---

Affects: 1.13.6 (Ingalls SR6)

Referenced from: pull request #465

1 votes, 3 watchers

[spring-projects-issues]

Jan Zeppenfeld commented

Hey guys, because I stumbled upon the same issue in our current project, I found this thread and analyzed what's going on under the hood and tried to figure out how it can be fixed.

Actually, two independent problems already mentioned in the comments of DATAREST-923 prevent this feature from working.

The @Primary issue was first tackled in DATACMNS-1448 by changing the Repositories code but because the 'primary' state didn't make it to the actual bean definition of the Spring Data Repositories, the actual 'primary' state never reached the Repositories object and therefore DATACMNS-1591 has been introduced in Spring Data 2.2.1 - I was about to analyze and submit a fix for this issue when I saw that it had already been fixed.

Now the 'primary' state reaches the Repositories object but as Bartosz Kielczewski mentioned in https://jira.spring.io/browse/DATAREST-923?focusedCommentId=182769&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-182769

ConfigurableListableBeanFactory.class::isInstance is false

This is the case because

• usually the ApplicationContext is directly passed to the Repositories constructor (as in RepositoryRestMvcConfiguration.repositories()) which is possible because all contexts implement ListableBeanFactory
• but there isn't any ApplicationContext implementing ConfigurableListableBeanFactory although all contexts contain such a bean factory (see below).

The implemented Test RepositoriesUnitTests.keepsPrimaryRepositoryInCaseOfMultipleOnes() also directly passes a DefaultListableBeanFactory to the Repositories object instead of the created GenericApplicationContext which obviously works as explained above.

So, there are four possible approaches to fix this issue from my point of view... from the cleanest and most invasive to the least invasive one:

1. From my point of view the root cause of this issue is that there are ApplicationContexts, especially ConfigurableApplicationContext, which just implement ListableBeanFactory directly or indrectly and contain a ConfigurableListableBeanFactory at the same time, which they return in methods and which they delegate to to fill out the ListableBeanFactory. This seems a bit contradictory because if it contains a ConfigurableListableBeanFactory, returns it explicitly via methods (public ConfigurableListableBeanFactory getBeanFactory()) and delegates to it to fill out the ListableBeanFactory, those contexts should implement ConfigurableListableBeanFactory... and not just ListableBeanFactory. But I guess that this happened to keep backward compatibility.
2. Change the Repositories constructor type from ListableBeanFactory to ConfigurableListableBeanFactory and explicitly pass a corresponding bean factory to the Repositories object instead of passing the ApplicationContext. This would be even simplier if the above issue was solved. But even without this change it would still be possible in most cases because all contexts implement ConfigurableApplicationContext which allows to get a ConfigurableListableBeanFactory via getBeanFactory(). However, in the end this change would be a breaking change and all depending packages/code would have to be updated.
3. Add an additional constructor with type ConfigurableListableBeanFactory and adapt dependent packages/code step by step. This would not introduce a breaking change but would only fix the current bug for cases in which this new constructor is used.
4. Adapt the Repositories.cacheFirstOrPrimary() method to check whether the given beanFactory implements ConfigurableApplicationContext and if so, get the ConfigurableListableBeanFactory from it.

Although I would prefer the cleaner solutions (1+2), I guess the fourth approach embracing backward compatibility would be the preferred one. So here is a pull request