Added claim IDs before firing ClaimBeforeCreateEvent

[mdcollins05]

This fixes a problem I was having where I needed to apply logic to claims, categorize them and determine if they would be allowed. If they were allowed, I needed to record the ID along with what category they were.

I tried to avoid any writing to disk and let the existing code determine when to do so.

[BCProgramming]

One issue I can see here would be that the Claim ID passed to the event has no guarantee of matching the claim ID that will be used to create the claim. It depends entirely on what the event does. For example if handler of the event creates another claim or really does anything in the GP API that ends up creating a claim, such as resizing a claim, the ID that was passed to the event- since it's not actually tied to a claim yet- might end up getting used for that claim. Basically Claim IDs don't exist for a claim unless that claim is saved- before it's saved, it could be anything and it's impossible to predict what it could be. This would work for most use-cases but the corner cases would require some seemingly silly limitations on what the event can do.

In your specific use-case, You would need to track both the Before and After, in the Before event you could perform the appropriate calculations to see if it is allowed; if not, you cancel the event, but if it is allowed you would add that Claim object to a List,Stack, or other data structure. By handling the ClaimAfterCreateEvent event you could check that structure and initialize your internal structures to the Claim Objects, which at that point will have a Claim ID.

[mdcollins05]

I'd certainly be willing to change the code so that the ID is incremented right after assigning the ID instead of only when it is not cancelled. This would potentially speed up the how fast the server runs through the available IDs however, it would only really affect long running servers with tons of claims consistently being added and removed.

[BCProgramming]

The issue is that, when ClaimBeforeCreateEvent is fired, that claim that is being created hasn't been created yet, so technically it doesn't actually 'exist' in the data Store.

So for example, if a Claim was being created, it would get the next ClaimID, let's say it's, 7. So it passed that to the Claim constructor, so that Claim object now has an id of 7. Then it fires the BeforeClaimCreated event.

Of course now if that event creates or resizes any claims, that nextClaimID will still be 7, so the 'new' claim the event creates will be created and saved during the event, the event returns, and now that first Claim get's "saved" as that id and overwrites the existing one.

One idea might be to as you note increment the ID right away. Then new claims won't get that ID and get overwritten by the claim that is being created. Then if the event is cancelled, and the nextClaimID is still the value we incremented to, we can decrement it. In this case with the above scenario:

New Claim is being created. Claim is created, given the incremented ID. Event is fired.
After event returns, if it's cancelled, an additional check is made- is that ID+1 still the nextClaimID? If it isn't than the handler created a claim and cancelled the event, using up that ID and incrementing it again. Otherwise, we incremented it for our event, it was cancelled, and we want to roll it back one.

[ryantheleach]

Why not UUID?

[BCProgramming]

Why not UUID?

what?

[ryantheleach]

Using UUID's would get around the issue of the API handing out ID's that conflict. at worst they will be getting a UUID that no longer is used.

[BCProgramming]

Yes, but sadnly It would also require a schema change and break any dependent plugin(s) that expects ClaimIDs to be int's which would be a problem. (not to mention the issues with migrating existing data, changing column types, and such). A better solution, IMO would be to eliminate the nextClaimID table entirely and instead just use the already existing capability of mySQL Serial fields to have the database manage and number claims as they are added, rather than have this tricky logic to try to come up with a new ID. IMO it would be better because it's more readable to list claims with short, int ids rather than UUID strings, as well as because it would be pretty much entirely compatible with the old schema (old versions of GP would still be able to read the serial fields, I believe).

[Alshain01]

Just playing devil's advocate. You don't have to remove the long ID to add the UUID. You could have both.

It would still require a schema change, but it would not break plugins, if you wanted you could even flag the long id as deprecated for later removal to give us time to adjust our plugins. GriefPrevention wouldn't have to be totally rewritten right away either.

It would be universal should the user move from flat file to SQL, it wouldn't change. Where as using the SQL automatic numbering would likely change when importing because SQL won't let you control that. That change would break dependent plugins like mine which store data based on that ID. All of my data would suddenly have the wrong ID's because the user moved storage types.

The event wouldn't get the long ID, but if the right getters are implemented, you can store the UUID from the event and grab the long ID after the event is finished and the claim is created (if that's even necessary).

[BCProgramming]

Yes, that would work. Claims should probably be changed to use UUIDs before Players, anyway- the latter(is going to be needed eventually as well). I say claims first because changing the Players would require some bukkit version-specific logic too.

I started it a bit (well, OK, I added a ALTER TABLE that would add a uid column char(36) and an appropriate index) but it will definitely be a far-reaching change, with repercussions throughout all the data stores. eg. All methods in dataStore and the derived classes would need overloads that accept UUIDs and act appropriately, and of course saving/loading. the column would have a default in case it isn't set (which has the interesting effect of allowing older versions of GP to work, since mysql will insert a new UUID for the new column since older versions won't specify a value). UUIDs would be generated when a Claim is instantiated- not just loaded- and would not rely on the save logic to get a UUID. when they save it will simply use that UUID to save, and it will remain the same.

Still quite a bit of things to change, but it would solve the annoying id issues relating to plugins and events- another side benefit is that subclaims would have their own unique UUIDs as well, so they could be referenced directly by plugins via a getClaim(UUID) overload, which was another corner case that was problematic within plugins.