Raising errors on invalid ip requires rewrite of client logic

[PerchunPak]

I have code, that parsed and ensured the ip is valid. And all this with object from MinecraftServer.lookup.
So I need to rewrite almost all code which uses mcstatus objects, just because da4770a.
Add switch, or something like this please.

Also I can't do fast-fix just with except: MineServer.lookup("not-valid.com") because object with offline server != object with invalid ip.

P.S. I don't ask remove this feature, it would be nice to give messages like Invalid IP, reason: Port must be lower than 36635.

[ItsDrike]

You could always just make a custom overridden class that doesn't run the check but I suppose I wouldn't mind adding a bool arg to toggle the check as long as it's enabled by default, like you did in the PR.

I do wonder though, what code do you have that ensures the validity of the IP that's not compatible with the current checking which we're doing? There is a custom error raised (TypeError or ValueError) which you could easily catch and go from there. It is also possible to get the error message so you would be able to capture exactly what went wrong too.

[PerchunPak]

This code is spread to many files, so better inspect my code yourself.
Better start here.

[ItsDrike]

Seems to me like what you're really looking for is a separation of the DNS lookup logic into it's own staticmethod, so that you could just check the DNS without having to even make an instance of MinecraftServer, that way, you would also bypass the IP validity check.

I think doing that makes a lot more sense and even though I don't necessarily mind the bool arg to toggle the verification, there shouldn't be any need to do that, the IP verification should always happen if you're making a full MinecraftServer instance, since it does prevent certain errors which are very hard to debug as was shown in #189 even though this argument would be true by default and so this issue wouldn't occur for most people, it's a quick way to introduce bugs and if you really need to bypass it, I think making a custom class with different __init__ logic should be the way to go instead such toggle into the library, even though we never actually want anyone to set that toggle to false since that could break other methods in that class. You should only do this at your own risk.

But I do think separating the DNS parsing logic into it's own static method makes a lot of sense.

[PerchunPak]

This also could be a fix (move checks to a classmethod)
#202 (review)

No, sorry I missed up

[kevinkjt2000]

With the better regex expression that is RFC compliant, is this already fixed?

mcstatus/mcstatus/server.py

Lines 17 to 19 in df81275

	VALID_HOSTNAME_REGEX = re.compile(
	r"(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])"
	)

If not, please provide an example code that was the cause for reporting this issue.

[PerchunPak]

What fixed? The issue is that, there is existing any check which raising exception.
My code was based on an invalid object and then I checked if the IP was valid using socket.gethostbyname. Now when you pass invalid ip (example not_valid), my code crashes, because was wrote for another logic of mcstatus.

[ItsDrike]

The issue is that, there is existing any check which raising exception.

Why don't you just handle for that exception with try-except block then? You can simply catch the errors which the check is raising. That way, you don't need to use socket.gethostbyname and you can instead rely on our RFC compliant regex. This will also make your code faster since I've noticed you were doing this from an async function and even though DNS lookups aren't slow, it's certainly not great to have a blocking synchronous DNS request in an asynchronous function.

Handling these exceptions shouldn't be hard at all, we only raise TypeErrors or ValueErrors, but I've said this earlier. Is our logic really so unreliable that you need to be using socket.gethostname instead of the regex? If so, why? I didn't see anything wrong with it from my testing (though it wasn't that extensive). And because we're using unique exceptions for each case (or at least a unique description), you can know exactly what error has occured, catch it and handle it in any way you want.

Nevertheless, if you just need to perform the DNS lookup, you shouldn't even need to construct an entire instance of MinecraftServer. You can just check if you have a port, and if not, you can just perform the DNS lookup logic, mimicking what lookup is doing, but without making the actual instance, and therefore avoiding the initialization checks alltogether. Because of this I made the #203 PR, which should fix your issue by moving the DNS lookup function(s) out of lookup and it can now be performed without making a full instance easily. Not just that, but this would also save you from calling socket.gethostname and instead call our function that performs A record lookup after you've obtained the host and port from the SRV lookup (or you already had them), if you were only using gethostname to get the actual numeric IP instead of something like mc.hypixel.net.

This logic would look like this:

We simply can't remove these checks from __init__, because doing so would just get too messy and would require us to instead run the check before each function that actually makes any connections, which is too slow and pointless if it can just be done once at initialization. Alternatively, there could be another function that makes these checks and sets a bool flag that's required and just check that in these functions, but again, that would end up cluttering the code a lot, and just isn't worth doing in comparison to having a simple check on init. Not doing that at all is also not an option because as I've said several times now, it leads to exceptions which aren't easy to track back and are very unintuitive. But I've said this already too, granted though it wasn't in the context of this issue.

[PerchunPak]

Yes, I can use try-except block, but all this, in the end, will result in rewriting all the code somehow related to mcstatus objects.

So, as an example, I will give my piece of code that receives DNS information from mcstatus and parses it:

async def parse_ip(input_ip: str) -> ServerInfo:
    """Pasing IP.

    Args:
        input_ip: IP, given by user.
    Returns:
        ServerInfo obj with information, about server.
    """

    dns_info = MinecraftServer.lookup(input_ip)  # Here, mcstatus raising exception

    try:
        num_ip = gethostbyname(dns_info.host)
        valid = True
    except gaierror:
        valid, num_ip = False, None

    return ServerInfo(valid, dns_info, num_ip, str(dns_info.port))

Try rewrite it with try-except block

    try:
        dns_info = MinecraftServer.lookup(input_ip)
    except ValueError as err_msg:
        if "Port must be within the allowed range" in err_msg:
            return "Port must be lower than 65535 and bigger than 0"
        elif "(doesn't match the required pattern)" in err_msg:
            return "You need provide valid hostname"

    # I ensure that, ip is valid, because mcstatus check it
    # so removing try except here
    # P.S. this bad idea, because my tests crashed code with input_ip='www'
    num_ip = gethostbyname(dns_info.host)

    return ServerInfo(True, dns_info, num_ip, str(dns_info.port))

But as you can see, client code doesn't know what to do with non-ServerInfo objects. So let's add error codes to ServerInfo.

        if "Port must be within the allowed range" in err_msg:
            return ServerInfo(dns_info=None, num_ip=None, port=None, err_code=1)
        elif "(doesn't match the required pattern)" in err_msg:
            return ServerInfo(dns_info=None, num_ip=None, port=None, err_code=2)
            
    num_ip = gethostbyname(dns_info.host)

    # Remove field valid, because for this now created err_code
    return ServerInfo(dns_info, num_ip, str(dns_info.port), err_code=0)

This looks ugly, better let's create a new class for the exceptions that are returned by my code...

So, step by step, we will rewrite all the code that is somehow related to the mcstatus object.
Also, be aware that #203 is not currently merged and I can't conveniently download it with pip. Even after it's merged, I still have to rewrite most of the code.

I understand. You can't remove checks from __init__, the best you can do is to merge #201 or #203, which will allow me to rewrite smaller piece of the code.

[ItsDrike]

I don't see how this is an issue with mcstatus though. I agree that separating DNS functions to make things like this easier is a good thing to do for cases like these, I personally don't think we should make the ip validation optional though since again, it just causes issues and we don't really have any good reason for doing that. If you need something like this on your side, why don't you just make a subclass, override our behavior and be done with it?

class MyMinecraftServer(mcstatus.MinecraftServer):
    def __init__(self, host: str, port: int = 25565, timeout: float = 3):
        # ensure_valid_ip(host, port)  # Don't do this in your subclass
        self.host = host
        self.port = port
        self.timeout = timeout

This way you get exactly the behavior you were looking for without any need to alter the library code, which requires this validation to be there. But it's up to you to debug any potential errors if they occur because the address was invalid in case you were using functions that require it. To circumvent that, you could implement any of the proposed solutions (bool flag with a function that blocks all other if false set by another function or running the check before each function making a connection), or something else if you can think of it. Though you don't really need it for your use-case, so it's up to you to implement those, just know that not doing that may cause issues.

What I'd actually suggest though is just not storing MinecraftServer class as "dns_info" parameter, that's not what this class is for at all, the lookup function does perform some DNS logic to get the port if it wasn't specified and there's an SRV record, however that's just the initial logic, you don't need the actual class for that, all you care about is really just the obtained host and port after this lookup.

This would mean you'd have to change your ServerInfo class and change everything that uses it, but that's not really an issue of mcstatus. You don't need to do that, you could just go with the custom class and it would allow to to keep everything as it was, but your original method wasn't great in the first place. What you could also do is making MinecraftServer-like class, except all it would do is store the host and port and contain the DNS lookup logic, perhaps with the use of PR 203 separated functions once it's merged. This would also allow you to preserve your original logic without that many changes. It's a bit weird to have a class just for dns info, but it would mean you wouldn't really need to change anything except maybe some typehints. (Unless you actually were using some other features of that class)

