Philosophy on LegacyUI backward compatibility with modules

kristopherschmidt · October 30, 2015, 3:26am

So, working on Legacy UI has brought a question to my mind about how modules are going to maintain compatibility with both platform 2.0 and older versions of OpenMRS.

Suppose a module has a ‘legacy’ ui component (JSP) that relies on the standard site header include, and we want the module to run against both 1.11 and platform 2.0. When the module runs on 1.11, its JSPs have to include the header from the ‘core’ WEB-INF path (WEB-INF/view). However when the module runs on 2.0, the header has been moved from core to the legacyui module. So the module’s JSP has to include the header from the ‘legacyui module’ WEB-INF path (WEB-INF/view/module/legacyui).

So, the module JSP either has to somehow code the header to be pulled from core or legacyui depending on the context it’s running in (ugh), or it will be necessary for the new version of the module to depend on legacyui, and for every 1.9-1.11 installation that wishes to use the new version of the module to also run legacyui.

Having every installation that upgrades a module to have to run legacyui has its own set of problems, including:

legacyui module currently depends on platform 2.0; it would need to be changed to depend on only 1.9
similar to the hibernatecompatibility module, it’s not ideal to force an additional module to be installed just to upgrade another module

Maybe I’m thinking about this all wrong and the idea is to force modules to support a new UI for running under platform 2, and don’t care if their JSPs break when running on it.

So for my own perspective, what is the ‘philosophy’ behind legacyui for module upgrades, is it

newer versions of modules should always depend on legacyui because that’s the new home of the code,
newer versions of modules should somehow depend on core or legacyui depending on their context
newer versions of modules should not touch their legacyuis, they will break on platform 2 but will have a slick platform 2 UI that will be used instead.
some other outcome that I haven’t considered

wyclif · November 2, 2015, 7:42pm

Why would someone running versions 1.9-1.11 install the legacyui module? Legacy UI module requires version 2.0+ of the platform because that is when it was moved out of the platform. It is pointless to install the module in an older openmrs version since it would already have the old UI

wyclif · November 2, 2015, 7:57pm

For any module that written against the old UI framework, it needs to be updated to reference the correct files in the legacy ui module to work with 2.0+

dkayiwa · November 2, 2015, 8:09pm

@wyclif i think what @kristopherschmidt is trying to say is that if a module developer who wants to support both platform 2.0 and below, changes the header reference from “/WEB-INF/template” to “/WEB-INF/view/module/legacyui/template”, then they would require the legacyui module to also be installed on versions of the platform before 2.0. Else they would need to release two separate modules, or a clever workaround if they are to release just one module supporting both.

burke · November 2, 2015, 8:15pm

Is it impossible for the Legacy UI module to host files in the original location? Could it introduce a symlink or copy files (if symlink isn’t an option) so references to /WEB-INF/template/ still work?

jdegraft · November 2, 2015, 8:28pm

I would like to add that whatever solutions we adopt have to work with other modules as well, such as the new UI modules based on uiframework.

darius · November 2, 2015, 9:46pm

On this specific topic, we shouldn’t have to do anything to handle this, because uiframework-based modules don’t depend on anything in the legacy UI.

kristopherschmidt · November 3, 2015, 12:18am

When the LegacyUI project first started, I suggested precisely this, that the module deployment process in core be changed to allow LegacyUI files to be copied to the original locations. That can theoretically be done (fairly easily, in the module deployer), but the solution that was adopted instead was to reference the assets under the legacyui module and re-write the JSPs to use paths like “/WEB-INF/view/module/legacyui/template”.

Now that the implementation is well underway, I think it’s necessary to discuss the impact these changes are going to have on other modules. Offhand I was thinking that new module versions that support legacy UI components would force legacyui to be installed, for the easiest compatibility path. (unless we were to completely reverse course r.e. physical deployment of files and change all the JSPs back). Module owners maintaining 2 different versions or introducing some sort of compatibility workaround would impact many lines of code in many modules forever, which to me is far worse than forcing legacyui to be installed (or making core deployment modifications).

Of course, the philosophy could be that we don’t expect legacy UI components in modules to work on platform 2.0, that the modules will have to support a 2.0-specific UI and that ‘legacyui’ only pertains to legacy UI elements of core. Which is one reason why I wanted to ask the community to clear up that particular requirement.

kristopherschmidt · November 3, 2015, 12:23am

@dkayiwa Precisely. Thanks for your clear explanation, you said it much better than I did

darius · November 3, 2015, 7:12am

+1 to @kristopherschmidt’s point that we shouldn’t require all modules to do (something) in order to expose a legacy UI in both Platform 1.x and Platform 2.x-with-legacyui-module.

As mentioned above, some solution where the module framework allows a module to put content directly under WEB-INF. (We should probably do this in a way that doesn’t require a new DTD version for module config.xml files.)

raff · November 3, 2015, 11:58am

It should be enough to hack the module loader to load legacyui as if it wasn’t a module rather a part of openmrs-core. It would also have the benefit of not having to change any mappings and jsps when moving them to the legacyui module.

kristopherschmidt · November 3, 2015, 1:13pm

@raff that was my initial suggestion. The main hesitation I would have about changing the module loader at this point is that it would undo a lot of the work that has already been done on legacyui – not only would the JSPs need to change but much of the work on Java classes and XML configs has been to replace viewnames with ‘/module/legacyui/…’ everywhere. I suppose there are several options for discussion:

