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.

Sign up for GitHub

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

Jump to bottom

Add public_suffix method to allow users to access the PublicSuffix::Domain object easily. #510

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add public_suffix method to allow users to access the PublicSuffix::Domain object easily. #510

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51c93e7
Add public_suffix method to allow users to access the
excid3 May 10, 2023
f37486a
Add subdomain and subdomains methods
excid3 May 10, 2023
965ebc0
Merge pull request #1 from excid3/subdomains
excid3 May 11, 2023
dbeafac
Make public_suffix private
excid3 Jun 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 29 additions & 2 deletions lib/addressable/uri.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1198,7 +1198,7 @@ def hostname=(new_hostname)
# @example
# Addressable::URI.parse("http://www.example.co.uk").tld # => "co.uk"
def tld
PublicSuffix.parse(self.host, ignore_private: true).tld
public_suffix.tld
end

##
Expand All @@ -1216,7 +1216,34 @@ def tld=(new_tld)
# @example
# Addressable::URI.parse("http://www.example.co.uk").domain # => "example.co.uk"
def domain
PublicSuffix.domain(self.host, ignore_private: true)
public_suffix.domain
end

##
# Returns the first subdomain for this host.
#
# @example
# Addressable::URI.parse("http://www.test.example.co.uk").domain # => "www"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Addressable::URI.parse("http://www.test.example.co.uk").domain # => "www"
# Addressable::URI.parse("http://www.test.example.co.uk").subdomain # => "www"

def subdomain
subdomains.first
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still think there's ambiguity around what's a subdomain and what a method like this is expected to return.

www is a subdomain of test.example.co.uk
test is a subdomain of example.co.uk
example is a subdomain of co.uk

This method would return nil when parsing http://example.co.uk

Copy link
Author

Choose a reason for hiding this comment

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

I was modeling this off of Rails which returns the first subdomain, but let me know what you'd like this to do. 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not familiar with that in Rails, can you link the source for method? When is it typically used?

I'm not sure what we want here, if the method should exist?

Copy link
Author

Choose a reason for hiding this comment

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

Multi-tenant applications or anything that uses subdomains (think Shopify) use this. I've always been surprised there's no way to get the subdomain(s) from URIs in Ruby. Rails will do it for the current request, but that's it and it's not Public Suffix aware. You can only specify a single TLD for your entire Rails app, so you need to reach for something else like addressable + public_suffix.

https://api.rubyonrails.org/classes/ActionDispatch/Http/URL.html#method-i-subdomain

Copy link
Author

Choose a reason for hiding this comment

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

Any thoughts on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah... if we should add this method, is it not better if it matches the subdomain on ActionDispatch? From the URL above

Returns all the subdomains as a string, so "dev.www" would be returned for “dev.www.rubyonrails.org”.

For http://www.test.example.co.uk that would be www.test, not just www?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about not adding the #subdomain method? Instead only #subdomains. It is easy for the user to do subdomains.first or subdomains.first(2) etc. depending on what they want.

end

##
# Returns the subdomains for this host.
#
# @example
# Addressable::URI.parse("http://www.test.example.co.uk").domain # => ["www", "test"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Addressable::URI.parse("http://www.test.example.co.uk").domain # => ["www", "test"]
# Addressable::URI.parse("http://www.test.example.co.uk").subdomains # => ["www", "test"]

def subdomains
public_suffix.trd.to_s.split(".")
end

##
# Returns a PublicSuffix::Domain object for this host.
#
# @example
# Addressable::URI.parse("http://www.example.org").public_suffix #=> #<PublicSuffix::Domain @sld="example", @tld="org", @trd="www">
def public_suffix(**kwargs)
excid3 marked this conversation as resolved.
Show resolved Hide resolved
PublicSuffix.parse(self.host, **{ignore_private: true}.merge(kwargs))
end

##
Expand Down
26 changes: 26 additions & 0 deletions spec/addressable/uri_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6799,3 +6799,29 @@ def to_str
expect(res).to be nil
end
end

describe Addressable::URI, "public suffix" do
it "should return a PublicSuffix::Domain object" do
expect(Addressable::URI.parse("http://example.org").public_suffix).to be_an(PublicSuffix::Domain)
end
end

describe Addressable::URI, "subdomain" do
it "should return the first subdomain" do
expect(Addressable::URI.parse("http://www.test.example.org").subdomain).to eq("www")
end

it "should return nil if no subdomain" do
expect(Addressable::URI.parse("http://example.co.uk").subdomain).to be_nil
end
end

describe Addressable::URI, "subdomains" do
it "should return the subdomains" do
expect(Addressable::URI.parse("http://www.test.example.org").subdomains).to eq(["www", "test"])
end

it "should return an empty array if no subdomain" do
expect(Addressable::URI.parse("http://example.co.uk").subdomains).to eq([])
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please keep the terminating newline

Screenshot 2023-06-03 at 22 18 31