Acceptig symbol as :as parameters when indexing and documents and adding documentation on how to use lambda's on it #446

Closed
wants to merge 2 commits into
from

Projects

None yet

5 participants

@mauricio
mauricio commented Sep 2, 2012

Hello guys,

This PR contains code to accept Symbols and lambdas as as values for :as options when indexing. It contains tests covering this new code and also tests for the current implementation.

I have added validation for when :as fields are set to make sure that if the user gives it an unknown or invalid value an exception will be raised instead of failing silently (or breaking as in the case of lambdas).

I have also added information to the README file pertaining these changes when indexing documents.

@travisbot

This pull request fails (merged 0646f7c8 into 5818e50).

@mauricio
mauricio commented Sep 9, 2012

Anyone? @karmi

@karmi
Owner
karmi commented Sep 9, 2012

Hi, sorry for the delay, busy lately. I definitely like the direction and the behaviour, but would change something in the implementation a bit... Allow me couple of days to look at it...

@karmi karmi commented on the diff Sep 9, 2012
lib/tire/model/indexing.rb
@@ -86,6 +89,15 @@ def mapping(*args)
def indexes(name, options = {}, &block)
mapping[name] = options
+ unless options[:as].nil?
+ case options[:as]
+ when String, Symbol, Proc
+ true # ok, don't do anything
+ else
+ raise Tire::InvalidAsOptionException.new( options[:as] )
@karmi
karmi Sep 9, 2012 Owner

Why raise here -- why not in the to_indexed_hash where we are doing the case anyway?

@mauricio
mauricio Sep 9, 2012

To avoid allowing you to define a config that's not supported, just failing early. But I can move this to the to_indexed_hash method too.

@karmi karmi commented on an outdated diff Sep 9, 2012
lib/tire/invalid_as_option_exception.rb
@@ -0,0 +1,9 @@
+module Tire
+ class InvalidAsOptionException < Exception
+
+ def initialize( value )
+ super("Unknown type for :as field, accepted types are `Symbol`, `String` and `Proc`s - #{value.class.name} - #{value.inspect}")
+ end
+
+ end
+end
@karmi
karmi Sep 9, 2012 Owner

1/ Beware of files without trailing newlines in git.
2/ I think we should have a file like errors or something, rather then create a new file for every exception there is.

