Skip to content
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

App and specs working under Ruby 3.1.4 #1138 #1139

Merged

Conversation

ferrisoxide
Copy link
Contributor

Fix for #1138

@ferrisoxide ferrisoxide changed the title 1138 app and specs working under Ruby 3.0.6 App and specs working under Ruby 3.0.6 #1138 Jul 29, 2023
@ferrisoxide ferrisoxide force-pushed the 1138_ruby_2_7_has_reached_eol branch from 9ad2907 to fec8655 Compare July 29, 2023 08:09
Gemfile.lock Outdated Show resolved Hide resolved
spec/lib/fat_free_crm/i18n_spec.rb Fixed Show fixed Hide fixed
spec/lib/fat_free_crm/i18n_spec.rb Fixed Show fixed Hide fixed
@CloCkWeRX
Copy link
Member

Can you see if bumping brakeman sorts out:

Notice:  Detected Rails 6 application
Processing configuration...
Notice:  Error while processing /home/runner/work/fat_free_crm/fat_free_crm/config/application.rb

@ferrisoxide
Copy link
Contributor Author

Can you see if bumping brakeman sorts out:

@CloCkWeRX From a cursory search, it looks like this might be specific to Github actions. The only reference I've found so far:

https://www.reddit.com/r/github/comments/nsc1p5/github_actions_outputting_a_sarif_file_but_next/

It looks like the brakeman config needs fixing as well. I'll have a look at both tomorrow.

@ferrisoxide
Copy link
Contributor Author

I'll try bumping brakeman, but it looks like I'll have to address the issues reported by brakeman as well. There's a number of new issues, mostly due to calling permit on params, that currently aren't in the brakeman.config.

@ferrisoxide
Copy link
Contributor Author

OK, I'm just making things worse here.

It looks like the checks are running Ruby 2.7.8, but I can't see where this is specified. I've tried upgrading to Ruby 3.1, both in the app and in the Dockerfile and ran into issues with Psych that forced an update to PaperTrail.

Stepping back from this for a moment, but if you have any insight into why it's still using Ruby 2.7 I'd be keen to get a clue.

spec/features/support/browser.rb Dismissed Show dismissed Hide dismissed
spec/features/support/browser.rb Dismissed Show dismissed Hide dismissed
@ferrisoxide ferrisoxide changed the title App and specs working under Ruby 3.0.6 #1138 App and specs working under Ruby 3.1.4 #1138 Aug 7, 2023
@ferrisoxide
Copy link
Contributor Author

@CloCkWeRX

OK, this is finally passing tests. Ruby version had to be updated to 3.1.4, but that's probably not a bad thing given the 3.0.x series isn't far out from EOL.

Happy to squash all these commits if it makes life easier.

@CloCkWeRX
Copy link
Member

CloCkWeRX commented Aug 9, 2023

It'd be nice to have a 3.0 version, so anyone still stuck on 2.7 can jump to 3.0... but working code is working code.

@@ -73,6 +73,9 @@ class Application < Rails::Application

# Configure sensitive parameters which will be filtered from the log file.
config.filter_parameters += %i[password encrypted_password password_salt password_confirmation]

# Enable support for loading via Psych, required by PaperTrail
config.active_record.use_yaml_unsafe_load = true
Copy link
Member

@CloCkWeRX CloCkWeRX Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
config.active_record.use_yaml_unsafe_load = true
config.active_record.use_yaml_unsafe_load = false
config.active_record.yaml_column_permitted_classes = [
::ActiveRecord::Type::Time::Value,
::ActiveSupport::TimeWithZone,
::ActiveSupport::TimeZone,
::BigDecimal,
::Date,
::Symbol,
::Time
]

https://github.com/paper-trail-gem/paper_trail/blob/master/doc/pt_13_yaml_safe_load.md#to-continue-using-the-yaml-serializer might be worth looking through, so that people upgrading don't have any issues with legacy data (but also no security issues)

not sure if we just need to add a bunch of internal models (Account, Lead, Opportunity etc) as well

Copy link
Contributor Author

@ferrisoxide ferrisoxide Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if we just need to add a bunch of internal models (Account, Lead, Opportunity etc) as well

Allowing all classes to be serialized/deserialized via Psych isn't great, but you can't specify the model classes in application.rb (they haven't been loaded at this point). Naively moving the configuration to an initializer breaks, with some Active Support classes being marked as a disallowed class in specs. I'm not sure why just yet.

I'm also a bit worried about serialization of associated objects - I suspect we'd have to declare all possible compositions, not just the base model classes.

Fixing this might be tricky. I'd suggest that we look at securing serialization in a separate ticket as I've no idea how far down the rabbit hole this is going to go. Right now, leaving use_yaml_unsafe_load set to true is no more unsafe than what is present in the app - and only appears to impact Paper Trail versions records that can't be directly manipulated by users.

In a perfect world we wouldn't be using YAML serialization for Paper Trail, but I think JSON-based serialization is only available on Postgres. I see you've already identified this as an issue per #1146.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ Actually, scrap that. The proposed change seems to be working without adding models - at least in specs. I'll be more comfortable making this change after I've given the front end a manual test - will look at doing that tomorrow.

@ferrisoxide
Copy link
Contributor Author

PR please for the last changes. They include the suggested changes from @CloCkWeRX as well as a spec to ensure Paper Trail is working as expected with the config - the latter is mostly just to reassure myself.

Re "It'd be nice to have a 3.0 version," I agree, and that was my original plan. I just found too many things were breaking out of the box. In any case, Ruby 3.0 reaches EOL later this year - so we would have only been postponing the inevitable.

@CloCkWeRX CloCkWeRX merged commit 5a708d6 into fatfreecrm:master Aug 10, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants