Whois component maintenance, PR 1 of 3: Light refactoring, adding abstraction #117749

mkrasowski · 2024-05-19T21:18:37Z

Add a thin abstraction layer to make it easy to replace the external whois-query library

Proposed change

This is Pull Request 1 of 3 that I have lined up as part of my contributions to "whois" component upkeep.
Order of changes:

(we are here) Light refactoring, adding an abstraction layer separating the use of an external/upstream whois library and home assistant component internals
Dependency update: Replacement of unmaintained whois upstream library with a maintained one
Breaking change: Removal of "Reseller" entity from the integration

Originally it was one larger PR, but I decided to split it up following the perfect PR recommendations.
Ideally, I'd like to get these 3 PRs merged in a relatively quick succession.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

Add a thin abstraction layer to make it easy to replace the external whois-query library

home-assistant · 2024-05-19T21:18:43Z

Hey there @frenck, mind taking a look at this pull request as it has been labeled with an integration (whois) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of whois can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign whois Removes the current integration label and assignees on the pull request, add the integration domain after the command.
@home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
@home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

frenck

Dependency update: Replacement of unmaintained whois upstream library with a maintained one

Which one are you planning to replace it with? It seems like the package maintenance simply moved into another repo/package?

There is mixed content in this PR IMHO. One is the refactoring of adding abstraction and some test refactoring as well as it seems.

I'm not in favor of the added abstraction. The attracting is what we use the Python packages for, there is no need to add another abstraction to this all (it will only add complexity), nor is it a goal to make packages easily swappable in Home Assistant.

../Frenck

frenck · 2024-05-20T07:35:52Z

homeassistant/components/whois/helper.py

@@ -0,0 +1,64 @@
+"""A helper class that abstracts away the usage of external whois libraries."""


This is an uncommon level of abstraction for Home Assistant. This is basically why we use libraries.

frenck · 2024-05-20T07:36:00Z

homeassistant/components/whois/helper.py

+
+
+@dataclass(kw_only=True)
+class Domain:


This should be typing from upstream

Please take a look at the final code after all 3 planned PRs listed in the description. I believe it will help you understand why I decided to do it with a helper, instead of putting all the processing logic directly in HA classes.

https://github.com/mkrasowski/homeassistant-core/blob/replace-unmaintained-whois-library/homeassistant/components/whois/helper.py

frenck · 2024-05-20T07:36:09Z

homeassistant/components/whois/helper.py

+class WhoisUnknownTLD(Exception):
+    """Exception class when unknown TLD encountered."""
+
+
+class GenericWhoisError(Exception):
+    """Exception class for all other exceptions originating from the external whois library."""


These should come from upstream

home-assistant · 2024-05-20T07:44:23Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

mkrasowski · 2024-05-20T13:47:02Z

Which one are you planning to replace it with? It seems like the package maintenance simply moved into another repo/package?

The replacement recommended by the maintainer of the current library is whoisdomain, but it currently has a reported memory leak issue, which IMO disqualifies it from being used in a long-running system like Home Assistant.
I also checked if it solves the issues I noticed with the current library, and it doesn't - for example getting expiration dates for .pl domains.

Ultimately, I quickly evaluated two replacement candidates: whoisdomain and python-whois and decided to go with python-whois.
My reasoning:

GitHub popularity
- Original whois: 287 stars / 136 forks
- python-whois: 335 stars / 180 forks
- whoisdomain: 36 stars / 7 forks
Active development
- python-whois: 5 contributing authors in the last month
  - disclosure: I am one of them, since I picked this library some time ago and wanted to contribute
- whoisdomain: 0 contributions in the last month
Codebase size (retrieved via GitHub API):
- python-whois: 488
- whoisdomain: 5943
Misc factors
- reported memory leak in whoisdomain

If you want to see the full scope of my changes, here is the complete branch that includes all changes I currently split into 3 parts listed in the PR description.

https://github.com/mkrasowski/homeassistant-core/tree/replace-unmaintained-whois-library/homeassistant/components/whois

There is mixed content in this PR IMHO. One is the refactoring of adding abstraction and some test refactoring as well as it seems.

IMHO, it still should be considered as one logical change - modifications to tests were needed as part of modifying code they were testing.

I'm not in favor of the added abstraction. The attracting is what we use the Python packages for, there is no need to add another abstraction to this all (it will only add complexity), nor is it a goal to make packages easily swappable in Home Assistant.

It seemed unnecessary at first to me as well, because the current library has a decent and reliable interface.
Once I poked around the other library python-whois, it doesn't have a reliable interface, so attributes have to be processed before being used in HA sensors. Adding an abstraction in the form of a helper class seemed like an appropriate solution that's not to difficult to understand for potential future contributors.

frenck · 2024-05-20T16:00:29Z

Ultimately, I quickly evaluated two replacement candidates: whoisdomain and python-whois and decided to go with python-whois.

This integration migrated away from that library because of many problems. Seems odd to revert that.

IMHO, it still should be considered as one logical change - modifications to tests were needed as part of modifying code they were testing.

The point here is, they could be separate, and thus should be in a separate PR to make PR as small as possible.

