rails / globalid

Identify app models with a URI
MIT License
1.23k stars 124 forks source link

Ability to make global id without loading the object #181

Open nhorton opened 3 months ago

nhorton commented 3 months ago

I have some use cases where I need to do almost exactly what is in your docs - MyModel.find(x).to_global_id. That find(x) is a bunch of extra work that I would like to avoid. It would be really nice if I could do MyModel.global_id_for(x) and get the global ID that way. Granted, if x was a bad ID, the global_id would be bad, but that is obviously my bug.

shettytejas commented 3 months ago

Hey @nhorton, I’m trying to understand the exact use case for needing this method. I am assuming it's one of these two scenarios where you might be getting the x (ID):

  1. You already have the object: If you already have the object whose GlobalID you need, it's better to pass that object around rather than constructing a GlobalID from the ID yourself.

  2. The ID comes from user input or similar sources: This is a security risk because GlobalID fetches objects directly, bypassing scopes and any security checks. If a bad ID is passed, it could lead to data leaks or unauthorised access.

If there's any other use case that I may have missed, feel free to call it out!

nhorton commented 2 months ago

Hi! The scenario is that we are serializing some information about associations, and are doing extra lookups. My app domain is pretty complex, but I will give an analogous example below:

Imagine you have an online store that sells many products, but they are different enough that you have multiple tables: books, clothes, shoes, etc.

When a user makes a purchase, we want to record all the products that they looked at in the record along with what they bought and store that. The main product purchased was done with a polymorphic association, so it is fine. We don't want to store the IDs of the items viewed because we would get something like JSON where we kept adding new keys for each table array values of the IDs, and for some reasons of how this record is viewed in other places, that would be problematic. Instead, we want to have an array of global IDs. But as it stands, we have to fetch all the products viewed by ID just so that we can generate the GlobalID for each one. If we could generate the global ID for each from the ID alone (and the class obviously) we could do this without touching the DB for all those records.

shettytejas commented 2 months ago

@nhorton, that makes sense to me, but I'm still unsure about a few things. However, those are more application-related concerns rather than library-specific ones. 😄

I've opened a PR (#186) that adds support for building GlobalId.

If you need this solution urgently, feel free to use the following monkey patch:

# lib/patches/global_id/identification.rb - Make sure you require this file in initializer.
module GlobalId::Identification
  class << self
    def included(base)
      base.extend(ClassMethods)
    end
  end

  module ClassMethods
    def build_global_id(obj, options = {})
      return obj.to_global_id(options) if obj.respond_to?(:to_global_id)
      raise ArgumentError, "Can't build a Global ID for #{obj.class}" unless obj.is_a?(String) || obj.is_a?(Integer)

      unless (app = options.fetch(:app) { GlobalID.app })
        raise ArgumentError, "An app is required to create a GlobalID. Pass the :app option or set the default GlobalID.app."
      end

      GlobalID.new(URI::GID.build(app: app, model_name: name, model_id: obj, params: options.except(:app, :verifier, :for)))
    end
  end
end
nhorton commented 2 months ago

This is great! Thank you so much.

nflorentin commented 1 month ago

That would be a great feature.

I have the following simple use case :

The two jobs are separated because the job to sync order make API calls so if API call fails, we want to retry only the job associated to the specific order.

I could do that:

class RecurringJob 
  def perform
    Order.not_paid.each { |order| SyncJob.perform_later(order) }
  end
end

But I'll be instantiating a lot of Order objects for nothing.

I prefer to do that:

class RecurringJob 
  def perform
    Order.not_paid.pluck(:id).each { |order_id| SyncJob.perform_later(order_id) }
  end
end

But then, I can't benefit from automatic global id deserialization in SyncJob.

I would like to do that:

class RecurringJob 
  def perform
    Order.not_paid.pluck(:id).each { |order_id| SyncJob.perform_later(Order.build_global_id(order_id)) }
  end
end

Thanks for the patch, although I will wait for a version of globalid with your pull request because I'm not a huge fan of monkey-patching gems.