change module loader to load _all_content under core. Pros: very simple change in module loader Cons: need to undo much work on legacyui. Also module loader changes probably are specific to legacyui unless the module config DTD is changed to allow any module to load content under core
change module loader to load only some content under core, specifically JSPs, taglibs, scripts that cannot be mapped through ordinary Spring configuration. Pros: Leaves most of the legacyui Java + Spring config the way it is, referring to /module/legacyui assets Cons: Module loader is more complicated. Setup is more difficult to understand, dividing legacyui module content into being served 2 different ways.
change module loader to load a copy of all content under core, content can be referenced through either path Pro: Allows existing legacyui changes to work. Module loader change is simplified. Con: Duplication. Content can be referenced either way, so no enforced practice.
Make no module loader changes, force installation of legacyui module Pro: Allows existing legacyui changes to work. Setup is easy to understand. Con: forces installation of legacyui module

I think it would be a pretty big decision to reverse course and go with #1, undoing a lot of the existing work, although it’s possible it could be the better long-term solution. I actually think it’s fine to go with #4, forcing installation of legacyui module – this is similar to any software distribution like Spring where if you upgrade any dependent library (say Jackson), you force an upgrade of any of Jackson’s dependent libraries as well. It’s just the nature of transitive software dependencies. Maybe the best thing is to improve OpenMRS’s ability to warn installers that a module is required, or find some easier way to install transitive module dependencies.

This is a pretty big architectural decision. Maybe worthy of a design call, or at least requiring consensus among /dev/5s as to the correct way to handle the sprint going forward.

raff · November 3, 2015, 1:24pm

will not work. The legacyui module must be compiled using Java 8 thus it won’t run on any version of platform prior to 2.0 (it depends on openmrs-api 2.0 compiled for Java 8).

I’m in favor of spiking on 1). It might not be such a simple change though. Remember our module loader is as old as OpenMRS Reverting changes done in legacyui can be actually quicker than continuing to adjust jsps and xml configs…

dkayiwa · November 3, 2015, 2:52pm

I would rather we hack the legacyui module instead of the platform’s module loader. So how about this?

raff · November 3, 2015, 3:16pm

I wouldn’t expect DWRServletRegistration to have such a feature but other than that it looks promising. It would be good to delete those files when a module is uninstalled. Does it mean that we no longer have to change controllers and mappings after copying them from openmrs-core?

mseaton · November 3, 2015, 5:31pm

To @dkayiwa’s point, in one of our early systems for Haiti, we introduced a mechanism for overriding core pages and templates. It’s a bit hacky, but it worked. You can see what we did here:

This does the actual work of copying files into the necessary places (and cleaning up after itself if module is removed)

github.com

PIH/openmrs-module-pihhaiti/blob/master/api/src/main/java/org/openmrs/module/pihhaiti/CustomBrandingHandler.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.pihhaiti;

import java.io.File;
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;

This file has been truncated. show original

This is invoked by this file: https://github.com/PIH/openmrs-module-pihhaiti/blob/master/omod/src/main/java/org/openmrs/module/pihhaiti/web/CustomBrandingInitializer.java

This is automatically run by Spring at the appropriate time by virtue of it being included here:

github.com

PIH/openmrs-module-pihhaiti/blob/master/omod/src/main/resources/webModuleApplicationContext.xml

<?xml version="1.0" encoding="UTF-8"?>

<beans xmlns="http://www.springframework.org/schema/beans"
       xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
       xmlns:context="http://www.springframework.org/schema/context"
       xmlns:util="http://www.springframework.org/schema/util"
       xsi:schemaLocation="http://www.springframework.org/schema/beans 
           http://www.springframework.org/schema/beans/spring-beans-2.5.xsd
           http://www.springframework.org/schema/context
           http://www.springframework.org/schema/context/spring-context-2.5.xsd
           http://www.springframework.org/schema/util
           http://www.springframework.org/schema/util/spring-util-2.0.xsd">
	  
	<bean class="org.openmrs.module.pihhaiti.web.CustomBrandingInitializer" />
</beans>

Mike

darius · November 3, 2015, 6:32pm

Yes, @dkayiwa, hacking legacyui instead of openmrs-core is great, and this is a great approach!

Like @raff says, this doesn’t need to be a servlet (and definitely shouldn’t be hacked into an existing servlet that does something else) and can be done more like @mseaton’s example.

I don’t particularly care about deleting the files afterwards (though it would be slightly nice to do this). The main use cases going forwards for new UIs is to (a) be provided by modules, or (b) be free-standing JS applications. So if the legacyui module pollutes WEB-INF/view even after the module has been uninstalled is not going to affect anything.

dkayiwa · November 3, 2015, 9:07pm

Moved the code into a servlet context aware bean as per @mseaton 's example and committed at:

kristopherschmidt · November 4, 2015, 1:08am

This is great!

This solution makes a copy of the content, though, rather than deploying it to a completely different location (closer to options #2 and #3 than #1)

Since the content can be referenced through either the original path or the updated /module/legacyui path, we could keep the existing changes to the legacyui module, but is that what we want to do? Having 2 different ways to reference things with no convention will be confusing.

So, do we reverse the path updates that have been done in legacyui and make the convention that everything be referenced out of original core locations, or keep the updates and make up some other convention? Should the convention be enforced? (like moving the content instead of copying it)

wyclif · November 4, 2015, 6:26pm

@dkayiwa in your commit i don’t seem to see where you delete the files when the module is stopped. I would also imagine that the copying of the files should happen in the module activator’s started method while deleting happens in the stopped method. You would need to add a spring bean and use @Autowire to autowire the ServletContext object into the this bean instead of implementing ServletContextAware, then in your activator methods you can get access to the servletContext object from this bean.

I would also imagine that we need to get rid of all the changes we made in the module for the URL mappings and controllers, although i don’t think we have to do it if time doesn’t permit since it is fairly a huge task