Dump fields using Ecto when serializing changes #83

rschef · 2020-05-01T01:15:10Z

Dump fields using Ecto when serializing changes

Ecto.embedded_dump/2 is now called when serializing changes, which means custom Ecto fields have their dump/1 callback called if their callback embed_as/1 return :dump (default is :self).

These changes were done only in this commit: nash-io@1c97a58

Fixed test/paper_trail/base_tests.exs file not being run, because it didn't end with the sufix _test.exs
Created PaperTrail.Serializer module and reused it across the application

The following functions were being declared in multiple modules, although they had the same implementation.

Now all modules delegate to the PaperTrail.Serializer module:

  defdelegate make_version_struct(version, model, options), to: Serializer
  defdelegate get_sequence_from_model(changeset), to: Serializer
  defdelegate serialize(data), to: Serializer
  defdelegate get_sequence_id(table_name), to: Serializer
  defdelegate add_prefix(changeset, prefix), to: Serializer
  defdelegate get_item_type(data), to: Serializer
  defdelegate get_model_id(model), to: Serializer

izelnakri · 2020-05-03T15:24:39Z

Hi @rschef, I'll review this PR soon. Thanks for submitting it!

izelnakri · 2020-05-03T15:29:55Z

test/paper_trail/bang_functions_simple_mode_test.exs

@@ -978,11 +993,6 @@ defmodule PaperTrailTest.SimpleModeBangFunctions do
    first(Person, :id) |> MultiTenant.add_prefix_to_query() |> repo().one()
  end

-  defp serialize(model) do


This made me like the PaperTrail.Serializer module even more, thank you. I'll read up on Ecto.embedded_dump

izelnakri · 2020-05-03T15:31:26Z

test/support/simple_models.exs

@@ -1,3 +1,32 @@
+
+defmodule LocationType do


This is also very interesting, thanks for adding it to the tests.

izelnakri · 2020-05-03T15:41:29Z

I'll read up on Ecto.embedded_dump and run the tests locally before merging to master. Thanks again for contributing @rschef !

izelnakri · 2020-05-05T15:42:48Z

lib/paper_trail/serializer.ex

+  def add_prefix(changeset, prefix), do: Ecto.put_meta(changeset, prefix: prefix)
+
+  def get_item_type(%Ecto.Changeset{data: data}), do: get_item_type(data)
+  def get_item_type(%schema{}), do: schema |> Module.split() |> List.last()


interesting trying this out now.

ok verified that behavior is exactly the same and all tests passing

izelnakri · 2020-05-05T15:57:23Z

@rschef all looks good to me thank you. I have one final request, since now serializer methods are public could you also add the doc strings/documentation to those as well? Then we can merge and release it the same day, thanks!

rschef · 2020-05-06T02:59:37Z

Hi @izelnakri, thanks for the feedback and taking the time to review this PR.

Unfortunately I'm short on time this week, so I'll only be able to address your request over the weekend.

izelnakri · 2020-06-14T23:36:17Z

Hi @rschef, I see more feature requests coming up. Could you finalize the PR sometime these days? Then we can merge can cut a release, thanks!

rschef · 2020-06-27T20:41:50Z

@izelnakri Sorry for only replying to you now, these have been busy weeks. Yes, I plan to submit other 3 feature requests after this one is merged:

But back to this PR, I found troublesome having to define a embed_as/1 function for every Ecto type, since a lot of times they are created internally by other packages or applications and it's easy to forget it. So I decided to dump all schema fields and do it the same way Ecto does when dumping to the database. I tried to find a public function that I could call in Ecto to dump a struct created by an Ecto schema, but unfortunately, they were all private functions. So I ended up copying them here.

With that, I solved that issue, but created another one. Some Ecto types, such as Ecto.UUID, convert from string to binary when dumping a field and, therefore, Jason failed to encode it to json. So my solution was to add a configuration that allowed to skip some Ecto.Types, which was implemented here.

If you agree with those decisions and solutions, I can work on this PR to implement them. Otherwise, if you have other suggestions for those issues, please let me know.

izelnakri · 2020-07-06T19:18:58Z

Hi @rschef I just saw your response, sorry for replying late!

This PR is pretty much done, I would just need documentation on the public methods. Please let me know when thats done, then we can merge it.

Also I checked the links you've shared, seems like they have been merged to nash fork already. I think it would be better for everyone if we keep one version of paper_trail for everyone including for nash, I understand nash might be using a fork for rapid development internally. I'm willing to review all open PRs to this repository.

rschef · 2020-07-09T15:11:32Z

Hi @izelnakri, thanks for your reply.

Sure, I agree with you about having only one version of paper_trail. I'll do my best to open those PRs in this repository after this one is merged.

I'll add the missing documentation as soon as I have some time and then I can open a new PR later to implement the changes I mentioned here

