TRUNK-6418: Follow up adjustments

Author: rkorytkowski

api/src/main/java/org/openmrs/api/AdministrationService.java

@@ -426,29 +426,33 @@ public interface AdministrationService extends OpenmrsService {
 	/**
 	 * Checks whether a core setup needs to be run due to a version change.
 	 *
+	 * @since 2.9.0
 	 * @return true if core setup should be executed because of a version change, false otherwise
 	 */
-	public boolean isCoreSetupOnVersionChangeNeeded();
+	boolean isCoreSetupOnVersionChangeNeeded();
 
 	/**
 	 * Checks whether a module setup needs to be run due to a version change.
 	 *
+	 * @since 2.9.0
 	 * @param moduleId the identifier of the module to check
 	 * @return true if the module setup should be executed because of a version change, false otherwise
 	 */
-	public boolean isModuleSetupOnVersionChangeNeeded(String moduleId);
+	boolean isModuleSetupOnVersionChangeNeeded(String moduleId);
 
 	/**
 	 * Executes the core setup procedures required after a core version change.
 	 *
-	 * @throws DatabaseUpdateException
+	 * @since 2.9.0
+	 * @throws DatabaseUpdateException if the core setup fails
 	 */
-	public void runCoreSetupOnVersionChange() throws DatabaseUpdateException;
+	void runCoreSetupOnVersionChange() throws DatabaseUpdateException;
 
 	/**
 	 * Executes the setup procedures required for a module after a module version change.
 	 *
+	 * @since 2.9.0
 	 * @param module the module for which the setup should be executed
 	 */
-	public void runModuleSetupOnVersionChange(Module module);
+	void runModuleSetupOnVersionChange(Module module);
 }

api/src/main/java/org/openmrs/api/context/Context.java

@@ -1009,20 +1009,22 @@ public static synchronized void startup(Properties props) throws DatabaseUpdateE
 		// do any context database specific startup
 		getContextDAO().startup(props);
 
-		// find/set/check whether the current database version is compatible
-		checkForDatabaseUpdates(props);
+		if (getAdministrationService().isCoreSetupOnVersionChangeNeeded()) {
+			log.info("Detected core version change. Running core setup hooks and Liquibase.");
+			getAdministrationService().runCoreSetupOnVersionChange();
+		}
 
 		// this should be first in the startup routines so that the application
 		// data directory can be set from the runtime properties
 		OpenmrsUtil.startup(props);
-
+		
 		openSession();
 		clearSession();
 
 		// add any privileges/roles that /must/ exist for openmrs to work
 		// correctly.
 		checkCoreDataset();
-
+		
 		getContextDAO().setupSearchIndex();
 
 		// Loop over each module and startup each with these custom properties
@@ -1282,44 +1284,6 @@ public static void checkCoreDataset() {
 				OpenmrsConstants.GP_ALLERGEN_OTHER_NON_CODED_UUID));
 	}
 
-	/**
-	 * Runs any needed updates on the current database if the user has the allow_auto_update runtime
-	 * property set to true. If not set to true, then {@link #updateDatabase(Map)} must be called.<br>
-	 * <br>
-	 * If an {@link InputRequiredException} is thrown, a call to {@link #updateDatabase(Map)} is
-	 * required with a mapping from question prompt to user answer.
-	 *
-	 * @param props the runtime properties
-	 * @throws InputRequiredException if the {@link DatabaseUpdater} has determined that updates
-	 *             cannot continue without input from the user
-	 * @see InputRequiredException#getRequiredInput() InputRequiredException#getRequiredInput() for
-	 *      the required question/datatypes
-	 */
-	private static void checkForDatabaseUpdates(Properties props) throws DatabaseUpdateException, InputRequiredException {
-		boolean updatesRequired;
-		try {
-			updatesRequired = DatabaseUpdater.updatesRequired();
-		}
-		catch (Exception e) {
-			throw new DatabaseUpdateException("Unable to check if database updates are required", e);
-		}
-
-		// this must be the first thing run in case it changes database mappings
-		if (updatesRequired) {
-			if (!DatabaseUpdater.allowAutoUpdate()) {
-				throw new DatabaseUpdateException(
-						"Database updates are required.  Call Context.updateDatabase() before .startup() to continue.");
-			}
-		}
-		
-		if (getAdministrationService().isCoreSetupOnVersionChangeNeeded()) {
-			log.info("Detected core version change. Running core setup hooks and Liquibase.");
-			getAdministrationService().runCoreSetupOnVersionChange();
-		}
-		
-		log.info("Database update check completed.");
-	}
-
 	/**
 	 * Updates the openmrs database to the latest. This is only needed if using the API alone. <br>
 	 * <br>

api/src/main/java/org/openmrs/api/db/AdministrationDAO.java

@@ -60,6 +60,14 @@ public interface AdministrationDAO {
 	 * @see org.openmrs.api.AdministrationService#saveGlobalProperty(org.openmrs.GlobalProperty)
 	 */
 	public GlobalProperty saveGlobalProperty(GlobalProperty gp) throws DAOException;
