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

Error using default_scope on subclass model #113

Closed
mbrock opened this issue Jan 8, 2015 · 6 comments
Closed

Error using default_scope on subclass model #113

mbrock opened this issue Jan 8, 2015 · 6 comments

Comments

@mbrock
Copy link
Contributor

mbrock commented Jan 8, 2015

Hello. I'm trying to port the test app for rails_admin to use NoBrainer, and ran across an issue with using default_scope in a model that's a subclass in a polymorphic hierarchy.

Here is a gist with a minimal example.

The exception is generated from this line. This precondition seems to be copied from another method. Is it perhaps there by mistake? Or are there actual problems with using default scopes on subclasses?

@nviennot
Copy link
Collaborator

nviennot commented Jan 8, 2015

Hi Mikael :)

The exception should have read default_scope() must be called on the parent class, not store_in. This was a copy/paste typo, which is now fixed: https://github.com/nviennot/nobrainer/blob/master/lib/no_brainer/document/criteria.rb#L46

I can implement the default scope feature for subclasses.
The behavior would be the following: when querying a subclass, the default scope of each of the ancestors would be applied. For example:

class Thing
  include NoBrainer::Document
  default_scope { where(:visible => true) }
end

class Subthing < Thing
  include NoBrainer::Document
  default_scope { order_by(:id) }
end

Subthing.to_a would use a query with where(:visible => true).order_by(:id).
Maybe that's too much magic, but maybe it's a reasonable feature after all.
But then the reasonable thing to do would be to allow specifying the default scope multiple times within a single class, and expect them to all get chained.

Are you on board with this behavior?

Note: the default scope of any model is order_by(:id) if left unspecified

@mbrock
Copy link
Contributor Author

mbrock commented Jan 8, 2015

Hi Nicolas!

Thanks. That sounds like what I think I'd expect. Multiple default scopes in a single class could be useful, I suppose, and perhaps simplifies the implementation?

I can imagine someone wanting to use the subclass default scope to override the parent default scope, but maybe that can be done with something like:

class Subthing < Thing
  include NoBrainer::Document
  default_scope { unscoped.order_by(:name) }
end

Re: the default ID order, that's good to know, but it was just an example. :)

@nviennot
Copy link
Collaborator

nviennot commented Jan 8, 2015

you wouldn't be able to override the parent default scope.
unscoped means that you would not want to apply any of the defined default scopes.
The example you showed is a bit of a heisenberg query. At the time you say that you want to not use default_scopes, you already did, which is weird.

So we have three possible behaviors:

  1. use all the default scopes defined in the class and all default scope in the ancestors
  2. the latest defined defaut scope wins (whether declared in a subclass or in the same class over and over)
  3. don't allow multiple default scope definitions

I don't think we should do 2) because of potential security issues: If a parent class has some where(:only_visible_by_admin => false) default scope, it would be a shame to discard it.

So I think I'll do 1) -- If you want to override the parent default scope, well, It probably means that you are not using subclasses properly, since the subclass would not share the same behavior as its parent.
And in the case where you really want that, then you can always put some conditional logic in your default_scope. e.g. default_scope { self == SubClass ? where(...) : where(...) }

@mbrock
Copy link
Contributor Author

mbrock commented Jan 8, 2015

That sounds reasonable to me!

@nviennot
Copy link
Collaborator

nviennot commented Jan 8, 2015

All done :)

@mbrock
Copy link
Contributor Author

mbrock commented Jan 8, 2015

Wow, excellent! Thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants