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

DeserializationError when not found with default_scope #66

Closed
philipgiuliani opened this issue Apr 23, 2015 · 17 comments · Fixed by #73
Closed

DeserializationError when not found with default_scope #66

philipgiuliani opened this issue Apr 23, 2015 · 17 comments · Fixed by #73
Labels

Comments

@philipgiuliani
Copy link

@philipgiuliani philipgiuliani commented Apr 23, 2015

When i have set a default_scope to my model, GlobalID can't find it. https://github.com/rails/globalid/blob/master/lib/global_id/locator.rb#L130

Error while trying to deserialize arguments: Couldn't find Image with 'id'=75

Maybe it should always be set to unscoped?

@philipgiuliani philipgiuliani changed the title Find with default_scope DeserializationError when not found with default_scope Apr 23, 2015
@kaspth

This comment has been minimized.

Copy link
Member

@kaspth kaspth commented May 3, 2015

If you have a default_scope it would make more sense to me that it should be applied by default 😄

But I haven't seen the code. Can you paste something that approximates the scenario?

@philipgiuliani

This comment has been minimized.

Copy link
Author

@philipgiuliani philipgiuliani commented May 4, 2015

I dont think that it should be applied for mailers/jobs (internal logic)
A little example for my use case:

# User Model
class User < ActiveRecord::Base
  default_scope { where(status: :active) }
  enum status: [:active, :blocked]
end

# Some method somewhere
def report_user
  user = User.find(params[:id])
  user.blocked!

  AdminMailer.user_blocked(user).deliver_later
end

This example will fail, because the default scope is blocked of course. Currently i am passing user_id instead of the user instead as workaround.

What do you think?

@kaspth

This comment has been minimized.

Copy link
Member

@kaspth kaspth commented May 4, 2015

Default scope should be applied here as well. Otherwise applying it sporadically and nondeterministically doesn't make sense to me.

The way I read your code, you only want active users in 99% of the cases, but in this one case where you're dealing with blocked users you should use unscope. ❤️

@kaspth kaspth closed this May 4, 2015
@philipgiuliani

This comment has been minimized.

Copy link
Author

@philipgiuliani philipgiuliani commented May 4, 2015

Thanks for the answer and explanation. With using unscope you mean passing the user_id instead, right?

@choonkeat

This comment has been minimized.

Copy link

@choonkeat choonkeat commented May 7, 2015

The way I read your code, you only want active users in 99% of the cases, but in this one case where you're dealing with blocked users you should use unscope.

meaning don't use GlobalID (aka "good ol pass by id")? or there's a way to specify for unscope?

@kaspth

This comment has been minimized.

Copy link
Member

@kaspth kaspth commented May 7, 2015

I mean use the unscoped method:

def report_user
  User.unscoped do  
    user = User.find(params[:id])
    user.blocked!

    AdminMailer.user_blocked(user).deliver_later
  end
end
@kaspth

This comment has been minimized.

Copy link
Member

@kaspth kaspth commented May 7, 2015

Eh, I just figured that might not work when GlobalID later calls find because deliver_later returns immediately.

@kaspth kaspth reopened this May 7, 2015
@kaspth kaspth added the help wanted label May 7, 2015
@philipgiuliani

This comment has been minimized.

Copy link
Author

@philipgiuliani philipgiuliani commented May 7, 2015

Yes thats what i meant.. :D

@kaspth

This comment has been minimized.

Copy link
Member

@kaspth kaspth commented May 7, 2015

@matthewd

This comment has been minimized.

Copy link
Member

@matthewd matthewd commented May 7, 2015

Yeah.. this is tricky. Do we need some sort of AR::Base.definitely_find(id)? I certainly agree that unlike a random User.find(params[:id]), if we know we started out with an extant User instance, it does rather seem that it's on us to make sure we manage to get it back again.

Anything we do here is going to change the API required from a Global ID-supporting model class... but I think directly relying on unscoped would be unreasonably broad. Maybe we should first try for no_really_do_actually_find, and if it doesn't respond to that, fall back to regular find? Without such a fallback, we would seem to have a rather unpleasant versioning/compatibility issue.

@cristianbica

This comment has been minimized.

Copy link
Member

@cristianbica cristianbica commented May 7, 2015

In GlobalID there's a default AR finder:

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

      mattr_reader(:default_locator) { ActiveRecordFinder.new }

This can be changed to model_class.unscoped.find gid.model_id.

@kaspth

This comment has been minimized.

Copy link
Member

@kaspth kaspth commented May 7, 2015

The ActiveRecordFinder is private and you shouldn't override locate in it. Instead you can use our custom app locators for this. Substitute bcx for your app name here:

GlobalID::Locator.use 'bcx' do |gid|
  gid.model_class.unscoped.find gid.model_gid
end
@cristianbica

This comment has been minimized.

Copy link
Member

@cristianbica cristianbica commented May 7, 2015

@kaspth I meant change that in GlobalID (if it's AR only code) versus @matthewd 's idea on creating AR::Base.definitely_find(id)

@philipgiuliani

This comment has been minimized.

Copy link
Author

@philipgiuliani philipgiuliani commented May 7, 2015

I also think changing the default ActiveRecordFinder would be the cleanest and simplest way. Since its only for AR there should be no compability issues.

@kaspth

This comment has been minimized.

Copy link
Member

@kaspth kaspth commented May 7, 2015

Well, the name is a little misleading. It's the default locator and is for any model that implements find which takes an id.

find is the contract for models to be locatable through Global ID. So if we add unscoped or something that's another method for models to implement. Which Active Record models do by default, but it might not make sense for a lot of other models.

@cristianbica

This comment has been minimized.

Copy link
Member

@cristianbica cristianbica commented May 7, 2015

@kaspth we could Have a DefaultFinder and an ActiveRecordFinder and the default_locator can be converted to default_locator_for(gid) and based on the gid.model_class decide if it should use DefaultFinder or ActiveRecordFinder

@kaspth

This comment has been minimized.

Copy link
Member

@kaspth kaspth commented May 7, 2015

Maybe we could do it as:

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

class ActiveRecordFinder < DefaultFinder
  def locate(gid)
    gid.model_class.unscoped { super }
  end
end

@matthewd what do you think about this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.