+
+	/**
+	 * Internal method for AdministrationService to overwrite or create a new global property without fetching.
+	 * <p>
+	 * It is to go around an issue with case-sensitive session.get and non-case-sensitive primary key resulting
+	 * in multiple objects in a session with the same primary key, but different casing...
+	 */
+	public GlobalProperty saveGlobalProperty(String name, String value, String description) throws DAOException;
 
 	/**
 	 * @see org.openmrs.api.db.AdministrationDAO#executeSQL(java.lang.String, boolean)

api/src/main/java/org/openmrs/api/db/hibernate/HibernateAdministrationDAO.java

@@ -113,10 +113,16 @@ public GlobalProperty getGlobalPropertyObject(String propertyName) {
 
 			query.where(condition);
 
-			return session.createQuery(query).uniqueResult();
-		} else {
-			return session.get(GlobalProperty.class, propertyName);
+			GlobalProperty gp = session.createQuery(query).uniqueResult();
+			if (gp != null) {
+				// GP may be null, but the session may contain an unflushed gp so
+				// we will do a final check with session.get below. It may happen,
+				// if flush is set to manual and running in a larger transaction.
+				return gp;
+			}
 		}
+		
+		return session.get(GlobalProperty.class, propertyName);
 	}
 
 	@Override
@@ -199,11 +205,17 @@ public GlobalProperty saveGlobalProperty(GlobalProperty gp) throws DAOException
 			sessionFactory.getCurrentSession().update(gpObject);
 			return gpObject;
 		} else {
-			sessionFactory.getCurrentSession().save(gp);
+			sessionFactory.getCurrentSession().saveOrUpdate(gp);
 			return gp;
 		}
 	}
-	
+
+	@Override
+	public GlobalProperty saveGlobalProperty(String name, String value, String description) throws DAOException {
+		GlobalProperty gp = new GlobalProperty(name, value, description);
+		return (GlobalProperty) sessionFactory.getCurrentSession().merge(gp);
+	}
+
 	/**
 	 * @see org.openmrs.api.db.AdministrationDAO#executeSQL(java.lang.String, boolean)
 	 */

api/src/main/java/org/openmrs/api/impl/AdministrationServiceImpl.java

@@ -25,12 +25,14 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Properties;
 import java.util.Set;
 import java.util.SortedMap;
 import java.util.TreeMap;
 
 import org.apache.commons.lang3.StringUtils;
+import org.jgroups.Global;
 import org.openmrs.ConceptSource;
 import org.openmrs.GlobalProperty;
 import org.openmrs.ImplementationId;
@@ -984,47 +986,46 @@ public static List<Class<?>> getSerializerDefaultWhitelistHierarchyTypes() {
 			PersonMergeLogData.class);
 		return types;
 	}