rschef · 2020-07-18T17:30:06Z

Added type specs and docs to Serializer, @izelnakri please take one more look

lib/paper_trail/serializer.ex

izelnakri · 2020-07-19T02:23:56Z

lib/paper_trail/serializer.ex

+  Dumps changes using Ecto fields
+  """
+  @spec serialize_changes(Ecto.Changeset.t()) :: map()
+  def serialize_changes(%Ecto.Changeset{data: %schema{}, changes: changes}) do


@rschef what was the reason of having this public method instead of just having serialize()? Was there a performance issue previously?

izelnakri · 2020-07-19T02:25:48Z

Hi @rschef, thanks for adding the documentation! I have only one question and a minor documentation suggestion. Then we can merge it!

izelnakri · 2020-10-01T00:14:13Z

Hi @rschef, could you rebase from master? There were few MRs merged prior, we'll need to rebase and adjust these changes. Thanks

rschef · 2020-10-02T17:52:12Z

Hi @izelnakri, I've rebased from master :)

* Add .tool-versions to .gitignore * Change logger level to warn in test environment * Fix warning when running tests * Create serializer module and reuse it across the application * Dump ecto fields when serializing changes * Add type specs and docs to Serializer * Replace get_sequence_from_model/1 by get_sequence_id/1 * Apply documentation

izelnakri reviewed May 3, 2020

View reviewed changes

izelnakri reviewed May 5, 2020

View reviewed changes

izelnakri approved these changes May 5, 2020

View reviewed changes

rschef requested a review from izelnakri July 18, 2020 17:21

izelnakri reviewed Jul 19, 2020

View reviewed changes

lib/paper_trail/serializer.ex Outdated Show resolved Hide resolved

izelnakri reviewed Jul 19, 2020

View reviewed changes

izelnakri force-pushed the master branch 2 times, most recently from a06c813 to 675d502 Compare October 1, 2020 20:42

rschef added 9 commits October 2, 2020 14:48

Add .tool-versions to .gitignore

50f833e

Change logger level to warn in test environment

3cccdbd

Fix warning when running tests

9e472a8

Create serializer module and reuse it across the application

b5989eb

Fix base tests not being run because of typo

a073d68

Dump ecto fields when serializing changes

b5cbfda

Add type specs and docs to Serializer

d4fde31

Replace get_sequence_from_model/1 by get_sequence_id/1

6c25995

Apply documentation suggestion

5ed018e

rschef force-pushed the support-json-field branch from 88dc238 to 5ed018e Compare October 2, 2020 17:49

izelnakri mentioned this pull request Oct 15, 2020

Support Ecto Embed #112

Merged

1 task

izelnakri merged commit 88dc4b2 into izelnakri:master Nov 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dump fields using Ecto when serializing changes #83

Dump fields using Ecto when serializing changes #83

rschef commented May 1, 2020

izelnakri commented May 3, 2020

izelnakri May 3, 2020

izelnakri May 3, 2020

izelnakri commented May 3, 2020

izelnakri May 5, 2020

izelnakri May 5, 2020

izelnakri commented May 5, 2020 •

edited

Loading

rschef commented May 6, 2020

izelnakri commented Jun 14, 2020

rschef commented Jun 27, 2020 •

edited

Loading

izelnakri commented Jul 6, 2020

rschef commented Jul 9, 2020

rschef commented Jul 18, 2020

izelnakri Jul 19, 2020 •

edited

Loading

izelnakri commented Jul 19, 2020

izelnakri commented Oct 1, 2020

rschef commented Oct 2, 2020

Dump fields using Ecto when serializing changes #83

Dump fields using Ecto when serializing changes #83

Conversation

rschef commented May 1, 2020

izelnakri commented May 3, 2020

izelnakri May 3, 2020

Choose a reason for hiding this comment

izelnakri May 3, 2020

Choose a reason for hiding this comment

izelnakri commented May 3, 2020

izelnakri May 5, 2020

Choose a reason for hiding this comment

izelnakri May 5, 2020

Choose a reason for hiding this comment

izelnakri commented May 5, 2020 • edited Loading

rschef commented May 6, 2020

izelnakri commented Jun 14, 2020

rschef commented Jun 27, 2020 • edited Loading

izelnakri commented Jul 6, 2020

rschef commented Jul 9, 2020

rschef commented Jul 18, 2020

izelnakri Jul 19, 2020 • edited Loading

Choose a reason for hiding this comment

izelnakri commented Jul 19, 2020

izelnakri commented Oct 1, 2020

rschef commented Oct 2, 2020

izelnakri commented May 5, 2020 •

edited

Loading

rschef commented Jun 27, 2020 •

edited

Loading

izelnakri Jul 19, 2020 •

edited

Loading