class DnsInfo:
    def __init__(self, address: str):
        self.address = address
        host, port = mcstatus.server.parse_address(address) 
        # If the port wasn't in the address, we should first perform and SRV DNS lookup
        # and only if that fails should we rely on the default port
        if port is None:
            try:
                host, port = mcstatus.server.MinecraftServer.dns_srv_lookup(host)
            except dns.resolver.NXDOMAIN:
                port = 25565
        self.host = host
        self.port = port

What may actually be worth doing is making lookup-like staticmethod that just performs this logic but it only returns the host and port, instead of a full class instance, since even with the DNS logic now separated, it's still non-trivial to implement this and we could just call this from the original cleanup. I don't know how should a method like this be called, but it may be worth doing.

[PerchunPak]

Okay, thanks. But the only reason I use mcstatus for DNS info, is because you already have the code I wanted to write to check SRV and other stuff.

Also, creating my own subclass means I'll have to check each time to see if anything in __init__ has changed on update. I can of course automate this, but it doesn't seem like good practice.

[ItsDrike]

Okay, thanks. But the only reason I use mcstatus for DNS info, is because you already have the code I wanted to write to check SRV and other stuff.

Also, creating my own subclass means I'll have to check each time to see if anything in __init__ has changed on update. I can of course automate this, but it doesn't seem like good practice.

Indeed, It's not, but again you shouldn't be storing the entire class just for DNS info, the only reason I suggested that, was to give you a solution that doesn't break your compatibility. The custom class is a lot better, but again, you shouldn't even be storing a class, just use a named tuple with host and port, or store them as attributes direcly in your class.

But we could make it even easier for something like this by making that staticmethod function with lookup logic in it that I suggested along with also making the DNS functions, I wouldn't mind adding that into 203 when I'll have some time.

[kevinkjt2000]

Thanks for the extra information and discussion! I only asked for the code that raised the exception.

    dns_info = MinecraftServer.lookup(input_ip)  # Here, mcstatus raising exception

what is input_ip? "www"? That doesn't raise anything:

>>> from mcstatus import MinecraftServer
>>> MinecraftServer.lookup("www")
<mcstatus.server.MinecraftServer object at 0x7f27c12af4f0>

"127.0.0.1"? Also nothing raised:

>>> from mcstatus import MinecraftServer
>>> MinecraftServer.lookup("127.0.0.1")
<mcstatus.server.MinecraftServer object at 0x7feb7b50ecd0>

Soooo my question remains, what code is giving you an exception? I'm of a mind to close this because of the RFC compliant regex that was added. Make sure you are running the latest released version (v8.0.0 is the latest stable at the time of writing this and is what I used above), please :)

[PerchunPak]

I am using 9.0.0-dev2, because #198 (comment).
In 8.0.0 this checks wasn't added, only in dev version.

>>> from mcstatus import MinecraftServer
>>> MinecraftServer.lookup("www")
<mcstatus.server.MinecraftServer object at 0x000001F3C7467580>

What expected:

>>> MinecraftServer.lookup("not_valid_ip")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\perch\AppData\Local\Programs\Python\Python310\lib\site-packages\mcstatus\server.py", line 73, in lookup
    return cls(host, port, timeout)
  File "C:\Users\perch\AppData\Local\Programs\Python\Python310\lib\site-packages\mcstatus\server.py", line 46, in __init__
    ensure_valid_ip(host, port)
  File "C:\Users\perch\AppData\Local\Programs\Python\Python310\lib\site-packages\mcstatus\server.py", line 32, in ensure_valid_ip
    raise ValueError(f"Invalid host address, {host!r} (doesn't match the required pattern)")
ValueError: Invalid host address, 'not_valid_ip' (doesn't match the required pattern)

[kevinkjt2000]

Underscores are not valid characters for hostnames... but they are for DNS names! That was my bad to conflate those two. Also my bad for forgetting which version this was in.

After doing some research, I'm convinced that maintaining this validity check is going to be awful. Let's go back to not checking it at all and relying on exceptions. We'd have to check if it's an IPv4 address, an IPv6 address, a valid hostname, a valid DNS name, or whatever else the connection software stack is capable of. I'll remove the check for now until we find a better way of doing it.

Research:
https://stackoverflow.com/questions/2532053/validate-a-hostname-string
https://stackoverflow.com/questions/2180465/can-domain-name-subdomains-have-an-underscore-in-it

[kevinkjt2000]

Another dev version: v9.0.0-dev3