getMembers() inconsistency

[DDuart]

Description

Describe the bug

getMembers() from the IslandsManager doesn't return TRUSTED players.

Steps to reproduce the behavior

BentoBox.getInstance().getIslands().getMembers(world, player.getUniqueId())

doesn't return TRUSTED players, but...

BentoBox.getInstance().getIslands().getIsland(world, player.getUniqueId()).getMembers()

works fine.

[Poslovitch]

@tastybento We'll need to discuss about that.

In my opinion, we should get rid of the "convenience" methods in the IslandsManager and just keep the #getIsland() ones, because they aren't convenient at all if they lead to inconsistencies. It'd also avoid such inconsistencies in the future, and it would avoid us having to maintain what are actually duplicate methods that technically aren't.

If we take a closer look at what's happening here:

BentoBox/src/main/java/world/bentobox/bentobox/managers/IslandsManager.java

Lines 393 to 403 in 4d8bb71

	/**
	* Returns a set of island member UUID's for the island of playerUUID
	* This includes the owner of the island. If there is no island, this set will be empty.
	*
	* @param world - world to check
	* @param playerUUID - the player's UUID
	* @return Set of team UUIDs
	*/
	public Set<UUID> getMembers(World world, UUID playerUUID) {
	return islandCache.getMembers(world, playerUUID);
	}

And the Island Cache is returning... the #getMemberSet() method!

BentoBox/src/main/java/world/bentobox/bentobox/managers/island/IslandCache.java

Lines 161 to 174 in 4d8bb71

	/**
	* @param world world to check
	* @param uuid uuid of player to check
	* @return set of UUID's of island members. If there is no island, this set will be empty
	*/
	@NonNull
	public Set<UUID> getMembers(@NonNull World world, @NonNull UUID uuid) {
	islandsByUUID.putIfAbsent(Util.getWorld(world), new HashMap<>());
	Island island = islandsByUUID.get(Util.getWorld(world)).get(uuid);
	if (island != null) {
	return island.getMemberSet();
	}
	return new HashSet<>(0);
	}

This can be error-prone (and it's likely already). That's why I'm putting this ticket to a High priority.

[tastybento]

In response to @DDuart's original post, let's look at why this is happening:

1. Island.getMembers() gets a Map of UUID and rank for all players that have a known relationship with the island object. This can include any player of any rank, including TRUSTED, but also BANNED as well.
2. The IslandManager.getMembers() returns an immutable set of only the actual members of the island, i.e., those with a rank of MEMBER or above. This pulls from the Island.getMemberSet() method.

The issues here are as follows:

1. The IslandManager.getMembers() JavaDoc does not make it clear that this method returns only the MEMBER rank and above.
2. There's no IslandManager method yet to get island members that include ranks higher than VISITOR but lower than MEMBER, e.g. TRUSTED, COOP or other future or custom ranks. The workaround is to get the raw member map from Island and filter appropriately.
3. The JavaDoc for Island.getMembers() does not make it clear that this map contains every ranked member of the island.
4. The Island.getMemberSet() method isn't named that well, but the JavaDoc is accurate. By the way, the IslandManager.getMembers() method must be called "getMembers" because the class is a JavaBean and getter and setter methods must be named exactly the same as the field "members". That is why there is a separate getMemberSet() method.

These issues can all be fixed easily.

@Poslovitch's in regards to your recommendation, getMembers in IslandManager is used by many, many classes (36 times in BentoBox itself) and is very useful to have. If it was removed, those classes would have to add code to obtain the island object first, make sure it existed and then get the member set. That's a lot of duplicated code that could also be done incorrectly and cause bugs. If you can find a way to shrink the code burden or minimize duplication, I'm all for it, but let's not remove the methods
without having a better solution.

To fix this bug I recommend we fix the JavaDoc and add an optional parameter to the IslandManager getMembers() method to stipulate the minimum rank requested. e.g., IslandManager.getMembers(world, user, RanksManager.TRUSTED) would provide all island members of TRUSTED and above.

[Poslovitch]

Okay, I get the point here.

And it seems that the current implementation suffers from the fact that the island "members" are not only those who are actually members of it. How could we rename that field to better reflect what's going on in it?

[Poslovitch]

A few methods got added in #628.
Hopefully it will do the job @DDuart 😉

[tastybento]

@Poslovitch In theory, the Island object could use the name rankedPlayers instead of members, but then all the existing games out there would end up not working. I think we're stuck with members for now.

[Poslovitch]

We can always use some deprecation.

[BONNe]

Create new method and current one deprecate... till next release.

[tastybento]

Yes, but the island members are stored in the database, so additional code would be required to transition from the older storage to the newer one. I'm afraid we're stuck with the current object layout and field names unless that's done, and that's a messy thing to do just to rename a method.