fix config.xml dtd file version 1.6

Development
,
1

Hi there @burke @darius,

I am trying to fix our document type definition files (we refer to undeclared elements) for our modules config.xml that we have uploaded at http://resources.openmrs.org/doctype/config-1.X.dtd and put them under version control in openmrs-core. So they are useful to devs when writing config.xml in their IDEs and also so we can potentially also use them for validation in our ModuleFileParser (better error messages to implementer, admins and devs, less code for us to write).

When creating the DTD for version 1.6 (which has never been uploaded) I run into an issue due to the conditionalResources element added in version 1.6 ( https://wiki.openmrs.org/display/docs/Module+Config+File).

Looking at https://wiki.openmrs.org/display/docs/Supporting+different+OpenMRS+versions and the implementation of the ModuleFileParser I see that 2 variations have been introduced:

Variation 1:

<module configVersion="1.6">
...
 <conditionalResources>
    <conditionalResource>
        <path>/lib/uiframework-omod-2.*</path>
        <openmrsVersion>2.0</openmrsVersion>
    </conditionalResource>
</conditionalResources>
...
</module>

Variation 2 (problematic one):

<module configVersion="1.6">
...
<conditionalResources>
    <conditionalResource>
        <path>/lib/yourmodule-api-1.10.*</path>
        <modules>
            <module>
                <moduleId>metadatasharing</moduleId>
                <version>1.*</version>
            </module>
        </modules>
    </conditionalResource>
</conditionalResources>
...
</module>

Variation 2 introduces another module element but the module is already defined in the DTD, it is our root element, which is why we cannot declare it again https://www.w3.org/TR/REC-xml/#EDUnique

We cannot create a valid DTD for 1.6 if we adhere to our wiki’s descriptions.

I was searching for conditionalResources on all github (not only openmrs org) and the only usages I found were of Variant 1

 <conditionalResources>
    <conditionalResource>
        <path>/lib/uiframework-omod-2.*</path>
        <openmrsVersion>2.0</openmrsVersion>
    </conditionalResource>
</conditionalResources>

Would it be possible for us to change the Variant 2?

2

Hmm, good catch.

I wonder if @dkayiwa, @wyclif, or @raff know of anyplace that variant #2 is used?

If that variant is never used, and it makes our XML invalid, then I guess we should change the naming of that xml element. It’s actually a different use case than variation 1 – I think it lets you load a resource if a given module is loaded – so we should still preserve it. Assuming that’s what it’s for we could give it a very precise name like <loadIfModulePresent>.

3

that would be great. lets wait for the others to comment :slight_smile:

4

@mksd i remember as if you used variant #2 for conditional loading based on missing or running modules. https://issues.openmrs.org/browse/TRUNK-5213

5

@dkayiwa thanks for pinging me on this.

I have never used the DTD for conditional resources loading, I always did it through class annotations. So this is no concern for me at this juncture. I have a feeling anyway that this hasn’t been used much for modules as your GitHub search seems to confirm, so it’s probably a good time to streamline it.

Maybe?

<openmrsModule>
  <artifactId>metadatasharing</artifactId>
  <version>1.*</version>
</openmrsModule>

I’m saying this because ‘version’ already refers to the Maven version, and it could be that the module ID might in fact just always be the Maven artifact ID.

Thanks @teleivo!

6

@mksd good thought. But I wonder if its not better to go with moduleId since this reflects what we have in our code at https://github.com/openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/module/ModuleConditionalResource.java#L92-L98

which is what this xml represents in the end I guess. I am not sure how much moduleId’s and artifactId’s diverge.

These changes would make it to

<conditionalResources>
  <conditionalResource>
      <path>/lib/yourmodule-api-1.10.*</path>
      <loadIfModulePresent>
          <openmrsModule>
              <moduleId>metadatasharing</artifactId>
              <version>1.*</version>
          </openmrsModule>
          <openmrsModule>
              <moduleId>metadatamapping</artifactId>
              <version>1.*</version>
          </openmrsModule>
      </loadIfModulePresent>
  </conditionalResource>
</conditionalResources>

If you are ok (@mksd @darius @dkayiwa) with this I’ll create the 1.6 dtd, adapt the code in the ModuleFileParser, the wiki’s https://wiki.openmrs.org/display/docs/Supporting+different+OpenMRS+versions, https://wiki.openmrs.org/display/docs/Module+Config+File

Another search on github for configVersion 1.6 also doesnt yield a match.

1 Like
7

Naming the fields consistent with the underlying Java object sounds good to me.

8

I now also found out that the conditionalResource element has

the child element openmrsVersion as shown in the wiki but it also has kind of an “alias” element named openmrsPlatformVersion although the wiki doesnt mention this. In openmrs-core the ModuleFileParser treats it the same as openmrsVersion and takes either or to set the fieldfield openmrsPlatformVersion of object ModuleConditionalResource. github shows no use of openmrsPlatformVersion instead openmrsVersion is used exclusively. I see 2 options

  1. keep both elements as possible children to conditionalResource, since removing openmrsVersion as child from conditionalResource would break existing config.xml
  2. I could remove openmrsPlatformVersion without breaking existing config.xml, but then I would also change ModuleConditionalResource.openmrsPlatformVersion to ModuleConditionalResource.openmrsVersion so our DTD is closest to actual java objects

let me know which approach you favor :slight_smile:

9

The ModuleConditionalResource class is not really part of the public API that clients code against, it’s used internally by the module engine therefore I think it’s fair to rename the openmrsPlatformVersion field to openmrsVersion becaise technically it shouldn’t break any client code.

10

If I had a time machine I would go back and tell us that the correct name for the attribute should be “openmrsCoreVersion”.

I guess that since “openmrsPlatformVersion” is not documented anywhere, and you couldn’t find any uses on github, I would choose #2 and remove it. (This is a weak preference though.)

11

@wyclif and @darius I researched more and found https://issues.openmrs.org/browse/TRUNK-4578 where we renamed the annotations OpenmrsProfile field openmrsVersion to openmrsPlatformVersion, so it seems as if we kept the openmrsVersion element in config.xml to not break compatibility but OpenmrsProfile now only uses openmrsPlatformVersion.

Therefore I don’t think I should rename ModuleConditionalResource.openmrsPlatformVersion and also allow the element openmrsPlatformVersion as alias to openmrsVersion in the config.xml. Since thats what is also implemented in the ModuleFileParser.

12

Hello!

My team has picked up the ticket https://issues.openmrs.org/browse/TRUNK-5403 recently, and we are not sure what there is left to do besides add openmrsPlatformVersion as a possible child of conditionalResource and create a pull request for the new dtd. Is there anything else that needs to be done besides that?

13

@teleivo do you happen to have any comments to the above? :slight_smile:

14

My team wants to understand more about what these config files are used for. Could someone give a brief explanation of the purpose of these config files?

Furthermore, we tested config_1.5 and found that there was a syntax error in the file. Which I believe we plan on fixing before releasing a config_1.6. @dkayiwa @teleivo @darius @wyclif

15

Our team has made changes, and fixed syntax errors in config_1.5. We submitted our pull request https://github.com/openmrs/openmrs-core/pull/2875 and also added test cases for the config files.

16

I put a comment for you on github.