UUID Support
odensc opened this issue · 34 comments
With name changing coming soon, it is going to be very important to implement methods that use UUIDs instead of player names.
Perhaps we could deprecate the old player name methods, and add new UUID methods?
Definitely important for the change to be done ASAP, as other plugins that use Vault won't be able to properly update until Vault does.
@ZephireNZ I have already updated my economy plugin to use UUIDs by using the Mojang Account API and getting the UUID from their name, but it would be much easier™ (and also future-proof™) if Vault supported UUIDs directly.
The only time this can possibly become an issue is if someone on your server swaps their name. It can be detected by verifying last login UUID vs current for that name etc and wiping player data if this is detected or caching the data and handling it outside. Vault will be updated to provide UUID based methods, but there are other ways to prevent edge-case problems or do the conversion seperate. The large issue is going to be if economy plugins actually update.
Also, Vault already supports UUIDs as account-names indirectly by using account-names via UUIDs rather than passing in player objects.
Are you guys going to update or not? Cuase if not you will no longer be used by any servers!
@skiingrocks111 read the message before yours, from 5 days ago.
I would still love Vault to have built in UUID support besides using the UUID as account names. I am writing my own economy plugin and would greatly appreciate this to update.
@flatbmx Yeah, because the current Vault Documentation says that the name is a player's username, not a UUID, which could break if a plugin uses a player's name.
there is no difference between identifying by UUID or player name once the player has logged in, they are the same player to Vault. Economy plugins storing the data are what needs to be resolved as they may still be storing as a reference to a different player name. If a player swaps names, the economy plugin needs to re-associate the player name on the account appropriately. Vault's economy interaction indicates the accounts should be grabbable by player name. How that is handled in the economy is up to the economy to determine, adding UUID support doesn't resolve the issue of data migration and is a seperate problem in itself and not one that is Vault's responsibility to fix.
As I already mentioned above, economy plugins need to account for UUIDs in the storage backend. After that is handled everything else will just work properly with no updates necessary to Vault.
To Clear up some confusion, the current methods should never be used with UUIDs, there will be a seperate method for getting values by UUID once it's available.
As there has been a lot of comments similar to "Vault needs to support UUID so it doesn't break" I'd like someone to try and explain what will actually break.
@Sleaker What will break is:
Plugin uses economy.depositPlayer(player.getName(), 1);
Economy stores playerName -> 1 in database.
Player changes name.
Plugin uses economy.getBalance(player.getName());
Player has no balance, because the plugin stored their name.
It's true that Vault doesn't have to update. You can always do a UUID conversion on both sides of Vault. But that could well involve using blocking methods to get their name, which is not particularly good if you want a fast Economy.
Vault doesn't have to update, but it sure would be faster/more efficient if it did.
Vault can't update until all implemented plugins update. Its that simple.
sent from mobile
On Apr 17, 2014 7:21 PM, "ZephireNZ" [email protected] wrote:
It's true that Vault doesn't have to update. You can always do a UUID
conversion on both sides of Vault. But that could well involve using
blocking methods to get their name, which is not particularly good if you
want a fast Economy.Vault doesn't have to update, but it sure would be faster/more
efficient if it did.—
Reply to this email directly or view it on GitHubhttps://github.com//issues/527#issuecomment-40779487
.
@ZephireNZ - all methods are blocking, even getting the UUID is blocking. The difference is whether or not it's cached in memory, which again, is not a Vault issue. but as @turt2live mentioned, unless economy providers add UUID methods to their backends there isn't much point in having UUID methods in vault as I would have to write an adapter to pull the playername anyhow since they're not supported directly.
@turt2live update Vault but have the new UUID methods not implemented for the inhouse interfaces until the econ plugins update. I am not saying to replace all name methods with UUID methods, I am proposing adding UUID methods. Also the plugins that vault softdepends on are only there since the plugins do not want to have their own implementation in their plugin. Just updating Vault doesnt mean that all plugins/owners will be screwed, they will just have to wait until their backend economy plugin updates for UUID then they can update Vault to the new UUID build.
Vault operates exclusively on the fact that all underlying economy plugins have the support Vault provides to developers. Simply adding UUID-based methods does not solve the problem of developers using Vault EXPECTING these methods to work. Not to mention that the underlying economy plugins can't just magically update and have Vault just "work". There needs to be code that actually forwards the getBalance(UUID) to the economy plugin - which would not be present after the "UUID build" of Vault.
Quite literally: There is NOTHING Vault can do in regards to UUIDs without the support of the underlying economy plugins. Providing methods which do nothing is a stupid decision alongside expecting things to "just work" later.
Sorry, but there is no nice way of saying "just wait" at this point.
All Vault is is an Interface that all other economy plugins can call abstract methods that will be called to the appropriate economy plugin that the server is using. If the Economy plugin handles UUID let it use the UUID Vault version, if not it will just stick with current build until it updates. I do not want to write my own fork so that I can continue writing my econ plugin and others that will use the vault API. I do not see it being a big deal to just update and telling people to not update their Vault until their underlying economy plugins update.
BOTH Vault and the economy plugin have to update, with the economy plugin updating first.
Vault is pretty much a forwarding API. It takes your request for a balance, sends it to whatever backend, and that returns a result to vault, which returns a result to you. This means all possible backends need to have the ability to understand wtf vault is asking for.
It is literally impossible to update Vault without the outrage of "why doesn't this work?" and bad design at this time.
Perhaps Vault could add in another Economy class, and presumably Permissions and Chat, one that is solely for UUIDs, and deprecating the old String-based classes. It would mean that to begin with, all the plugins would work as before by using String methods. Then when they update to use UUIDs, they can switch to this new class, with no problems.
No, because that would still declare that all underlying backends are updated.
The actual plugins don't use the Vault interface at all. Vault writes the communication layer between itself and the economy plugin.
Developers would see the "new" class and assume that it would simply work for all backends, then they would get support requests (both the author and Vault) saying "it doesn't work with ". So you are still at frame 1: Not possible to update.
Now I understand. But this probably won't ever happen, because I'm pretty sure some of the economy plugins with Vault support haven't updated in >1 year(s) and won't be updating anytime soon, if ever. (iConomy for example)
Since getting offlineplayers by name is now blocking the API should be changed since we should not be using deprecated methods all over the place. Stop being lazy and add UUID support to vault instead of half assing it. The reason why I use Vault is due to the abstraction of all econ plugins, If this is not to nativly support UUID then might aswell throw this out the window when people change their names and econ plugins make changes to offline players who arent really who they are.
Vault depends on the underlying economy plugins... Its not a magic layer.
Why does no one understand this.
sent from mobile
On Apr 18, 2014 5:44 PM, "David" [email protected] wrote:
Since getting offlineplayers by name is now blocking the API should be
changed since we should not be using deprecated methods all over the place.
Stop being lazy and add UUID support to vault instead of half assing it.
The reason why I use Vault is due to the abstraction of all econ plugins,
If this is not to nativly support UUID then might aswell throw this out the
window when people change their names and econ plugins make changes to
offline players who arent really who they are.—
Reply to this email directly or view it on GitHubhttps://github.com//issues/527#issuecomment-40853802
.
@TheSBros - that's not a bad thing. iConomy should have been completely removed from the landscape when Niji didn't fix the glaring bugs in it.
Not sure if this has been covered yet..But vault does need to be updated, because while trying to find a players primary group(or Trying to set a group), you have to use a player variable, and there is no way to get it using a UUID variable
Not sure if this has been covered yet..But vault does need to be updated, because while trying to find a players primary group(or Trying to set a group), you have to use a player variable, and there is no way to get it using a UUID variable
Both gm and EssEco have updated to uuid internally, I don't believe there is any exposed methods in either plugin to use uuid quite yet.
I'll probably wait to see how things settle before adding uuid methods to the api layers of EssEco.
Essentials internally keeps a record of every username it sees, and what uuid it maps to, so there shouldn't be much overhead with the username methods.
That said the internal switch to UUID's from within the Ess code-base is still new... so expect bugs.
Edit:
GM is also keying by uuid, but will allow a slower username lookup, files are going to look like this:
http://pastie.org/9121215
This shouldn't cause a problem to most plugins, since they will be using the player.hasPermission() methods.
Plugins which pass player name by string to vault for permissions checking is probably going to hurt.
In my opinion Vault should offer an UUID interface already now. Even if not a single of the supported plugins has UUID support yet. The reason for that is pretty simple:
Vault is an API. The most important thing about this API is what the developers that hook into it can work with. As long as Vault does not update to support UUIDs no depending plugin can update. Right now there might not be any technical advantages, but it will allow other developers to start adjusting their code. And then, the very moment that MySuperEconSystem switches to UUIDs and Vault adjusts, poof, 500 plugins suddenly have full UUID support without changing a single line of code.
UUIDs are the future. So why not offer the API already now?
Yes, @Sleaker, you would have to write an "adapter". But in the new CraftBukkit builds the UUID-lookup is already the most efficient one and when Econ plugins etc offer new APIs you can slowly adjust in the background.
Either you type getMoney(UUID) { getMoney(getName(UUID)) } once or 200 other developers type getMoney(getName(UUID)) five times and then still have to adjust later on.
This is quite a topic...
UUID support is important however I can't take either side on this. Other than the fact is to know what the naming convention will be for when it does update and add the new api.
I think @Sleaker and @PhoenixIV are both right in their own thoughts on this.
If I developed interface code I would be leaning towards @PhoenixIV 's point of view more though personally. However @Sleaker has very good points. What if most of the economy plugins don't update backends to use UUID's the way we expect it anyways?
Vault will not be using UUIDs for direct lookups as this is an implementation-specific way of pulling an OfflinePlayer. Vault will instead be using offline player objects on it's API frontend and translate these calls to whatever the Economy works with.
As far as futureproofing, I've marked all name-based methods as deprecated. the correct solution should be to use the offlineplayer methods going forward. I'll be implementing these back onto the permission changes and removing the few UUID methods I added in.