[B+C] Add Merchant Trade API - Adds BUKKIT-5663

[gabizou]

The Issue:

There is no way to modify Villager trades through the Bukkit API.

Justification

Without the API to allow for modifications being made on a Merchant's offers list, plugins have no Bukkit API way to affect trades, leading to messy uses of NMS classes and modified CraftBukkit classes, which can cause plugins to break or the server to crash. With an API, all plugin developers will be able to safely modify merchant trades (which as of current writing is only implemented through Villagers).

PR Breakdown:

The API adds the ability to retrieve current trades offered by a Merchant, add trades, remove trades and modify trades. There is also the added ability to influence event specific trades such that modified trades added to the MerchantTradeEvent do not alter the more permanent TradeOffers supplied by the Merchant.
In this sense: A MerchantTradeEvent's offers are a copy of the Merchant's offers that do not affect the Merchant's offers. However, the Merchant is still able to have their trades altered, but the MerchantTradeEvent will not be affected.

Testing Results / Materials:

Plugin code used to test: https://github.com/gabizou/TradeAPITest
Compiled Binary: https://dl.dropboxusercontent.com/u/9058563/TradingTest.jar

Steps to test:

• Spawn villager
• Interact with villager to bring up the Trade window

Console shows the debugging text

Associated PRs
Bukkit PR: #1077
CraftBukkit PR: Bukkit/CraftBukkit#1393

* Relevant PRs*
Closed Bukkit PR: #921
Closed Bukkit PR: #833
Closed Bukkit PR: #832
Closed CraftBukkit PR: Bukkit/CraftBukkit#1240
Closed CraftBukkit PR: Bukkit/CraftBukkit#1111

JIRA Tickets

Main ticket: https://bukkit.atlassian.net/browse/BUKKIT-5663
Subsequent (duplicate) tickets
https://bukkit.atlassian.net/browse/BUKKIT-3935
https://bukkit.atlassian.net/browse/BUKKIT-2138

[mbax]

It can't be modified. Don't suggest it :P

[Wolvereness]

You leak mutable objects everywhere, which is unacceptable. The inventory API is bad enough as it is, we don't want to make it worse. Examine how ItemMeta works for a good example of how mutability needs to be handled; ItemStack always returns a copy. Any of the new events with items return clones.

[gabizou]

You leak mutable objects everywhere, which is unacceptable.
Any of the new events with items return clones.

Should be fixed in latest commit. I've added a utility method to MerchantEvent that performs a deep copy of a list of TradeOffers through ImmutableList and added the relevant ItemStack.clone where necessary.

I should note, I initially didn't have such cloning immutability because of the pre-existing API leading me to believe that mutability was more of a standard and over immutability/cloning from getters would be counter productive. I appreciate knowing now that future API should have an general sense of immutability than mutability.

[Wolvereness]

Adding a comma at the end would be appropriate.

[Wolvereness]

• @Utility was designed for ItemStack or other API (read: CB) where extending it is intended.
• MerchantAddOfferEvent.getCurrentOffers() leaks mutable references.
• Why isn't TradeOffer immutable to begin with?
• Can TradeOffer have all public/protected constructors removed? Something like fireworks or Color. Relying on parameter types without a good name isn't the best idea for a sane API, especially when I don't think extending it is intended.
• When accepting an ItemStack via API that goes back into CB, always use new ItemStack(ItemStack), as we can't confirm the plugin doesn't have a malicious clone() method. ItemMeta instances are fine as long as they have been passed through the ItemFactory at some point (for example, in book events, it's compared to null, which throws an exception for malicious input).
• TradeOffer implements equals(Object) but not hashCode().

I'm sure there are many more issues, but I can't take much more at the moment.

[gabizou]

Why isn't TradeOffer immutable to begin with?

It's possible, just, as I've said before, I was basing this off of the current Inventory API, I can refactor all of this to use a TradeOffer Builder just as well.

Can TradeOffer have all public/protected constructors removed? Something like fireworks or Color. Relying on parameter types without a good name isn't the best idea for a sane API, especially when I don't think extending it is intended.

Yes. I'll refactor to a static builder to produce an immutable TradeOffer instead of constructors.

[gabizou]

My one worry is that doing this breaks a line where TradeOfferMeta isn't used for a singular Item or Entity, but can have several. Is it your intention to see this as an example:

TradeOffer offer = TradeOffer.builder()
                        .withFirstItem(new ItemStack(Material.DIAMOND, 1))
                        .withResultingItem(new ItemStack(Material.STICK, 1))
                        .build();
TradeOfferMeta meta = offer.getTradeMeta();
meta.setFirstItem(new ItemStack(Material.GOLD_INGOT, 1));
offer.setTradeMeta(meta);

I'm guessing it's doable, however, I can foresee issues with a TradeOffer being inherently mutable unless the method #setTradeMeta returns a clone of TradeOffer with the new meta (unlike ItemStack).

[Wolvereness]

I don't see a purpose in having a 'meta' or having 'setters' anywhere.

[gabizou]

I don't see a purpose in having a 'meta' or having 'setters' anywhere.

So, there would be no link whatsoever from a TradeOffer to MerchantRecipe aside from constructing a TradeOffer from a MerchantRecipe for event throwing and org.bukkit.entity.Merchant#getOffers()?

[Wolvereness]

So, there would be no link ...

If you can remove almost every possible way to change data, other than a place that explicitly defines what is being changed, that's a good thing.

If an event is designed to change the 'result', then the only place to change that result should be in a method for that event called setResult(...). If an event is designed to only be observed, based on the predicate state, then no part of that event should have any way to change the data, unless it's by virtue of the representation (like, providing a location.clone()). If we're adding something new to represent the data, why not make it immutable?

[gabizou]

If we're adding something new to represent the data, why not make it immutable?

Make sense. I'm just worried that opening the ability to set the TradeType.VANILLA to the API may be a bad thing.

[Wolvereness]

If we need to restrict a specific way to represent data, then it must be in CraftBukkit, with interfaces provided in Bukkit. If you think we need a way to differentiate trades, then it should be provided in the event. A MultiMap<TradeData, TradeSource> would work for that perfectly (as it stores a count, source, and data).

[gabizou]

A MultiMap<TradeData, TradeSource> would work for that perfectly (as it stores a count, source, and data).

I'm don't understand how that would work. It's probably because it's late and I should go to sleep. I'll read up on MultiMap and see how to implement it for this.

[Wolvereness]

type.hashCode()

[Wolvereness]

You haven't tested the serialization methods at all. You should make unit tests for that.

In addition, you leak ItemStack references in it.

[Wolvereness]

I don't like the existence of many of those methods that return copies. Just add a method in the builder to copy values.

Considering it's in the same source / package, you can even use the fields as a shared secret (assuming you never leak it).

[gabizou]

You haven't tested the serialization methods at all. You should make unit tests for that.

Added tests to the CB PR.

In addition, you leak ItemStack references in it.

Fixed.

I don't like the existence of many of those methods that return copies. Just add a method in the builder to copy values.

Considering it's in the same source / package, you can even use the fields as a shared secret (assuming you never leak it).

Added.

[amaranth]

added to?

[amaranth]

• Why do we care if the trade is from vanilla or a plugin? That doesn't seem like useful information.
• Why do you let someone modify the trade being removed? That could make the removal fail and if they want to remove something else they can cancel this event and do it with other API.
• You seem to have also designed this as a way to open custom trading GUIs based on your setTradingPlayer method and the fact that you can modify the trades shown without modifying the trades of the villager. If you want custom trading GUIs you should add support for them to openInventory and friends. I don't think these are appropriate for the villager API.
• Do the changes in 1.8 break the functionality of this plugin? They seem to break some adventure maps and a problem with an API like this in the past has been that Mojang keeps redesigning how trading works.

[turt2live]

@throws is not required.

[gabizou]

When accepting an ItemStack via API that goes back into CB, always use new ItemStack(ItemStack), as we can't confirm the plugin doesn't have a malicious clone() method. ItemMeta instances are fine as long as they have been passed through the ItemFactory at some point (for example, in book events, it's compared to null, which throws an exception for malicious input).

@turt2live As requested.

[turt2live]

@gabizou source? The following is from this PR:

#1077 (comment)

Everywhere you return an ItemStack, you should be using .clone() instead of new ItemStack(itemStack). The spots that you are receiving an ItemStack should keep the safe-copy behavior.

[gabizou]

@gabizou source? The following is from this PR:

https://github.com/Bukkit/Bukkit/pull/1077/files#r15209292
In this case, the Builder is taking in an ItemStack, of which case, we must defensively copy as the comment mentions.
#1077 (comment)

For all other cases where I am returning an ItemStack, I am performing ItemStack#clone() since it was already defensively copied from the Builder.

[turt2live]

That still doesn't tell me where you got that quote from.

I like pie. ~ Albert Einstein

[gabizou]

That still doesn't tell me where you got that quote from.

Ok, sorry, so we've linked to Wolvereness's comment twice now, I was doing as told, unless you're saying that I should go against what Wolvereness was saying about defensively copying the ItemStack as I have done in the builder.

For the sake of repetition:
#1077 (comment)

Is it clear that I didn't know enough GitHub-comment-fu to link the direct comment itself?

[ST-DDT]

The link does indeed state @Wolvereness option.
I guess he have to clearify his opinion himself here to solve your issues.

Maybe i'm wrong but i read the comment this way:
If you recieve the item always from a save source (Bukkit code) use clone().
Use this version if you already made a defensive copy (in the constructor or somewhere else/Bukkit code) and should return that result as "immutable" copy.
If you potentially recieve an altered ItemStack from some plugin use the defensive cloning new ItemStack(item) to avoid issues due to corrupted clone methods in child classes.

[Wolvereness]

@ST-DDT has it about right. I'd only make the exception for an assumption that a certain code path is safe. If somewhere in the chain a plugin can implement an interface, or extend a class included in the Bukkit jar, then it's considered unsafe. Those places are gradually shrinking though, as more of the API becomes defensively safe.

[gabizou]

@turt2live Updated the javadocs. I didn't change any of the new ItemStack(ItemStack) lines yet. I've removed most all of the @throws Illegal.... as requested.