Restore context option. #384

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

tovodeverett commented Dec 13, 2012

Restore context option. Collections inherit context by default, but associations can specify either a static context or a lambda. Validation for options in a number of methods.

This is an implementation of the discussion in #372.

Implementation notes:

  • Validation of passed option keys was implemented for a number of methods. I isolated the logic in Draper.validate_options and had the files that used that method require 'draper'. I'm not sure if this is the best place for it - I looked around for a similar method built into Rails and couldn't find one. I did find a gem that handled this issue, but I didn't want to add another dependency. I decided against writing a test suite for the method itself since each of the individual methods that uses it has their own set of tests for testing that options are validated.
  • It appears that the existing code base doesn't prevent the use of :scope on a singular association. I'm not sure if we want to change this behavior.
  • I had some fun figuring out how to test some of the behavior. When testing that CollectionDecorator#context= doesn't enumerate items unnecessarily, I ended up stubbing out Array#loaded?. When testing :scope in DecoratedAssociation#decorator_options I had a lot of trouble trying to figure out how to stub out a fake scope when I realized I could just use :to_a as a fake scope that would do nothing. I ended up using :decorate for a similar purpose testing the singular association.
Restore context. Collections inherit context by default, but associat…
…ions

can specify either a static context or a lambda.  Validation for options
in a number of methods.
Contributor

tovodeverett commented Dec 13, 2012

Looks like the Travis build on Rubinius failed 4 tests for one of the attempts and succeeded on the other attempt.

One of the things I added in this commit was that I marked #options and #options= for Decorator and CollectionDecorator as protected. If we're going to go to the trouble of validating the options keys when they get passed in, I figured we might as well defend the options hashes. Users can always used send to get around security, but this way we're at least making it known that options is part of the private interface. For whatever reason, one of the Rubinius builds failed on those 4 tests, but the other Rubinius build succeeded. I don't have Rubinius on my box - I tried plugging the seed value in on my machine to see if that would have an impact, but I couldn't recreate.

One possibility is that the way I'm marked those as protected might trigger a race condition. I'm using the following approach:

attr_accessor :source, :options
protected :options, :options=

Note that rather than split the accessors up and switch into and out of protected mode, I just create all the accessors and then mark the ones I want protected as such. This approach could be more vulnerable to a race condition or a cache coherency issue.

lib/draper.rb
@@ -43,6 +43,13 @@ def self.setup_orm(base)
end
end
+ def self.validate_options(options, *valid_keys)
+ options_errors = options.keys - valid_keys
+ unless options_errors.empty?
@steveklabnik

steveklabnik Dec 13, 2012

Owner

Don't see why this can't just be one line.

@tovodeverett

tovodeverett Dec 13, 2012

Contributor

If you're referring to assigning options_errors, it does get used in the next line as well (I return the list of invalid keys as part of the error). No reason we can't duplicate the calculation (validation failures are pretty unlikely), but it will raise the complexity on the #{} interpolation in the raise call even more.

If you're referring to using unless as flow-control instead of as a modifier, I wobbled back and forth. Using it as a modifier made the raise line even longer (unless I wrapped it) and harder to understand.

@steveklabnik

steveklabnik Dec 13, 2012

Owner

I didn't see it on the next line because I'm an idiot. This is the best way, then, I agree, carry on.

lib/draper/collection_decorator.rb
@@ -1,21 +1,25 @@
+require 'draper'
@steveklabnik

steveklabnik Dec 13, 2012

Owner

This is kinda weird. Why do we need this here?

@tovodeverett

tovodeverett Dec 13, 2012

Contributor

I added the require 'draper' lines to the modules that use Draper.validate_options. Again, I wobbled on this (why would someone be using Draper::Decorator without Draper), but the presence of require 'active_support/core_ext/array/extract_options' in decorator.rb was what tipped me in favor of making the requirement explicit.

@steveklabnik

steveklabnik Dec 13, 2012

Owner

Can we require the exact file we need, just like require 'active_support/core_ext/array/extraction_options'?

@tovodeverett

tovodeverett Dec 13, 2012

Contributor

Unfortunately, that is the exact file we need! We could move Draper.validate_options into its own file, but since core support logic that is shared by a number of classes (Decorator, DecoratedAssociation, and CollectionDecorator), in the end I decided that draper.rb was the most logical place. But if you have a better idea on where to put it, I concur!

@steveklabnik

steveklabnik Dec 13, 2012

Owner

Oh, you put them there. Duh. I checked the existing source, and saw they weren't there. ;)

Really, I'd prefer that draper.rb just require all the other files, and we put everything else in other files. Not sure what that one should be named, though.

@haines

haines Dec 13, 2012

Contributor

I think we should just use ActiveSupport's assert_valid_keys, then this won't be an issue.

@tovodeverett

tovodeverett Dec 13, 2012

Contributor

ARGH! I knew I'd seen something like that, and I even looked through http://apidock.com/rails/Hash/ looking for that exact method and didn't see it.

@tovodeverett

tovodeverett Dec 13, 2012

Contributor

Give me a few minutes to get that swapped in and I'll submit a new pull request!

- @options = options
+ # Accessor for `:context` option
+ def context
+ options.fetch(:context, {})
@steveklabnik

steveklabnik Dec 13, 2012

Owner

I almost made a joke about options.fetch :context {{}} but.....yeah.

@tovodeverett

tovodeverett Dec 13, 2012

Contributor

I actually pulled that code straight from the red lines in aeac106.

@steveklabnik

steveklabnik Dec 13, 2012

Owner

Yeah no it's good, I was just teasing. Nobody should ever write {{}}.

@tovodeverett

tovodeverett Dec 13, 2012

Contributor

I do it all the time with let, but I do stick in some spaces, as in let(:options) { {} }. At least they're not fingernail clippings! :-)

+ # Setter for `:context` option
+ def context=(input)
+ options[:context] = input
+ each {|item| item.context = input } unless respond_to?(:loaded?) && !loaded?
@steveklabnik

steveklabnik Dec 13, 2012

Owner

When don't we have a loaded? method? Non-AR stuff? I hate checks like these. :/

@tovodeverett

tovodeverett Dec 13, 2012

Contributor

Yup. First ran into it because the test suite uses a plain Array. Originally I just stubbed it out for the test suite, but then I got to thinking and remembered seeing stuff sprinkled through the code base for non-AR stuff, and figured I should code more defensively. The point of that unless modifier is to ensure that we don't trigger the loading of the collection on an AR collection if it hasn't already been loaded.

@steveklabnik

steveklabnik Dec 13, 2012

Owner

Right. 👌

Owner

steveklabnik commented Dec 13, 2012

Overall, I like it. @nashby ? @jcasimir ? @haines?

Contributor

haines commented Dec 13, 2012

Yeah looking pretty good. I have a couple of minor changes I'd like to make, I'll send @tovodeverett a pull request.

@haines haines referenced this pull request Dec 13, 2012

Merged

Restore context #385

Owner

steveklabnik commented Dec 13, 2012

Superseded by #385.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment