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

What is the meaning of the item_changes column? #237

Open
tjchambers opened this issue Nov 20, 2024 · 3 comments
Open

What is the meaning of the item_changes column? #237

tjchambers opened this issue Nov 20, 2024 · 3 comments

Comments

@tjchambers
Copy link

At first that might seem like a silly question, but it is an important one.

It is a list of "changes". But represented as a key -> value map, what is the meaning of the key portion?

Is it an Ecto Field name? Is it a database column name?

It would be helpful to know the answer to that.

@tjchambers
Copy link
Author

I am going to leave this question open, but IMHO the key should represent the Ecto schema's field name.

Currently afaict the current paper_trail implementation refers to the database column name, which is not traditionally how I would refer to the changes if I was to try and re-apply them. Regardless of the intent of what the key means there is the issue of changes in column names or field names in Ecto referring to the same column re: backwards compatibility of data (not sure that is solvable). Making this refer to the internal Ecto field name insulates column name changes for the future similar to the use of the schema module name insulating the database table name from being in the Versions content.

Since making the item_changes refer to field names as the key is a breaking change I am going to fork the repo and make that change for my app.

I am still of the opinion that the definition of the intent of the key in the item changes JSON content to be a database column name should be documented in the README for clarity.

@tjchambers
Copy link
Author

In reading the README I read the statement which I strongly support:

Data integrity is an important aim of this project, ...

Paper_trail is an important addition to my app and ensuring that the item_changes capture all of the related DB changes is key to reconstructing events.

I appreciate all the work that goes into projects like this (I was a user of the Paper_trail ruby gem) and want to thank again all the contributors.

IMO this is all the more reason to alert users of this OSS that if the use Ecto field source options that those changes to those fields are not captured due to the difference between field and column names.

Thanks to all for contributing!

@tjchambers
Copy link
Author

here are the changes I made for supporting Ecto field sourcing for posterity:

  @doc """
  Dumps changes using Ecto fields
  """
  @spec serialize_changes(Ecto.Changeset.t()) :: map()
  def serialize_changes(%Ecto.Changeset{changes: changes, data: %schema{}} = changeset) do
    changed_fields = changes
    |> Map.keys()
    |> Enum.map(fn key -> {key, schema.__schema__(:field_source, key)} end)

    columns = Enum.map(changed_fields, fn {_, column} -> column end)

    changeset
    |> serialize_model_changes()
    |> serialize()
    |> Map.take(columns)
    |> Map.new(fn {key, value} ->
      case Enum.find(changed_fields, fn {_field, column} -> column == key end) do
        {field, _column} -> {field, value}
      end
    end)
  end

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

1 participant