Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple serialisation-related logic from AsyncHandler #1334

Open
milgner opened this issue May 24, 2022 · 13 comments
Open

Decouple serialisation-related logic from AsyncHandler #1334

milgner opened this issue May 24, 2022 · 13 comments

Comments

@milgner
Copy link

milgner commented May 24, 2022

Trying to use the async handler in my ActiveJob class like so:

class MyEventProcessingJob < ApplicationJob
  prepend RailsEventStore::CorrelatedHandler
  prepend RailsEventStore::AsyncHandler.with(serializer: RubyEventStore::NULL)

  def perform(the_event)
  end
end

This works flawlessly in a regular setup: the perform method receives instances of the ActiveRecord-based event model.

But now it's not possible to use the job manually anymore: calling MyEventProcessingJob.perform_now(some_event) will raise undefined method symbolize_keys'` because that's not a method of the Rails model class.

The current implementation reads

    def self.with(event_store: Rails.configuration.event_store, serializer: YAML)
      Module.new do
        define_method :perform do |payload|
          super(event_store.deserialize(serializer: serializer, **payload.symbolize_keys))
        end
      end
    end

I propose moving the ** and symbolize_keys somewhere else, probably into the serializer.

That way, when using the NULL serializer and calling perform with an event instance, the same event instance will end up in the job class.

@milgner
Copy link
Author

milgner commented May 24, 2022

Potentially related to #945

@mostlyobvious
Copy link
Member

mostlyobvious commented Jun 21, 2022

I can totally feel your pain with regard to current async handlers implementation and various limitations coming from the framework it leans on:
https://gist.github.com/pawelpacana/295337e92dd9edd2e443e6b31ac2683a

What works for me, which can be considered as a hack, is to:

class MyEventProcessingJob < ApplicationJob
   prepend RailsEventStore::AsyncHandler

   def perform(event)
     call(event)
   end

   def call(event)
     # ...
   end
 end

or — to see it clearly — inlined:

class EventHandler < ApplicationJob
  def perform(payload)
    event = event_store.read.event(payload.symbolize_keys.fetch(:event_id))
    call(event)
  end
end

class MyEventProcessingJob < EventHandler
  def call(event)
    # ...
  end
end

and then:

  • calling MyEventProcessingJob.new.call(event) when working with event objects at hand
  • leaving ActiveJob.perform(payload) shell for typical framework calls

I consider this a hack because of instantiating framework class (inheriting from ActiveJob after all) and its a bit of grey area. Like every hack, what is acceptable for me might not be acceptable by someone else.

@mostlyobvious
Copy link
Member

perform method receives instances of the ActiveRecord-based event model

Just to expand a bit:

@mostlyobvious
Copy link
Member

Also to keep in mind, a likely direction we'll take in the future: #755 (comment)

@mostlyobvious
Copy link
Member

Perhaps there's also some quick win with globalid and its support in ActiveJob: https://edgeguides.rubyonrails.org/active_job_basics.html#globalid

@mostlyobvious
Copy link
Member

That said, I'm aware that async handler experience can be improved here. I'm not convinced yet with proposed implementation. I'd also be very cautious when introducing changes here — we'll have to support them for a longer term in the future. I'll keep the issue open as a reminder.

@msencenb
Copy link

msencenb commented Jun 6, 2023

I don't know enough about the internals of this gem yet (and I'm happy to delete my comment if this is not helpful context), but I ran into an error I think is related to this issue. It seems like async handlers can't handle OpenStruct object types as we end up with this error:

NoMethodError (undefined method `fetch' for "OpenStruct":String):
  
ruby_event_store (2.9.1) lib/ruby_event_store/mappers/transformation/preserve_types.rb:147:in `block in restore_types'

I can work around it of course, but it would be great if I was able to poke that through the un-modified OpenStruct somehow. An external API I'm consuming is returning OpenStructs and I am hoping to keep the integration layer there as slim as possible.

@mostlyobvious
Copy link
Member

@msencenb Can you please share some more bits of the code — i.e. RailsEvenStore instance configuration and the sample event that is causing this? I'm happy to help, executable code to reproduce the issue you're experiencing would make it much easier.

@mostlyobvious
Copy link
Member

@msencenb I've managed to recreate the issue you're experiencing: https://gist.github.com/pawelpacana/f39026514185e8f72e4daaca2d99b85a

I'll look into it more in the upcoming days. It seems that PreserveTypes transformation used by default in JSONClient is assuming a bit too much about the shape of the data.

I'm still guessing what is your exact configuration (RailsEventStore, database engine, etc.).
But if you're not tied to JSON columns in the database for event_store_events table, perhaps switching to YAML serialization would give you the most of "keeping the integration layer there as slim as possible".

@mostlyobvious
Copy link
Member

@msencenb There's a workaround which might work without introducing too much changes:

preserve_types_as_seen_in_json_client.register(OpenStruct, serializer: ->(v) { YAML.dump(v) }, deserializer: ->(v) { YAML.unsafe_load(v) }),

That is using YAML only for particular type for storage. It works because it fits into current shape assumptions of PreserveTypes, as opposed to this:

preserve_types_as_seen_in_json_client.register(OpenStruct, serializer: ->(v) { v.to_h }, deserializer: ->(v) { OpenStruct.new(v) }),

@msencenb
Copy link

msencenb commented Jun 24, 2023

@pawelpacana Thank you and apologies for the slow response. You reproduced what I have, here is my config, which is very minimal and using the JSONClient.

require "rails_event_store"

Rails.configuration.to_prepare do
  Rails.configuration.event_store = RailsEventStore::JSONClient.new

  # Subscribe event handlers below
  Rails.configuration.event_store.tap do |store|
    store.subscribe(WiqListeners::Justifi::SubAccountEventHandler, to: [WiqEvents::Justifi::SubAccountUpdated])

    store.subscribe_to_all_events(RailsEventStore::LinkByEventType.new)
    store.subscribe_to_all_events(RailsEventStore::LinkByCorrelationId.new)
    store.subscribe_to_all_events(RailsEventStore::LinkByCausationId.new)
  end
end

I'm using Postgres as my data store. In my case I was able to just poke through an id in my event and re-pull the external resource from my partner's API.

fidel added a commit that referenced this issue Jul 19, 2023
Restoring type shouldn't rely on stored argument (data). It should
solely rely on type that was persisted in metadata.

E.g. using OpenStruct for data failed before this change, see:

  #1334 (comment)

As a sideeffect, two obsolete conditionals could be removed.

Co-authored-by: Łukasz Reszke <lukaszreszke93@gmail.com>
Co-authored-by: Paweł Pacana <pawel.pacana@gmail.com>
@fidel
Copy link
Contributor

fidel commented Jul 20, 2023

@msencenb we've just released v2.11.0 which should address the issue you experienced 🙂

@msencenb
Copy link

msencenb commented Jul 20, 2023

wow that was fast—thank you for the patch, I appreciate it 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants