activity_notification
activity_notification copied to clipboard
Add yaml_column_permitted_classes to fix broken serialization with latest Rails patches
Issue #, if available: #166
Summary
I added the required classes to yaml_column_permitted_classes
on ActiveRecord::Base
. With these changes all tests are green again.
Other Information
I'm not sure if this is the best way to fix this issue. It works, however if somebody has other classes on their whitelist, then it can break them gem again. For example this code snippet will get rid of the list I added to ActiveRecord::Base
diff --git a/spec/rails_app/config/application.rb b/spec/rails_app/config/application.rb
index 5b6b717..f98502d 100644
--- a/spec/rails_app/config/application.rb
+++ b/spec/rails_app/config/application.rb
@@ -47,6 +47,9 @@ module Dummy
end
end
end
+
+ config.active_record.yaml_column_permitted_classes ||= []
+ config.active_record.yaml_column_permitted_classes << MyWhitelistedClass
end
end
If you check Rails's code, it has an initializer that override ActiveRecord::Base.yaml_column_permitted_classes
if it has any value set in the config (as in the diff). Should we just add this to the readme somewhere or should we look for a better way to add the whitelist?
I hope my PR help, even if it's not the final solution :)
@kiskoza Thank you for your contribution. We’ll wait for a new Rails release for a moment, as discussed in #166.
I think we should add a description for whitelisting of ActiveRecord::Base
to README if this patch would be merged.
There's an interesting PR to Rails, it would remove the neccessity of setting global permitted classes: https://github.com/rails/rails/pull/45660 . Let's see where it goes
Hi. I have some updates:
- on the linked Rails PR they stated that they won't backport the new feature we're waiting for, so it will be able to help this gem with Rails 7.1+, but we should not wait for it
- I changed the code to use gem version ranges to be explicit about what we're aiming for. It's easier to remove old code when we drop support in the future for older Rails versions
- .. and I got a few failing spec with my changes. Turned out it's not my code, but some of the (test) dependencies:
committee-rails
released a new version which is not compatible with Rails 5.0 and a few other breaking changes which lead to a failing pipeline here. I pinned the committee-rails version to the last working version, we could upgrade it later in an other PR if we want to. Should I open a new PR or is it okay how I resolved the problem? - .. and I still got a few failing spec on Ruby 3 & Rails 7. I looked at the messages and the found that we have two gems from github which are causing problems. Mongoid override could be removed as they resolved Rails 7 support in 7.3.4, but
devise_token_auth
is still waiting for its release. Unfortunately their master branch is failing... Should I fix the mongoid pinning here or should I open a new PR? What should we do with the failingdevise_token_auth
gem? - I would like to add some notes to the readme to be able to merge this PR, could you help me where it belongs? Could it be the setup guide for activerecord?
LGTM, but tests are still failing. We'll fix the tests after this PR merged.