Separate DNS logic into their own staticmethods

[py-mine-bot]

Authored by ItsDrike
Feb 1, 2022

---

Addresses #200

As I described in Dinnerbone/mcstatus#200 (comment), splitting out the DNS lookup functionality into it's own standalone staticmethod that can be called independently without creating a full server instance which can be beneficial if someone would just want to utilize the DNS lookup logic without also trigerring the initialization checks for IP validation and needlessly making an entire sever instance when a simple DNS lookup with this, now separated, logic would suffice.

[py-mine-bot]

Authored by kevinkjt2000
Feb 12, 2022

---

I'm assuming this is the relevant comment from #200:

But the only reason I use mcstatus for DNS info

Hmmm, I am going to draw the line on mcstatus diving too far down being a DNS resolver library. Someone should be using a library/tool that is specific to DNS if that is their main mission. To draw an example from something tangentially related, if I wanted to get the status of something such as Discord, I'd use a tool that understood HTTP to download https://discordstatus.com/, and I wouldn't use that same tool to check the DNS records of discordstatus.com. Likewise, for mcstatus, it is meant to check the status of Minecraft servers and not their DNS records. Sure, DNS is necessary for both of these tools, but it is not their goal.

These dns_* functions do clean up the code and make it easier to follow. I'm open to having these as private functions so that our burden to maintain them as part of the public API is not a requirement.

[py-mine-bot]

Authored by ItsDrike
Feb 12, 2022

---

That's a fair point, yeah I suppose it really isn't our task to do something like that,

I'm open to having these as private functions so that our burden to maintain them as part of the public API is not a requirement.

Python doesn't really have "private functions" per se, best we can do is prefix these methods with _ which is a convention for "this is only used internally and we don't expect you to mess with it, use/alter at your own risk". I'll rename these methods accordingly.

[py-mine-bot]

Authored by PerchunPak
Feb 13, 2022

---

You understood me wrong. Full sentence:

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.

I also use mcstatus to ping and get information from servers. But at some point I needed to give the user a port from the SRV record, so I started to implement a DNS lookup system.
After a while, I noticed that mcstatus was already doing all of this, so I just replaced my entire DNS system with mcstatus.

[py-mine-bot]

Authored by ItsDrike
Feb 13, 2022

---

You understood me wrong. Full sentence: ...

@PerchunPak
I don't believe the point was necessarily about how you're using mcstatus, it was more about just saying that we shouldn't do things outside of the scope of just being a library to get status of minecraft servers. While this process may include DNS lookups, it doesn't mean we should be adding and exposing functions to make these lower level lookups available and easily usable for the library users, it simply isn't what this library is for and if someone does want to use it like that, that's on them and we're not responsible for maintaining backwards compatibility on these nor for some exceptions you may encounter while using them.

[py-mine-bot]

Authored by kevinkjt2000
Feb 13, 2022

---

so I just replaced my entire DNS system with mcstatus.

Seems as if understood perfectly.

it simply isn't what this library is for and if someone does want to use it like that, that's on them and we're not responsible for maintaining backwards compatibility on these nor for some exceptions you may encounter while using them.

Well said! Of course, we could visit properly supporting this as a feature. While working on brand new documentation, I'm also brainstorming what mcstatus' purpose is. This should help design the public API. Perhaps SRV lookups are something that's expected as part of the "status" of a Java Minecraft server. Asserting the existence of that record could be viewed as a possible health check. Probably best to discuss this further in the Discord server until something is concrete enough to make a GitHub issue, so this PR can stay focused on the refactoring that it's doing.

[py-mine-bot]

Authored by ItsDrike
Feb 11, 2022

---

To further address #200, it may help to also separate the lookup logic into it's own staticmethod (such as resolve_ip) and just call that from the actual alternative lookup constructor. I'm proposing this because even with separate DNS fucnctions, the author would still have to mostly rewrite our entire lookup logic to get the valid IP just to avoid making the final instance. That's not great, especially because they would have to constantly monitor our development, in case we made some changes to lookup, so that they could copy it, which is obviously ridiculous. This further separation would allow for something as easy as: lookup_ip = MinecraftServer.resolve_ip(my_ip).

Should I address this here too or make another PR for that later?

NOTE: Even though the issue was closed already with a different fix, that just addressed another part of that issue, the point I also brought up in there about the fact that we shouldn't need to make an instance to perform these static functions still stays and it's still relevant.

[ItsDrike]

Adding do-not-merge tag because this PR conflicts with #227, before we decide which of these approaches to take, or think of a new approach entirely, neither of them should get merged.

