Error using default_scope on subclass model #113

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

Comments

Projects
None yet
2 participants
@mbrock
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

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jan 8, 2015

Owner

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

Owner

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

This comment has been minimized.

Show comment
Hide comment
@mbrock

mbrock Jan 8, 2015

Contributor

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. :)

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jan 8, 2015

Owner

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(...) }

Owner

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

This comment has been minimized.

Show comment
Hide comment
@mbrock

mbrock Jan 8, 2015

Contributor

That sounds reasonable to me!

Contributor

mbrock commented Jan 8, 2015

That sounds reasonable to me!

@nviennot nviennot closed this in b5d2efa Jan 8, 2015

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jan 8, 2015

Owner

All done :)

Owner

nviennot commented Jan 8, 2015

All done :)

@mbrock

This comment has been minimized.

Show comment
Hide comment
@mbrock

mbrock Jan 8, 2015

Contributor

Wow, excellent! Thank you very much.

Contributor

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