Fix slice specific repository commands from being nil

[bkuhlmann]

Why

Hello. 👋 When using ROM commands within a slice, they end up as nil objects. My initial guess is something is broken with object inheritance because commands work if used with repository objects within the application slice (i.e. the main slice).

How

In my case, I have a Tasks slice which uses ROM commands for create, update, and delete:

Source Code

# frozen_string_literal: true

module Tasks
  module Repositories
    # Defines the tasks repository.
    class Task < DB::Repository
      ORDER = proc { created_at.asc }

      # TODO: Enable once Hanami 2.2.0 supports commands.
      # commands :create, update: :by_pk, delete: :by_pk

      def initialize(order: ORDER, **)
        @order = order
        super(**)
      end

      def find(id) = tasks.combine(:user).by_pk(id).one

      def find_by_description text
        tasks.combine(:user)
             .where { description.ilike "%#{text}%" }
             .to_a
      end

      def all = tasks.combine(:user).order(&order).to_a

      # TODO: Remove once Hanami 2.2.0 supports commands.
      def create(attributes) = tasks.changeset(:create, attributes).commit

      # TODO: Remove once Hanami 2.2.0 supports commands.
      def update(id, **) = tasks.by_pk(id).update(**)

      # TODO: Remove once Hanami 2.2.0 supports commands.
      def delete(id, **) = tasks.by_pk(id).delete(**)

      private

      attr_reader :order
    end
  end
end

Notice that I've disabled (commented) the commands macro in favor of creating the equivalent methods for create, update, and delete. That's the workaround. ...but I'd prefer to use only the commands macro and delete the additional methods.

This appears to work when working within a normal Hanami 2.2.0, Beta 2 application with no slices.

Steps to Recreate

1. Run: https://github.com/bkuhlmann/hemo.
2. Run bin/setup.
3. Open the Task repository object.
4. Uncomment the commands macro and comment the create, update, and delete methods.
5. Run the test suite to see nil exceptions being thrown. You can revert the changes to make the specs pass again.

[bkuhlmann]

ℹ️ This still doesn't work on Hanami 2.2.0, RC1. This seems like a major issue for releasing 2.2.0.

[cllns]

Agree that commands not working for slices is problem.

Can you take an attempt at opening a PR fixing this @bkuhlmann? I hope it's a simple fix since they already work on the main application slice

[bkuhlmann]

OK, I'll drop you a line if I find anything (no promises, got a bunch of irons in the fire).

[wuarmin]

@bkuhlmann I think this should be fixed in hanami/hanami#1478

[bkuhlmann]

Hey Armin, thanks, but I'm afraid I still get the NoMethodError: undefined method by_pk for nil errors when uncommeting the code example provided above and using this Gemfile modification:

gem "hanami", github: "wuarmin/hanami", branch: "fix_slice_repo_root_auto_setting"

My initial glance at your fixes might stem from the fact that you check for constants that end in Repo but I name all of my repositories as Repository. I didn't look too closely but that struct me as potentially being a problem.

[wuarmin]

Hey @bkuhlmann,
thanks for checking. Ok, until now it only worked if the Hanami naming convention (i.e. UserRepo < DB::Repo) was used. But of course Repository should also work. I have adapted the code a little. I think that should fix your error. Can you check again?

thanks

[bkuhlmann]

Hmm, I'm afraid there was no change in behavior with your latest change either. If it helps, I updated the steps to recreate above so they are more clear. You might want to clone my project and run the test to see. You'd only need to make an additional modification to the Gemfile to include this line:

gem "hanami", github: "wuarmin/hanami", branch: "fix_slice_repo_root_auto_setting"

Here's the output from the test suite as well:

RSpec Output

Tasks::Repositories::Task
  #all
    answers empty array when records don't exist
    answers all records
  #find_by_description
    answers empty array unknown description
    answers record by description
  #find
    answers nil for unknown ID
    answers record by ID

Home
  renders home page

Tasks::Actions::Show
  #call
    answers 200 OK status with valid parameters
    answers 422 Unprocessable Entity with invalid parameters

Tasks::Views::Parts::Task
  #assignee
    answers user
  #checked
    answers nil when not completed
    answers checked when completed
  #css_class
    answers completed when completed
    answers default class when not completed

Tasks::Actions::New
  #call
    answers 200 OK status

Tasks::Views::Scopes::Description
  #message
    answers nil when error description doesn't exist
    answers error message when error description exists
  #value
    answers value

Tasks::Actions::Patch
  #call
    answers 200 OK status with ID and complete date/time (FAILED - 1)
    answers 200 OK status with ID only (FAILED - 2)
    answers errors with no ID

Hemo::Refines::Actions::Response
  #with
    answers response with given body and status
    answers itself

Tasks::Actions::Edit
  #call
    answers 200 OK status with valid parameters
    answers errors with invalid parameters

Tasks::Actions::Delete
  #call
    answers 422 Unprocessable Entity with invalid parameters
    answers 200 OK status and blank body with valid parameters (FAILED - 3)

Tasks::Actions::Create
  #call
    answers 200 OK status with valid parameters (FAILED - 4)
    answers errors with invalid parameters

Tasks
  displays no tasks when there are none
  displays task
  successfully destroys a task (FAILED - 5)
  is unable to edit a task with errors
  returns to index when canceling new task
  successfully edits a task (FAILED - 6)
  successfully creates a new task (FAILED - 7)
  successfully completes a task (FAILED - 8)
  fails to create task with errors

Tasks::Actions::Update
  #call
    answers 200 OK status with user ID, description, and completed (FAILED - 9)
    answers 200 OK status with user ID and description only (FAILED - 10)
    answers errors with missing description
    answers error when given ID only

Tasks::Actions::Index
  #call
    answers no HTMX response headers when search query is filled
    answers 200 OK status
    answers HTMX Push URL response header when search query is blank

Health::Actions::Show
  #call
    answers green background

Tasks::Views::Show
  #call
    renders view

Failures:

  1) Tasks::Actions::Patch#call answers 200 OK status with ID and complete date/time
     Failure/Error: response = action.call id: task.id, task: {completed: "2023-01-27T22:50:37+00:00"}

     NoMethodError:
       undefined method `by_pk' for nil
     # ./slices/tasks/actions/patch.rb:37:in `complete'
     # ./slices/tasks/actions/patch.rb:27:in `handle'
     # ./spec/slices/tasks/actions/patch_spec.rb:17:in `block (3 levels) in <top (required)>'

  2) Tasks::Actions::Patch#call answers 200 OK status with ID only
     Failure/Error: response = action.call id: task.id

     NoMethodError:
       undefined method `by_pk' for nil
     # ./slices/tasks/actions/patch.rb:42:in `uncomplete'
     # ./slices/tasks/actions/patch.rb:28:in `handle'
     # ./spec/slices/tasks/actions/patch_spec.rb:12:in `block (3 levels) in <top (required)>'

  3) Tasks::Actions::Delete#call answers 200 OK status and blank body with valid parameters
     Failure/Error: response = action.call id: Factory[:task].id

     NoMethodError:
       undefined method `by_pk' for nil
     # ./slices/tasks/actions/delete.rb:18:in `handle'
     # ./spec/slices/tasks/actions/delete_spec.rb:10:in `block (3 levels) in <top (required)>'

  4) Tasks::Actions::Create#call answers 200 OK status with valid parameters
     Failure/Error: response = action.call task: {user_id: user.id, description: "Test"}

     NoMethodError:
       undefined method `command' for nil
     # ./slices/tasks/actions/create.rb:25:in `handle'
     # ./spec/slices/tasks/actions/create_spec.rb:12:in `block (3 levels) in <top (required)>'

  5) Tasks successfully destroys a task
     Failure/Error: arity = root.__send__(view_name).arity

     NoMethodError:
       undefined method `by_pk' for nil
     # ./slices/tasks/actions/delete.rb:18:in `handle'
     # ------------------
     # --- Caused by: ---
     # Capybara::ExpectationNotMet:
     #   expected not to find text "A test task" in "Home | Tasks\nClear\nA test task\nJane Doe\nEdit\nDelete\nNew"
     #   ./spec/features/tasks_spec.rb:87:in `block (2 levels) in <top (required)>'

  6) Tasks successfully edits a task
     Failure/Error: arity = root.__send__(view_name).arity

     NoMethodError:
       undefined method `by_pk' for nil
     # ./slices/tasks/actions/update.rb:46:in `save'
     # ./slices/tasks/actions/update.rb:35:in `handle'
     # ------------------
     # --- Caused by: ---
     # Capybara::ExpectationNotMet:
     #   expected to find text "An edited task" in "Home | Tasks\nClear\nJane Doe\nCancel\nNew"
     #   ./spec/features/tasks_spec.rb:68:in `block (2 levels) in <top (required)>'

  7) Tasks successfully creates a new task
     Failure/Error: root.command(type, **opts).call(*input)

     NoMethodError:
       undefined method `command' for nil
     # ./slices/tasks/actions/create.rb:25:in `handle'
     # ------------------
     # --- Caused by: ---
     # Capybara::ExpectationNotMet:
     #   expected to find text "This is a test" in "Home | Tasks\nClear\nDescription\nOwner\nJane Doe\nCancel\nNew"
     #   ./spec/features/tasks_spec.rb:30:in `block (2 levels) in <top (required)>'

  8) Tasks successfully completes a task
     Failure/Error: arity = root.__send__(view_name).arity

     NoMethodError:
       undefined method `by_pk' for nil
     # ./slices/tasks/actions/patch.rb:37:in `complete'
     # ./slices/tasks/actions/patch.rb:27:in `handle'
     # ------------------
     # --- Caused by: ---
     # Capybara::ElementNotFound:
     #   Unable to find css ".task-completed"
     #   ./spec/features/tasks_spec.rb:58:in `block (2 levels) in <top (required)>'

  9) Tasks::Actions::Update#call answers 200 OK status with user ID, description, and completed
     Failure/Error:
       response = action.call id: task.id,
                              task: {user_id: user.id, description: "Test", completed: "on"}

     NoMethodError:
       undefined method `by_pk' for nil
     # ./slices/tasks/actions/update.rb:46:in `save'
     # ./slices/tasks/actions/update.rb:35:in `handle'
     # ./spec/slices/tasks/actions/update_spec.rb:13:in `block (3 levels) in <top (required)>'

  10) Tasks::Actions::Update#call answers 200 OK status with user ID and description only
      Failure/Error: response = action.call id: task.id, task: {user_id: user.id, description: "Test"}

      NoMethodError:
        undefined method `by_pk' for nil
      # ./slices/tasks/actions/update.rb:46:in `save'
      # ./slices/tasks/actions/update.rb:35:in `handle'
      # ./spec/slices/tasks/actions/update_spec.rb:20:in `block (3 levels) in <top (required)>'

Finished in 8.64 seconds (files took 1.01 seconds to load)
47 examples, 10 failures

Failed examples:

rspec ./spec/slices/tasks/actions/patch_spec.rb:16 # Tasks::Actions::Patch#call answers 200 OK status with ID and complete date/time
rspec ./spec/slices/tasks/actions/patch_spec.rb:11 # Tasks::Actions::Patch#call answers 200 OK status with ID only
rspec ./spec/slices/tasks/actions/delete_spec.rb:9 # Tasks::Actions::Delete#call answers 200 OK status and blank body with valid parameters
rspec ./spec/slices/tasks/actions/create_spec.rb:11 # Tasks::Actions::Create#call answers 200 OK status with valid parameters
rspec ./spec/features/tasks_spec.rb:81 # Tasks successfully destroys a task
rspec ./spec/features/tasks_spec.rb:61 # Tasks successfully edits a task
rspec ./spec/features/tasks_spec.rb:21 # Tasks successfully creates a new task
rspec ./spec/features/tasks_spec.rb:53 # Tasks successfully completes a task
rspec ./spec/slices/tasks/actions/update_spec.rb:11 # Tasks::Actions::Update#call answers 200 OK status with user ID, description, and completed
rspec ./spec/slices/tasks/actions/update_spec.rb:18 # Tasks::Actions::Update#call answers 200 OK status with user ID and description only

Randomized with seed 61946

The only spec failures are with the Tasks::Repositories::Task object when enabling the commands macro.

[wuarmin]

@bkuhlmann oh, I see, I hadn't looked at your code that closely, I just experienced the same behavior. Of course, you completely omit the term “Repo” or “Repository” in your class name.

Then I would recommend adapting the repo-class-name convention:

module Tasks
  module Repositories
    class TaskRepo < DB::Repository # or TaskRepository
    end
  end
end

or to set the root explicitly:

module Tasks
  module Repositories
    class Task < DB::Repository[:tasks] # set root explicitly
    end
  end
end

I would adapt my repo-class-name convention.
WDYT?

[bkuhlmann]

Ah, yes, using DB::Repository[:tasks] definitely resolves the issue but this feels...wrong because I would expect Tasks::Repositories::Task to be able to inherit from DB::Repository without having to break outside of Hanami so-to-speak and be ROM specific by using [:tasks] via the .[] method. In truth, this is one of the downsides of using the Module Builder Pattern because you can't ask the current class if it's a subclass of Hanami::DB::Repo. To clarify, if I put a breakpoint in the middle of my Tasks::Repositories::Task class and ask for the ancestry (i.e. self.class.ancestors) I'll get the following:

Ancestors

[Tasks::Repositories::Task,
 ROM::Repository::RelationReader::InstanceMethods,
 #<ROM::Repository::RelationReader:0x0000000129a9af18>,
 Dry::Initializer::Mixin::Local[Tasks::Repositories::Task],
 #<Class:0x0000000128e33e00>,
 Dry::Initializer::Mixin::Local[#<Class:0x0000000128e33e00>],
 Tasks::DB::Repository,
 Dry::Initializer::Mixin::Local[Tasks::DB::Repository],
 Hemo::DB::Repository,
 Dry::Initializer::Mixin::Local[Hemo::DB::Repository],
 Hanami::DB::Repo,
 Hanami::Extensions::DB::Repo,
 Dry::Initializer::Mixin::Local[Hanami::DB::Repo],
 ROM::Repository,
 ROM::Initializer::InstanceMethods,
 Dry::Initializer::Mixin::Root,
 Dry::Initializer::Mixin::Local[ROM::Repository],
 Dry::Core::Cache::Methods,
 Object,
 PP::ObjectMixin,
 JSON::Ext::Generator::GeneratorMethods::Object,
 DEBUGGER__::TrapInterceptor,
 Kernel,
 BasicObject]

You'll notice the 0x0000000* objects in the ancestry. Those are instances, not classes, which means you can't use self.class.is_a? Hanami::DB::Repo because the answer will always be false. If this would answer true, then your patch could be vastly simplified -- if I understand correctly -- because you should only care if my Tasks::Repositories::Task class is a descendant of Hanami::DB::Repo?

In summary, I think fixing this so people can think in terms of normal inheritance would be the least surprising behavior because the new Hanami DB gem is already wrapping and attempting to mask ROM behavior. One way to do this -- albeit not entirely elegant -- is to ask if the current object (my class in this case) can respond (i.e. #respond_to?) to a method that is specific to the superclass. Doing that means your patch would work. I think.