Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] Fix domain detection #281

Closed
wants to merge 1 commit into from

Conversation

Bixilon
Copy link
Contributor

@Bixilon Bixilon commented Mar 21, 2023

Should fix #273, not tested

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this change with a few examples of why the current way is not working and the new way is better, please?

@Bixilon
Copy link
Contributor Author

Bixilon commented Mar 21, 2023

I have no clue what the library is doing, but it should cut off the first part of a domain (test.abc.de -> abc.de). The library just does it with the top level domain (test.abc.de -> de, but not even then correct, check a domain with a dash test.ab-cd.de, it then just returns nothing).

It is used in a header (nginx), and if that header is not set correctly, things like chart loading fail.

@Bixilon
Copy link
Contributor Author

Bixilon commented Mar 24, 2023

Tested it, working fine!

Screenshot_20230324_162010

@Bixilon
Copy link
Contributor Author

Bixilon commented Oct 13, 2023

Any chance of getting this merged?

@nemesifier
Copy link
Member

Testing with tldextract-5.1.2:

In [9]: print(tldextract.extract('openwisp.org').registered_domain)
openwisp.org

In [10]: print(tldextract.extract('api.openwisp.org').registered_domain)
openwisp.org

In [11]: print(tldextract.extract('test.api.openwisp.org').registered_domain)
openwisp.org

In [12]: print(tldextract.extract('test.abc.de').registered_domain)
abc.de

In [13]: print(tldextract.extract('abc.de').registered_domain)
abc.de

In [14]: print(tldextract.extract('test.test.abc.de').registered_domain)
abc.de

I cannot replicate the issue you are mentioning here. I will try to update this library and modify the different parts of the code to make sure they're consistent.

@Bixilon
Copy link
Contributor Author

Bixilon commented May 22, 2024 via email

@nemesifier
Copy link
Member

nemesifier commented May 22, 2024

try extracting from api.open-wisp.de

As far as I know, dashes are not allowed in the domain.
Yes they are allowed.

try extracting from api.open-wisp.de

In [1]: import tldextract

In [2]: ext=tldextract.extract('api.open-wisp.org')

In [3]: ext
Out[3]: ExtractResult(subdomain='api', domain='open-wisp', suffix='org', is_private=False)

In [4]: ext.registered_domain
Out[4]: 'open-wisp.org'

What's the issue? Please update your local copy of the library.
PR in #300.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] device charts are not loading
2 participants