rom-rb / rom

Data mapping and persistence toolkit for Ruby
https://rom-rb.org
MIT License
2.07k stars 161 forks source link

Fix duplicated input processing in update commands #595

Closed flash-gordon closed 4 years ago

flash-gordon commented 4 years ago

It should be backported ofc

timriley commented 4 years ago

@flash-gordon Hmm, I've now tried this branch on the application where I found the issue and my write type constructors are still getting called twice, firstly with Array and then again with Sequel::Postgres::JSONBArray.

To confirm it's not just due to a quirk of the larger application, I've dropped my reproduction script from https://github.com/rom-rb/rom/issues/594 into the app and run it against the app's gem bundle, and it indeed still shows the erroneous behavior:

$ bundle exec ruby rom_double_write_bug.rb
tags constructor called with Array
tags constructor called with Sequel::Postgres::JSONBArray

Did you test against the error reproduction script when making your adjustments here?

timriley commented 4 years ago

Here's a copy of the repro script with bundler/inline enabled to show I'm using this git branch etc.

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  git_source(:github) { |repo_name| "https://github.com/#{repo_name}" }

  gem "pg"
  gem "rake"
  gem "rom", github: "rom-rb/rom", branch: "594-fix-duplicated-inputs-in-restrictible-commands"
  gem "rom-sql"
end

require "rom"
require "rom/sql"

# Configure database URL
config = ROM::Configuration.new(:sql, "postgres://localhost/rom_double_attribute_write_type_call")
rom = ROM.container(config)

# Migrate database
rom.gateways[:default].tap do |gateway|
  migration = gateway.migration do
    change do
      drop_table? :articles

      create_table :articles do
        primary_key :id
        column :title, :text, null: false
        column :tags, :jsonb, null: false, default: "[]"
      end
    end
  end

  migration.apply gateway.connection, :up

  # Populate data
  gateway.connection.tap do |connection|
    connection.execute("INSERT INTO articles (title) VALUES ('Test article')")
  end
end

# Relation
module Relations
  class Articles < ROM::Relation[:sql]
    schema :articles, infer: true do
      attribute :tags, ROM::SQL::Types::PG::JSONB.prepend { |value|
        ::Kernel.puts "tags constructor called with #{value.class}"
        value
      }
    end
  end
end

# Register relation, finalize rom container
config.register_relation Relations::Articles
rom = ROM.container(config)

# Repository with update method
class ArticleRepo < ROM::Repository[:articles]
  def update_tags(tags)
    articles.by_pk(1).changeset(:update, tags: tags).commit
  end
end

# Run the update and witness the constructor being called twice
repo = ArticleRepo.new(rom)
repo.update_tags(%w[new tags here])

# e.g.
# tags constructor called with Array
# tags constructor called with Sequel::Postgres::JSONBArray
flash-gordon commented 4 years ago

@timriley apologies to the delay, I thought I messed with something and it'll require a longer investigation... But it seems that inlined gemfile doesn't work in the rom environment where a few gems reside in a single repo. I checked it by adding gem 'pry' and looking at the sources:

puts ROM::Plugins::Command::Schema::ClassInterface.instance_method(:build).source
          def build(relation, **options)
            if relation.schema?
              input = options.fetch(:input, self.input)
              relation_input = relation.input_schema

              input_handler =
                if input.equal?(ROM::Command.input)
                  relation_input
                else
                  -> tuple { relation_input[input[tuple]] }
                end

              super(relation, **options, input: input_handler)
            else
              super
            end
          end

👆 it's the old code. When I tested it locally using ./bin/console setup it worked. What's your Gemfile setup? IIRC it should be like

gem 'rom', github: 'rom-rb/rom', branch: 'branch-name' do
  gem 'rom-core'
  gem 'rom-changeset'
  gem 'rom-repository'
end
timriley commented 4 years ago

Thanks for this @flash-gordon, I really appreciate you following up again!

So I can confirm this works now!! 🎉

What I had to do, though, is set up my Gemfile like this:

  gem "rom", github: "rom-rb/rom", branch: "594-fix-duplicated-inputs-in-restrictible-commands"
  gem "rom-core", github: "rom-rb/rom", branch: "594-fix-duplicated-inputs-in-restrictible-commands"
  gem "rom-changeset", github: "rom-rb/rom", branch: "594-fix-duplicated-inputs-in-restrictible-commands"
  gem "rom-repository", github: "rom-rb/rom", branch: "594-fix-duplicated-inputs-in-restrictible-commands"

For some reason the block form didn't work, at least not with my inlined gemfile script.