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

update oj to ~> 3.3.9, and use :rails mode to mimic the ActiveSupport… #98

Merged
merged 1 commit into from
Nov 2, 2017

Conversation

respire
Copy link
Contributor

@respire respire commented Oct 31, 2017

… version 5 encoder

more details at https://github.com/ohler55/oj/blob/master/pages/Rails.md

when i try to integrate Oj 3.x with Rails 5.1.x, bundler could not find compatible versions for Oj since sequent is using ~> 2.17.5.

Oj 3.x has better Rails 5.x compatibility. Since #88 supports rails 5.1, I think it's better to upgrade Oj to 3.x as well.

@@ -6,7 +6,7 @@ module Core
class Oj
::Oj.default_options = {
bigdecimal_as_decimal: false,
mode: :compat,
Copy link
Member

@lvonk lvonk Oct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi thanks for the PR!

I can't really asses if changing modes breaks anything?

According to https://github.com/ohler55/oj/blob/master/pages/Options.md there are some differences between :rails and :compat like empty_string and circular. So to be sure we are backward compatible maybe it is better to keep :compat as default and provide possibility to override from Sequent::Configuration.

What do you think? Is this something you would like to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point! but actually, i change the mode because of test failures. looks like in Oj 3.x compact mode, the event model cannot be serialized or deserialized correctly.
After investigating several hours, I have no idea how to make it work without modifying the mode.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I tested the rails mode against our apps in which we use sequent and this seems to work okay. Give me some time to test the compat to see if it breaks or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! You can run the unit tests at first. I met the failures like below before. when I tried to debug one of the specs, I found that it cannot deserialize MyEvent properly

1) Sequent::Core::AggregateSnapshotter can take a snapshot
     Failure/Error: expect(Sequent::Core::EventRecord.last.event_type).to eq Sequent::Core::SnapshotEvent.name

       expected: "Sequent::Core::SnapshotEvent"
            got: "MyEvent"

       (compared using ==)
     # ./spec/lib/sequent/core/aggregate_snapshotter_spec.rb:40:in `block (2 levels) in <top (required)>'
     # ./spec/lib/sequent/core/aggregate_snapshotter_spec.rb:16:in `block (2 levels) in <top (required)>'

  2) Sequent::Core::AggregateSnapshotter loads aggregates with snapshots loads both events
     Failure/Error: raise DeserializeEventError.new(event_hash)

     Sequent::Core::EventStore::DeserializeEventError:
       Event hash: {"event_type"=>"MyEvent", "event_json"=>"\"{MyEvent: @aggregate_id=[909396b7-7745-45b4-b430-e4f510f8ea8b], @sequence_number=[1], @created_at=[2017-10-31T23:06:35+08:00]}\""}
       Cause: #<ArgumentError: invalid date>
     # ./lib/sequent/core/event_store.rb:186:in `rescue in deserialize_event'
     # ./lib/sequent/core/event_store.rb:182:in `deserialize_event'
     # ./lib/sequent/core/event_store.rb:75:in `block in load_events_for_aggregates'
     # /Users/shimakaze/.rvm/gems/ruby-2.3.5/gems/activerecord-5.1.4/lib/active_record/result.rb:55:in `block in each'
     # /Users/shimakaze/.rvm/gems/ruby-2.3.5/gems/activerecord-5.1.4/lib/active_record/result.rb:55:in `each'
     # /Users/shimakaze/.rvm/gems/ruby-2.3.5/gems/activerecord-5.1.4/lib/active_record/result.rb:55:in `each'
     # ./lib/sequent/core/event_store.rb:74:in `map'
     # ./lib/sequent/core/event_store.rb:74:in `load_events_for_aggregates'
     # ./spec/lib/sequent/core/aggregate_snapshotter_spec.rb:64:in `block (3 levels) in <top (required)>'
     # ./spec/lib/sequent/core/aggregate_snapshotter_spec.rb:16:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # ArgumentError:
     #   invalid date
     #   ./lib/sequent/core/ext/ext.rb:59:in `iso8601'

Copy link
Member

@lvonk lvonk Nov 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found why it no longer works. The compat mode as of Oj 2.6 no longer calls as_json if available on an object to when Oj.dump is called. Since sequent relies on this via AttributeSupport the compact mode no longer works. So instead of rails as mode I'd rather keep the mode as compat. This minimizes the differences with the current way of serializing. So if you change this to:

      ::Oj.default_options = {
        bigdecimal_as_decimal: false,
        mode: :compat,
        use_to_json: true
      }

it works as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting. the document said use_to_json is ignored in the :compat mode. but it actually works. https://github.com/ohler55/oj/blob/master/pages/Options.md#use_to_json-boolean

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. That doesn't sound promising. Let's stick to the :rails more then, otherwise we rely on a possible bug in oj.

@lvonk lvonk merged commit c256d13 into zilverline:master Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants