Unnecessary breaking change was made to ServicePlugin interface

[tonygermano]

The ServicePlugin interface included an additional method since version 3.9.0 in commit bc9c18f, causing any classes implementing this interface to require updating. This broke multiple 3rd-party plugins which otherwise should have been able to be compiled against newer mirth versions.

All of the open-source implementations contain an auto-generated method stub which returns null.

[cturczynskyj]

The idea was to make it so that anyone implementing their own servlets, including NextGen, would have to implement the swagger example. It looks like there are some places where we neglected to do this and we believe the better way is to keep this behavior where any implementing classes must implement the method and we will go back and remove the null returns and have them return actual, accurate examples.

Anyone implementing their own extensions already has to go in and change the version at least, so you would have to make code changes for each version anyway.

[tonygermano]

I just saw @cturczynskyj 's comment, but it doesn't make sense. Not all extensions implementing ServicePlugin have Servlets, and obviously the method is not required to return a non-null value. Not that it would be a bad thing for at least one open source plugin to have an implementation to show how the method is intended to be used since there is no documentation for it.

Any new methods to existing interfaces should have a default method if possible as a general best practice.

Also, a new mirth version does not require a code change for every plugin unless breaking changes are introduced in the core product. It only requires a change to the plugin.xml. Recompilation is not necessary.

[tonygermano]

To expound on my point,

• I compiled the current development branch of connect with the default method as in Add default impl for getObjectsForSwaggerExamples in ServicePlugin #4868 but with a logger statement added to the method before returning null.
• I have a plugin implementing ServicePlugin which was compiled against mirth 3.8.1. I unpacked the zip file to the extensions directory of my new server. I modified the plugin.xml to contain <mirthVersion>4.2.0,3.8.1</mirthVersion> and started the server.
• I tested to make sure the plugin was installed and operating correctly.
• I navigated in a browser to https://localhost:8443/apiexamples/ext_fakeObject_xml
• It returned {"summary":"ext_fakeObject","value":"<null/>"}
• The message I added to the default method was sent to the log

So,

1. It is in everyone's best interest to not introduce breaking changes that require code updates.
2. A mirth version bump does not necessarily require a plugin code change.

[narupley]

I think Chris' point was that if we made default implementations, then the methods would be forgotten about or just wouldn't ever be implemented. So the compile-time errors force the developers (us included) to remember and consider those methods when creating a new instance of that interface.

The tradeoff of course, is that now it requires code changes in plugins for 3.9+. That was intentional though, to encourage all plugin devs to implement those methods, so that the Swagger UI could have useful examples for people to use.

In general I agree, a mirth version bump shouldn't necessarily require any plugin code changes, and doesn't really today right? Only the version in the plugin.xml metadata.

Of course there are improvements we could make there too, like supporting the ability to put like "3.9+" in the plugin.xml mirthVersion, and then you'd truly never need to make any changes (unless there is something in a newer version that breaks code).

[ChristopherSchultz]

It also means that current code won't compile against older Mirth sources. Let's say you wanted to update your plug-in for Mirth 3.8.0 because it contains a bug of security vuln. Now you have to maintain a separate code branch for "pre-3.9 Mirth" because you can't use @Override in your sources. Sure, you could just not put it in there, but it's important documentation for both the compiler and for the reader.

Interfaces evolve over time. Java's answer to "how do we evolve without breaking everyone's code" is "default method implementations." I can't think of a more obvious case where this kind of thing is appropriate.

The idea was to make it so that anyone implementing their own servlets, including NextGen, would have to implement the swagger example.

That's fine: make it a requirement for another interface, like ServletPlugin or something like that. Not all plugins need any of this. For example, I wrote two plugins for Mirth and neither of them need this stuff.

Anyone implementing their own extensions already has to go in and change the version at least, so you would have to make code changes for each version anyway.

Other than adding new version support to the XML file (and, notably, this particular issue being discussed here), I haven't had to update any of my plugin code since it was originally written for Mirth 3.8.0. At least not for changes in Mirth.

[tonygermano]

I think having javadocs for the plugin points and abstract classes with replaceable default implementations would be more beneficial than a compiler warning in ensuring that occasionally used methods were not forgotten, and that would have the extra benefit of explaining the intended use of each method. As it is now, a plugin developer needs to be intimately familiar with the code and research multiple files to see how a method should even be implemented. This is especially true when there are no reference implementations in the open source code base.

Case in point, I really had to dig into the code to figure out how to even trigger the getObjectsForSwaggerExamples() method. Nothing indicates what the method should return when not null. There are no examples of it returning anything except null. Not all plugins require this method to be implemented because there can be ServicePlugins that do not have any related servlets which require examples.

As described in https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html,

Default methods enable you to add new functionality to the interfaces of your libraries and ensure binary compatibility with code written for older versions of those interfaces.

[tonygermano]

Of course there are improvements we could make there too, like supporting the ability to put like "3.9+" in the plugin.xml mirthVersion, and then you'd truly never need to make any changes (unless there is something in a newer version that breaks code).

Other than adding new version support to the XML

I opened #5672 specifically to address this. I don't want to have to touch my plugins at all when a new mirth version comes out.

[narupley]

I think that "ServicePlugin" is the sub-interface of ServerPlugin that denotes "a plugin that has one or more API routes". But the issue is, that also happens to be the interface where it calls init/update with Properties too, and that can be useful even if your plugin doesn't have any servlets. So yeah, maybe another sub-interface like ServletPlugin would make sense.

Or maybe we just add the default implementation yeah. If it's causing this many headaches, then I don't think it's worth it just to address some "forgetfulness hazard" for Swagger UI!