[B+C] Add MetadataProvider for on-demand metadata. Fixes BUKKIT-3625.

[crast]

The Issue/Justification
At the very core of the metadata system is the idea of asking a question. A plugin asks a question, like “What is this player’s chat prefix?” and some other plugin is able to provide an answer.

The way it’s currently designed, for some plugin (say a chat plugin) to ask what a prefix is, some other plugin (say a permissions plugin or user manager) would have to set the "chatPrefix" key on every user before it's asked. (say when they log in.) This isn’t very effective, and it isn’t quite as “lazy” as one would hope, what if the value is never asked for? Then we've done all that work for nothing.

What would be useful, instead, if the metadata system provided a way for a plugin to register a metadata ‘provider’ for a key. Calling .getMetadata(key) would query the provider if no value is set, passing it the object, for an appropriate metadata value.

To use the chatPrefix example from before, MyAwesomePermissions would register a provider on a class target (say, Player or OfflinePlayer). Then at some point, BobsChatPlugin calls player.getMetadata("chatPrefix") which asks the provider for the chatPrefix. This also means if the chatPrefix is never requested (say you don't have a chat plugin to use it), there's no work done or memory used in creating the values.

For a full dissertation on issues with Metadata as a whole see: http://goo.gl/jRsX9

PR Breakdown:
Metadata Providers are a new "dispatch" type system for allowing metadata to
be done truly on-demand, via calling into a contextual "provider" when metadata
is requested. Through use of generics providers can be registered for a large
range of bukkit types and they will be dispatched to the providers which respond
for those specific types.

• Created provider interface
• Added unit tests for provider usage
• Updated bukkit.Server interface with methods for registering /unregistering providers
• Updated bukkit.Bukkit with static implementations
• Update generic type requirements of MetadataStore
• Make OfflinePlayer Metadatable for consistency and generic guarantees.

Using the interface would require the user to do something like

server.getMetadataManager().registerProvider(Sheep.class, plugin, new MetadataProvider<Sheep> { 
    public MetadataValue getValue(Sheep subject, String key) {
        if (key.equals("name")) {
             // using isSheared mostly to demonstrate that we don't need to cast types here
            String name = createRandomName(subject.isSheared())
            return new FixedMetadataValue(plugin, name)
        }
        return null; // We're not interested in this request
    }
});

Testing Results and Materials:
Wrote extensive new unit tests that provides full coverage on the Metadata Provider interface/implementation, including writing some sample implementations and testing how providers receive data. This is in addition to some new full-stack unit tests in the craftbukkit companion to this ticket: Bukkit/CraftBukkit#1124

Relevant PR(s):
CB-1124 Bukkit/CraftBukkit#1124 - Implement MetadataProvider backend inside craftbukkit

JIRA Ticket:
BUKKIT-3625 - https://bukkit.atlassian.net/browse/BUKKIT-3625

[EdGruberman]

I have a personal disdain for the Metadata system and how it encourages abstracting inter-plugin communication down to a String key versus using an actual class reference in the way Java was designed to leverage. I see the system akin to turning off all compiler warnings. It's simply a bad practice that is fraught with encouraging bad code.

I suppose this commit has the right idea towards minimizing the performance and resource impact of using the Metadata system, but... meh.

[crast]

@EdGruberman While you make an excellent point, the main problem I see is in plugins where there's a class of plugins which interacts with another class of plugins, (not specifically one plugin with another specific plugin, where an API specifically into the other works swimmingly)

My example from above, chat plugins, they often depend on something like a "user data" or "permissions" plugin. While it would be nice if there was a common interface between all of these, there isn't (excepting things like Vault, which adds more dependencies) and thus you're forcing everyone who wants to do this kind of communication (in realms less clear defined than permissions) to implement some sort of 3rd party API.

Now, when the bukkit API takes into itself to standardize an interface (like permission checking) and provide ways for the permission plugins to hook into it, that is awesome, and it has worked swimmingly... for that specific use case. But when someone wants to go slightly outside that (like say, checking for a list of groups or other concepts outside basic permission checks) suddenly they're back to coding to individual plugins or using something a la Vault.

The basic problem here is a question of dependencies, something that has plagued all sorts of package management systems. Heck, the java language itself has this problem and Java 8 has a major project as a part of it (google "java 8 jigsaw") to standardize on modules. The problem is how does a module declare "I provide feature X" in a non-strong-coupled way? The only way is if there's an interface available outside of the package that all the other plugins which provide feature X also implement (will that happen? no, probably not, but one can dream) and also a place to register them. And even if this ideal scenario were to happen, there would still need to be a place to register them, and metadata could be a way to do that.

On the note of this devolving into a string key use, I've been trying to encourage standarization in users of metadata by having a spreadsheet of key exchange: http://goo.gl/HVRlX

[EdGruberman]

That spreadsheet does not remove the concern of String keys replacing interface definitions, it reinforces it. It epitomizes the point of my disdain. You are creating an external registry to compensate for plugins avoiding an external reference. That undermines the benefit of a dependency/reference that is managed internally with Java.

The architectural concept of a shared library like Vault is good. It's a central registry of how plugins agree to interact utilizing standard Java mechanisms. I would propose it'd be even more ideal to have a plugin library that was essentially just common interfaces and let other plugins sign up as providers and implement accordingly. (Actually Vault may do this on some level at least.) It's essentially what the Metadata system and your spreadsheet are attempting to address, except it'd use standard Java mechanisms.

[crast]

While Vault works great, I'd say Vault is actually a terrible example of a good design in this case.

Vault isn't a central registry of how plugins agree to interact, because Vault is the only provider of the three main interfaces, and all of the interop is done inside a huge number of modules in Vault: https://github.com/MilkBowl/Vault/tree/master/src/net/milkbowl/vault/permission/plugins This means that the author of Vault is providing a huge amount of shim code to make this inter-op work, and has to update his shims every time a permissions plugin changes their API. This does not encourage good design at all, as it can break in theory at any moment, and the onus is on one guy to fix it, not the authors of the various permissions plugins.

Out of interest, I tried to find any other plugins which used the RegisteredServiceProvider paradigm and as far as public API is concerned, there's only other, and the same is true of both this and Vault: the plugin with the service provides the -only- implementations of the API. This has some bonuses of course, like you can add methods without breaking things.

Now, there may be plugins which are split into multiple parts that are using RegisteredServiceProvider internally that I'm not aware of, but for this point, for plugin interop, why is it that there's only 2 plugins out there using this? (I have a number of possible explanations, but for brevity I will leave it out, suffice it to say there's a lot of limitations which prevent this from being a really viable way of plugin inter-op, some are systemic to java and some are systemic to the plugin developer base)

[EdGruberman]

My point is not how Vault does it specifically, the architecture of the centralized library plugin is what I'm encouraging... instead of some random spreadsheet people need to register with. That was my point about letting other plugins register implementations and using a centralized library/interface only plugin. If you did that instead of that spreadsheet, I'd be singing your praises.

The reason a plugin hasn't done it to date is because either they get greedy and think they can do it better or they don't think they can get people to agree. If you are getting people to agree to String keys, what makes you think you can't get a centralized interface library working?

[crast]

If you are getting people to agree to String keys, what makes you think you can't get a centralized interface library working?

1. Usage / scope locality - block.hasMetadata("isProtected") versus something like

RegisteredServiceProvider<ProtectionChecker> checkerProvider = Bukkit.getServicesManager().getRegistration(us.crast.ProtectionChecker.class);
if (checkerProvider != null) {
   ProtectionChecker checker = checkerProvider.getProvider();
   return checker.checkProtected(block);
} else {
    return false;
}

2. Linking issues - most Bukkit developers are junior developers and are likely to screw up on the linking somehow (embedding a class that should only be a link-time dependency, causing other potential issues where classloaders and such are concerned)
3. Dependencies - unless it's a hard dependency (most people want to write plugins with a soft dependency) it'll have to be wrapped in a number of try-catch so it doesn't cause a crash in the class loading (e.g. if they have MyClass implements us.crast.SomeInterface and the plugin with the interface is missing, any reference to MyClass will crash)
4. Versioning - Once the interface is written, it's set in stone, you can't change it (though you can develop new ones)
5. Do you as a server admin really want a whole lot of little interface plugins to have to put in to make your plugin work? Plugin developers inherently recognize this and thus they are loathe to do it unless it provides a gigantic added value over not doing it (Vault is a very key example, granted, and it's been very successful, but there's few things that polarizing)
6. Everyone always wants more(tm) - The interface no matter how well thought out will always miss out on something someone really wanted, and due to how interfaces are done, the only option is to come up with a new interface.

P.S.: It's not that I don't think that having interfaces is better, but unless the interface is something implemented inside org.bukkit and distributed with the Bukkit API (like permissioning) it's bound to create so much headache that nobody will use it. This is systemic to Java and there's not much one can do about it.

P.P.S: we should move this argument to IRC or forum private message

[EdGruberman]

For clarity in our discussion, Bukkit's RegisteredServiceProvider is just as bad of a concept as its Metadata system is. Neither are good substitutes for how Java is natively designed to manage class references. They both require an external reference in order for things to function as expected. In the RegisteredServiceProvider case it doesn't even eliminate the need for an external class reference, it only adds another layer to access it. In the Metadata system, if you stick to primitives or common Java objects you can eliminate the need for an external class reference, but you trade that advantage out for the need to agree externally to even Java itself to what you are asking for (a spreadsheet).

In regards to your point about convenience (what you refer to as usage/scope locality): Convenience should not be the defining criteria for encouraging a solution that encourages bad code design. Proper dependency management is a fact of good architecture. If you have examples on how to reference your centralized library plugin to get at a provider, it's a simple copy/paste anyways.

Assuming a junior developer will screw up a class import over agreeing to an external spreadsheet reference is an odd claim to make. They are equally problematic. A junior developer should be encouraged to learn how to properly manage their dependencies, not sheltered from the reality which will encourage further bad code.

Your dependency concern falls under the same realm as your convenience concern. An example for copy/paste implementation will resolve this.

Versioning (and your later point of concern over new interfaces) is clearly a concern and again not eliminated from the spreadsheet solution either. If someone wants a new way to handle data, they make a new String key. That's versioning through String keys. (e.g. chatPrefix gets replaced with advancedChatPrefix because it's "better".)

The server admin concern over an additional download is a valid point, but also it has been proven to not be a concern with how Vault has been successful. You can't just dismiss it's success because you think it's a rarity. The point is it's possible if you market it and support it properly.

You make the claim that if Bukkit doesn't implement it, it won't work. Yet your spreadsheet is exactly that. Bukkit does not implement the data creation that those String keys return.

You are right about the forum discussion. If you want to continue further, let's hit the Bukkit forums and just tag me there.

[EdGruberman]

For continuance of the Metadata general discussion: http://forums.bukkit.org/threads/metadata-system-benefits.139846/

[turt2live]

@crast (or anyone else) - If you are still interested in this, please address the following current concerns:

• CONTRIBUTING.md
• Ability to be pulled (update to master)

Thanks!

[crast]

@turt2live Is rebasing appropriate? I believe it will cause the comments on specific lines to be lost. If so, then I will proceed and make those changes.

[turt2live]

GitHub will supress them. So I would recommend rebasing as required and following up on the line comments before pushing to GitHub

[crast]

Rebased onto master.

[crast]

Rebased again, squashed commits, updated PR description, cleaned up everything, much cleaner now.

[turt2live]

Extra whitespace above this line