Apps can hook in their own locator. #15

kaspth · 2014-08-18T09:31:03Z

Adds #7.

jeremy · 2014-08-18T14:04:26Z

lib/global_id/locator.rb

+      #   end
+      def use(locator_name, locator = nil)
+        @locator = locator || Base.new(Proc.new)
+      end


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

jeremy · 2014-08-18T14:05:55Z

Thanks @kaspth! Commented on the link between app and their locators - idea is to be able to bind a locator to an app.

kaspth · 2014-08-18T17:54:25Z

@jeremy I think I got it now.

seuros · 2014-08-18T17:58:17Z

test/cases/global_locator_test.rb

+      assert_equal options[:returns], GlobalID::Locator.locate(gid)
+    ensure
+      GlobalID.app = old_app
+    end
 end


new line at the end of the file

kaspth · 2014-08-18T19:41:20Z

Cramped build queue? It's been saying waiting for two hours...

jeremy · 2014-08-18T19:47:34Z

lib/global_id/locator.rb

@@ -3,13 +3,60 @@ module Locator
    class << self
      # Takes either a GlobalID or a string that can be turned into a GlobalID
      def locate(gid)
-        GlobalID.find gid
+        @locators[GlobalID.app].locate gid


Rather than the current default app that's used for creating new gids, we need to use the app that's in the URI we're parsing: gid://<app here>/model/id. That means we'll need to parse the gid first and use @locators[gid.app] to do the find.

That suggests that GlobalID#find is what really needs to change. Rather than hardcoding model_class.find(model_id) it needs to ask for the locator for its app, then ask the locator to do the find. The default locator would be an ActiveRecordFinder instance that does def locate(gid) gid.model_class.find gid.model_id end 😁

seuros · 2014-08-18T19:48:15Z

@kaspth you PR need be rebased
This pull request contains merge conflicts that must be resolved.

kaspth · 2014-08-18T20:11:07Z

Sorry, guys. I'm too sleepy to finish this now. This is as far as I will come today.

jeremy · 2014-08-18T20:38:31Z

❤️

On Mon, Aug 18, 2014 at 1:11 PM, Kasper Timm Hansen <
notifications@github.com> wrote:

Sorry, guys. I'm too sleepy to finish this now. This is as far as I will
come today.

—
Reply to this email directly or view it on GitHub
#15 (comment).

kaspth · 2014-08-19T15:10:19Z

@jeremy @seuros I need some advice on what to do with the find_allowed? method.

test 'by GID with only: restriction by module no match' do
  found = GlobalID::Locator.locate(@gid, only: Forwardable)
  assert_nil found
end

The above test fails because the locator doesn't have the only restriction.

What makes the most sense is applying the restriction to all locators. This way users get that behavior for free and we don't complicate the locate method interface.
What do you guys think?

We'd essentially change locate to this:

def locate(gid, options = {})
  if gid = GlobalID.parse(gid)
    locator_for(gid).locate gid if locate_allowed?(options[:only])
  end
end

# global_id.rb
def find(options = {})
  Locator.locate(self, options) # just delegate to the app specific locator.
end

jeremy · 2014-08-20T13:44:18Z

lib/global_id/global_id.rb

+    def parse_encoded_gid(gid)
+      new Base64.urlsafe_decode64(repad_gid(gid)) rescue nil
+    end
+


^ Merge issue, method is down below private now

💩 Sorry.

jeremy · 2014-08-20T13:54:23Z

Looking good! ❤️

jeremy · 2014-08-20T14:15:36Z

lib/global_id/locator.rb

+      #   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?
+        raise ArgumentError, 'Invalid app name provided. App names with underscores are invalid URIs.' if app.to_s.index('_')


👍 for now - when #25 is merged, we'll want to use its app validator for a more thorough check.

jeremy · 2014-08-21T02:47:39Z

Looks like this is ready to go @kaspth - could you rebase + squash?

jeremy · 2014-08-21T02:48:34Z

@kaspth Got a test failure on 1.9.3: https://travis-ci.org/rails/globalid/jobs/33109509#L56

Move only resctriction to locator. Memoize default locator. Privatize class methods properly. Make app name validating public. Use app validation on GlobalID. Normalize app names when reading or setting locators.

kaspth · 2014-08-21T07:57:31Z

@jeremy All done ❤️

Weird test failure. It seems 1.9.3 doesn't like an empty block, so I just inserted some text.

seuros · 2014-08-21T08:03:13Z

test/cases/global_locator_test.rb

+    end
+  end
+
+  test 'app locator is case insensitive' do


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

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?

Let see with @jeremy

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.

Apps can hook in their own locator.

jeremy reviewed Aug 18, 2014
View reviewed changes

seuros reviewed Aug 18, 2014
View reviewed changes

jeremy reviewed Aug 18, 2014
View reviewed changes

jeremy reviewed Aug 20, 2014
View reviewed changes

Apps can hook in their own locator.

c776146

Move only resctriction to locator. Memoize default locator. Privatize class methods properly. Make app name validating public. Use app validation on GlobalID. Normalize app names when reading or setting locators.

seuros reviewed Aug 21, 2014
View reviewed changes

jeremy added a commit that referenced this pull request Aug 21, 2014

Merge pull request #15 from kaspth/app-locater

bda0b7e

Apps can hook in their own locator.

jeremy merged commit bda0b7e into rails:master Aug 21, 2014

kaspth deleted the app-locater branch August 21, 2014 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apps can hook in their own locator. #15

Apps can hook in their own locator. #15

kaspth commented Aug 18, 2014

jeremy Aug 18, 2014

jeremy commented Aug 18, 2014

kaspth commented Aug 18, 2014

seuros Aug 18, 2014

kaspth commented Aug 18, 2014

jeremy Aug 18, 2014

seuros commented Aug 18, 2014

kaspth commented Aug 18, 2014

jeremy commented Aug 18, 2014

kaspth commented Aug 19, 2014

jeremy Aug 20, 2014

kaspth Aug 20, 2014

jeremy commented Aug 20, 2014

jeremy Aug 20, 2014

jeremy commented Aug 21, 2014

jeremy commented Aug 21, 2014

kaspth commented Aug 21, 2014

seuros Aug 21, 2014

kaspth Aug 21, 2014

seuros Aug 21, 2014

jeremy Aug 21, 2014

Apps can hook in their own locator. #15

Apps can hook in their own locator. #15

Conversation

kaspth commented Aug 18, 2014

Choose a reason for hiding this comment

jeremy commented Aug 18, 2014

kaspth commented Aug 18, 2014

Choose a reason for hiding this comment

kaspth commented Aug 18, 2014

Choose a reason for hiding this comment

seuros commented Aug 18, 2014

kaspth commented Aug 18, 2014

jeremy commented Aug 18, 2014

kaspth commented Aug 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremy commented Aug 20, 2014

Choose a reason for hiding this comment

jeremy commented Aug 21, 2014

jeremy commented Aug 21, 2014

kaspth commented Aug 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment