search

paper-trail-gem / paper_trail

Track changes to your rails models
MIT License
6.79k stars 896 forks source link

errors are not raised when update, update_columns, or destroy versions fail to create #1466

Open modosc opened 6 months ago

modosc commented 6 months ago

NOTE: this was https://github.com/paper-trail-gem/paper_trail/issues/1449 but it was automatically closed. i've provided a bug report as well as a fix. can i please get feedback on this?

paper_trail is inconsistent when Version records cannot be created. in some cases (create) an error is raised, in others (update, update_columns, and destroy) an error is logged but not raised. this can result in history silently being lost which is certainly unexpected behavior.

Currently when creating a new versioned record an error is raised if the Version cannot be created - this is because #save! is used:

https://github.com/paper-trail-gem/paper_trail/blob/47dbc2271c7055fde6f0fe7d3c8bf44f62cbef63/lib/paper_trail/record_trail.rb#L62

However, when updating, using update_columns, or destroying a versioned record no error is raised if the Version cannot be created:

https://github.com/paper-trail-gem/paper_trail/blob/47dbc2271c7055fde6f0fe7d3c8bf44f62cbef63/lib/paper_trail/record_trail.rb#L111

https://github.com/paper-trail-gem/paper_trail/blob/47dbc2271c7055fde6f0fe7d3c8bf44f62cbef63/lib/paper_trail/record_trail.rb#L301

https://github.com/paper-trail-gem/paper_trail/blob/47dbc2271c7055fde6f0fe7d3c8bf44f62cbef63/lib/paper_trail/record_trail.rb#L84

proposal

i think the behavior should be consistent - all failures should either raise errors or not. personally i don't see any advantage to not raising an error in these cases - it likely means that whodunnit isn't defined which i would expect to be an oversight rather than intentional. i realize this is not backwards compatible so i propose a two step process:

  1. an opt-in configuration parameter is added to the current major version which causes update and destroy failures to raise an error.
  2. in the next major version bump this becomes the default behavior.
github-actions[bot] commented 3 months ago

This issue has been automatically marked as stale due to inactivity. The resources of our volunteers are limited. Bug reports must provide a script that reproduces the bug, using our template. Feature suggestions must include a promise to build the feature yourself. Thank you for all your contributions.

modosc commented 3 months ago

this still isn't stale but if it's auto-closed (again) i'm giving up. i've been trying for 8 months to and i honestly don't care anymore.

github-actions[bot] commented 3 weeks ago

This issue has been automatically marked as stale due to inactivity. The resources of our volunteers are limited. Bug reports must provide a script that reproduces the bug, using our template. Feature suggestions must include a promise to build the feature yourself. Thank you for all your contributions.

modosc commented 3 weeks ago

@jaredbeck can we get a new release since #1450 has been merged? that would close this issue out. i'm happy to help if there's anything i can do on my end.