Fix MemberCacheViewImpl#getElementsWithRoles and Guild#findMembersWithRoles ignoring the public role

[Almighty-Satan]

Pull Request Etiquette

• I have checked the PRs for upcoming features/bug fixes.
• I have read the contributing guidelines.

Changes

• Internal code
• Library interface (affecting end-user code)
• Documentation
• Other: _____

Closes Issue: #2291

Description

Fixes an issue that caused MemberCacheViewImpl#getElementsWithRoles to return an empty list when given the public role.

[DManstrator]

Another thing:

The JavaDocs of Guild#getMembersWithRoles states it will be checked if roles are from a different guild. But according to the impl, this is not the case. Could you add that check?

Only Guild#findMembersWithRoles checks for that yet.

[DManstrator]

One another thing:

I'm not sure if it is a good idea to call getMembersWithRoles in case the public role is present. If it's loaded or empty, it makes sense to me. I don't know enough to be sure that it won't have any negative effect to not make a request in such a situation.

Can somebody confirm it's okay?

[Almighty-Satan]

One another thing:

I'm not sure if it is a good idea to call getMembersWithRoles in case the public role is present. If it's loaded or empty, it makes sense to me. I don't know enough to be sure that it won't have any negative effect to not make a request in such a situation.

Can somebody confirm it's okay?

That method seems to be broken too. isLoaded() makes sense but the other two don't

[DManstrator]

That method seems to be broken too. isLoaded() makes sense but the other two don't

I think you are right. I thought doing it for isEmpty is now also valid since you added a "roles is empty? return all members" check for getMembersWithRoles. But yeah, what if the locally cached member list is outdated? Then wrong members might be returned.

So I think in the end, the same changes you made need to be applied for findMembersWithRoles too. And regarding performance, if the list is empty (after removing the everyone role), loadMembers() could be returned.
You already did that, nice. :D

Sadly, this is not really a good performance improvement since this will still iterate each member (loadMembers(Consumer<Member>) is used under the hood which returns a Task<Void>).

Perhaps in a different MR, this could be adjusted but it's definitely not in the scope of this MR.

[DManstrator]

Re commit 2480ef9:

Couldn't you add that role check to MemberCacheViewImpl instead? The Varargs -> List transformation is also done there.

[Almighty-Satan]

Couldn't you add that role check to MemberCacheViewImpl instead? The Varargs -> List transformation is also done there.

I don't think that's possible because MemberCacheViewImpl does not have a reference to the guild. Also Guild#findMembersWithRoles has to run that check anyway

[DManstrator]

True, didn't saw that, thanks.

Speaking about Guild#findMembersWithRoles: Could you add the Checks.noneNull(roles, "Roles"); for consistency too?

[DManstrator]

No, you got me wrong. As said before, it can't be null due to two methods having the the same name and the same number of parameters. The compiler would simply not allow it without explicit casting. And if somebody does that, it's more or less their fault imo.

And no need to check for noneNull again since you already do that in getMembersWithRoles(List<Role>). So you can remove that check again (or delete the last two commits via. rebase).

[Almighty-Satan]

That method has to check notNull otherwise something like Role[] roles = null; guild.getMembersWithRoles(roles); would cause Arrays#asList to throw an NPE

And no need to check for noneNull again since you already do that in getMembersWithRoles(List<Role>)

Yes

[DManstrator]

That method has to check notNull otherwise something like Role[] roles = null; guild.getMembersWithRoles(roles); would cause Arrays#asList to throw an NPE

Yes, technically you're right. But as said, you would explicitly need to cast it / define it like that so it compiles. And as a user you should see this is simply wrong. So imo it's fine to let the user run into this. Because essentially you just trade a NullPointerException with an IllegalArgumentException. Doesn't help much but needs more code (maintenance).

From what I see it's also not used in other places and consistency is more important. So imo, it's really fine to remove that check again.

Just checked it again, it's actually used in MemberCacheViewImpl#getElementsWithRoles.

So my bad, it should actually stay since as I just said, consistency is more important. Good catch!

[DV8FromTheWorld]

Added the semi-breaking tag as this is a long standing behavioral change that might effect existing code.