Do not double serialize YAML in Legacy Repository

[paneq]

I tried adding a test but it is passing:

      specify "does not serialize to YAML twice" do
        repository = LegacyEventRepository.new
        repository.append_to_stream(
          SRecord.new(
            data:     d = YAML.dump({when: Time.now}),
            metadata: m = YAML.dump({timestamp: Time.now}),
          ),
          RubyEventStore::Stream.new('stream_1'),
          RubyEventStore::ExpectedVersion.none
        )
        row = LegacyEventRepository.const_get(:LegacyEvent).last
        puts row.inspect
        expect(row.data).to eq(d)
        expect(row.metadata).to eq(m)

which is surprising to me because I expected

      class LegacyEvent < ::ActiveRecord::Base
        self.primary_key = :id
        self.table_name = 'event_store_events'
        serialize :metadata
        serialize :data
      end

to serialize the string again with YAML due to serialize :metadata etc.

[paneq]

@replaid please provide ruby version, rails version and RES configuration snippet.

[mostlyobvious]

I've managed to reproduce in Rails 4.2.10 and Ruby 2.4.2 (first choice, chosen at random).

1. Installed RES 0.18.2, setup and migrated.
2. Upgraded to RES 0.28.0 and changed client to reference legacy repository.
3. Opened console:

irb(main):003:0> es = Rails.configuration.event_store
=> #<RailsEventStore::Client:0x00007fb2a2bfcb40 @repository=#<RailsEventStoreActiveRecord::Legacy::EventRepository:0x00007fb2a2bfce88>, @mapper=#<RubyEventStore::Mappers::Default:0x00007fb2a2bfcaf0 @serializer=Psych, @events_class_remapping={}>, @event_broker=#<RubyEventStore::PubSub::Broker:0x00007fb2a2bfc9d8 @subscribers={}, @global_subscribers=[], @thread_global_subscribers=#<Concurrent::ThreadLocalVar:0x00007fb2a2bfc848 @default_block=nil, @default=[], @index=0>, @thread_subscribers=#<Concurrent::ThreadLocalVar:0x00007fb2a2bfc758 @default_block=#<Proc:0x00007fb2a2bfc730@/Users/pawelpacana/.rubies/ruby-2.4.2/lib/ruby/gems/2.4.0/gems/ruby_event_store-0.28.0/lib/ruby_event_store/pub_sub/broker.rb:13>, @default=nil, @index=1>, @dispatcher=#<RailsEventStore::ActiveJobDispatcher:0x00007fb2a2bfca50 @async_proxy_strategy=#<RailsEventStore::AsyncProxyStrategy::Inline:0x00007fb2a2bfca28>>>, @page_size=100, @metadata_proc=#<Proc:0x00007fb2a2bfc438@/Users/pawelpacana/.rubies/ruby-2.4.2/lib/ruby/gems/2.4.0/gems/rails_event_store-0.28.0/lib/rails_event_store/client.rb:7 (lambda)>, @clock=#<Proc:0x00007fb2a2bfc3e8@/Users/pawelpacana/.rubies/ruby-2.4.2/lib/ruby/gems/2.4.0/gems/ruby_event_store-0.28.0/lib/ruby_event_store/client.rb:8 (lambda)>>
irb(main):004:0> es.read_all_streams_forward
  RailsEventStoreActiveRecord::Legacy::EventRepository::LegacyEvent Load (0.1ms)  SELECT  "event_store_events".* FROM "event_store_events"  ORDER BY "event_store_events"."id" ASC LIMIT 100
=> []
irb(main):005:0> 
irb(main):005:0> es.read_all_streams_forward
  RailsEventStoreActiveRecord::Legacy::EventRepository::LegacyEvent Load (0.1ms)  SELECT  "event_store_events".* FROM "event_store_events"  ORDER BY "event_store_events"."id" ASC LIMIT 100
=> []
irb(main):006:0> DummyEvent = Class.new(RailsEventStore::Event)
=> DummyEvent
irb(main):007:0> meta = { foo: 'bar' }
=> {:foo=>"bar"}
irb(main):008:0> es.publish_event(DummyEvent.new(metadata: meta))
   (0.1ms)  begin transaction
  SQL (0.4ms)  INSERT INTO "event_store_events" ("event_id", "data", "metadata", "event_type", "stream", "created_at") VALUES (?, ?, ?, ?, ?, ?)  [["event_id", "54db8b2e-b726-45a3-ae8c-9f9971cfa21e"], ["data", "--- \"--- {}\\n\"\n"], ["metadata", "--- |\n  ---\n  :foo: bar\n  :timestamp: 2018-05-06 10:28:32.382499000 Z\n"], ["event_type", "DummyEvent"], ["stream", "#<Object:0x00007fb2a78504d8>"], ["created_at", "2018-05-06 10:28:32.393679"]]
   (1.0ms)  commit transaction
=> :ok
irb(main):009:0> es.read_all_streams_forward
  RailsEventStoreActiveRecord::Legacy::EventRepository::LegacyEvent Load (0.1ms)  SELECT  "event_store_events".* FROM "event_store_events"  ORDER BY "event_store_events"."id" ASC LIMIT 100
=> [#<DummyEvent:0x00007fb2a35e2e48 @event_id="54db8b2e-b726-45a3-ae8c-9f9971cfa21e", @metadata=#<RubyEventStore::Metadata:0x00007fb2a35e2df8 @h={:foo=>"bar", :timestamp=>2018-05-06 10:28:32 UTC}>, @data={}>]
irb(main):010:0> es.read_all_streams_forward[0].metadata
  RailsEventStoreActiveRecord::Legacy::EventRepository::LegacyEvent Load (0.1ms)  SELECT  "event_store_events".* FROM "event_store_events"  ORDER BY "event_store_events"."id" ASC LIMIT 100
=> #<RubyEventStore::Metadata:0x00007fb2a35bef20 @h={:foo=>"bar", :timestamp=>2018-05-06 10:28:32 UTC}>
irb(main):011:0> es.read_all_streams_forward[0].metadata[:foo]
  RailsEventStoreActiveRecord::Legacy::EventRepository::LegacyEvent Load (0.1ms)  SELECT  "event_store_events".* FROM "event_store_events"  ORDER BY "event_store_events"."id" ASC LIMIT 100
=> "bar"
irb(main):012:0> es.read_all_streams_forward[0].metadata[:timestamp]
  RailsEventStoreActiveRecord::Legacy::EventRepository::LegacyEvent Load (0.1ms)  SELECT  "event_store_events".* FROM "event_store_events"  ORDER BY "event_store_events"."id" ASC LIMIT 100
=> 2018-05-06 10:28:32 UTC

and then

irb(main):017:0> lr = RailsEventStoreActiveRecord::LegacyEventRepository
=> RailsEventStoreActiveRecord::Legacy::EventRepository
irb(main):022:0> le = lr.const_get(:LegacyEvent)
=> RailsEventStoreActiveRecord::Legacy::EventRepository::LegacyEvent(id: integer, stream: string, event_type: string, event_id: string, metadata: text, data: text, created_at: datetime)
irb(main):025:0> puts le.first.metadata_before_type_cast
  RailsEventStoreActiveRecord::Legacy::EventRepository::LegacyEvent Load (0.1ms)  SELECT  "event_store_events".* FROM "event_store_events"  ORDER BY "event_store_events"."id" ASC LIMIT 1
--- |
  ---
  :foo: bar
  :timestamp: 2018-05-06 10:28:32.382499000 Z
=> nil

The problem doesn't manifest as obvious if you only have double serialized metadata and that's why the test from https://github.com/RailsEventStore/rails_event_store/issues/345#issuecomment-386783486 doesn't expose it.

[mostlyobvious]

Adding integration test similar to v1_v2_schema_migration_spec.rb would reveal it and ensure smooth compatibility in near future.

[mostlyobvious]

Addressed in #348 and v0.28.2 release. Feel free to reopen if the problem (or different aspect of it) persists.

[paneq]

@pawelpacana I think I know what was wrong in my test.

I should have used row.data_before_typecast instead of row.data to verify that we don't serialize twice. Do you think that would be more beneficial unit test over the integration one you wrote?

[mostlyobvious]

I think adding additional test wouldn't hurt. Moving that integration test to ruby_event_store as a form of configuration check (if we for some reason change default YAML serializer) would be ok too.

[paneq]

@pawelpacana done. _before_type_cast was the key indeed.