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

Apps can hook in their own locator. #15

Merged
merged 1 commit into from
Aug 21, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 10 additions & 19 deletions lib/global_id/global_id.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,15 @@ def parse(gid, options = {})
parse_encoded_gid(gid, options)
end

def app=(value)
validate_hostname(value)
@app = value
def app=(app)
@app = validate_app(app)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setters return the passed in value. So this can be one line.

end

def validate_app(app)
URI.parse('gid:///').hostname = app
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeremy @sebasoga This whole method seems like it would belong well in the URI::GlobalID class #18.

Copy link
Member

Choose a reason for hiding this comment

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

Yes! Agree - depends what is ready to merge first 😁

rescue URI::InvalidComponentError
raise ArgumentError, 'Invalid app name. ' \
'App names must be valid URI hostnames: alphanumeric and hyphen characters only.'
end

Copy link
Member

Choose a reason for hiding this comment

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

^ Merge issue, method is down below private now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💩 Sorry.

private
Expand All @@ -39,17 +45,6 @@ def repad_gid(gid)
padding_chars = gid.length.modulo(4).zero? ? 0 : (4 - gid.length.modulo(4))
gid + ('=' * padding_chars)
end

def validate_hostname(hostname)
begin
URI.parse('gid:///').hostname = hostname
rescue URI::InvalidComponentError
raise ArgumentError,
'Invalid app name. App names must be valid URI hostnames: ' \
'alphanumeric and hyphen characters only.'
end
end

end

attr_reader :uri, :app, :model_name, :model_id
Expand All @@ -59,7 +54,7 @@ def initialize(gid, options = {})
end

def find(options = {})
model_class.find model_id if find_allowed?(options[:only])
Locator.locate self, options
Copy link
Member

Choose a reason for hiding this comment

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

👍

end

def model_class
Expand Down Expand Up @@ -95,8 +90,4 @@ def extract_uri_components(gid)
raise URI::InvalidURIError, "Expected a URI like gid://app/Person/1234: #{@uri.inspect}"
end
end

def find_allowed?(only = nil)
only ? Array(only).any? { |c| model_class <= c } : true
end
end
66 changes: 65 additions & 1 deletion lib/global_id/locator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ class << self
# instances of returned classes to those including that module. If no classes or
# modules match, +nil+ is returned.
def locate(gid, options = {})
GlobalID.find gid, options
if gid = GlobalID.parse(gid)
locator_for(gid).locate gid if find_allowed?(gid.model_class, options[:only])
end
Copy link
Member

Choose a reason for hiding this comment

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

👍

end

# Takes either a SignedGlobalID or a string that can be turned into a SignedGlobalID
Expand All @@ -24,6 +26,68 @@ def locate(gid, options = {})
def locate_signed(sgid, options = {})
SignedGlobalID.find sgid, options
end

# Tie a locator to an app.
# Useful when different apps collaborate and reference each others' Global IDs.
#
# The locator can be either a block or a class.
#
# Using a block:
#
# GlobalID::Locator.use :foo do |gid|
# FooRemote.const_get(gid.model_name).find(gid.model_id)
# end
#
# Using a class:
#
# GlobalID::Locator.use :bar, BarLocator.new
#
# class BarLocator
# def locate(gid)
# @search_client.search name: gid.model_name, id: gid.model_id
# end
# end
def use(app, locator = nil, &locator_block)
raise ArgumentError, 'No locator provided. Pass a block or an object that responds to #locate.' unless locator || block_given?
Copy link
Member

Choose a reason for hiding this comment

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

We'll also need to validate that the app format is an allowed URI hostname, so no underscores (see #23).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Writing one now.

Den 20/08/2014 kl. 15.46 skrev Jeremy Kemper notifications@github.com:

In lib/global_id/locator.rb:

  •  #
    
  •  #   GlobalID::Locator.use :foo do |gid|
    
  •  #     FooRemote.const_get(gid.model_name).find(gid.model_id)
    
  •  #   end
    
  •  #
    
  •  # Using a class:
    
  •  #
    
  •  #   GlobalID::Locator.use :bar, BarLocator.new
    
  •  #
    
  •  #   class BarLocator
    
  •  #     def locate(gid)
    
  •  #       @search_client.search name: gid.model_name, id: gid.model_id
    
  •  #     end
    
  •  #   end
    
  •  def use(app, locator = nil, &locator_block)
    
  •    raise ArgumentError, 'No locator provided. Pass a block or an object that responds to #locate.' unless locator || block_given?
    
    We'll also need to validate that the app format is an allowed URI hostname, so no underscores (see App name cannot contain an underscore #23).


