Introducing Hanami::Repository#query

[jodosha]

The Problem

Until hanami-model 0.6, all the queries are private.
This was a strong guidance for developers to write intention revealing methods in repositories instead of leaking storage abstractions all over the code base.

It avoided code like ArticleRepository.where(state: 'draft') to be used directly in actions code.
Developers were forced to write a meaningful method ArticleRepository.drafts to get access to the data they were interested into.

This has the huge downside of being not convenient for testing code and Hanami console.

I found myself to define over and over methods in repositories only for testing purposes.

RSpec.describe ArticleRepository do
  it "creates draft articles" do
    repository = described_class.new
    draft      = repository.create_draft(title: "Introducing Hanami")

    drafts = repository.drafts_by_title("Introducing Hanami")
    expect(drafts).to include(draft)
  end
end

class ArticleRepository < Hanami::Repository
  def create_draft(data)
    # ...
  end

  # NOTE: this is defined only for testing purposes.
  def drafts_by_title(title)
    # ...
  end
end

As the time passed, half of the methods defined in repositories were defined because of testing purposes (like #drafts_by_title) in the example above.

The Solution

This PR introduces Repository#query (aliased as #q) to allow developers to directly access the database relation and build inline queries.

The documentation warns developers to use inline queries only for testing purposes and Hanami console.

Let's rewrite the testing code above:

RSpec.describe ArticleRepository do
  it "creates draft articles" do
    repository = described_class.new
    draft      = repository.create_draft(title: "Introducing Hanami")

    drafts = repository.q.where(state: 'draft').where(title: "Introducing Hanami")
    expect(drafts).to include(draft)
  end
end

We don't need to define #drafts_by_title in that repository anymore.

---

/cc @hanami/core-team @hanami/contributors @solnic @flash-gordon

[AlfonsoUceda]

use q instead query?

[jodosha]

You're right, thank you!

[AlfonsoUceda]

all perfect @jodosha it makes sense that change. could you take a look that comment?

[pascalbetz]

Why add an alias? Having a q method does not make things clearer IMHO. Aliases just increase the API size.

[jodosha]

@pascalbetz I consider it a handy shortcut for Hanami console. WDYT?

[solnic]

What about using relations from a repo directly? They simply return data back as structs so should be enough for tests and console

[jodosha]

@solnic Do you mean to avoid as(:entity)? If so, why?

[beauby]

I think this is dangerous as it will certainly be abused. I definitely understand the benefits in the console but as far as tests are concerned, I think one should not test stuff that is not exposed via a clean API (in that case, is it really relevant that the draft lives in the database?).

[jodosha]

@beauby

I think this is dangerous as it will certainly be abused.

Yes, I understand the risk.

I think one should not test stuff that is not exposed via a clean API

This isn't always true. Again in my projects half of methods in repositories were defined because of testing purposes. This is especially true for complex commands that create multiple records at once.

I definitely understand the benefits in the console

How can we solve this problem?

[pascalbetz]

@jodosha i prefer unique names for things, if I could i would get rid of aliases in ruby (map / collect...) 💣 😄
I don't think that there is a gain in having q (which does not communicate the purpose, it could as well be quit) over query.

Other solutions:

You can of course rename the method from query to do_not_use_this_in_production but i doubt this would help 😄

But more seriously:
For your console testing you could of course use

repo.send(:query)
// or even shorter, i would not mind the alias if it is not in the public API
repo.send(:q)

to access the query...this would clearly say "this is private API, do not use".

[cllns]

What about naming the method private_query, test_query, console_query, dev_query, secret_query something like that?

[jodosha]

This requires more discussion, I'd keep out from 0.7.0 milestone.

[AMHOL]

@jodosha how about having a method on Hanami::Model that will expose this method (and possible other candidates in the future), i.e.

Hanami::Model.enable_test_mode!

I see that you already have descendant tracking via Hanami::Model.repositories so it should be pretty easy to do

[kucaahbe]

if one needs to write methods just for test purposes, this probably could mean either:

• API is not good enough prepared to be test-able
• incorrect testing strategy was selected

In case, described in description of this PR it is not clear (personally to me) why not

RSpec.describe ArticleRepository do
  it "creates draft articles" do
    repository = described_class.new
    draft      = repository.create_draft(title: "Introducing Hanami")

    drafts = repository.drafts
    expect(drafts).to include(draft)
  end
end

(following original style of test-case)

And btw, it seems (to me), if one needs query interface as public, than why choose repository-pattern based solution instead of active record or smth. like that?

[TiteiKo]

A little late to the party, most of what I'd say has already been said, so to make it really short:

• in the codebase, such a method should not be used. (codebase including test files)
• what happens in the console stays in the console, so it would be ok to use it there
• using this method (whatever its name or scope) should generate a textual warning to discourage people to use it anywhere in the codebase.

[jodosha]

@TiteiKo I like your approach. How would you implement the first two points?

[beauby]

@jodosha You could have your $ hanami console command automatically require a set of helpers that are not required from apps.

[pascalbetz]

Either expose it or not. If it is exposed, then it's public API and people will and should use it. If you need to go such great lengths to expose something but only in the console/development then this is an indicator that the API is flawed.

[beauby]

@pascalbetz I disagree – the console should be used mainly for debugging purposes, and thus should come equipped with all possible tools to inspect data, but those tools should not be available from within the application.

[pascalbetz]

@beauby my 5 cents:

myobject.send(:something_that_you_do_not_want_to_expose).do_something_you_should_not_do_on_production

problem solved. A bit longer but hardly a problem. No explanation required why you have a method but should not use it.

[beauby]

@pascalbetz I agree – I was more thinking of a method that would be added to Repository only in the console environment (though arguably, it does not provide much value). The idea of a method that is public but that one should not use does not make much sense to me either.

[pascalbetz]

Have some candy. But don't eat it. Will go wrong:-)

[jodosha]

@beauby What about testing code? What's your opinion on that?

[jodosha]

@pascalbetz In other words, do you want to introduce query, but as a private method?

[beauby]

@jodosha My opinion on testing is that one should not test stuff that is not exposed, because one should test behavior rather than implementation. I may be missing the point, but I'd like to see a situation where using query for tests is not simply a symptom of the underlying code being in need of a refactor.

[solnic]

Just like in case of #351 issue - you can use relations, they are the interface for talking to a database, and they are public in repositories. It is up to you where you're OK with having them leak into your app or test code. Also, if you don't like to use "raw" data in the form of rom structs, you can always do as(:entity) which is provided by hanami-model. Having a query method feels superfluous.

[pascalbetz]

@jodosha not necessarily. I just don't like to have a public interface that is not really public (only console...). If you need to get access to the internals for debugging/testing/playing around you can do so with send

[jodosha]

This seems to be controversial, until we find a good solution, I'm closing this. 😄