Plugin loader

[nezroy]

This is an overhaul of the way the extensibility layer works based on my needs trying to create an extension for the web server.

Rather than using reflection on assemblies, the plugin loader looks through the user mods in the ColossalFramework PluginManager and picks out those implementing the ICityWebPlugin interface (some reflection is still required to do this consistently). Valid plugins are then loaded under unique ID's which make up the base of all of their URL's. Each plugin can implement any number of request handlers under its unique URL path (e.g. the plugin with the unique ID of "sample" can only handle requests under an absolute path of "/sample/*").

In addition to the updated plugin loader arch, several related changes were also made. The default request handler will look for a "wwwroot" path under each ICityWebPlugin's mod path. If the request URL is in the plugin's path but is not handled by any of its request handlers, the default handler will attempt to serve a matching static file from the plugin's wwwroot.

The template system was cleaned up a bit and made more generic so it can be used by plugins. Additionally, registered plugins can opt to show up in the top menu nav area. The default template for index was also updated slightly to reflect this, and a new matching template added for the log page.

Core services were moved into three fake/default plugins (Root, Log, and API) in order to leverage this system. In theory, with a simple change to the plugin loader logic (last loaded wins instead of first loaded wins), these default plugins could become replaceable by custom implementations defined in extensions. As a result of this change, the API endpoints have moved from living under the / URL path to the /api/ URL path (as "api" is that plugin's unique slug/ID). The sample extension project has likewise been updated to use this system.

[Rychard]

I looked over your write-up, and you've basically explained how you implemented everything that I always wanted to do, but never actually took the time to do. This is all very exciting stuff, and I can't wait to dig in and look at this more in-depth. In particular, I'm really pleased with your decision to pull the "core" functionality out of the server.

I also have to applaud the fact that you were thorough enough to update the sample project to use the new approach.

I haven't actually looked at the code yet, but I'll definitely be reviewing the changes at some point today!

[nezroy]

I just committed a significant refactor to this PR in order to clean things up a bit. Basically I standardized my naming to "CityWebMod" since "Plugin" was really confusing with the PluginManager plugins. I also consolidated several overlapping classes and interfaces that had evolved unnecessarily while I was fixing issues previously with loading things from the PluginManager.

Nothing functional changed, but if you haven't looked at the code yet this pass should hopefully make things a lot more concise :)

[Rychard]

Can you explain a bit more about your reasoning behind ICityWebMod/CityWebMod?

From what I can gather, this allows us to leverage the functionality of the PluginsManager so that we prevent the registration of any IRequestHandler if it belongs to an IUserMod that is not enabled. While I think this particular feature is great, I think we could accomplish the same thing without requiring other mods to implement the ICityWebMod extension on their IUserMod. Remember, we can get a reference to the assembly that contains the IUserMod by using the Type.Assembly Property. From there, we can iterate through its types with the Assembly.GetTypes Method.

I'm on the fence about the benefits of "grouping" request handlers. It seems to provide some structure where otherwise there wouldn't be any, but it is an extra step that needs to be done before the server will recognize a request handler, and there's no way for someone to "opt-out" of this, and simply indicate that they want the server to "load all my types that implement the IRequestHandler interface". One of the design-goals I had with the server was to enable other mod authors to add support for the server very easily. Currently, all they have to do is to create a new class that implements a specific interface and expose the appropriate data using the helper methods in the CityWebServer.Extensibility project. While I admit it does have its faults, the current implementation could be revised to fix them without breaking backwards compatibility.

Furthermore, the ICityWebMod interface doesn't require implementations to expose a WebRoot property (and I'm not entirely sure it should, but read on), nor does it require the implementation of the HandleRequest method (and again, perhaps it's best that it doesn't). The CityWebMod class seems to handle most of this internally, but this means that any externally-defined request handlers would have to implement their own logic to determine their root location, and if they wanted to return the contents of a static file, they'd also have to re-implement this as well. The process of doing this is quite generic already, and I believe this is a prime example of something that we could handle internally by getting the file-location of the assembly that contains any given ICityWebMod and allowing those mods to specify a relative-path to their wwwroot location. This is more in-line with how a regular web-server might operate, and it may be a nice improvement to the overall design.

I may be missing something (which is quite likely) but I'm not seeing a lot of added value regarding ICityWebMod and CityWebMod.

I'd really like to incorporate the vast majority of your changes, I just want to make sure that our plugin strategy is solid and doesn't back us into a corner later on.

The remaining stuff is just related to coding style and formatting. I'm not asking you to change any of this stuff (they're all trivial for me to either automate or simply do by hand) but I'm just making note of my thoughts.

• I dislike Yoda Conditions. The C# language doesn't benefit from it like some other languages do, and I find it confusing to read.
• I try not to use string and bool, instead preferring their non-aliased counterpart in the System namespace: System.String, and System.Boolean. What makes this really confusing is that I do use the aliased types for almost everything else.
• I never use multi-line /* ... */ comments.
• Supplemental to the above, I try not to commit large blocks of commented-out code.
• I try to always include braces, even when it is unnecessary to do so.
  • i.e. if(obj == null) { return; }