@karmi karmi commented on an outdated diff Sep 9, 2012
README.markdown
@@ -440,7 +440,37 @@ You can define different [_analyzers_](http://www.elasticsearch.org/guide/refere
or any other configuration for _elasticsearch_.
You're not limited to 1:1 mapping between your model properties and the serialized document. With the `:as` option,
-you can pass a string or a _Proc_ object which is evaluated in the instance context (see the `content_size` property).
+you can pass a symbol, string, _lambda_ or a _Proc_ object to generate the value. The evaluation is done as follows:
+
+If `String` the text given is evaluated at the current instance (using `instance_eval`), look at the `content.size` property above.
+
+If `Symbol` a method with the same name as the symbol is called at the current instance, as in:
+
+```ruby
+ mapping do
+ indexes :id, :as => :some_other_id
@karmi
karmi Sep 9, 2012 Owner

Thanks for having added also docs for it, that's rare :) But I think we should try for a better example? I usually hate "foobar", "someotherid" in documentation...

@karmi karmi commented on the diff Sep 9, 2012
test/unit/model_search_test.rb
@@ -907,6 +907,28 @@ def self.find(*args); new; end
end
+ context 'trying to index with invalid :as option' do
+
+ should 'raise a Tire::InvalidAsOptionException if :as is of an invalid type' do
+
+ assert_raise ::Tire::InvalidAsOptionException do
+
+ class SomeFakeModel
+ include Tire::Model::Search
+ include Tire::Model::Callbacks
+
+ mapping do
+ indexes :name, :as => Time.now
@karmi
karmi Sep 9, 2012 Owner

I'm trying to figure out if there isn't a valid use case for passing an object like this... Some application-level configuration value, which is determined at application startup?

@mauricio
mauricio Sep 9, 2012

You could possibly use it for something like that, but you could also use a proc/lambda for it, my main concert here is to only accept something that we know how to handle.

When I first used :as I was using it with a lambda and it was working, somewhere along the path it stopped working and I was clueless as to what I was missing (which was my own mistake, as I didn't notice the docs did not talk about lambdas, only procs), that's when I decided to check the code :)

So, this is mostly me scratching my own itch and making sure it is not possible to send in a value that's not "known".

Now that's for you to decide if we should validate or just keep it as is ;D (i'm personally much more inclined to validate)

@karmi
karmi Sep 10, 2012 Owner

I understand your reasoning. Anybody else has any opinion on this?

@ches
ches Oct 19, 2012

It seems sensible to me. If someone came along with a good use case, it'd probably have to be something where a given object would need to respond_to? some common convention, right? I don't know how else you could sensibly handle it, really. In that case it'd be a simple matter to add a when clause to the case statement @mauricio added to support that.

I like the clarification/definition of the semantics here otherwise.

@karmi
karmi Oct 19, 2012 Owner

@ches Don't understand your reasoning 100%, I'm afraid, but still -- it's really the same thing as defining scopes in ActiveRecord, see eg. http://paulbarry.com/articles/2008/12/18/named-scope-to-lambda-or-not-to-lambda-that-is-the-question

@ches
ches Oct 19, 2012

Can you give an example of the scope-like usage you're referring to?

@karmi
karmi Oct 19, 2012 Owner

@ches, in the article above: named_scope :published_before, lambda{ |date| {:conditions => ["published_at < ?", date] } }

@ches
ches Oct 19, 2012

@karmi I understand ActiveRecord scopes, I was asking for an example of how you imagine that being applied in the case of indexes :as 😄

@karmi
karmi Oct 19, 2012 Owner

Like this:

require 'tire'

Tire.index('articles').delete

class Article

  include Tire::Model::Persistence

  property :title
  property :size,       :as => proc { title.size }
  property :created_at, :default => lambda { Time.now }

end


p Article.create(title: 'Hello').attributes,

  Article.index.refresh,

  "Size: %s" % [ Article.first.size ]
@karmi
karmi Oct 19, 2012 Owner

Or, more to the point:

require 'tire'

Tire.index('articles').delete

class Article
  include Tire::Model::Persistence

  property :title
  property :mydate,     :as => proc { Time.now }
  property :created_at, :default => lambda { Time.now }
end

p "Current date: %s" % [ Time.now ]
sleep 5

Article.create
Article.index.refresh

p "My date:      %s" % [ Article.first.mydate ]
@ches
ches Oct 19, 2012

But shouldn't that work as-is? You're using proc/lambda, the same way we naturally would when defining AR scopes like this, and I don't think that is at question here. Your initial question was if there might be a use case for passing some arbitrary object for :as -- that's what I'm asking, for a (hypothetical) example of how you could imagine that being used, sorry if that was unclear. I'm not aware of any analog to that in AR scope definitions.

@mauricio hasn't changed anything about Tire's existing ability to handle procs/lambdas here, he just wants to bail if given something else that we don't know what to do with. That seems sane and doesn't lock you out of later accepting something else if you came up with something that would be meaningful.

mauricio added some commits Sep 2, 2012
@mauricio mauricio Acceptig symbol and proc as :as parameters, adding tests covering the…
…ir usage, validate that the :as field contains a valid value (if not an exception is raised) and complementing documentation about them on README
3498181
@mauricio mauricio Updating readme and creating exceptions file 20372f0
@mauricio
mauricio commented Sep 9, 2012

I have updated the README and also added an errors.rb file.

@mauricio

No other comments on this @karmi ?

@kenshin54

+1

@karmi karmi added a commit that referenced this pull request Oct 23, 2012
@karmi [#446] Make the `:as` mapping option strict about the passed object
Based on discussion with @mauricio and @ches in #446.

Still not sure... why you shouldn't be able to do:

    class MyModel
        include Some::ORM
        include Tire::Model::Search

        mapping do
          indexes :field, :type => 'string', :analyzer => 'keyword', :as => SomeOtherObject.whatever
        end
      end
    end

or:

    class MyModel
        include Some::ORM
        include Tire::Model::Search

        mapping do
          indexes :field, :type => 'string', :analyzer => 'keyword', :as => [1, 2, 3]
        end
      end
    end

/cc @vhyza
0ccee42
@karmi karmi added a commit that referenced this pull request Oct 23, 2012
@karmi [#446] Changed, that arbitrary objects passed in the `:as` mapping op…
…tion are indexed

Previously, objects other then `String` or `Proc` were silently discarded
when evaluating the `:as` mapping option.

With this change, you may pass whatever object you like in the `:as` mapping option:

    class MyModel
        include Some::ORM
        include Tire::Model::Search

        mapping do
          indexes :field, :type => 'string', :analyzer => 'keyword', :as => SomeOtherObject.whatever
        end
      end
    end

    class MyModel
        include Some::ORM
        include Tire::Model::Search

        mapping do
          indexes :field, :type => 'string', :analyzer => 'keyword', :as => [1, 2, 3]
        end
      end
    end

This commit is a conceptual opposite of karmi/retire@0ccee42. Until #446 is decided,
this change brings some consistency to the behaviour.
bbbf372
@karmi
Owner
karmi commented Oct 23, 2012

Guys, see both commits added to this issue:

I'm leaning towards the latter. It is, though, based only on a gut feeling, not some hard-held opinion; I'd rather keep the behaviour open ended then be too strict about it.

/cc @vhyza

@NOX73 NOX73 added a commit to NOX73/tire that referenced this pull request Oct 23, 2012
@karmi @NOX73 + NOX73 [#446] Changed, that arbitrary objects passed in the `:as` mapping op…
…tion are indexed

Previously, objects other then `String` or `Proc` were silently discarded
when evaluating the `:as` mapping option.

With this change, you may pass whatever object you like in the `:as` mapping option:

    class MyModel
        include Some::ORM
        include Tire::Model::Search

        mapping do
          indexes :field, :type => 'string', :analyzer => 'keyword', :as => SomeOtherObject.whatever
        end
      end
    end

    class MyModel
        include Some::ORM
        include Tire::Model::Search

        mapping do
          indexes :field, :type => 'string', :analyzer => 'keyword', :as => [1, 2, 3]
        end
      end
    end

This commit is a conceptual opposite of karmi/retire@0ccee42. Until #446 is decided,
this change brings some consistency to the behaviour.
8934d7c
@NOX73 NOX73 added a commit to NOX73/tire that referenced this pull request Oct 23, 2012
@NOX73 NOX73 Merge branch 'master' into works-br
* master: (36 commits)
  Added test for facet_filter
  Added support for boosting query
  [#472] Added support for loading partial fields
  [#269] Added the `min_score` DSL method
  [#269] Added the `min_score` DSL method
  [TEST] Refactored equality checks in expectations fot the `Index` unit tests
  [#256] Added support for the `parent` URL parameter in `Index#store`
  [REFACTOR] Cleaned up the `Index#store` method
  [#446] Changed, that arbitrary objects passed in the `:as` mapping option are indexed
  Improved support for Kaminari pagination
  Added proper `Results::Collection#slice` implementation
  Added Rake task to generate ENV['COUNT'] records into the template for the Rails example application
  Added `attr_accessible` to Article model in the template for example Rails application
  [FIX] Simplified `method_missing` in `Item`
  [FIX] Implemented `respond_to?` for `Item`
  [FIX] Added, that a warning is displayed when the index cannot be created on class load
  Changed, that "edge" version of Tire is used in the Rails application template
  Exit with an error in the Rake import task when the index as not created in elasticsearch
  Added the ability to pass arguments to the `all` query
  [FIX] [#453] Changed Hash syntax to be Ruby 1.8 compatible in Tire::Model::Persistence::Finders
  ...
fdd8caf
@karmi
Owner
karmi commented Oct 26, 2012

Closing, going with karmi/retire@bbbf372 for the time being.

@karmi karmi closed this Oct 26, 2012
@ches
ches commented Nov 9, 2012

Regardless of the decision on the handling of arbitrary objects that was discussed here, I still think the documentation @mauricio contributed was helpful -- I was just looking for similar myself (not the first time I've needed a reference!), and after going to code where it isn't obvious, I remembered this pull request.

@karmi
Owner
karmi commented Nov 9, 2012

@ches I'm afraid I don't understand what you want Tire to do here. The behaviour is pretty expectable -- when you put as: Time.now, you're "freezing" Time.now and that's it. Of course, it should be properly documented in the code -- but definitely not in the README.

@ches
ches commented Nov 9, 2012

Oh, actually I didn't realize until a second look just now that the possibility of giving a Symbol as an instance method reference was not actually accepted -- otherwise I was going to say there's nothing additional I'm looking for from Tire, just the docs. The Symbol support is a small nicety though IMO, it's a common pattern and a little cleaner than thinking about instance_eval any time it isn't absolutely necessary.

Where would you want such documentation to be? I assume expanding the explanation of :as in the RDoc for indexes on Tire::Model::Indexing?

@ches
ches commented Nov 9, 2012

(If you look at that method, and you're not familiar with Tire's codebase more fully, it's not at all apparent where :as actually gets handled -- that's why I'm being vocal about a wish for more docs).

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