DevLeoko / AdvancedBan

AdvancedBan is a Spigot plugin to manage punishments on single servers and server networks
GNU General Public License v3.0
156 stars 128 forks source link

PunishmentManager gives empty list for banned player #637

Closed Andre601 closed 5 months ago

Andre601 commented 5 months ago

What version of AdvancedBan (/AdvancedBan) are you using? 2.3.0 (Latest downloaded from Spigot)

What kind of server do you have (Bungeecord/single server)? BungeeCord

What server version (/version) are you using? Proxy: 1.20-R0.2-SNAPSHOT (92b5149 build 562)

Please provide the EXACT steps required to reproduce the problem...

  1. Get a PunishmentManager instance with PunishmentManager.get()
  2. Try getting a Punishment from a player with PunishmentManager#getPunishments(uuid, type, boolean);
  3. Receive an empty list of Punishments despite the player being banned

Any error/log post it through pastebin.com and link it here. (Also include /plugins/AdvancedBan/logs/latest.log) No latest.log available by the plugin. Folder is empty.

Add any additional information below. I'm currently in the process of implementing an addon for my own plugin to allow placeholders using AdvancedBan data on the proxy... But right now whenever my plugin tries to obtain a Punishment from a guaranteed punished player is the List empty, resulting in a false negative output.

The full Class where I handle placeholders and use the PunishmentManager of AdvancedBan:

/*
 * MIT License
 *
 * Copyright (c) 2022-2024 Andre_601
 *
 * Permission is hereby granted, free of charge, to any person obtaining a copy
 * of this software and associated documentation files (the "Software"), to deal
 * in the Software without restriction, including without limitation the rights
 * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
 * copies of the Software, and to permit persons to whom the Software is
 * furnished to do so, subject to the following conditions:
 *
 * The above copyright notice and this permission notice shall be included in all
 * copies or substantial portions of the Software.
 *
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
 * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 * SOFTWARE.
 *
 */

package ch.andre601.advancedserverlist.advancedban.placeholders;

import ch.andre601.advancedserverlist.api.PlaceholderProvider;
import ch.andre601.advancedserverlist.api.objects.GenericPlayer;
import ch.andre601.advancedserverlist.api.objects.GenericServer;
import me.leoko.advancedban.manager.PunishmentManager;
import me.leoko.advancedban.utils.Punishment;
import me.leoko.advancedban.utils.PunishmentType;

import java.util.List;

public class AdvancedBanPlaceholders extends PlaceholderProvider{
    public AdvancedBanPlaceholders(){
        super("advancedban");
    }

    @Override
    public String parsePlaceholder(String placeholder, GenericPlayer player, GenericServer server){
        PunishmentManager manager = PunishmentManager.get();

        String[] values = placeholder.split("\\s", 2);
        Punishment punishment = getPunishment(player.getUUID().toString(), manager, values[0]);

        System.out.println("Placeholder called!");
        System.out.println("Data:");
        System.out.println("  - Player: " + player.getName() + " (" + player.getUUID() + ")");
        System.out.println("  - Has Punishment: " + (punishment == null ? "No" : "Yes"));

        if(punishment == null)
            return null;

        return switch(values[0]){
            case "isMuted" -> String.valueOf(manager.isMuted(player.getUUID().toString()));
            case "isBanned" -> String.valueOf(manager.isBanned(player.getUUID().toString()));
            // Mute/Ban reason
            case "muteReason", "banReason" -> punishment.getReason();
            // Mute/Ban duration
            case "muteDuration", "banDuration" -> {
                boolean fromStart = false;
                if(values.length > 2)
                    yield null;

                if(values.length == 2)
                    fromStart = Boolean.getBoolean(values[1]);

                yield punishment.getDuration(fromStart);
            }
            // Unknown placeholder
            default -> null;
        };
    }

    private Punishment getPunishment(String uuid, PunishmentManager manager, String placeholder){
        PunishmentType type = null;
        switch(placeholder){
            case "isMuted", "muteReason", "muteDuration" -> type = PunishmentType.MUTE;
            case "isBanned", "banReason", "banDuration" -> type = PunishmentType.BAN;
        }

        if(type == null){
            System.out.println("Type was null");
            return null;
        }

        System.out.println(type.getName());

        List<Punishment> punishments = manager.getPunishments(uuid, type, true);
        if(punishments.isEmpty()){
            System.out.println("Punishment list was empty");
            return null;
        }

        return punishments.get(0);
    }
}

The logs print as follows:

[17:34:42] [Netty Worker IO Thread #1/INFO]: [/127.0.0.1:60306] <-> InitialHandler has pinged
[17:34:42] [Netty Worker IO Thread #1/INFO]: Ban
[17:34:42] [Netty Worker IO Thread #1/INFO]: Punishment list was empty
[17:34:42] [Netty Worker IO Thread #1/INFO]: Placeholder called!
[17:34:42] [Netty Worker IO Thread #1/INFO]: Data:
[17:34:42] [Netty Worker IO Thread #1/INFO]:   - Player: Andre_601 (286f8d0c-b571-4720-b7ab-f2929cb38120)
[17:34:42] [Netty Worker IO Thread #1/INFO]:   - Has Punishment: No

This shows that the PunishmentType is Ban, which I used for testing here, yet getPunishments with a valid UUID and punishment type returns an empty list...

I'm out of ideas what the issue is. My only guess would be either that the database does not have the UUID stored and instead assumes "target" is a username? Alternatively is this a caching issue on your end, or an issue with the boolean arg to get recent and not historical punishments.

Either way, I would appreciate some help here, because Javadocs don't help with this at all and the plugin lacks a proper wiki for developers to work with this plugin...

Hopefuls commented 5 months ago

Use the UUIDManager for the uuid parameter

Andre601 commented 5 months ago

I would assume it's UUIDManager#getInMemoryName(String) here? Or should the getNameFromUUID(String, boolean) be used here? If so, should I also force init on it?

And finally, why does AdvancedBan handle UUIDs like that (As Strings)? Why not utilize actual UUID instances and have the resolving done behind the scenes (As in making a method like getPunishments that actually takes a UUID parameter and then have it resolved in the method using the UUIDManager)? This here feels like a lot of extra work that devs shouldn't really need, not to mention a lot of confusion on what you need to provide for what method.

But given this resource hasn't seen an active commit since 2 years would my suggestions most likely not see any possible implementation here...

Andre601 commented 5 months ago

Also, looking at the getPunishments method's description, I cannot see where using UUIDManager is helpful here...

The description for the target parameter says "the uuid or ip to search for", so it should accept a String of a UUID, which I do give here... So what does the UUIDManager do differently here?

Andre601 commented 5 months ago

I found the problem, which from what I get is, that AdvancedBan wants UUIDs without dashes which is.... why? If you use UUIDs, support the default format?

Either way, it seems to work now.

Hopefuls commented 5 months ago

I found the problem, which from what I get is, that AdvancedBan wants UUIDs without dashes which is.... why? If you use UUIDs, support the default format?

Either way, it seems to work now.

This is to keep the lookups on UUIDs consistent.

The reason why the UUIDManager exists is to also support the lookup of non-premium users. So best is to stick with it rather than using the getUniqueID one

Andre601 commented 5 months ago

Well, I don't support nor endorse offline-mode servers or proxies, so there is another reason now for why I won't use the UUIDManager... The solution I have right now seems to work, and if people complain will I simply tell them the same as I just did here: That I don't support offline mode.