• Additionally, the opening/closing braces should always exist on their own line, unless the entirety of the statement is included on a single-line.

[nezroy]

Furthermore, the ICityWebMod interface doesn't require implementations to expose a WebRoot property (and I'm not entirely sure it should, but read on), nor does it require the implementation of the HandleRequest method (and again, perhaps it's best that it doesn't). The CityWebMod class seems to handle most of this internally, but this means that any externally-defined request handlers would have to implement their own logic to determine their root location, and if they wanted to return the contents of a static file, they'd also have to re-implement this as well.

Some of the comments suggest to me that you are expecting external mods still need to deal with their own static files. I'm not sure if I'm just misunderstanding these comments though, so just to clarify, an external mod implementing ICityWebMod registered under "slug" already gets automatic static file handling under the current changes.

If the mod is registered to "/slug/" and has no request handlers setup to handle a request to "/slug/something.ext" (e.g. ShouldHandle returns false for that path), then the internal implementation in CityWebMod will automatically try to serve up the "something.ext" static file from a "wwwroot" folder in that specific mod's directory (from the location of its assembly). The only time an external CityWebMod would need to re-implement static file handling is if they wanted to change this default behavior, which should be an uncommon need.

I do also have some uncommitted changes to pass the wwwroot path determined by the underlying server to each call of a request handler. This is so external handlers have trivial access to their wwwroot location if needed; e.g. for calls to the TemplateHelper. (Uncommitted since I'm not sure if passing it for every request is really the best way to go about this, though it does keep things very simple).

I'm on the fence about the benefits of "grouping" request handlers. It seems to provide some structure where otherwise there wouldn't be any, but it is an extra step that needs to be done before the server will recognize a request handler, and there's no way for someone to "opt-out" of this, and simply indicate that they want the server to "load all my types that implement the IRequestHandler interface". One of the design-goals I had with the server was to enable other mod authors to add support for the server very easily.

The grouping provided a number of benefits to me. It supported the ability for the internal classes to handle all static file serving needs for my mod by internally finding and using the mod's wwwroot. It also allowed for a way to integrate a single path for my mod into the existing template/top menu without having to clump everything from my mod into a single request handler. While I can only speak from my experience, I definitely found it much easier to create a single ICityWebMod with relevant metadata plus the list of handlers I wanted to use provided by GetHandlers, rather than the experience of repeating a bunch of boilerplate in each individual request handler. I was also able to do things like use a factory to generate a number of handlers automatically in response to the call to GetHandlers. Trying to do so without the single-point GetHandlers hook was problematic and required me to start looking into how to independently init my mod first, etc.

Static file handling in particular was the breakpoint where I ended up switching to this idea when originally trying to work on my own extension. Initially I had tried to approach this by simply implementing a "StaticResponseFormatter" to allow custom request handlers to easily serve up static files, but this didn't make a whole lot of sense. By far the most common use-case I wanted was simply to have one wwwroot and one matching static request handler for a single mod, regardless of how many other request handlers that mod implemented. And the behavior I always wanted was "try to run a relevant request handler first, then look for static files". But defining "relevant" request handlers became tricky because they each lived in isolation. By grouping them at a mod level, it becomes trivial for the underlying server to figure out which handlers are "relevant" to a request, and then where to look for static files if no handlers match.

From what I can gather, this allows us to leverage the functionality of the PluginsManager so that we prevent the registration of any IRequestHandler if it belongs to an IUserMod that is not enabled. While I think this particular feature is great, I think we could accomplish the same thing without requiring other mods to implement the ICityWebMod extension on their IUserMod.

Initially I was using the PluginManager's GetInstance on the ICityWebMod to determine if it was a valid mod or not without any reflection at all. This was nice and simple, but unfortunately not reliable. Since I have updated that to use reflection anyway, it's not necessary at all for a mod to actually officially implement ICityWebMod, though I personally found it useful for code validation purposes. That would mostly just be semantic, though, since for the reasons I mentioned above I still think common mod metadata/GetHandlers as a hook, rather than iterating through looking for each individual request handler, is the way to go, and the reflection that identifies a valid CityWebMod still expects to find those things.

Backwards compatibility/co-functionality that continues to iterate through assemblies looking for just request handler implementations would be doable, though. I think it would be simpler and make more sense to me if it is iterating through the assemblies of plugins, looking only at enabled plugins, and not just considering everything. Second, the handlers would still need to belong to SOME plugin under the hood, but I think the core RootCWM would actually make a reasonably good place for these to live.

Anyhow I will probably be taking a stab at some of this tonight, time allowing, and will likely address some of the style issues too (several have probably already been fixup'd anyway; I also try not to commit large blocks of commented code :)

[nezroy]

OK I've pushed some new commits that add support for "naked" handlers (essentially the original approach, minus the GUID and adding fetching them via loaded plugins rather than AppDomain). This co-exists alongside the new CityWebMod style; extensions can use either approach. I've split the sample project into two to demonstrate both methods.

Lesser stuff includes cleaned up style throughout, added an API endpoint listing to the default APICWM page, and a new HandlerCWM that not only acts as the container for naked handlers but also provides a listing of them (replacing the list that used to show up on the index page).