Register loaders to Bukkit services

[Engineous]

This allows other plugins to utilize the loaders that the SWM plugin creates. This would help prevent additional, redundant connections to the same data source.

[davidmayr]

I get why you'd want to implement that, but I do question if doing so would end up in people making plugins for ASP that require the official slime plugin.

Plugins are generally expected to shade, relocate and manage their loaders themselves so they explicitly don't need the slime plugin at all (TBH most servers running in production don't need the slime plugin IMO).

With the plugin being installed, reducing connections would certainly be a good idea. But with this approach, other plugins must depend on the official plugin if they want to integrate this and wouldn't even be able to provide a fallback loader because of classpath collisions if the plugins would shade the loader (without relocating - which would be required to use both) in addition to supporting the loaders created by the ASP plugin

I already don't like that people don't shade and relocate the loaders and just use the ones from the default plugin, and I was thinking about relocating the loaders in the plugin because of that in the next breaking API release. It's not really an issue for private plugins, but public facing ones should always be able to run without the slime plugin, IMO.

@kyngs @AverageGithub what do we think about this?

[CommandDan]

If you were to reuse the same connection in several plugins wouldn't you also risk messing up data between plugins?

[davidmayr]

If you were to reuse the same connection in several plugins wouldn't you also risk messing up data between plugins?

I don't think that this should be an issue. I don't think any of the official slime loaders expose raw connections anywhere, and as the server only allows for one world with a specific name at a time, this should not cause any issues except if some plugin writes raw bytes to the loader. But if that happens, the plugin has most likely malicious intent anyway and should probably be removed.

[kyngs]

I don't think this is a good idea.

The whole purpose of separating the plugin was to ensure it remains independent of ASP - and vice versa. As David pointed out, tying things together like this would effectively force the use of the official plugin just for the loaders, which, frankly, doesn't make much sense.

This would help prevent additional, redundant connections to the same data source.

This really isn't a valid concern. How many running plugins are realistically going to use the ASP API? One or two? Certainly not dozens.

On top of that, this approach would effectively force custom plugins to use the same database as the main one. But what if I want my SkyBlock plugin to use its own world database, while the official plugin uses a different one for the lobbies? Sharing a single DB across unrelated plugins would limit flexibility and introduce unnecessary coupling.

I would keep this as-is; plugins should shade and relocate the official loaders. What happens if a plugin needs a different version of the loaders? Without shading and relocation, you’re setting yourself up for classpath conflicts and all kinds of difficult-to-debug issues.

[Engineous]

I intended this feature to be used when the ASP plugin is a hard dependency, and I agree that public plugins should avoid this. This addition would be for developers of bespoke plugins who would rather rely on the loaders from the ASP plugin already installed on their server.

@davidmayr #172 (comment)

But with this approach, other plugins must depend on the official plugin if they want to integrate this and wouldn't even be able to provide a fallback loader because of classpath collisions if the plugins would shade the loader (without relocating - which would be required to use both) in addition to supporting the loaders created by the ASP plugin

With this feature requiring a hard dependency on the ASP plugin, any plugins relying on these loaders would already be disabled if the ASP plugin was disabled and the ASP API classes were not loaded. It would not necessarily prevent fallback loaders as a developer could still create their own, assuming that the ASP plugin was enabled, but lacking the loader for a certain data source:

    // Works!
    public MysqlLoader getMysqlLoader() throws SQLException {
        MysqlLoader mysqlLoader = Bukkit.getServicesManager().load(MysqlLoader.class);

        if (mysqlLoader != null) {
            return mysqlLoader;
        }

        mysqlLoader =  new MysqlLoader(...);

        Bukkit.getServicesManager().register(MysqlLoader.class, mysqlLoader, this, ServicePriority.Normal);

        return getMysqlLoader();
    }

@kyngs #172 (comment)

The whole purpose of separating the plugin was to ensure it remains independent of ASP - and vice versa.

It was my impression that the ASP plugin requires the ASP server runtime (or at least some other implementation of the ASP API, which there doesn't seem to be) therefore making the ASP plugin dependent on ASP. Please correct me if this is wrong.

As David pointed out, tying things together like this would effectively force the use of the official plugin just for the loaders, which, frankly, doesn't make much sense.

This feature does not in any way force the use of the official plugin. It's just helpful to use the existing loaders since they have a configuration system.

On top of that, this approach would effectively force custom plugins to use the same database as the main one. But what if I want my SkyBlock plugin to use its own world database, while the official plugin uses a different one for the lobbies? Sharing a single DB across unrelated plugins would limit flexibility and introduce unnecessary coupling.

All plugins that rely on the ASP API would still be able to create data sources whether or not they are utilizing this addition.

I would keep this as-is; plugins should shade and relocate the official loaders. What happens if a plugin needs a different version of the loaders? Without shading and relocation, you’re setting yourself up for classpath conflicts and all kinds of difficult-to-debug issues.

Any plugin using this feature would rely on the ASP plugin to provide the loaders and therefore not shade them. If a developer needed a different version of the loaders, then it would be their job to change the version of the ASP plugin they are using.

---

Overall, I don't believe many of the concerns raised are related to this pull request specifically. They are mainly to do with using the ASP plugin as a dependency. If this method is worrisome, then I'd suggest developing a more traditional API for the plugin to allow developers to use that instead. Perhaps move the plugin to its own GitHub repo and add it to the Maven repository. This might help people understand how these two things are less related and use them appropriately. I'd like to hear your thoughts :)

[davidmayr]

The solution to the fallback loaders would not work because of 2 simple reasons:

• Plugins should always relocate their loaders. When using the fallback solution, this would not really be possible (since the reference would be changed to the relocated one and it wouldn't find your service) unless you do some pretty unconventional hacks to do so.
• When the version of the loader differs between your plugin and the slime plugin, this could potentially cause issues eventually

This feature does not in any way force the use of the official plugin. It's just helpful to use the existing loaders since they have a configuration system.

It doesnt matter if it forces it or not. From our perspective its considered bad practice so why should we allow even one person to ever do that? People have been using slime pretty incorectly and have ignored all of our advice. So why should they listen to us this time?

All plugins that rely on the ASP API would still be able to create data sources whether or not they are utilizing this addition.

Once again, the fact that plugins could have the option to use it is worrying to me.

I talked this through with kyngs the other day and we figured that the fact that the slime loaders are not relocated in the official plugin is actually a bug and not a feature. Unfortunately, some people create the loader from the classloader of the plugin instead of shading their own so this would potentially be end up beeing a breaking change in API 5.0.0.

If you (and others) mind sharing how their server (in production) requires the slime plugin to be installed and what the actual resource savings would be, we can continue talking about this. But in all honesty, which server (except development servers) actually needs the slime plugin? If you host minigames you'd expect the game plugin to load worlds. Most of my systems would break if I load the worlds manually instead of letting my plugin do it. If you have per player worlds, you'd probably want your world or island system to manage those worlds for you too.