-
-    @Override
-    @SuppressWarnings("unchecked")
-    public <T> T getRefByUuid(Class<T> type, String uuid) {
-        if (GlobalProperty.class.equals(type)) {
-            return (T) getGlobalPropertyByUuid(uuid);
-        }
-        throw new APIException("Unsupported type for getRefByUuid: " + type != null ? type.getName() : "null");
-    }
-
-    @Override
-    public List<Class<?>> getRefTypes() {
-        return Arrays.asList(GlobalProperty.class);
-    }
-
+	
 	/**
 	 * @see org.openmrs.api.AdministrationService#isCoreSetupOnVersionChangeNeeded() 
 	 */
 	@Override
 	public boolean isCoreSetupOnVersionChangeNeeded() {
+		boolean forceSetup = Boolean.parseBoolean(Context.getRuntimeProperties().getProperty("force.setup", "false"));
+		if (forceSetup) {
+			return true;
+		}
+		
 		String stored = getStoredCoreVersion();
 		String current = OpenmrsConstants.OPENMRS_VERSION_SHORT;
-		boolean forceSetup = Boolean.parseBoolean(getGlobalProperty("force.setup", "false"));
-		
-		return forceSetup || !Objects.equals(stored, current);
+		return !Objects.equals(stored, current);
 	}
 
 	/**
 	 * @see org.openmrs.api.AdministrationService#isModuleSetupOnVersionChangeNeeded(String)
 	 */
 	@Override
 	public boolean isModuleSetupOnVersionChangeNeeded(String moduleId) {
-		String stored = getStoredModuleVersion(moduleId);
 		Module module = ModuleFactory.getModuleById(moduleId);
 		if (module == null) {
+			log.info("{} module is no longer installed, skipping setup", moduleId);
 			return false;
 		}
-		String current = module.getVersion();
+
 		boolean forceSetup = Boolean.parseBoolean(getGlobalProperty("force.setup", "false"));
+		if (forceSetup) {
+			return true;
+		}
 
-		return forceSetup || !Objects.equals(stored, current);
+		String stored = getStoredModuleVersion(moduleId);
+		String current = module.getVersion();
+		if (!Objects.equals(stored, current) || isCoreSetupOnVersionChangeNeeded()) {
+			return true;
+		} else {
+			log.info("{} module did not change, skipping setup", moduleId);
+			return false;
+		}
 	}
 
 	/**
@@ -1034,7 +1035,7 @@ public boolean isModuleSetupOnVersionChangeNeeded(String moduleId) {
 	@Transactional
 	public void runCoreSetupOnVersionChange() throws DatabaseUpdateException {
 		if (!ModuleFactory.getLoadedModules().isEmpty()) {
-			String prevCoreVersion = getStoredCoreVersion() != null ? getStoredCoreVersion() : OpenmrsConstants.OPENMRS_VERSION_SHORT;
+			String prevCoreVersion = getStoredCoreVersion();
 
 			for (Module module : ModuleFactory.getLoadedModules()) {
 				String prevModuleVersion = getStoredModuleVersion(module.getModuleId());
@@ -1043,7 +1044,24 @@ public void runCoreSetupOnVersionChange() throws DatabaseUpdateException {
 			}
 		}
 
+		boolean updatesRequired;
+		try {
+			updatesRequired = DatabaseUpdater.updatesRequired();
+		}
+		catch (Exception e) {
+			throw new DatabaseUpdateException("Unable to check if database updates are required", e);
+		}
+
+		// this must be the first thing run in case it changes database mappings
+		if (updatesRequired) {
+			if (!DatabaseUpdater.allowAutoUpdate()) {
+				throw new DatabaseUpdateException(
+					"Database updates are required. Call Context.updateDatabase() before .startup() to continue.");
+			}
+		}
+		
 		DatabaseUpdater.executeChangelog();
+		
 		storeCoreVersion();
 	}
 
@@ -1058,7 +1076,7 @@ public void runModuleSetupOnVersionChange(Module module) {
 		}
 
 		String moduleId = module.getModuleId();
-		String prevCoreVersion = getStoredCoreVersion() != null ? getStoredCoreVersion() : OpenmrsConstants.OPENMRS_VERSION_SHORT;
+		String prevCoreVersion = getStoredCoreVersion();
 		String prevModuleVersion = getStoredModuleVersion(moduleId);
 
 		ModuleFactory.runLiquibaseForModule(module);
@@ -1068,39 +1086,23 @@ public void runModuleSetupOnVersionChange(Module module) {
 	}
 
 	protected String getStoredCoreVersion() {
-		return getGlobalProperty("core.version");
+		return dao.getGlobalProperty("core.version");
 	}
 
 	protected String getStoredModuleVersion(String moduleId) {
-		return getGlobalProperty("module." + moduleId + ".version");
+		return dao.getGlobalProperty("module." + moduleId + ".version");
 	}
 
 	protected void storeCoreVersion() {
-		saveGlobalProperty("core.version", OpenmrsConstants.OPENMRS_VERSION_SHORT, "Saved the state of this core version for future restarts");
+		String propertyName = "core.version";
+		GlobalProperty gp = new GlobalProperty(propertyName, OpenmrsConstants.OPENMRS_VERSION_SHORT, 
+			"Saved core version for future restarts");
+		dao.saveGlobalProperty(gp);
 	}
 
 	protected void storeModuleVersion(String moduleId, String version) {
 		String propertyName = "module." + moduleId + ".version";
-		saveGlobalProperty(propertyName, version, "Saved the state of this module version for future restarts");
-	}
-
-	/**
-	 * Convenience method to save a global property with the given value. Proxy privileges are added so
-	 * that this can occur at startup.
-	 */
-	protected void saveGlobalProperty(String key, String value, String desc) {
-		try {
-			GlobalProperty gp = getGlobalPropertyObject(key);
-			if (gp == null) {
-				gp = new GlobalProperty(key, value, desc);
-			} else {
-				gp.setPropertyValue(value);
-			}
-
-			saveGlobalProperty(gp);
-		}
-		catch (Exception e) {
-			log.warn("Unable to save the global property", e);
-		}
+		GlobalProperty gp = new GlobalProperty(propertyName, version, "Saved module version for future restarts");
+		dao.saveGlobalProperty(gp);
 	}
 }