However the most likely scenario will probably be that this PR gets closed and 227 will be merged once it's ready, since it introduces a much cleaner way of doing things, rendering this mostly irrelevant.

Explaining the conflict

The issue is that the Address PR mostly provides the abstractions we wanted in the server class, the actual DNS separation doesn't make that much sense to implement after it would get merged, it made sense before since we implemented these shared methods and used them from both bedrock and java servers, but when all that logic lives in address class, there's no code repetition there which this would fix.

I mean, it does make the code in Address class a little bit cleaner, but it only does that by moving the less clean code elsewhere. But all of these methods are only used once and we since didn't want to expose them over to the public API users wouldn't use them either, so I'm just not sure it makes sense now. Though I don't mind it getting added either, it does group the DNS logic into a single place, which is nice, but that's the only real benefit.

I've created a diff with a quick re-implementation of these DNS handling methods with the address class in place to demonstrate just how little code repetition would these methods remove, for a lot of newly introduced code.

Attached diff

diff --git a/mcstatus/address.py b/mcstatus/address.py
index 5f32db1..69b89bc 100644
--- a/mcstatus/address.py
+++ b/mcstatus/address.py
@@ -9,6 +9,8 @@ import dns.asyncresolver
 import dns.resolver
 from dns.rdatatype import RdataType
 
+import mcstatus.dns
+
 if TYPE_CHECKING:
     from typing_extensions import Self
 
@@ -123,11 +125,7 @@ class Address(_AddressBase):
             # ValueError is raised if the given address wasn't valid
             # this means it's a hostname and we should try to resolve
             # the A record
-            answers = dns.resolver.resolve(self.host, RdataType.A, lifetime=lifetime)
-            # There should only be one answer here, though in case the server
-            # does actually point to multiple IPs, we just pick the first one
-            answer = answers[0]
-            ip_addr = str(answer).rstrip(".")
+            ip_addr = mcstatus.dns.resolve_a_record(self.host, lifetime=lifetime)
             return ipaddress.ip_address(ip_addr)
 
     async def async_resolve_ip(self, lifetime: Optional[float] = None) -> Union[ipaddress.IPv4Address, ipaddress.IPv6Address]:
@@ -142,11 +140,7 @@ class Address(_AddressBase):
             # ValueError is raised if the given address wasn't valid
             # this means it's a hostname and we should try to resolve
             # the A record
-            answers = await dns.asyncresolver.resolve(self.host, RdataType.A, lifetime=lifetime)
-            # There should only be one answer here, though in case the server
-            # does actually point to multiple IPs, we just pick the first one
-            answer = answers[0]
-            ip_addr = str(answer).rstrip(".")
+            ip_addr = await mcstatus.dns.async_resolve_a_record(self.host, lifetime=lifetime)
             return ipaddress.ip_address(ip_addr)
 
 
@@ -183,7 +177,7 @@ def minecraft_srv_address_lookup(
     # port which we should use. If there's no such record, fall back
     # to the default_port (if it's defined).
     try:
-        answers = dns.resolver.resolve("_minecraft._tcp." + host, RdataType.SRV, lifetime=lifetime)
+        host, port = mcstatus.dns.resolve_mc_srv(host, lifetime=lifetime)
     except dns.resolver.NXDOMAIN:
         if default_port is None:
             raise ValueError(
@@ -191,11 +185,6 @@ def minecraft_srv_address_lookup(
                 " and default_port wasn't specified, can't parse."
             )
         port = default_port
-    else:
-        # The record was found, use it instead
-        answer = answers[0]
-        host = str(answer.target).rstrip(".")
-        port = int(answer.port)
 
     return Address(host, port)
 
@@ -220,7 +209,7 @@ async def async_minecraft_srv_address_lookup(
     # port which we should use. If there's no such record, fall back
     # to the default_port (if it's defined).
     try:
-        answers = await dns.asyncresolver.resolve("_minecraft._tcp." + host, RdataType.SRV, lifetime=lifetime)
+        host, port = await mcstatus.dns.async_resolve_mc_srv(host, lifetime=lifetime)
     except dns.resolver.NXDOMAIN:
         if default_port is None:
             raise ValueError(
@@ -228,10 +217,5 @@ async def async_minecraft_srv_address_lookup(
                 " and default_port wasn't specified, can't parse."
             )
         port = default_port
-    else:
-        # The record was found, use it instead
-        answer = answers[0]
-        host = str(answer.target).rstrip(".")
-        port = int(answer.port)
 
     return Address(host, port)
diff --git a/mcstatus/dns.py b/mcstatus/dns.py
new file mode 100644
index 0000000..554505e
--- /dev/null
+++ b/mcstatus/dns.py
@@ -0,0 +1,89 @@
+import dns.resolver
+import dns.asyncresolver
+from dns.rdatatype import RdataType
+from typing import Tuple, Optional
+
+
+def resolve_a_record(hostname: str, lifetime: Optional[float] = None) -> str:
+    """Perform a DNS resolution for an A record to given hostname
+
+    :param str hostname: The address to resolve for.
+    :return: The resolved IP address from the A record
+    :raises dns.exception.DNSException:
+        One of the exceptions possibly raised by dns.resolver.resolve
+        Most notably this will be `dns.exception.Timeout` and `dns.resolver.NXDOMAIN`
+    """
+    answers = dns.resolver.resolve(hostname, RdataType.A, lifetime=lifetime)
+    # There should only be one answer here, though in case the server
+    # does actually point to multiple IPs, we just pick the first one
+    answer = answers[0]
+    hostname = str(answer).rstrip(".")
+    return hostname
+
+
+async def async_resolve_a_record(hostname: str, lifetime: Optional[float] = None) -> str:
+    """Asynchronous alternative to resolve_a_record.
+
+    For more details, check the docstring of resolve_a_record function.
+    """
+    answers = await dns.asyncresolver.resolve(hostname, RdataType.A, lifetime=lifetime)
+    # There should only be one answer here, though in case the server
+    # does actually point to multiple IPs, we just pick the first one
+    answer = answers[0]
+    hostname = str(answer).rstrip(".")
+    return hostname
+
+
+def resolve_srv_record(query_name: str, lifetime: Optional[float] = None) -> Tuple[str, int]:
+    """Perform a DNS resolution for SRV record pointing to the Java Server.
+
+    :param str address: The address to resolve for.
+    :return: A tuple of host string and port number
+    :raises dns.exception.DNSException:
+        One of the exceptions possibly raised by dns.resolver.resolve
+        Most notably this will be `dns.exception.Timeout` and `dns.resolver.NXDOMAIN`
+    """
+    answers = dns.resolver.resolve(query_name, RdataType.SRV, lifetime=lifetime)
+    # There should only be one answer here, though in case the server
+    # does actually point to multiple IPs, we just pick the first one
+    answer = answers[0]
+    host = str(answer.target).rstrip(".")
+    port = int(answer.port)
+    return host, port
+
+
+async def async_resolve_srv_record(query_name: str, lifetime: Optional[float] = None) -> Tuple[str, int]:
+    """Asynchronous alternative to resolve_srv_record.
+
+    For more details, check the docstring of resolve_srv_record function.
+    """
+    answers = await dns.asyncresolver.resolve(query_name, RdataType.SRV, lifetime=lifetime)
+    # There should only be one answer here, though in case the server
+    # does actually point to multiple IPs, we just pick the first one
+    answer = answers[0]
+    host = str(answer.target).rstrip(".")
+    port = int(answer.port)
+    return host, port
+
+
+def resolve_mc_srv(hostname: str, lifetime: Optional[float] = None) -> Tuple[str, int]:
+    """Resolve SRV record for a minecraft server on given hostname.
+
+    :param str address: The address, without port, on which an SRV record is present.
+    :return: Obtained target and port from the SRV record, on which the server should live on.
+    :raises dns.exception.DNSException:
+        One of the exceptions possibly raised by dns.resolver.resolve
+        Most notably this will be `dns.exception.Timeout` and `dns.resolver.NXDOMAIN`
+
+    Returns obtained target and port from the SRV record, on which
+    the minecraft server should live on.
+    """
+    return resolve_srv_record("_minecraft._tcp." + hostname, lifetime=lifetime)
+
+
+async def async_resolve_mc_srv(hostname: str, lifetime: Optional[float] = None) -> Tuple[str, int]:
+    """Asynchronous alternative to resolve_mc_srv.
+
+    For more details, check the docstring of resolve_mc_srv function.
+    """
+    return await async_resolve_srv_record("_minecraft._tcp." + hostname, lifetime=lifetime)

[ItsDrike]

I've applied the diff above which means this PR is now ready to be merged.

However the issue that I've pointed out above still remains, and even though the PR isn't conflicting anymore, we should still decide on whether it's even worth implementing something like this, and if not, this PR should simply be closed.