[STORE] Refactor Repository as mixin

[estolfo]

This pull request offers an alternative refactor to this pull request of Elasticsearch::Persistence::Repository. It provides a mixin that users can include to get Repository behavior on classes.

example:

class MyTestRepository
  include Elasticsearch::Persistence::Repository
  client DEFAULT_CLIENT
end

repo_instance = MyTestRepository.new(index_name: 'my_test_repository', document_type: 'test')
other_repo_instance = repo_instance.with(index_name: 'other_test_repository')
repo_instance.index_name != other_repo_instance.index_name
repo_instance.document_type == other_repo_instance.document_type
repo_instance.client == other_repo_instance.client
repo_instance.client == MyTestRepository.client

[jits]

(Following on from the discussion on #822)

This looks great! :) Appreciate you going to the effort to put this together.

Some questions / thoughts:

1. Given you can pass in options to each new instance, to configure just that instance, do you still need the class level settings (like index_name)? I can see how they provide added flexibility (to configure defaults at the class level), so this isn't a big deal (in my opinion), but just something that consumers need to be aware of (especially as you also have defaults for these class level options, that get used if a value is not provided at all by the consumer, instead of erroring).
2. Is there much need for the .with helper? Don't get me wrong, it does seem like a useful helper, but given that it does a shallow .clone it's not certain that you'll get a completely isolated instance. Instead I'd advocate for consumers creating a fresh new instance when they need and managing common settings themselves (as with regular classes and objects).
3. Alternatively to the above point, if a helper is really needed to instantiate a similar repo, you could have a class level helper method MyRepo.from(my_repo_instance, new_options) which just returns MyRepo.new(my_repo_instance.options.merge(new_options))?
4. This covers nicely the need for flexibility in creating different – and isolated – repository instances that could talk to different clusters and/or indexes (with the caveat of the comment above); I'd still suggest having a GenericRepository class that can be used to instantiate a very basic repository without having to extend anything. This is certainly not crucial, and more a "nice-to-have", but for a use case that I think is most common.

I suspect my comments above are because I don't fully understand how the different pieces interact (e.g. the Store and Elasticsearch::Model::Indexing::ClassMethods modules), so apologies if I'm off the mark with my comments.

[estolfo]

This looks great! :) Appreciate you going to the effort to put this together.

@jits Thanks again for all your feedback! It's really nice of you to take the time to look at the pull requests : )

Some questions / thoughts:

1. Given you can pass in options to each new instance, to configure just that instance, do you still need the class level settings (like index_name)? I can see how they provide added flexibility (to configure defaults at the class level), so this isn't a big deal (in my opinion), but just something that consumers need to be aware of (especially as you also have defaults for these class level options, that get used if a value is not provided at all by the consumer, instead of erroring).

There are two main reasons for having the class-level settings. The first is so that you can use the DSL to set up the repository, for example:

class NoteRepository
  include Elasticsearch::Persistence::Repository

  document_type 'note'
  index_name 'notes_repo'

  settings number_of_shards: 1, number_of_replicas: 0 do
    mapping dynamic: 'strict' do
      indexes :foo do
      indexes :bar
    end
    indexes :baz
  end
end

The second is, create_index!, mappings, settings, etc are implemented in this module called ClassMethods. I would actually rather not provide class-level methods and settings and could include the ClassMethods module to the Repository mixin so that they are only instance methods; however, then you wouldn’t have any of the DSL methods you see above and you’d have the awkwardness of including ClassMethods that are, in fact, instance methods. I’m not sure how important it would be to users to be able to set the options using the DSL. What do you think?

1. Is there much need for the .with helper? Don't get me wrong, it does seem like a useful helper, but given that it does a shallow .clone it's not certain that you'll get a completely isolated instance. Instead I'd advocate for consumers creating a fresh new instance when they need and managing common settings themselves (as with regular classes and objects).

This is a good point. I provided the #with helper to keep instances immutable and to satisfy anyone who would still want a kind of “mutability” of repository instances. It doesn’t matter to me, so for simplicity, we can just take #with out and if someone absolutely needs to spawn repository instances from other repository instances, I can add it later.

