activity_notification icon indicating copy to clipboard operation
activity_notification copied to clipboard

Add yaml_column_permitted_classes to fix broken serialization with latest Rails patches

Open kiskoza opened this issue 1 year ago • 3 comments

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 avatar Jul 13 '22 10:07 kiskoza

@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.

simukappu avatar Jul 16 '22 13:07 simukappu

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

kiskoza avatar Aug 01 '22 08:08 kiskoza

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 failing devise_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?

kiskoza avatar Aug 08 '22 09:08 kiskoza

LGTM, but tests are still failing. We'll fix the tests after this PR merged.

simukappu avatar Mar 20 '23 14:03 simukappu