Hi Aaron, thanks for looking into this. However, I don't really understand how it approaches the pitfalls I've outlined in the linked discussion...

That really isn't helped by the lack of documentation and integration tests :) I'll try to return to this as soon as I'm done with the work on the persistence features.

I should have stated that I was looking for feedback and there was (likely) more work needed.

I read the linked discussion several times, mainly to see if this is a feature that made sense as part of the gem.

The main pitfalls seems to be the concern around the complexities of returning records and magic behaviour. I think what I've put in place is really just a minor improvement of your original code. The code I'm suggesting avoids those concerns with the improvement being that it isn't monkey-patching Elasticsearch::Model.

If I'm going in the wrong direction on this, I'm cool with that. I can either abandon this or modify if you have additional suggestion or comments.

And yea, documentation and integration tests will/would be added. ;-)

Right, so I think there are two main principles in play here:

1. The feature by itself is very much desired by the users, and we should have it,
2. while preventing incorrect user expectations at all costs.

So, the majority work is actually not in writing the Ruby code itself -- as is the surprising reality observable in many other tickets and features of this library :) The majority work is in playing with these sketches in some real-world environment, trying different approaches, writing instructive integration tests, writing documentation, ...

I really do appreciate that you've picked it up from the sketch in #10 -- if you have the time and energy to continue, let's start adding documentation, code annotations, tests, and review the feature on the fly!

Just a starter for the discussion, let's figure out how to approach the fact that you can't call response.records on the multi-model search response. Should we raise exception? Should we investigate if it would be actually doable if we refactored the adapters? Will it work for both Activerecord and Mongoid?

👍 will dedicate more time and ideas very soon.

Has anyone looked into maybe implementing multi-model with the persistence gem? It seems like it might be more straighforward: 1.) Setup the 2 models to use one index, 2.) Setup a repository model to read from said index, and make a Struct-like class to deserialize the composite data?

So I got around to adding some documentation, integration tests, and I expanded the unit tests.
I thought I'd start with just simply throwing the exception if records is called on a response that originated from a multiple model search.

Seems like there is some interest in this functionality, even if it doesn't return actual database records.

I think we should investigate the option getting records to work. Seem reasonable that that functionality should be there.

Refactoring the adapters may be needed, but I wonder if we can create a MultipleAdapter that wraps actual ActiveRecord or Mongoid adapters. I'm not sure how it'll work with mongoid (never worked with it).

Thoughts appreciated.

Hi @bloodycelt, I'm not very experienced with the persistence gem and I've only just read the documentation. The multi-search feature that's being considered in this PR would allow searching across multiple indexes/types, and it's not concerned about the organizational structure of the indexes/mappings, other than understanding the index name and what types are being searched for.

@AaronRustad yeah, just a thought I had... I'm using MultiSearch to currently return an openstruct, as I sort of see the returned objects as composites.

However I grab the original object when needed by indexing the class name from the model.
I would say records should check if _type can be constantized and use that to initialize the record, and if it does not, then it should raise an error.

@AaronRustad Great!, will have a look.

@bloodycelt Agreed, the multi-model search is something useful to the persistence gem as well; it does already what you suggest, ie. trying initialize the object based on the _type of the document

I gave this some thought overnight, and I wonder if refectoring adapters would really be necessary. A general response that wraps the individual model responses might be good, leaving the Adadapter to remain unchanged.

response = Elasticsearch::Model.search('fox', [Book,Author])
# Calling records on the response is what you'd expect
response.records
# => [Book, Author ...]
# Calling ActiveRelation operations would be difficult. Some models won't have given attributes
response.records.where(isbn: 12345)
# => NotImplementedError: Method not implemented for multiple model search
# We can be specific about the model groups we want returned 
response.book_records
# => [Article, ...]
# We can specify ActiveRelation operations that are specific to the model
response.book_records.where(isbn: 12345)
response.author_records.where(last_name: "Hrabal")

It would also be feasible to return multiple models that use different adapters. It likely wouldn't happen too often, but a search across multiple models might return ActiveRecord & Mongoid models.

I'm not going to have time I was hoping to continue on with this for the next month. I'll gladly continue then, but if anyone else wants to take over now, I'm cool with that.

The PR as it stands now adds value. It provides Multi type search returning results (which is something I would actually use in my production app today), but it does not support records.

AR

It's all fine, Aaron. Whoever needs a multi-model search can just patch their code with your branch's codebase (it's one file after all), come back here with feedback, etc. I myself want to keep focus on the persistence gem right now, as soon as that is out of the door, I can of course return to this patch and do the necessary retouching/fixups. Thanks!

I'm going to close this, at least for now. Unfortunately my focus has been shifted to other priorities.

@AaronRustad I think it's fine to keep it open :) It's easier for people to find it, and it's an important feature.

OK, maybe it'll encourage me to revisit it earlier if other people get involved... I just didn't want to give anyone false hope. :-P

@AaronRustad No worries, really, and thanks for the effort! It's reasonably easy for people to work around this, and over time, we'll get this right and solved.

@karmi @AaronRustad hey guys - I've been reading up on issue 10, the related code, and this PR - we are going to need this functionality, so we'll start by hacking this in - but would love to find out what is needed to help finalize this and get it merged in.

As for the .records question above, I think we should just raise with "NotImplemented" or something, add a nice message explaining it was a multi-model search, and worry about that down the road. Most people that want this would probably understand the sacrifice and be ok with just handling raw json or whatever the response object is.

Happy to see this closed and and the functionality added via a separate PR. Nice work.