1. Alternatively to the above point, if a helper is really needed to instantiate a similar repo, you could have a class level helper method MyRepo.from(my_repo_instance, new_options) which just returns MyRepo.new(my_repo_instance.options.merge(new_options))?
2. This covers nicely the need for flexibility in creating different – and isolated – repository instances that could talk to different clusters and/or indexes (with the caveat of the comment above); I'd still suggest having a GenericRepository class that can be used to instantiate a very basic repository without having to extend anything. This is certainly not crucial, and more a "nice-to-have", but for a use case that I think is most common.

I’d like to encourage users to create their own Repository classes so that they can be explicit about what settings they want for it. I don’t see a huge benefit of having a generic repository class, when you can just write class BaseRepository; include Elasticsearch::Persistence::Repository; end

I suspect my comments above are because I don't fully understand how the different pieces interact (e.g. the Store and Elasticsearch::Model::Indexing::ClassMethods modules), so apologies if I'm off the mark with my comments.

I think I'll try out some changes discussed above and see how it works out.

Update:
I've removed the #with method in this commit.
I disallow administrative index methods at the class-level on the Repository in this commit. I still allow configurations like client, mapping, settings, index_name, to be defined with the DSL. Basically, anything that actually makes a request to Elasticsearch must be called on a Repository instance.

[jits]

Hi @estolfo,

[…] I’m not sure how important it would be to users to be able to set the options using the DSL. What do you think?

Personally, I don't think it's useful to have this class level DSL to configure repositories, as I prefer the flexibility and predictability of configuring them when instantiating, at runtime. But then, others might find this DSL approach useful and simpler to manage, so I can't say definitively.

I understand now that you're building on top of the Elasticsearch::Model indexing class methods mixin – do you think this could be considered to be a bit of a code smell? I say this because it's mixing the pattern for models – where class level config seems to make much more sense (like in ActiveRecord) – with the repository pattern, which is purely a wrapper abstraction on top a collection of models, without dealing with the model classes themselves. Again, this feature may not necessarily be considered a bad thing, it's just that it sounds like a few hoops need to be jumped through to achieve what you'd like with repositories, when using that mixin. To be honest, I don't see any particular logic in that mixin that's useful to this repository pattern, but again, this DSL approach might be favoured by others, so how about offering a separate mixin that the consumer can use explicitly if they need this DSL? E.g.:

class MyRepository
  include Elasticsearch::Persistence::Repository
  include Elasticsearch::Persistence::Repository::ConfigDSL

  index_name :foo
  …
end

… the methods in Elasticsearch::Persistence::Repository would then check for the presence of these class level options and fall back to them if available.

[…] It doesn’t matter to me, so for simplicity, we can just take #with out and if someone absolutely needs to spawn repository instances from other repository instances, I can add it later.

I've removed the #with method in this commit.

Great! Agreed :)

I’d like to encourage users to create their own Repository classes so that they can be explicit about what settings they want for it. I don’t see a huge benefit of having a generic repository class, when you can just write class BaseRepository; include Elasticsearch::Persistence::Repository; end

Yeah, that's fair – it's easy enough to do this in your own codebase, and means you can extend functionality in your particular repository as you please. Having a BaseRepository would purely be a convenience factory.

[estolfo]

Hi @jits

I understand now that you're building on top of the Elasticsearch::Model indexing class methods mixin – do you think this could be considered to be a bit of a code smell?

Yes, this is definitely not ideal but in order to keep this refactor manageable and modular, I am ok with having the awkwardness for now. I can always go back and decouple the two gems later in a non-major version, as long as the API remains constant. For now, I do like your suggestion of having the methods be kept in their own DSL mixin. That way, I can isolate the less ideal link between the two gems in one place and refactor it later, if I need to.

Having a BaseRepository would purely be a convenience factory.

I'm not opposed to adding one, but I'd rather start as simple as possible and add functionality and convenience if people ask for it. A base repository would be easy and no problem to add in a minor version.

Thanks again!

[estolfo]

@jits Just FYI, I've merged this and released 6.0.0.pre if you'd like to try it out. Thanks!

[jits]

@estolfo – awesome! Thanks for the heads up. I'm hoping to upgrade one of our apps to ES 6.x in the foreseeable future, so may get a chance to play with this then.

[estolfo]

@jits The gems will most likely work with ES 5 as well, but I'd recommend sticking with your test environment.