Why is MessageSourceServiceImpl transactional?

wyclif · August 14, 2015, 8:29pm

I’m running into a bug with the UI framework where if i submit a form and i get sent back to the form with validation errors, the changes i made get persisted to the DB (premature hibernate flushes), after some debugging i figured out the cause of this was due to the fact that MessageSourceServiceImpl is transactional, spring will attempt a flush on returning from any transactional method where readOnly attribute is not set to true:

github.com

openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/messagesource/impl/MessageSourceServiceImpl.java#L37




/**
* Extensible implementation of the MessageSourceService, which relies on injected implementations
* of MutableMessageSource to actually provide the services. The sub-services are loaded by special
* BeanPostProcessor handling, which looks for beans named For example:
* ResourceBundleMessageSourceService can be specified in the applicationContext-service.xml file to
* use the usual .properties files to provide messages.
*/
public class MessageSourceServiceImpl implements MessageSourceService {
	
	private static final Logger log = LoggerFactory.getLogger(MessageSourceServiceImpl.class);
	
	private Set<MutableMessageSource> availableMessageSources = new HashSet<>();
	
	private MutableMessageSource activeMessageSource;
	
	/**
	 * @see org.openmrs.messagesource.MessageSourceService#getMessage(java.lang.String)
	 */
	@Override
	public String getMessage(String s) {

How is this related to the ui framework and the bug i was running into? Because when the form has validation errors, the field error widget in uicommons module calls a method in MessageSourceServiceImpl to translate the error codes that doesn’t have readOnly set to true therefore the changes always gets flushed to the DB. I believe MessageSourceServiceImpl shouldn’t be transactional in the first place since it doesn’t have any DAO interaction. I will go ahead and create a ticket to fix this unless some one thinks otherwise.

burke · August 17, 2015, 4:18pm

MutableMessageSource is marked as transactional as well:

github.com

openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/messagesource/MutableMessageSource.java#L24




import org.springframework.context.HierarchicalMessageSource;
import org.springframework.context.MessageSource;


/**
* Extended MessageSource interface, which provides more information about the available messages
* and can be changed.
*/
public interface MutableMessageSource extends MessageSource, HierarchicalMessageSource {
	
	/**
	 * Gets the locales for which messages are available from this source.
	 * 
	 * @return available message locales
	 */
	public Collection<Locale> getLocales();
		
	/**
	 * Gets all of the available messages, packaged as PresentationMessages.
	 * 
	 * @return collection of presentation messages

burke · August 17, 2015, 4:23pm

I don’t see where MessageSourceService or MutableMessageSource use the database. They seem to be working with hashes in memory. So, unless someone can enlighten us, I would think you should remove @Transactional from both classes.

mogoodrich · August 17, 2015, 4:41pm

Generally sounds good to me… however, maybe it should stay @Transactional but readOnly=true? Doesn’t the custom messaging module register a custom message handler (which I assume would still be accessed through the message source service) which pulls messages from a new DB table it creates?

wyclif · August 17, 2015, 4:47pm

That is interesting Mark, I’m not sure how the custom messaging module works, does it register a custom MessageSource and configures it as the activeMessageSource in MessageSourceServiceImpl?

wyclif · August 17, 2015, 4:49pm

I have created a JIRA issue to address this:

mogoodrich · August 17, 2015, 9:11pm

Not 100% sure, but I believe so. @mseaton knows more.

mseaton · August 17, 2015, 11:36pm

Yes, see the “setApplicationContext” method on this class:

github.com

openmrs/openmrs-module-custommessage/blob/master/api/src/main/java/org/openmrs/module/custommessage/CustomMessageSource.java

/**
 * The contents of this file are subject to the OpenMRS Public License
 * Version 1.0 (the "License"); you may not use this file except in
 * compliance with the License. You may obtain a copy of the License at
 * http://license.openmrs.org
 *
 * Software distributed under the License is distributed on an "AS IS"
 * basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See the
 * License for the specific language governing rights and limitations
 * under the License.
 *
 * Copyright (C) OpenMRS, LLC.  All Rights Reserved.
 */
package org.openmrs.module.custommessage;

import java.text.MessageFormat;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
import java.util.LinkedHashMap;

This file has been truncated. show original

wyclif · August 18, 2015, 4:42pm

Looks like we need to update CustomMessageServiceImpl to make the get methods readOnly too, but this worries me that someone else might later register a new message source and carelessly introduce the same issue, i believe our code convention is that getters in the service layer should always be readOnly and we should try to enforce it when reviewing code.