api/src/main/java/org/openmrs/module/ModuleActivator.java

@@ -58,11 +58,19 @@ public interface ModuleActivator {
 
 	/**
 	 * Called before Liquibase runs, but only if core or this module version changed.
+	 * 
+	 * @param previousCoreVersion previous core version of <code>null</code> if first install
+	 * @param previousModuleVersion previous module version or <code>null</code> if first install   
+	 * @since 2.9.0
 	 */
 	default void setupOnVersionChangeBeforeSchemaChanges(String previousCoreVersion, String previousModuleVersion) {}
 
 	/**
 	 * Called after Liquibase runs, but only if core or this module version changed.
+	 * 
+	 * @param previousCoreVersion previous core version of <code>null</code> if first install
+	 * @param previousModuleVersion previous module version or <code>null</code> if first install   
+	 * @since 2.9.0
 	 */
 	default void setupOnVersionChange(String previousCoreVersion, String previousModuleVersion) {}
 

api/src/main/java/org/openmrs/module/ModuleFactory.java

@@ -692,7 +692,7 @@ public static Module startModuleInternal(Module module, boolean isOpenmrsStartup
 				}
 
 				if (Context.getAdministrationService().isModuleSetupOnVersionChangeNeeded(module.getModuleId())) {
-					log.info("Detected version change for module {}. Running setup hooks and module Liquibase.", module.getModuleId());
+					log.info("Module {} changed, running setup.", module.getModuleId());
 					Context.getAdministrationService().runModuleSetupOnVersionChange(module);
 				}
 
@@ -752,7 +752,7 @@ public static Module startModuleInternal(Module module, boolean isOpenmrsStartup
 				module.clearStartupError();
 			}
 			catch (Exception e) {
-				log.warn("Error while trying to start module: " + moduleId, e);
+				log.error("Error while trying to start module: {}", moduleId, e);
 				module.setStartupErrorMessage("Error while trying to start module", e);
 				notifySuperUsersAboutModuleFailure(module);
 				// undo all of the actions in startup
@@ -768,7 +768,7 @@ public static Module startModuleInternal(Module module, boolean isOpenmrsStartup
 				catch (Exception e2) {
 					// this will probably occur about the same place as the
 					// error in startup
-					log.debug("Error while stopping module: " + moduleId, e2);
+					log.debug("Error while stopping module: {}", moduleId, e2);
 				}
 			}
 
@@ -944,6 +944,7 @@ private static void runDiff(Module module, String version, String sql) {
 
 	/**
 	 * This is a convenience method that exposes the private {@link #runLiquibase(Module)} method.
+	 * @since 2.9.0
 	 */
 	public static void runLiquibaseForModule(Module module) {
 		runLiquibase(module);