Support for Sequel?

[halostatue]

Sequel doesn’t follow the same protocol as ActiveRecord, even when ActiveModel compliant (or so it seems). I’ve come up with what looks like the right level of monkey-patching for this, and can turn this into a real patch (with tests) for GlobalID if there is interest. The changes are only necessary in GlobalID::Locator::BaseLocator for #locate and #find_records.

The only other change is including GlobalID::Identification into Sequel::Model, but in my basic testing, these changes create the same result as AR-backed GlobalID, mod the appropriate exceptions.

Also opened as TalentBox/sequel-rails#111.

module SequelBaseLocator
  def locate(gid)
    if defined?(::Sequel::Model) && gid.model_class < Sequel::Model
      gid.model_class.with_pk!(gid.model_id)
    else
      super
    end
  end

  private

  def find_records(model_class, ids, options)
    if defined?(::Sequel::Model) && model_class < Sequel::Model
      model_class.where(model_class.primary_key => ids).tap do |result|
        if !options[:ignore_missing] && result.count < ids.size
          fail Sequel::NoMatchingRow
        end
      end.all
    else
      super
    end
  end
end

GlobalID::Locator::BaseLocator.prepend SequelBaseLocator
Sequel::Model.send(:include, ::GlobalID::Identification)

[rafaelfranca]

Could not you implement a SequelLocaltor and register it to be used in your application?

clas SequelLocator < BaseLocator
  def locate(gid)
      gid.model_class.with_pk!(gid.model_id)
  end

  private

  def find_records(model_class, ids, options)
    model_class.where(model_class.primary_key => ids).tap do |result|
      if !options[:ignore_missing] && result.count < ids.size
        fail Sequel::NoMatchingRow
      end
    end.all
  end
end

GlobalID::Locator.use :my_sequel_app, SequelLocaltor.new

This way you don't need to monkey patch anything in GlobalID.

[halostatue]

Yes-ish. I was looking at that as a possibility, but what I really want is a way to replace the default locator rather than having to specify with use. This particular app doesn’t use ActiveRecord at all, so if a GlobalID comes in that isn’t gid://my_sequel_app/Model/id, things will go sideways in ways that may not be clear.

I actually want to do a lot more for Sequel support than just the locator because Sequel makes it easier to have composite or non-sequence primary keys, and I would like to make GlobalID::Identification do the right thing. It’s not as important to me at the moment as the lookup functionality.

In general, how open is the Rails team to supporting non-AR lookup, or getting a modification in to allow for class/module-based dispatch, not just app-name based dispatch? Something like this, maybe:

class GlobalID::Locator
  class BaseLocator
    def locate(gid)
      if class_locator = Locator.class_locators.find { |c| gid.model_class <= c }
        class_locator.locate(gid)
      else
        gid.model_class.find gid.model_id
      end
    end
  end

  def self.use_class_locator(klass)
    @class_locators << klass
  end

  def self.class_locators
    @class_locators ||= []
  end
end

class SequelLocator
  def locate
    gid.model_class.with_pk!(gid.model_id)
  end
end

GlobalID::Locator.use_class_locator SequelLocator.new

[kaspth]

Yes-ish. I was looking at that as a possibility, but what I really want is a way to replace the default locator rather than having to specify with use.

use replaces the locator if you use the right app name. Just match config.global_id.app to this:

globalid/lib/global_id/railtie.rb

Line 17 in 14f5a57

app.config.global_id.app ||= app.railtie_name.remove('_application').dasherize

.

I think our existing app locators can do what you ask just fine and I don't think we need to overload the standard locators for Sequel support (or add class/module locating). Happy to hear other arguments, but I'll close for now ❤️

[jeremy]

We could duck type to a model_class.locate_global_id call. A Sequel
plugin could provide that class method and redispatch to its preferred way
to find.
On Wed, May 18, 2016 at 08:04 Austin Ziegler notifications@github.com
wrote:

Yes-ish. I was looking at that as a possibility, but what I really want is
a way to replace the default locator rather than having to specify with
use. This particular app doesn’t use ActiveRecord at all, so if a
GlobalID comes in that isn’t gid://my_sequel_app/Model/id, things will go
sideways in ways that may not be clear.

I actually want to do a lot more for Sequel support than just the locator
because Sequel makes it easier to have composite or non-sequence primary
keys, and I would like to make GlobalID::Identification do the right thing.
It’s not as important to me at the moment as the lookup functionality.

In general, how open is the Rails team to supporting non-AR lookup, or
getting a modification in to allow for class/module-based dispatch, not
just app-name based dispatch? Something like this, maybe:

class GlobalID::Locator
class BaseLocator
def locate(gid)
if class_locator = Locator.class_locators.find { |c| gid.model_class <= c }
class_locator.locate(gid)
else
gid.model_class.find gid.model_id
end
end
end

def self.use_class_locator(klass)
@class_locators << klass
end

def self.class_locators
@class_locators ||= []
endend
class SequelLocator
def locate
gid.model_class.with_pk!(gid.model_id)
endend
GlobalID::Locator.use_class_locator SequelLocator.new

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#87 (comment)

[kaspth]

@jeremy sure that could work for the singular case and we could have a model_class.locate_many_global_ids for the locate many case.

[halostatue]

@kaspth Having looked at the code, no, use doesn’t replace the default locator in any way. If I receive a GlobalID that translates to gid://my_app/Model/1 instead of gid://my_sequel_app/Model/1, a use-provided locator will cause failure because I cannot replace the default locator without doing a remove_const, const_set dance. An ActiveRecord-based application will continue to work because the locator discovery code uses fetch with a default of DEFAULT_LOCATOR, a BaseLocator which uses an AR-specific protocol, but my Sequel-based app will blow up.

You could say that it should fail, because this GlobalID isn’t for this app, but that isn’t the way that GlobalID is currently written. If the Railtie instead did a default use and the locator code failed if they key doesn’t exist, that would increase the overall security. That is a breaking change, though.

I sort of like the idea of duck-typing to model_class.locate_global_id (&c) because that would also be a good idea to provide on the Identification side as well (to enable support for multiple PKs; this is something that, in addition to users of sequel, the folks who develop the composite_keys gem could use).

[kaspth]

If I receive a GlobalID that translates to gid://my_app/Model/1 instead of gid://my_sequel_app/Model/1

I was assuming you had only one app, but true this is a different case. I'm curious, why would you receive different apps? Are you building an app that could potentially locate Global IDs with unknown apps?

that would also be a good idea to provide on the Identification side as well (to enable support for multiple PKs; this is something that, in addition to users of sequel, the folks who develop the composite_keys gem could use).

That's a lot of Sequel inside baseball there. Can you try some pseudo code for the Identification module to demonstrate what you mean? 😁

For instance it seems like Sequel deals with the singular and many cases differently than Active Record's find and where. How would it work with model_class.locate_global_id and model_class.locate_many_global_ids?

[halostatue]

OK. Let me try to break this down, because some of what I mentioned was hypothetical, but this discussion has (to me) pointed out a potential problem for GlobalID that I alluded to. There’s a couple of things to look at here, and I can explain them in non-Sequel-specific terms (but I’ll also answer your questions re: Sequel). There are probably an issue or two that should come out of this, so this might get a bit long. I apologize in advance.

The Problem of the Default Locator

First, the way that the default locator is implemented may be a vulnerability. The implementation of GlobalID::Locator.locator_for is:

def locator_for(gid)
  @locators.fetch(normalize_app(gid.app)) { DEFAULT_LOCATOR }
end

So if GlobalID.app is set to my_app and I do not have a use specifically for my_other_app, then locator_for will locate the DEFAULT_LOCATOR for my_other_app and attempt to locate the models. That is, if there is no use for my_other_app, these two GlobalIDs are functionally equivalent:

gid://my_app/User/1337
gid://my_other_app/User/1337

This can be mitigated by removing the DEFAULT_LOCATOR and changing the config.after_initialize block of the Railtie to include something like:

GlobalID::Locator.use GlobalID.app, UnscopedLocator.new

That way, GlobalID only resolves for apps that are either the default app “detected” through the Railtie or that are explicitly configured.

This change alone would be fairly useful as I could create a SequelGlobalIDLocator class and make a PR to TalentBox/sequel-rails to support this, and the Railtie for sequel-rails could then implement:

GlobalID::Locator.use GlobalID.app, SequelGlobalIDLocator.new

As long as the sequel-rails initializer executes after the global_id initializer, this would not be a problem…but if there’s a way that sequel-rails could hook into running after the global_id initializer, that would be mitigated (I don’t know enough about Rails initialization to be able to say whether this is possible).

This fixes both the default locator vulnerability and the problem of Sequel not implementing find the way that Rails does (in the best case, it just doesn’t work; in the worst case, it throws a exception if you have an extension turned on that requires that you use Sequel.lit("foo") if you want to use a literal string).

(I do have multiple apps, but when I get around to doing all of the locators in general, each app will implement its own, but there will be an external version so we can request data across the wire from the app that owns it. It’s the right thing to do, but I mentioned this as an example of something that could be wrong, although I think the problem is real.)

Alternate Approach: model_class.locate_global_id and model_class.locate_many_global_ids

Purely as pseudo-code, this would probably look something like:

Sequel::Model.plugin :global_id_locators

This would become Sequel::Plugins::GlobalIdLocators which would add a ClassMethod that implements the appropriate GlobalID locator methods, probably like I have shown in the code I put here in the first place, just inside model_class instead of calling model_class.

If the default locator mechanism is changed…then this is not necessary, probably. It’s only probably because of an issue that goes all the way to the implementation of URI::GID.create:

      # Shorthand to build a URI::GID from an app, a model and optional params.
      #
      #   URI::GID.create('bcx', Person.find(5), database: 'superhumans')
      def create(app, model, params = nil)
        build app: app, model_name: model.class.name, model_id: model.id, params: params
      end

The use of model.id is also a very strong AR-ism as id is implemented as (more or less, including in the composite-primary-keys/composite_primary_keys gem) read_attribute(self.class.primary_key). If I have a composite primary key in Sequel, model.id will probably return nil, whereas model.pk will return [1, 1] (if there are two ID-based columns used as the primary key). So to really make GlobalID work properly with Sequel, I also need to override URI::GID.create to do something like:

build app: app, model_name: model.class.name, model_id: model.pk, params: params

That isn’t really perfect, because it would probably be better if I could do model.pk_hash.to_query (which, rather than "[1, 1]" would be "category_id=1&product_family_id=1", but that potentially conflicts with params and would probably cause problems for parsing for locators as well).

Let me be clear: the models that I want to use GlobalID with right now definitely use sequence-based IDs, so the specific problem I’m talking about is hypothetical, not actual for my application. If we had a duck-typed method model#global_id_pk or something like that I could then build something that has all the awareness required.

Yes, I can replace GlobalID::Identification with something that is Sequel-aware as a proposed change to sequel-rails, or as my own plug-in to Sequel (sequel-globalid) but some of this feels like the wrong place to do this.

Where next?

That’s mostly up to you guys. I actually think that this can be closed, but there should be two issues opened in its place, both described herein. I think that DEFAULT_LOCATOR has to be removed and replaced with an explicitly set default locator, even if it is “automatic” in the Railtie. If it isn’t removed, your app may respond to GlobalIDs that you don’t intend. This will fix the first issue. Putting some sort of duck-typed method so that URI::GID.create works even if the locator doesn’t respond to id the way you expect is also a good idea and will fix the second issue.

If and when those changes are made, I can do the rest of the heavy lifting for Sequel support.

I am also happy to provide PRs for these, but I am in a fairly busy season and now that I have a patch for GlobalID support in my app, I don’t need this right now so I may not have time to complete such a patch quickly (although the DEFAULT_LOCATOR is, at the risk of repeating myself, a vulnerability and should be fixed before Rails 5 is released, but that’s just me).