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.

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

Jump to bottom

jsonb diff integration issue #30

Closed
wants to merge 2 commits into from
Closed

jsonb diff integration issue #30

wants to merge 2 commits into from

Conversation

jcsrb
Copy link
Contributor

@jcsrb jcsrb commented Mar 30, 2017 •
edited
Loading

when calculating the diff of a model version that has a jsonb column data is incorrectly deserialized

added failing tests, happy to help but not sure where to start

Failures:
  1) Logidze triggers without blacklisting diff generates the correct diff
     Got 2 failures:
     1.1) Failure/Error: expect(diff["meta"]["new"].class).to eq diff["meta"]["old"].class
          
            expected: Hash
                 got: String
          
            (compared using ==)
          
            Diff:
            @@ -1,2 +1,2 @@
            -Hash
            +String
            
          # ./spec/integrations/triggers_spec.rb:71:in `block (4 levels) in <top (required)>'
          # ./spec/acceptance_helper.rb:9:in `block (3 levels) in <top (required)>'
          # ./spec/acceptance_helper.rb:8:in `chdir'
          # ./spec/acceptance_helper.rb:8:in `block (2 levels) in <top (required)>'
          # ./spec/spec_helper.rb:40:in `block (2 levels) in <top (required)>'
     1.2) Failure/Error: expect(diff["meta"]).to eq expected_diff_meta
          
            expected: {"old"=>{"tags"=>["some", "tag"]}, "new"=>{"tags"=>["other"]}}
                 got: {"old"=>{"tags"=>["some", "tag"]}, "new"=>"{\"tags\": [\"other\"]}"}
          
            (compared using ==)
          
            Diff:
            @@ -1,3 +1,3 @@
            -"new" => {"tags"=>["other"]},
            +"new" => "{\"tags\": [\"other\"]}",
             "old" => {"tags"=>["some", "tag"]},
            
          # ./spec/integrations/triggers_spec.rb:72:in `block (4 levels) in <top (required)>'
          # ./spec/acceptance_helper.rb:9:in `block (3 levels) in <top (required)>'
          # ./spec/acceptance_helper.rb:8:in `chdir'
          # ./spec/acceptance_helper.rb:8:in `block (2 levels) in <top (required)>'
          # ./spec/spec_helper.rb:40:in `block (2 levels) in <top (required)>'

houndci-bot
houndci-bot reviewed Mar 30, 2017
View reviewed changes
spec/integrations/triggers_spec.rb Outdated
expect(diff["meta"]["old"].class).to eq diff["meta"]["new"].class
expect(diff["meta"]).to eq expected_diff_meta
end
end
Copy link

Choose a reason for hiding this comment

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

Lint/BlockAlignment: end at 74, 4 is not aligned with describe "diff" do at 61, 5.

spec/integrations/triggers_spec.rb Outdated
diff = post.reload.log_data.diff_from version: (post.reload.log_version-1)
expected_diff_meta = {
"old"=>{"tags"=>["some", "tag"]},
"new"=>{"tags" => ["other"]}
Copy link

Choose a reason for hiding this comment

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

Style/SpaceAroundOperators: Surrounding space missing for operator =>.
Style/SpaceInsideHashLiteralBraces: Space inside { missing.
Style/SpaceInsideHashLiteralBraces: Space inside } missing.

spec/integrations/triggers_spec.rb Outdated
post.update!(meta:{tags:['other']})
diff = post.reload.log_data.diff_from version: (post.reload.log_version-1)
expected_diff_meta = {
"old"=>{"tags"=>["some", "tag"]},
Copy link

Choose a reason for hiding this comment

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

Style/IndentHash: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
Style/SpaceAroundOperators: Surrounding space missing for operator =>.
Style/SpaceInsideHashLiteralBraces: Space inside { missing.
Style/WordArray: Use %w or %W for an array of words.
Style/SpaceInsideHashLiteralBraces: Space inside } missing.

spec/integrations/triggers_spec.rb Outdated

it "generates the correct diff", :aggregate_failures do
post.update!(meta:{tags:['other']})
diff = post.reload.log_data.diff_from version: (post.reload.log_version-1)
Copy link

Choose a reason for hiding this comment

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

Style/SpaceAroundOperators: Surrounding space missing for operator -.

spec/integrations/triggers_spec.rb Outdated
let(:post) { Post.create!(params).reload }

it "generates the correct diff", :aggregate_failures do
post.update!(meta:{tags:['other']})
Copy link

Choose a reason for hiding this comment

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

Style/SpaceAfterColon: Space missing after colon.
Style/SpaceInsideHashLiteralBraces: Space inside { missing.
Style/SpaceInsideHashLiteralBraces: Space inside } missing.

spec/integrations/triggers_spec.rb Outdated
@@ -58,6 +58,21 @@
end
end

describe "diff" do
Copy link

Choose a reason for hiding this comment

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

Style/IndentationConsistency: Inconsistent indentation detected.

spec/integrations/triggers_spec.rb Outdated
@@ -30,7 +30,7 @@

after(:all) { @old_post.destroy! }

let(:params) { { title: 'Triggers', rating: 10, active: false } }
let(:params) { { title: 'Triggers', rating: 10, active: false, meta: { tags: ['some','tag'] } } }
Copy link

Choose a reason for hiding this comment

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

Style/WordArray: Use %w or %W for an array of words.
Style/SpaceAfterComma: Space missing after comma.

@palkan
Copy link
Owner

palkan commented Apr 7, 2017

Thanks for the report and the test! That's gonna be a little bit tricky to handle such cases, but we'll try

@palkan palkan mentioned this pull request Apr 7, 2017
Closed
@yuriy-pavlov-castle-co
Copy link

I think it was broken, when you started use hstore_to_jsonb_loose( hstore(NEW.) - hstore(OLD.() ) instead of jsonb_minus e73587d#diff-fd3849333d83fe38f43eb6bde43836e6
@palkan , what is the reason of it? only benchmark?

@palkan
Copy link
Owner

palkan commented Dec 13, 2017

@yuriy-pavlov-castle-co

what is the reason of it? only benchmark?

Yep, mainly performance, jsonb_minus is 2x slower.

Using something different from hstore can solve the problem.

I think, the better solution here is to provide different diff functions and allow users to choose the right on for them.

@gorkunov
Copy link

@palkan what is the reason to provide several options if one of the options is partially broken?

@palkan
Copy link
Owner

palkan commented Dec 13, 2017

what is the reason to provide several options if one of the options is partially broken?

Frankly speaking, all of the options are partially broken: none of the above considers the column/model information. What if we want to use non-JSON serialization? The same problem. And it can't be solved DB-side.

Thus we have to use ActiveRecord Attributes API (Post.column_for_attribute('tags').type). That would be a working solution.

@palkan
Copy link
Owner

palkan commented Dec 27, 2017

Merged by #52

@palkan palkan closed this Dec 27, 2017
@palkan
Copy link
Owner

palkan commented Dec 27, 2017

Release as 0.6.0

@jcsrb jcsrb deleted the jsonb-diff-issue branch August 6, 2018 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jcsrb @palkan @yuriy-pavlov-castle-co @gorkunov @houndci-bot