-
Couldn't load subscription status.
- Fork 808
[MODEL] Support for querying multiple models #345
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
Conversation
Hello Miguel, thanks for the patch, it looks very promising!
I went over the code a bit, and I like the approach, naming and implementation. I'm starting a two week vacation now, however, so I'd like to get back to it later, and evaluate it in more detail. Is that OK for you? Can you ping me in two weeks if this gets buried under hundreds of other tasks and e-mails after I return? :)
I have one plea, actually -- your patch modifies a lot of source code locations, some are unrelated to the feature (like fixing the lograge test). Would it be possible for you to do git reset master (that's a soft reset, no files will be changed), and then commit logical chunks of the feature? Then you can just force push git push --force to the multimodel-support branch, and the pull request will be automatically updated.)
If this would be too complicated, no problem, I can always do it by myself.
Of course @karmi, I will undo rebasing and commit again the changes.
And in relation to the timings, no problem. I know you've been busy with the query DSL last week, so don't worry. I will comment on the issue in a couple of weeks if I don't have any news from you.
@miguelff Perfect, thanks!
Done splitting PR into meaningful commits
Registry is not an array Modify _types to only iterate to necessary models
juankiz
commented
Mar 31, 2015
This PR really helps me +1
Thanks for the hard work @miguelff
@karmi Hope your holidays went well 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the check for ancestors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multimodel.new is an instance of Multimodel, and not an instance of Class, hence it doesn't respond to ancestors. An alternative approach will be that multimodel would implement:
def ancestors [] end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or let Multimodel be an instance of class, which I found it a bit tricky.
👍 Thank you @karmi 🌈
...cord_associations.rb` Related: #345
Msadek02
commented
Apr 5, 2016
@karmi @miguelff - I dont see anything specific in the documentation for how to filter on multiple models
So for example i want to do something like
Elasticsearch::Model.search(params[:search], [Conversation, Message]).results.to_a.map(&:to_hash) => [{"_index"=>"conversations", "_type"=>"conversation", "_id"=>"20", "_score"=>0.6684941, "_source"=>{"id"=>20, "subject"=>"This is my subject", "created_at"=>"2016-03-30T18:55:55.000-04:00"}}, {"_index"=>"conversations", "_type"=>"conversation", "_id"=>"21", "_score"=>0.6684941, "_source"=>{"id"=>21, "subject"=>"Another subject ", "created_at"=>"2016-03-30T18:57:01.000-04:00"}}] # I want to do something like this Elasticsearch::Model.search(params[:search], [Conversation, Message], {term: {id: "21"}}).results.to_a.map(&:to_hash) # which should only return this object [ {"_index"=>"conversations", "_type"=>"conversation", "_id"=>"21", "_score"=>0.6684941, "_source"=>{"id"=>21, "subject"=>"Another subject ", "created_at"=>"2016-03-30T18:57:01.000-04:00"}}]
Is that possible to do?
Is that possible to do?
As per this line, options are being forwarded to the search request. Then, they will merged with search definition provided (params[:search] in your example) and then run against Elasticsearch.
So, yes, it should be possible, although is not a behavior that has been properly tested.
This PR adds support for multiple models search, as discussed in:
A previous attempt of implementing this was done in:
But although the PR is open, it was rejected mainly due to:
#resultswas part of the multimodel-search response,#recordswas not.This PR tries to address the problems outlined above, and also implement a performant hit-to-record deserialization.
Thank you very much to @AaronRustad for his PR, which motivated this one.
Example usage:
NOTE: It is not possible to chain other methods on top of the
recordsobject, since itis a heterogenous collection, with models potentially backed by different databases.