Once I poked around the other library python-whois, it doesn't have a reliable interface,

Which is why it was removed.

Adding an abstraction in the form of a helper class seemed like an appropriate solution that's not to difficult to understand for potential future contributors.

Sorry, as per review comment, we are not in agreement.

mkrasowski · 2024-05-20T16:25:19Z

IMHO, it still should be considered as one logical change - modifications to tests were needed as part of modifying code they were testing.

The point here is, they could be separate, and thus should be in a separate PR to make PR as small as possible.

Okay, I'm definitely open to that, but first, we have to agree on the approach to the unmaintained and broken whois library first

Once I poked around the other library python-whois, it doesn't have a reliable interface,

Which is why it was removed.

Adding an abstraction in the form of a helper class seemed like an appropriate solution that's not to difficult to understand for potential future contributors.

Sorry, as per review comment, we are not in agreement.

Okay, I would appreciate some constructive feedback then.

HA uses a broken whois library and the recommended replacement is broken as well (talking about .pl domains + some other problems), in addition to having memory leaks.

My proposed replacement library is far from perfect, but it seems to be working better for the TLDs I tested (including com, net, org, io, me, us, pl...).

The choice is between two libraries that have their issues. python-whois issues seem to be easily addressable by adding a wrapper/thin abstraction. whoisdomain issues don't seem to be so easily addressable, especially the disqualifying memory problem.

What do you suggest we do?

frenck · 2024-05-20T16:59:34Z

we have to agree on the approach to the unmaintained and broken whois library first

I'm not sure if it is broken, this PR doesn't reference anything that indicates that?
Unmaintained, sure, there are options. I do not agree on the suggestion option though, especially since this integration came from there.

Okay, I would appreciate some constructive feedback then.

Please remove the additional layer of abstraction... 🤷
It doesn't belong here, and isn't a common thing in Home Assistant as well. Libraries provide this abstraction.

mkrasowski · 2024-05-20T17:34:53Z

we have to agree on the approach to the unmaintained and broken whois library first

I'm not sure if it is broken, this PR doesn't reference anything that indicates that? Unmaintained, sure, there are options. I do not agree on the suggestion option though, especially since this integration came from there.

I listed some issues in #117805.

Okay, I would appreciate some constructive feedback then.

Please remove the additional layer of abstraction... 🤷 It doesn't belong here, and isn't a common thing in Home Assistant as well. Libraries provide this abstraction.

This particular Pull Request is essentially for adding this layer of abstraction. If it's not acceptable for Home Assistant, this PR should be wholly rejected and closed.

Side note: rejecting this PR would render the rest of my work on the Whois integration (https://github.com/mkrasowski/homeassistant-core/tree/replace-unmaintained-whois-library) inapplicable, because it's built on top of this abstraction.

Refactor whois component: add an abstraction layer

4b1f50a

Add a thin abstraction layer to make it easy to replace the external whois-query library

mkrasowski requested a review from frenck as a code owner May 19, 2024 21:18

home-assistant bot added cla-signed code-quality has-tests integration: whois labels May 19, 2024

home-assistant bot assigned frenck May 19, 2024

home-assistant bot added the Quality Scale: No score label May 19, 2024

frenck requested changes May 20, 2024

View reviewed changes

home-assistant bot marked this pull request as draft May 20, 2024 07:44

mkrasowski mentioned this pull request May 20, 2024

Whois integration issues #117805

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Whois component maintenance, PR 1 of 3: Light refactoring, adding abstraction #117749

Whois component maintenance, PR 1 of 3: Light refactoring, adding abstraction #117749

mkrasowski commented May 19, 2024

home-assistant bot commented May 19, 2024

frenck left a comment

frenck May 20, 2024

frenck May 20, 2024

mkrasowski May 20, 2024

frenck May 20, 2024

home-assistant bot commented May 20, 2024

mkrasowski commented May 20, 2024

frenck commented May 20, 2024

mkrasowski commented May 20, 2024 •

edited

frenck commented May 20, 2024 •

edited

mkrasowski commented May 20, 2024

		@@ -0,0 +1,64 @@
		"""A helper class that abstracts away the usage of external whois libraries."""

Whois component maintenance, PR 1 of 3: Light refactoring, adding abstraction #117749

Are you sure you want to change the base?

Whois component maintenance, PR 1 of 3: Light refactoring, adding abstraction #117749

Conversation

mkrasowski commented May 19, 2024

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented May 19, 2024

frenck left a comment

Choose a reason for hiding this comment

frenck May 20, 2024

Choose a reason for hiding this comment

frenck May 20, 2024

Choose a reason for hiding this comment

mkrasowski May 20, 2024

Choose a reason for hiding this comment

frenck May 20, 2024

Choose a reason for hiding this comment

home-assistant bot commented May 20, 2024

mkrasowski commented May 20, 2024

frenck commented May 20, 2024

mkrasowski commented May 20, 2024 • edited

frenck commented May 20, 2024 • edited

mkrasowski commented May 20, 2024

mkrasowski commented May 20, 2024 •

edited

frenck commented May 20, 2024 •

edited