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

Strict Mode with UUID #134

Open
maennchen opened this issue Feb 9, 2021 · 4 comments
Open

Strict Mode with UUID #134

maennchen opened this issue Feb 9, 2021 · 4 comments

Comments

@maennchen
Copy link
Contributor

Currently Strict Mode is not supported when using UUIDs.

It could however easily be supported by generating the UUIDs on the client side.

This could look something like this:

multi
|> Ecto.Multi.run(initial_version_key, fn repo, %{} ->
  version_id = get_sequence_id("versions") + 1

  changeset_data =
    Map.get(changeset, :data, changeset)
    |> Map.merge(%{
      uuid: Ecto.UUID.generate(),
      first_version_id: version_id,
      current_version_id: version_id
    })

  initial_version = make_version_struct(%{event: "insert"}, changeset_data, options)
  repo.insert(initial_version)
end)
|> Ecto.Multi.run(model_key, fn
  repo, %{^initial_version_key => initial_version} ->
    updated_changeset =
      changeset
      |> change(%{
        uuid: initial_version.item_changes["uuid"],
        first_version_id: initial_version.id,
        current_version_id: initial_version.id
      })

    repo.insert(updated_changeset, ecto_options)
end)
@maennchen
Copy link
Contributor Author

(If UUIDs are also used for the Version id, the work could even be done outside of Ecto.Multi.run and 2 inserts could be used instead.)

@izelnakri
Copy link
Owner

Hmm this could be a good breaking change on strict_mode env config, however:

  • Im still not sure why we would want to use uuids for %PaperTrail.Version{}s instead of ids.
  • Tests for strict_mode are going to be way more complicated.
  • Probably then trackings should be first_version_uuid and current_version_uuid as well.

So I'm not sure, we probably need good use-cases for this.

@maennchen
Copy link
Contributor Author

@izelnakri Actually it's the other way around:

https://github.com/izelnakri/paper_trail/blob/main/lib/paper_trail/multi.ex#L51

            Map.get(changeset, :data, changeset)
            |> Map.merge(%{
              id: get_sequence_id(changeset) + 1,
              first_version_id: version_id,
              current_version_id: version_id
            })

get_sequence_id does not work with models that use uuids (if Version also uses uuids is not relevant). It is however not required since a new UUID can just be generated instead of calling a sequence.

This would allow to use strict_mode when using uuids in your model.

I do not think that this needs a breaking change.

@izelnakri
Copy link
Owner

Yep depending how tests and _id to _uuid config works out i guess we might be able to ship this depending on the end-tests complexity

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

No branches or pull requests

2 participants