-
Notifications
You must be signed in to change notification settings - Fork 10
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
Bump paper_trail from 13.0.0 to 15.0.0 #3236
Conversation
@@ -0,0 +1,26 @@ | |||
class ConvertPapertrailYamlToJson < ActiveRecord::Migration[6.1] | |||
def change | |||
safety_assured do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically not safe, but doing this to keep StrongMigrations from yelling at me.
This migration could be slow as mentioned here: https://github.com/paper-trail-gem/paper_trail#convert-existing-yaml-data-to-json
They provide another approach which will be faster, but requires 2 migrations and a backfill where we may have a period of time when version history is not being captured.
So be curious what approach the team thinks we should take 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we benchmark this so we know what impact it would have?
It doesn't look like it would be possible to roll back this operation. What risks are there and how might we mitigate them?
We should cut a release after merging this so that it's applied while this thinking is still fresh.
This one-step approach is probably better in our context than two deployments. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good. Just a couple of questions.
PaperTrail::Version.where.not(object: nil).find_each do |version| | ||
version.update_column(:new_object, YAML.unsafe_load(version.object)) | ||
|
||
if version.object_changes | ||
version.update_column( | ||
:new_object_changes, | ||
YAML.unsafe_load(version.object_changes) | ||
) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have version.object_changes
where version.object
is nil
?
PaperTrail::Version.where.not(object: nil).find_each do |version| | |
version.update_column(:new_object, YAML.unsafe_load(version.object)) | |
if version.object_changes | |
version.update_column( | |
:new_object_changes, | |
YAML.unsafe_load(version.object_changes) | |
) | |
end | |
PaperTrail::Version.where.not(object: nil).find_each do |version| | |
version.update_columns new_object: nil, object: YAML.load(version.object) | |
end | |
PaperTrail::Version.where.not(object_changes: nil).find_each do |version| | |
version.update_columns new_object_changes: nil, object_changes: YAML.load(version.object_changes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah great question. I assume you can't have a case where you just have a object_changes
and no object
.
According to paper trial README, object_changes
is a diff of the previous object. It is completely optional, and Paper Trial itself doesn't use it internally, apparently. So I assume you can't have a object_changes
if you don't have an object, since how would the diff get generated in the first place 🤔?
As a result, probably why they wrote the migration for this like they did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, so this migration came from a generator or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep it's from their README on how to migrate from YAML to JSON serializers: https://github.com/paper-trail-gem/paper_trail#convert-existing-yaml-data-to-json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
It doesn't look like it would be possible to roll back this operation. What risks are there and how might we mitigate them? Will need to add some extra instructions about benchmarking this before it hits production in the release notes.
Context
This PR upgrades PaperTrial from v13 to v15. In order to do so, PaperTrial has a breaking change with the YAML Serializer which unfortunately we are using. This is due to CVE-2022-32224 in Rails which forces YAML to use
safe_load
instead ofunsafe_load
which PT is now doing as well in v14.So we have some options, one is to continue using YAML Serializer but either opt into
unsafe_load
for YAML or provide a safe list viaActiveRecord.yaml_column_permitted_classes
. PaperTrial themselves highly recommend you to switch to the JSON Serializer too (especially if you are using postgres) and may be required in Rails 7 as suggested from this issue: paper-trail-gem/paper_trail#1407So I opted for switching from the YAML serializer to JSON serializer. Which requires a DB migration. Paper trial gives some guidance for this here: https://github.com/paper-trail-gem/paper_trail/blob/v12.3.0/README.md#convert-existing-yaml-data-to-json
Bumps paper_trail from 13.0.0 to 15.0.0.
Changelog
Sourced from paper_trail's changelog.
... (truncated)
Commits
d627260
Merge pull request #1435 from paper-trail-gem/release-15.0.048e8453
Release 15.0.0607eb7d
Drop Ruby 2.7 support, reached Eol 2023-03-3184f81de
Merge pull request #1414 from Nuzair46/version-table-with-uuid2291641
Docs: migrationdb46feb
Version table will use uuid as primary key type if uuid flag is specified on ...c2fa8e2
Drop Rails 6.0 support, reached EoL on 2023-06-015180551
Test: ubuntu-latest (was ubuntu-18.04)eb58b54
Merge pull request #1429 from 38tter/fix/typo-in-spec_helper8c8ac3c
Fix typo