Review methods which return TriStates but do not utilise all three possible states

[lokka30]

There are a bunch of methods in Treasury's API which use TriStates for return types, however, it's possible that some of these methods might be better off with a different return type.

Methods to review

Tick if resolved (either 'changed' or 'cancelled', doesn't matter).

• Account

  • [ ] I want to review methods that return a TriState seemingly for zero reason
  • [x] hasPermission edit: this uses TSs because it allows a plugin to assume a default if it's unspecified. no
  • [x] EconomyProvider#hasAccount. either the account exists or it doesn't. why is there a level of uncertainty?
  • [x] Account#isMember ^^ similar reason. either they're a member or they're not.
  • [ ] Anything which returns a TriState depending upon whether an action made changes, made no changes, or failed (true/unspecified/false respectively).

    I want to review these because not all data implementations use SQL. SQL returns how many rows were modified which was considered when making these methods return a TriState for that purpose. However, other data implementations such as a Gson system do not return if any changes occurred. In this case we would have to make actions more expensive by performing an unnecessary lookup to check whether the old data equals the new. Lastly, it is completely redundant to know if something has changed or not, all we care about is whether or not the action we called has succeeded. If it's succeeded, regardless of whether the result was already there e.g. balance was already $50, then that's exactly what we as an implementer wanted so who cares if no rows were changed. End of poorly-formatted ramble.

  • [ ] Account#setPermissions
  • [ ] Account#setName

• ... list incomplete; add any methods you would like to be reviewed for TriStates ...

On another note, we need to make sure all methods returning TriStates document what TRUE, FALSE, and UNSPECIFIED mean for each of those methods. For instance, UNSPECIFIED usually means 'no change occurred from this action'.

[MrIvanPlays]

hasPermission can return TRUE, FALSE, and when not set, UNSPECIFIED. the same logic applies for all the other methods using it.

[lokka30]

hasPermission can return TRUE, FALSE, and when not set, UNSPECIFIED. the same logic applies for all the other methods using it.

you're using hasPermission to check if someone has or doesn't have a permission - there's no in-between. if you want a third state, then that's where you can use retrievePermissions.

[MrIvanPlays]

Yes there is. Check all modern apis that have permissions. TriState is not the exception, it's the norm.

[lokka30]

Yes there is. Check all modern apis that have permissions. TriState is not the exception, it's the norm.

Ok, we'll leave permissions as-is, as usual I have realised why we need 3 states for hasPermission (because we want plugins to be able to assume their own default if the permission is not specified).

Issue remains open for other methods which I'll need to find and list here.

[MrIvanPlays]

still waiting for your finds (which are probably none).

[lokka30]

still waiting for your finds (which are probably none).

I've had no motivation to fire up IntelliJ in the past week. 😄 I'll get to it soon.

[lokka30]

Apologies for the delay. I've updated the entire initial post (please check from top to bottom).

[MrIvanPlays]

Nice finds for hasAccount and isMember, I am gonna change these. As for setPermissions and setName, as for someone like me I'd want to have such outcome because now I can do something like this: I call setName:

• got TRUE: send a message "Name was successfully changed to ..."
• got FALSE: send a message "Name stayed the same"
• got UNSPECIFIED: usually in this situation UNSPECIFIED means some uncaught error occurred either in the provider or in my handling (or it was a caught error but errors are errors and we can't just "solve them" or know what's exactly wrong in the code, so we print it), hence send a message along the lines of "Something went wrong" Kinda the same logic applies for setPermissions ^^.

As for "not all data implementations use SQL", every self respecting developer will have a cache of the data if it's using any type of flatfile storage (be it json or yaml), because querying a whole file for a single account (for example) creates much more overhead on the µP and DRAM rather than it staying cached.

[lokka30]

As for "not all data implementations use SQL", every self respecting developer will have a cache of the data if it's using any type of flatfile storage (be it json or yaml), because querying a whole file for a single account (for example) creates much more overhead on the µP and DRAM rather than it staying cached.

Absolutely, everybody should be using a cache (though we can't force that as an API). However, still, there is a performance and code overhead of checking if data has changed from a transaction. I don't really care about the performance overhead, since CFs make it clear that nothing is instantaneous. However, the amount of code that Treasury requires over Vault to implement things is already high. I'm on the fence about it, I wouldn't mind whether we choose to keep or remove this TriState 'data has changed from action' logic. If we wanted this feature, I'm thinking it would be a better component for Response, i.e. Response#dataChanged or something like that. Then again, #256 may be agreed upon which would remove Responses, so unsure on this.

[MrIvanPlays]

I can't see any overhead if implemented correctly no matter the storage type.

[lokka30]

I can't see any overhead if implemented correctly no matter the storage type.

That's alright, I don't mind if we go forward with keeping TriState usages purposed for 'Has data changed from this action?', although with the condition that we inform in the javadocs that an implementation may choose to not do the 'has data changed?' check and just return 'true' if it was successful.

[MrIvanPlays]

'has data changed' directly correlates to whether the method went through successfully. first of all how are we defining 'successfully'? some implementations may call 'successfully' is that they've received the call and it went through without any errors (no matter whether data changed), others might call 'successfully' as in 'data changed successfully'.

[lokka30]

'has data changed' directly correlates to whether the method went through successfully. first of all how are we defining 'successfully'? some implementations may call 'successfully' is that they've received the call and it went through without any errors (no matter whether data changed), others might call 'successfully' as in 'data changed successfully'.

I would classify 'success' as the former. Not only have no errors occurred, but the data is exactly what the caller wanted it to be, regardless whether or not it already had the desired value. If someone cares whether or not data changed as the result of an action, they should check and save such data, then modify it, then compare it themselves.

Again, I don't ultimately mind if we choose to use TriStates as a 'has data changed' indicator, so long it is optional to use (i.e. an implementation may decide to just return TRUE if the action happened without any errors). I see that this feature is useful for an economy provider to send a message e.g. for 'set balance' alike 'Notch's balance is already $50'. That being said, I am still concerned about increasing the amount of code required for non-SQL implementations just for a feature which is mostly cosmetic. Also recall my message above discussing how this could be better implemented in Response (if we decide to keep that system for whatever reason - doesn't seem like that will happen).

[MrIvanPlays]

If someone cares whether or not data changed as the result of an action, they should check and save such data, then modify it, then compare it themselves.

No... that is just forcefully making a plugin cache the data (which may be already cached by the provider). Plugins which utilize the API (and by utilize I mean they do not implement it) shall not store any data that the provider already handles.

Again, I don't ultimately mind if we choose to use TriStates as a 'has data changed' indicator, so long it is optional to use (i.e. an implementation may decide to just return TRUE if the action happened without any errors).

That's like saying you have a REST(ful) API call which sometimes outputs one type of data, but sometimes it will output completely different type of data. We need API standards. We can't just do it like "yeah it returns whether the data changed, but it sometimes might return whether the call was successful, so figure it out by yourself what type of data you're dealing with". See the problem?

Also recall my message above discussing how this could be better implemented in Response (if we decide to keep that system for whatever reason - doesn't seem like that will happen).

Response is getting ditched. It has literally 0 purpose except some code overhead that everyone has to deal with it.

[lokka30]

No... that is just forcefully making a plugin cache the data (which may be already cached by the provider). Plugins which utilize the API (and by utilize I mean they do not implement it) shall not store any data that the provider already handles.

I think we're talking about a vast minority of plugins here.

@Jikoo may I bring you into this? I'd like to hear what you think on requiring methods to return TriStates with the purpose of the UNKNOWN value meaning 'no change happened to the data from this action', e.g. 'balance is already $50'. Ivan has a point that this is very easy to do on SQL impls (5 rows changed), etc. However, not everything is using SQL.

That's like saying you have a REST(ful) API call which sometimes outputs one type of data, but sometimes it will output completely different type of data. We need API standards. We can't just do it like "yeah it returns whether the data changed, but it sometimes might return whether the call was successful, so figure it out by yourself what type of data you're dealing with". See the problem?

I see the problem. You're right there, we need to make sure our API is used in a strict manner.

Response is getting ditched. It has literally 0 purpose except some code overhead that everyone has to deal with it.

:+1:

[Jikoo]

I'd like to hear what you think on requiring methods to return TriStates with the purpose of the UNKNOWN value meaning 'no change happened to the data from this action', e.g. 'balance is already $50'. Ivan has a point that this is very easy to do on SQL impls (5 rows changed), etc. However, not everything is using SQL.

I don't mind an indicator of unchanged data, but UNKNOWN has negative connotations here, i.e. "Something went wrong, status is unknown," instead of "Nothing happened, balance was 50 already."

This almost feels like a case where we want a more specific return value. As much as more clutter isn't good, if it adds clarity it might be worth it. Self-documenting code, etc.

[lokka30]

Thanks for the reply @Jikoo.

If we are to keep this feature then I would like to have addressed:

• Who benefits from his feature?
  • Any example use case that can take advantage of the effort required to keep this feature in?
• How are non-SQL data storage solutions expected to implement this in a clean manner?
• If we choose to make a new return type to replace these specific usages of TriState (see Jikoo's reply above), how will we not clutter Treasury more? Additionally,
  • How will we keep consistency in the API since there are methods in the API which perform an action but return a custom value rather than a TriState.
  • Would this be any better than just keeping TriState and people can see the javadocs for what UNKNOWN and TRUE represent for these cases?

[MrIvanPlays]

If we are to keep this feature then I would like to have addressed:

You are making an elephant from a fly.

Who benefits from this feature? Any example use case that can take advantage of the effort required to keep this feature in?

What effort? It's java: write once, if it works don't touch it, run anywhere.

How are non-SQL data storage solutions expected to implement this in a clean manner?

Non-SQL data storage solutions are basically MongoDB and flatfile (flatfile as in yaml, gson, etc.). iirc mongo already provides such feature as in SQL, so then we have flatfile. When the storage is flatfile, the implementation should ALWAYS have some type of a cache, because file IO is much more expensive than just having a cache. Hence, a simple contains check does it.

If we choose to make a new return type to replace these specific usages of TriState

And why is that needed?

How will we keep consistency in the API since there are methods in the API which perform an action but return a custom value rather than a TriState.

Methods which already return some other result do not need such indication, because the result is actually the indication ; when you have an error - something went wrong ; when you don't have the result - something went wrong ; when you have the result - success.

I don't mind an indicator of unchanged data, but UNKNOWN has negative connotations here, i.e. "Something went wrong, status is unknown," instead of "Nothing happened, balance was 50 already."

@Jikoo please elaborate I can't understand what you mean here.

[lokka30]

You are making an elephant from a fly.

You say this often, and I agree. I am a perfectionist. I don't want to break this API any further and I am finding anything which could possibly cause issues for the API in the future. I will continue making elephants from flies until Treasury is squeaky clean. 🙂

Methods which already return some other result do not need such indication, because the result is actually the indication ; when you have an error - something went wrong ; when you don't have the result - something went wrong ; when you have the result - success.

If this is the case for all current and future usages of 'unchanged data = return UNKNOWN' then I am fine with keeping this feature. 👍🏻

Jikoo please elaborate I can't understand what you mean here.

I am quite certain he meant 'the name UNKNOWN might not be a suitable label for unchanged data'. Reflecting upon this now, I see the merit in this, however I disagree. The javadocs (should) clearly explain what each three states represent when a TriState is returned in a method. TRUE, UNKNOWN, and FALSE have completely different meanings when comparing some methods, this is intentional. To fix this we would need to create several custom return types, mostly used just once, which would clutter the code and achieve little to no betterment of the API.

[MrIvanPlays]

The name is UNSPECIFIED not UNKNOWN.

If this is the case for all current and future usages of 'unchanged data = return UNKNOWN' then I am fine with keeping this feature.

By methods which return some kind of result I meant methods such as findCurrency. There's no need for indication there.

[Jikoo]

The name is UNSPECIFIED not UNKNOWN.

Quite right, sorry.

Jikoo please elaborate I can't understand what you mean here.

I am quite certain he meant 'the name UNKNOWN might not be a suitable label for unchanged data'. Reflecting upon this now, I see the merit in this, however I disagree. The javadocs (should) clearly explain what each three states represent when a TriState is returned in a method. TRUE, UNKNOWN, and FALSE have completely different meanings when comparing some methods, this is intentional. To fix this we would need to create several custom return types, mostly used just once, which would clutter the code and achieve little to no betterment of the API.

I don't think that data unchanged is a non-specific result, either. I'm willing to cede this because both of you think it's fine, but I'm still firmly of the opinion that behavior not matching the name of the constant confuses things unnecessarily.

As an alternative, would UNMODIFIED not be appropriate TriState for a permission node that has not been set?

[MrIvanPlays]

Every implementation that uses TriState is using some form of UNSPECIFIED (be it UNDEFINED, NOT_SET or something else).

https://github.com/PaperMC/Velocity/blob/dev/3.0.0/api/src/main/java/com/velocitypowered/api/permission/Tristate.java#L39 https://github.com/KyoriPowered/adventure/blob/main/4/api/src/main/java/net/kyori/adventure/util/TriState.java#L41 https://github.com/SpongePowered/SpongeAPI/blob/api-8/src/main/java/org/spongepowered/api/util/Tristate.java#L65

So imho UNMODIFIED is not suitable.

[Jikoo]

Every implementation that uses TriState is using some form of UNSPECIFIED (be it UNDEFINED, NOT_SET or something else).

PaperMC/Velocity@dev/3.0.0/api/src/main/java/com/velocitypowered/api/permission/Tristate.java#L39 KyoriPowered/adventure@main/4/api/src/main/java/net/kyori/adventure/util/TriState.java#L41 SpongePowered/SpongeAPI@api-8/src/main/java/org/spongepowered/api/util/Tristate.java#L65

So imho UNMODIFIED is not suitable.

Their use cases do overlap in most areas with Treasury's, but they don't appear to have anything that quite lines up here. Those are used exclusively for situations where there is inheritance or fall-through behavior. UNMODIFIED's definition matches well enough with UNSPECIFIED/UNDEFINED/NOT_SET that I think it is comprehensible, but it also has the benefit of being a clear return value here. NOT_SET/UNSET also seems relatively synonymous if that suits your fancy, because at least that way it lines up with no data setting operation being needed.

It just seems wrong to return a constant whose name contradicts what it signifies - the return value correlates with a state that is neither unspecified nor undefined, it is that the data is in the requested state but required no change to achieve that state.

A general comment on the point not related to TriState naming conventions: a modification operation normally either returns the previous value or a boolean result. The boolean result is not success/fail, it is changed/unchanged. For example, Collection#add. One expects errors to be just that - errors. If an operation fails to execute, it should fail exceptionally, just like everywhere else in Treasury. In the light of that, I would argue that the TriState usage is unnecessary.

[MrIvanPlays]

So this leaves TriStates only for permission usage. I'm fine with that and will change it (currently working on removing responses, will change that too).

[lokka30]

Great points Jikoo. Thanks