PaperTrail creates an update version even when there's no update

[patmaddox]

Hi there, I found that PaperTrail creates an update version, even if there are no changes in the changeset and so there's not really an update. This was surprising to me, and I'd like to understand the rationale if this is intentional - and for a suggestion on how to proceed if I don't want this behavior. One possibility I can think of is constructing the version myself when needed, essentially duplicating the current behavior of PaperTrail.update. Another possibility is to add an option to PaperTrail to not create versions on no-op updates. I have included a failing test demonstrating what I consider surprising behavior.

Here's the failure message. You can see that item_changes in the update version is %{}, so I don't think it warrants a PaperTrail version.

  1) test updating a company with current params does not create a version (PaperTrailTest)
     test/paper_trail/base_tests.exs:139
     Assertion with == failed
     code:  assert PaperTrail.get_version(insert_result[:model]) == insert_result[:version]
     left:  %PaperTrail.Version{
              __meta__: #Ecto.Schema.Metadata<:loaded, "versions">,
              event: "update",
              id: 2,
              inserted_at: ~N[2020-08-25 18:00:44],
              item_changes: %{},
              item_id: 1,
              item_type: "SimpleCompany",
              meta: nil,
              origin: nil,
              originator_id: nil,
              user: #Ecto.Association.NotLoaded<association :user is not loaded>
            }
     right: %PaperTrail.Version{
              __meta__: #Ecto.Schema.Metadata<:loaded, "versions">,
              event: "insert",
              id: 1,
              inserted_at: ~N[2020-08-25 18:00:44],
              item_changes: %{
                address: nil,
                city: "Greenwich",
                facebook: nil,
                founded_in: nil,
                id: 1,
                inserted_at: ~N[2020-08-25 18:00:44],
                is_active: true,
                name: "Acme LLC",
                twitter: nil,
                updated_at: ~N[2020-08-25 18:00:44],
                website: nil
              },
              item_id: 1,
              item_type: "SimpleCompany",
              meta: nil,
              origin: nil,
              originator_id: nil,
              user: #Ecto.Association.NotLoaded<association :user is not loaded>
            }
     stacktrace:
       test/paper_trail/base_tests.exs:149: (test)

[izelnakri]

Hi there,

Thanks for your bug report. There was a bug in the tests helper function, I replaced the explicit nils to PaperTrail.insert(x, nil) and your test case is now failing:

5f2e72c

no-op thing I assume introduced to Ecto recently, I'll also need to read about it. Checking it now, meanwhile closing this PR, let me know if you would like it to be reopened again.

[izelnakri]

also CI was ignoring tests/paper_trail/base_tests.exs since the filename was plural, it should have been test/paper_trail/base_test.exs, fixed it on master

[izelnakri]

This is a very interesting finding because Ecto docs says Ecto.Multi behaves exactly as Repo.update, and default is no-op:
https://hexdocs.pm/ecto/Ecto.Multi.html#update/4

We use multi here:
https://github.com/izelnakri/paper_trail/blob/master/lib/paper_trail/multi.ex#L102

This could be a bug in Ecto, checking it now.

[izelnakri]

Ok after investigating this, this is not a bug in Ecto but in our transaction logic. yes this is a logical situation that is hard to prevent since %Ecto.Multi{} is lazy. https://github.com/izelnakri/paper_trail/blob/master/lib/paper_trail/multi.ex#L102 transaction simply does two actions in one transaction, we can match the behavior to Repo.update but this would be even harder on strict_mode since version gets inserted first. I'm not particularly keen on matching this behavior with Ecto.Repo.update, however if you would like to solve it, I'd be happy to review and merge it. Thanks for bringing it up again! Even after years and thousands of downloads, as we can see software is never perfect there are always bugs/logical mismatches like these 😄

[izelnakri]

Hi @patmaddox this issue is now fixed in this PR: #224 I'm closing this PR, feel free to open a new one if it persists.