Skip to content

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

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.

Sign up for GitHub

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

Merged
lvonk merged 1 commit into from
Nov 2, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/sequent/core/sequent_oj.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ module Core
class Oj
::Oj.default_options = {
bigdecimal_as_decimal: false,
mode: :compat,
Copy link
Copy Markdown
Member

@lvonk lvonk Oct 31, 2017 •
edited
Loading

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
Copy Markdown
Contributor Author

@respire respire Oct 31, 2017

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
Copy Markdown
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.

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
Copy Markdown
Contributor Author

@respire respire Oct 31, 2017

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
Copy Markdown
Member

@lvonk lvonk Nov 1, 2017 •
edited
Loading

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
Copy Markdown
Contributor Author

@respire respire Nov 2, 2017

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
Copy Markdown
Member

@lvonk lvonk Nov 2, 2017

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.

mode: :rails,
}

def self.strict_load(json)
Expand Down
2 changes: 1 addition & 1 deletion sequent.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ Gem::Specification.new do |s|
s.add_dependency 'activemodel', active_star_version
s.add_dependency 'pg', '~> 0.18'
s.add_dependency 'postgresql_cursor', '~> 0.6'
s.add_dependency 'oj', '~> 2.17.5'
s.add_dependency 'oj', '~> 3.3.9'
s.add_dependency 'thread_safe', '~> 0.3.5'
s.add_development_dependency 'rspec', '~> 3.2'
s.add_development_dependency 'rspec-mocks', '~> 3.2'
Expand Down