Circular dependency between openmrsEventListeners and adminServiceTarget

Development
,
#1

Hi,

I am working on this issue https://issues.openmrs.org/browse/TRUNK-4663. as part of this issue, I am trying to convert applicationContext-service.xml to java config. During this conversion i came across this circular dependency issue between openmrsEventListeners and adminServiceTarget beans, they works fine with xml configuration and throws “globalPropertyListener threw exception; nested exception is java.lang.StackOverflowError” when i run with translated spring java config. please find both xml and java bean configurations below. The problem is in the adminServiceTarget which is being referenced in openmrsEventListeners and if you look at adminServiceTarget bean definition, it intern requires openmrsEventListeners.

Existing xml config

  <bean id="adminServiceTarget" class="org.openmrs.api.impl.AdministrationServiceImpl">
	<property name="administrationDAO"><ref bean="adminDAO"/></property>	
	<property name="eventListeners"><ref bean="openmrsEventListeners"/></property>
	<property name="globalLocaleList"><ref bean="globalLocaleList"/></property>
	<property name="implementationIdHttpClient"><ref bean="implementationIdHttpClient"/></property>
</bean>

    <bean id="openmrsEventListeners" class="org.openmrs.api.EventListeners" depends-on="clearOpenmrsEventListeners">
	<property name="globalPropertyListeners">
		<list value-type="org.openmrs.api.GlobalPropertyListener">
			<bean class="org.openmrs.util.LocaleUtility" />
			<bean class="org.openmrs.util.LocationUtility" />
			<bean class="org.openmrs.api.impl.PersonNameGlobalPropertyListener" />
			<ref bean="globalLocaleList" />
			<ref bean="adminServiceTarget" />
            <ref bean="orderServiceTarget" />
		</list>
	</property>
</bean>

After translating the above xml config to Java config I have the below code

    @Bean
@DependsOn("clearOpenmrsEventListeners")
public EventListeners openmrsEventListeners() throws IOException {
	EventListeners eventListeners = clearOpenmrsEventListeners();
	List<GlobalPropertyListener> globalPropertyListeners = new ArrayList<GlobalPropertyListener>();
	globalPropertyListeners.add(localUtility());
	globalPropertyListeners.add(locationUtility());
	globalPropertyListeners.add(personNameGlobalPropertyListener());
	globalPropertyListeners.add(globalLocaleList());
	//Commented to resolve the cyclic dependency
	globalPropertyListeners.add(adminServiceTarget());
	globalPropertyListeners.add(orderServiceTarget());
	
	eventListeners.setGlobalPropertyListeners(globalPropertyListener());
	return eventListeners;
}

    @Bean
public AdministrationServiceImpl adminServiceTarget() throws IOException {
	AdministrationServiceImpl administrationServiceImpl = new AdministrationServiceImpl();
	administrationServiceImpl.setAdministrationDAO(adminDAO());
	administrationServiceImpl.setEventListeners(openmrsEventListeners());
	administrationServiceImpl.setGlobalLocaleList(globalLocaleList());
	administrationServiceImpl.setImplementationIdHttpClient(implementationIdHttpClient());
	return administrationServiceImpl;
}
#2

The globalPropertyListeners list is actually a field on the AdministrationServiceImpl (adminServiceTarget) class, it just happens that the AdministrationService is also one of the event listeners to be added to the list hence some sort of circular dependency which seems like a bad design. You could set one dependent bean via a BeanFactoryAware instance or BeanPostProcessor, possibly the AdministrationService.

But personally, I think the better solution would be to move the logic in question out of the AdministrationService to avoid the circular dependency in the first place, it’s the logic that requires it to listen for GP changes or deletes. The reason why admin service is an event listener is because of 2 global property caches that it needs to update i.e locale.allowed.list to update the presentationLocales cache and search.caseSensitiveDatabaseStringComparison to update the databaseStringComparisonCaseSensitive cache. We could move the presentationLocales to the LocaleUtility class which is already GP listener, move databaseStringComparisonCaseSensitive to DatabaseUtil and make it a GlobalPropertyListener and then AdministrationServiceImpl will no longer have to implement GlobalPropetyListener.

As a side note, I think the databaseStringComparisonCaseSensitive cache doesn’t get updated when the GP value changes because the AdministrationServiceImpl.supportsPropertyName method doesn’t take it into account, will create a ticket for this.

1 Like
#3

Maybe this is why XML configuration may still be better because Spring would find a way to work around the possible circular dependency, I commented on TRUNK-4663 about getting a consensus on switching from XML anyways

1 Like