Skip to content
This repository has been archived by the owner on Jun 30, 2018. It is now read-only.

Fix the issue with a single object vs array being returned #725

Closed
wants to merge 2 commits into from

Conversation

SixiS
Copy link
Contributor

@SixiS SixiS commented May 8, 2013

If you use load: true when the search would return a single object it raises the error
NoMethodError:
undefined method `detect' for #<--elastic search persistance model-->

This has been fixed before but it must have been lost when the code was refactored.
See #353

Original issue:
http://stackoverflow.com/questions/10592005/tire-gem-undefined-method-detect

@SixiS
Copy link
Contributor Author

SixiS commented May 9, 2013

Passes locally.

807 tests, 807 passed, 0 failures, 0 errors, 0 skips, 1827 assertions

@@ -136,7 +136,7 @@ def __get_results_with_load(hits)
"based on _type '#{type}'.", e.backtrace
end

records[type] = __find_records_by_ids klass, items.map { |h| h['_id'] }
records[type] = [*(__find_records_by_ids klass, items.map { |h| h['_id'] })]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about Array(__find_records_by_ids klass, items.map { |h| h['_id'] })) instead?

@karmi
Copy link
Owner

karmi commented May 11, 2013

That might happen, I guess. Would it be possible to add a unit/integration test demonstrating the issue?

@SixiS
Copy link
Contributor Author

SixiS commented May 12, 2013

Oh awesome, didn't know the Array constructor worked like that.
Added some tests to show the problem. Added integration tests for active model and persisted model.
It only breaks using the persisted model.

test/integration/persisted_model_test.rb:47 is the test that should break if you undo the fix.

@karmi karmi closed this in 44eda24 May 12, 2013
@karmi
Copy link
Owner

karmi commented May 12, 2013

@SixiS Yeah, the Array() method is really convenient .) I polished the patch a bit, and in fact wasn't able to produce a failing test for ActiveRecord in reasonable time. But based on the way you describe the behaviour, I can see what goes wrong and the Array() wrapper really shouldn't hurt anything.

BTW, I have removed the load: true test in the persistence test, since it does not make any sense -- the persistence model is always loaded from Elasticsearch. I have changed the wording a bit to make the test cases clearer.

@SixiS
Copy link
Contributor Author

SixiS commented May 12, 2013

Great, thanks! The active record test was not failing, I was having the issue using the persistence model.

If you do not use load: true with the persistence model, methods on the returned objects return nil. If you do use load :true before the array fix you got the detect problem.

Tho maybe the fact that the methods return nil and not what you expect is another problem on its own.
It only occurs when using Tire.search, items returned by Model.search work fine if you call methods on them.

I think it is because the objects returned are not real instances of their models but a sort of casted Item model.

Thanks for taking the time to close this issue!

@karmi
Copy link
Owner

karmi commented May 12, 2013

So, you're using Tire.search against an index eg. "my_documents" which is handled by MyDocument model which is a Tire::Model::Persistence? I guess I have never tried that, so this might be a missing test :)

@SixiS
Copy link
Contributor Author

SixiS commented May 12, 2013

Exactly 😄

We are using Tire.search for more complicated searches on the persisted indexes (multiple index and multiple search). Which is where we were running into the 'no method detect' problem because we wanted to use load: true.

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

Successfully merging this pull request may close these issues.

None yet

2 participants