Reply to this email directly or view it on GitHub.

Kasper


GlobalID.validate_app(app)

@locators[normalize_app(app)] = locator || BlockLocator.new(locator_block)
end
Copy link
Member

Choose a reason for hiding this comment

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

The locators are for a specific app, so gid://foo/Model/id would be located using the block above and gid://bar/Other/id would be located using BarLocator.

def use(app, locator = nil, &locator_block)
  raise ArgumentError, 'No locator provided. Pass a block or an object that responds to #locate.' unless locator || locator_block
  @locators[app.to_s.downcase] = locator || BlockLocator.new(&locator_block)
end


private
def locator_for(gid)
Copy link
Member

Choose a reason for hiding this comment

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

Note that this doesn't pick up the private method visibility above - that applies to instance methods that are defined.

Can stick within the existing class << self block and do a private within it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had totally forgotten that's how it works 👍

@locators.fetch(normalize_app(gid.app)) { default_locator }
end

def find_allowed?(model_class, only = nil)
only ? Array(only).any? { |c| model_class <= c } : true
end

def normalize_app(app)
app.to_s.downcase
end
end

private
@locators = {}

class ActiveRecordFinder
def locate(gid)
gid.model_class.find gid.model_id
end
end

mattr_reader(:default_locator) { ActiveRecordFinder.new }

class BlockLocator
def initialize(block)
@locator = block
end

def locate(gid)
@locator.call(gid)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

👍

end
end
46 changes: 46 additions & 0 deletions test/cases/global_locator_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -135,4 +135,50 @@ class GlobalLocatorTest < ActiveSupport::TestCase
assert_nil GlobalID::Locator.locate 'gid://app/Person'
assert_nil GlobalID::Locator.locate 'gid://app/Person/1/2'
end

test 'use locator with block' do
GlobalID::Locator.use :foo do |gid|
:foo
end

with_app 'foo' do
assert_equal :foo, GlobalID::Locator.locate('gid://foo/Person/1')
end
end

test 'use locator with class' do
class BarLocator
def locate(gid); :bar; end
end

GlobalID::Locator.use :bar, BarLocator.new

with_app 'bar' do
assert_equal :bar, GlobalID::Locator.locate('gid://bar/Person/1')
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Will want to test that the proper locator is used for the gid, including case-insensitivity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've got a whole case of insensitivity for you 😉


test 'app locator is case insensitive' do
Copy link
Member

Choose a reason for hiding this comment

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

if gid is not going to be used use _ instead or remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought of it as an extra form of documentation. If people peruse the tests they will know what the argument passed in is.

You still want me to change it?

Copy link
Member

Choose a reason for hiding this comment

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

Let see with @jeremy

Copy link
Member

Choose a reason for hiding this comment

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

Prefer naming the argument that's yielded, too. Omitting the arg is an option. But this is nice and clear for anyone reading the tests to understand how the software behaves.

GlobalID::Locator.use :insensitive do |gid|
:insensitive
end

with_app 'insensitive' do
assert_equal :insensitive, GlobalID::Locator.locate('gid://InSeNsItIvE/Person/1')
end
end
Copy link
Member

Choose a reason for hiding this comment

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

When #25 adds stricter app validation, this use call will probably raise an ArgumentError.

Let's flip the test around: use :insensitive then locate a gid://InSeNsItIvE to address that.


test 'locator name cannot have underscore' do
assert_raises ArgumentError do
GlobalID::Locator.use('under_score') { |gid| 'will never be found' }
end
end
Copy link
Member

Choose a reason for hiding this comment

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

👍


private
def with_app(app)
old_app, GlobalID.app = GlobalID.app, app
yield
ensure
GlobalID.app = old_app
end
end
Copy link
Member

Choose a reason for hiding this comment

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

new line at the end of the file :trollface: