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.

Sign up for GitHub

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

Add support for Rails 7.1 #686

Merged
merged 2 commits into from
Sep 30, 2023
Merged

Add support for Rails 7.1 #686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bbedccb
Add support for Rails 7.1
yuki24 Sep 24, 2023
bbfb4db
Add fixes for test builds for Ruby 2.3 & 2.4
yuki24 Sep 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ jobs:
- rails60
- rails61
- rails70
- rails71
db: [POSTGRES, MYSQL, SQLITE]
exclude:
# MySQL has issues on Ruby 2.3
Expand Down Expand Up @@ -96,6 +97,16 @@ jobs:
- appraisal: rails70
ruby: 2.6

# Rails 7.1 supports Ruby 2.7+
- appraisal: rails71
ruby: 2.3
- appraisal: rails71
ruby: 2.4
- appraisal: rails71
ruby: 2.5
- appraisal: rails71
ruby: 2.6

services:
postgres:
image: postgres
Expand Down
10 changes: 10 additions & 0 deletions Appraisals
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ appraise "rails50" do
gem "pg", ">= 0.18", "< 2.0"
gem "sqlite3", "~> 1.3.6"
gem "psych", "~> 3.1"
gem "loofah", "2.20.0"
end

appraise "rails51" do
Expand All @@ -15,6 +16,7 @@ appraise "rails51" do
gem "pg", ">= 0.18", "< 2.0"
gem "sqlite3", "~> 1.3.6"
gem "psych", "~> 3.1"
gem "loofah", "2.20.0"
end

appraise "rails52" do
Expand All @@ -23,6 +25,7 @@ appraise "rails52" do
gem "pg", ">= 0.18", "< 2.0"
gem "sqlite3", "~> 1.3.6"
gem "psych", "~> 3.1"
gem "loofah", "2.20.0"
end

appraise "rails60" do
Expand All @@ -45,3 +48,10 @@ appraise "rails70" do
gem "pg", ">= 1.1"
gem "sqlite3", ">= 1.4"
end

appraise "rails71" do
gem "rails", ">= 7.1.0.beta1", "< 7.2"
gem "mysql2", ">= 0.4.4"
gem "pg", ">= 1.1"
gem "sqlite3", ">= 1.4"
end
2 changes: 1 addition & 1 deletion audited.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ Gem::Specification.new do |gem|

gem.required_ruby_version = ">= 2.3.0"

gem.add_dependency "activerecord", ">= 5.0", "< 7.1"
gem.add_dependency "activerecord", ">= 5.0", "< 7.2"
gem.add_dependency "request_store", "~> 1.2"

gem.add_development_dependency "appraisal"
Expand Down
1 change: 1 addition & 0 deletions gemfiles/rails50.gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ gem "mysql2", ">= 0.3.18", "< 0.6.0"
gem "pg", ">= 0.18", "< 2.0"
gem "sqlite3", "~> 1.3.6"
gem "psych", "~> 3.1"
gem "loofah", "2.20.0"

gemspec name: "audited", path: "../"
1 change: 1 addition & 0 deletions gemfiles/rails51.gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ gem "mysql2", ">= 0.3.18", "< 0.6.0"
gem "pg", ">= 0.18", "< 2.0"
gem "sqlite3", "~> 1.3.6"
gem "psych", "~> 3.1"
gem "loofah", "2.20.0"

gemspec name: "audited", path: "../"
1 change: 1 addition & 0 deletions gemfiles/rails52.gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ gem "mysql2", ">= 0.4.4", "< 0.6.0"
gem "pg", ">= 0.18", "< 2.0"
gem "sqlite3", "~> 1.3.6"
gem "psych", "~> 3.1"
gem "loofah", "2.20.0"

gemspec name: "audited", path: "../"
10 changes: 10 additions & 0 deletions gemfiles/rails71.gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# This file was generated by Appraisal

source "https://rubygems.org"

gem "rails", ">= 7.1.0.beta1", "< 7.2"
gem "mysql2", ">= 0.4.4"
gem "pg", ">= 1.1"
gem "sqlite3", ">= 1.4"

gemspec name: "audited", path: "../"
3 changes: 2 additions & 1 deletion lib/audited.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ def audit_class

# remove audit_model in next major version it was only shortly present in 5.1.0
alias_method :audit_model, :audit_class
deprecate audit_model: "use Audited.audit_class instead of Audited.audit_model. This method will be removed."
deprecate audit_model: "use Audited.audit_class instead of Audited.audit_model. This method will be removed.",
deprecator: ActiveSupport::Deprecation.new('6.0.0', 'Audited')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This addresses:

DEPRECATION WARNING: Module.deprecate without a deprecator is deprecated (called from singleton class at ~/lib/audited.rb:25)


def store
RequestStore.store[:audited_store] ||= {}
Expand Down
6 changes: 5 additions & 1 deletion lib/audited/audit.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,11 @@ class Audit < ::ActiveRecord::Base
cattr_accessor :audited_class_names
self.audited_class_names = Set.new

serialize :audited_changes, YAMLIfTextColumnType
if Rails.version >= "7.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to use

Gem::Version.new(Rails.version) >= Gem::Version.new("7.1")

see

[8] pry(main)> Rails.version > "10.2.3"
=> true
[9] pry(main)> Gem::Version.new(Rails.version) >= Gem::Version.new("10.2.3")
=> false

Copy link

Choose a reason for hiding this comment

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

String version comparison is bad, agreed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for pointing this out!
See proposal for its resolution: #689

serialize :audited_changes, coder: YAMLIfTextColumnType
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This addresses the deprecation warning below:

DEPRECATION WARNING: Passing the coder as positional argument is deprecated and will be removed in Rails 7.2.

Please pass the coder as a keyword argument:

  serialize :audited_changes, coder: Audited::YAMLIfTextColumnType
 (called from <class:Audit> at ~/lib/audited/audit.rb:55)

else
serialize :audited_changes, YAMLIfTextColumnType
end

scope :ascending, -> { reorder(version: :asc) }
scope :descending, -> { reorder(version: :desc) }
Expand Down
2 changes: 1 addition & 1 deletion spec/audited/audit_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require "spec_helper"

SingleCov.covered! uncovered: 1 # Rails version check
SingleCov.covered! uncovered: 2 # Rails version check

class CustomAudit < Audited::Audit
def custom_method
Expand Down
25 changes: 24 additions & 1 deletion spec/rails_app/config/application.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,34 @@ class Application < Rails::Application
config.root = File.expand_path("../../", __FILE__)
config.i18n.enforce_available_locales = true

if !Rails.version.start_with?("5.0") && !Rails.version.start_with?("5.1") && config.active_record.respond_to?(:yaml_column_permitted_classes=)
if Rails.version.start_with?("7.1") && config.active_record.respond_to?(:yaml_column_permitted_classes=)
config.active_record.yaml_column_permitted_classes = [
String,
Symbol,
Integer,
NilClass,
Float,
Time,
Date,
FalseClass,
Hash,
Array,
DateTime,
TrueClass,
BigDecimal,
ActiveSupport::TimeWithZone,
ActiveSupport::TimeZone,
ActiveSupport::HashWithIndifferentAccess
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently the elements of permitted classes need to be actual class objects rather than strings. Otherwise there would be a nuch of failing tests with the error message below:

  19) Audited::Auditor revisions should be able to get time for first revision
      Failure/Error: ActiveRecord::Coders::YAMLColumn.new(Object).dump(obj)

      Psych::DisallowedClass:
        Tried to dump unspecified class: ActiveSupport::TimeWithZone
      # ./lib/audited/audit.rb:30:in `dump'
      # ./lib/audited/auditor.rb:363:in `block in write_audit'
      # ./lib/audited/auditor.rb:362:in `write_audit'
      # ./lib/audited/auditor.rb:331:in `audit_create'
      # ./spec/audited/auditor_spec.rb:820:in `block (3 levels) in <top (required)>'

I have taken a look at Psych's code, and it does seem like that this has always been class objects rather than strings. Not sure where the original setting has derived, but either way, this should fix the test for Rails 7.1.

elsif !Rails.version.start_with?("5.0") && !Rails.version.start_with?("5.1") && config.active_record.respond_to?(:yaml_column_permitted_classes=)
config.active_record.yaml_column_permitted_classes =
%w[String Symbol Integer NilClass Float Time Date FalseClass Hash Array DateTime TrueClass BigDecimal
ActiveSupport::TimeWithZone ActiveSupport::TimeZone ActiveSupport::HashWithIndifferentAccess]
end

if Rails.version >= "7.1"
config.active_support.cache_format_version = 7.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This addresses:

DEPRECATION WARNING: Support for `config.active_support.cache_format_version = 6.1` has been deprecated and will be removed in Rails 7.2.

Check the Rails upgrade guide at https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#new-activesupport-cache-serialization-format
for more information on how to upgrade.
 (called from <top (required)> at ~/spec/rails_app/config/environment.rb:5)

end
end
end

Expand Down
7 changes: 6 additions & 1 deletion spec/support/active_record/models.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,12 @@ class User < ::ActiveRecord::Base
attribute :non_column_attr if Rails.version >= "5.1"
attr_protected :logins if respond_to?(:attr_protected)
enum status: {active: 0, reliable: 1, banned: 2}
serialize :phone_numbers, Array

if Rails.version >= "7.1"
serialize :phone_numbers, type: Array
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This addresses:

DEPRECATION WARNING: Passing the class as positional argument is deprecated and will be removed in Rails 7.2.

Please pass the class as a keyword argument:

  serialize :phone_numbers, type: Array
 (called from <class:User> at ~/spec/support/active_record/models.rb:15)

else
serialize :phone_numbers, Array
end

def name=(val)
write_attribute(:name, CGI.escapeHTML(val))
Expand Down