diff --git a/Gemfile b/Gemfile index 1ba6a8d68c..0df173b631 100644 --- a/Gemfile +++ b/Gemfile @@ -20,8 +20,6 @@ gem "activejob-uniqueness" # Needed for XML serialization of ActiveRecord::Base gem 'activemodel-serializers-xml' -gem 'protected_attributes_continued', '~> 1.9.0' - gem 'rails-observers' gem 'strong_migrations', '~> 2.1.0' diff --git a/Gemfile.lock b/Gemfile.lock index e805cbf169..efba18e773 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -618,8 +618,6 @@ GEM bigdecimal logger rb_sys (~> 0.9.117) - protected_attributes_continued (1.9.0) - activemodel (>= 5.0) pry (0.15.2) coderay (~> 1.1) method_source (~> 1.0) @@ -1101,7 +1099,6 @@ DEPENDENCIES prawn prawn-svg prawn-table! - protected_attributes_continued (~> 1.9.0) pry-byebug (>= 3.11.0) pry-doc (>= 0.8) pry-rails diff --git a/app/events/applications/application_deleted_event.rb b/app/events/applications/application_deleted_event.rb index 46afefb366..6657e47fd7 100644 --- a/app/events/applications/application_deleted_event.rb +++ b/app/events/applications/application_deleted_event.rb @@ -5,7 +5,7 @@ class Applications::ApplicationDeletedEvent < ApplicationRelatedEvent # @param [Cinstance] application # :reek:NilCheck but proxy can be nil at this point def self.create(application) - service = application.service || Service.new({id: application.service_id}, without_protection: true) + service = application.service || Service.new(id: application.service_id) new( application: MissingModel::MissingApplication.new(id: application.id), service_backend_id: service.backend_id, diff --git a/app/events/zync_event.rb b/app/events/zync_event.rb index 3061958838..31b6bede09 100644 --- a/app/events/zync_event.rb +++ b/app/events/zync_event.rb @@ -70,7 +70,7 @@ def self.type_for(model) def non_persisted_dependencies case record when Proxy, Cinstance - [Service.new({id: data[:service_id]}, without_protection: true)] + [Service.new(id: data[:service_id])] else NONE end diff --git a/app/lib/backend_api_logic/service_extension.rb b/app/lib/backend_api_logic/service_extension.rb index 64388c99ca..47a624ee2b 100644 --- a/app/lib/backend_api_logic/service_extension.rb +++ b/app/lib/backend_api_logic/service_extension.rb @@ -33,7 +33,11 @@ def backend_api_config end def backend_api - @backend_api ||= backend_api_configs.first&.backend_api || account.backend_apis.build(system_name: service_system_name, name: "#{service_name} Backend", description: "Backend of #{service_name}") + # Return early if we already have a persisted backend_api + return @backend_api if @backend_api&.persisted? + + # Try to get backend_api from backend_api_configs, or keep the memoized unpersisted one, or build a new one + @backend_api = backend_api_configs.first&.backend_api || @backend_api || build_backend_api end def update!(attrs = {}) @@ -52,6 +56,18 @@ def update(attrs = {}) rescue ActiveRecord::RecordInvalid false end + + private + + def build_backend_api + # Build without adding to association to avoid polluting it with unpersisted records + BackendApi.new( + account: account, + system_name: service_system_name, + name: "#{service_name} Backend", + description: "Backend of #{service_name}" + ) + end end def backend_api_proxy diff --git a/app/lib/fields/fields.rb b/app/lib/fields/fields.rb index d207e8828f..668436a863 100644 --- a/app/lib/fields/fields.rb +++ b/app/lib/fields/fields.rb @@ -124,7 +124,7 @@ def initialize(attributes = nil, *args) # otherwise it raises exception on unknown (extra) fields # check http://api.rubyonrails.org/classes/ActiveRecord/AttributeAssignment.html#method-i-assign_attributes - def assign_attributes(extra_attributes, options = {}) + def assign_attributes(extra_attributes) # dup the fields because we are mutating them (params hash) attributes = extra_attributes.present? ? extra_attributes.dup : {} @@ -135,7 +135,7 @@ def assign_attributes(extra_attributes, options = {}) self.extra_fields = fields_attributes end - super(attributes, options) + super(attributes) end alias attributes= assign_attributes diff --git a/app/lib/fields/provider.rb b/app/lib/fields/provider.rb index 9b00cb5fff..a32be3bfda 100644 --- a/app/lib/fields/provider.rb +++ b/app/lib/fields/provider.rb @@ -11,9 +11,8 @@ def initialize(provider) def for(klass) columns = klass.column_names defined = fields_definitions.by_target(klass.name.underscore).map(&:name) - protected = klass.protected_attributes.to_a - ((columns + defined) - protected).uniq + (columns + defined).uniq end protected diff --git a/app/lib/finance/billing.rb b/app/lib/finance/billing.rb index ff9acf7fd1..62235a9c84 100644 --- a/app/lib/finance/billing.rb +++ b/app/lib/finance/billing.rb @@ -8,7 +8,7 @@ def initialize(invoice) def create_line_item(params) bill do - @invoice.line_items.create(params, {without_protection: true}) + @invoice.line_items.create(params) end rescue Invoice::InvalidInvoiceStateException line_item_with_error(params, :invalid_invoice_state) @@ -18,7 +18,7 @@ def create_line_item(params) def create_line_item!(params) bill do - @invoice.line_items.create!(params, {without_protection: true}) + @invoice.line_items.create!(params) end end @@ -37,7 +37,7 @@ def bill end def line_item_with_error(params, error_type) - LineItem.new(params, {without_protection: true}).tap do |line_item| + LineItem.new(params).tap do |line_item| line_item.errors.add(:base, error_type.to_sym) end end diff --git a/app/lib/logic/cms.rb b/app/lib/logic/cms.rb index 78f84bcf8e..e0992614aa 100644 --- a/app/lib/logic/cms.rb +++ b/app/lib/logic/cms.rb @@ -64,7 +64,7 @@ def cms_toolbar_intro_visible? end def create_cms_assets - self.builtin_sections.create!({ title: 'Root', system_name: 'root', parent_id: nil }, without_protection: true) + self.builtin_sections.create!(title: 'Root', system_name: 'root', parent_id: nil) # TODO: add SimpleLayout logic here end end diff --git a/app/lib/signup/impersonation_admin_builder.rb b/app/lib/signup/impersonation_admin_builder.rb index 69d36fa028..14f6d8b38b 100644 --- a/app/lib/signup/impersonation_admin_builder.rb +++ b/app/lib/signup/impersonation_admin_builder.rb @@ -6,14 +6,12 @@ def self.build(account:) config = ThreeScale.config.impersonation_admin username = config[:username] impersonation_admin = account.users.new( - { - username: username, - email: "#{username}+#{account.internal_domain}@#{config[:domain]}", - first_name: '3scale', - last_name: 'Admin', - state: 'active' - }, - without_protection: true) + username: username, + email: "#{username}+#{account.internal_domain}@#{config[:domain]}", + first_name: '3scale', + last_name: 'Admin', + state: 'active' + ) impersonation_admin.role = :admin impersonation_admin.signup_type = :minimal impersonation_admin diff --git a/app/models/account_setting.rb b/app/models/account_setting.rb index 741cbd5fce..2902f6284d 100644 --- a/app/models/account_setting.rb +++ b/app/models/account_setting.rb @@ -11,9 +11,6 @@ def self.find_sti_class(type_name) self end - # TODO: remove attr_accessible once protected_attributes_continued gem is removed - attr_accessible :type, :value, :account, :tenant_id - belongs_to :account, inverse_of: :account_settings audited associated_with: :account diff --git a/app/models/cms/template.rb b/app/models/cms/template.rb index 20206459c0..02895ecdea 100644 --- a/app/models/cms/template.rb +++ b/app/models/cms/template.rb @@ -94,7 +94,7 @@ def current def build_version(attrs = {}) version = versions.build - version.assign_attributes(version_attributes.merge(attrs), :without_protection => true) + version.assign_attributes(version_attributes.merge(attrs)) version end diff --git a/app/models/cms/template/version.rb b/app/models/cms/template/version.rb index 3057a8dc32..f3574c43f5 100644 --- a/app/models/cms/template/version.rb +++ b/app/models/cms/template/version.rb @@ -32,10 +32,8 @@ def diff(other) end def revert_attributes - accessible = template.class.accessible_attributes.delete('draft').add('updated_at').add('updated_by') - - accessible << state.to_s - attributes.slice *accessible + accessible = [:updated_at, :updated_by, state.to_s] + attributes.slice(*accessible) end end diff --git a/app/models/contract.rb b/app/models/contract.rb index 308eb9441b..7db00465d9 100644 --- a/app/models/contract.rb +++ b/app/models/contract.rb @@ -4,7 +4,7 @@ class Contract < ApplicationRecord # https://github.com/collectiveidea/audited/blob/f03c5b5d1717f2ebec64032d269316dc74476056/lib/audited/auditor.rb#L305-L311 self.table_name = 'cinstances' - audited allow_mass_assignment: true + audited # FIXME: This class should be an abstract class I think, but doing so makes plenty of tests fail # self.abstract_class = true diff --git a/app/models/feature.rb b/app/models/feature.rb index efd7c1a8e2..092250d419 100644 --- a/app/models/feature.rb +++ b/app/models/feature.rb @@ -2,7 +2,7 @@ class Feature < ApplicationRecord self.background_deletion = [:features_plans] - audited :allow_mass_assignment => true + audited #TODO: these need tests #OPTIMIZE subclassing a better solution for these 2 diff --git a/app/models/finance/billing_strategy.rb b/app/models/finance/billing_strategy.rb index 25a89e7602..869d46845e 100644 --- a/app/models/finance/billing_strategy.rb +++ b/app/models/finance/billing_strategy.rb @@ -11,7 +11,7 @@ class << self prepend NonAuditedColumns end - audited :allow_mass_assignment => true + audited attr_reader :failed_buyers diff --git a/app/models/invoice.rb b/app/models/invoice.rb index d4a73d3386..b8c2874891 100644 --- a/app/models/invoice.rb +++ b/app/models/invoice.rb @@ -14,7 +14,7 @@ class Invoice < ApplicationRecord enum creation_type: {manual: 'manual', background: 'background'} include AfterCommitQueue - audited :allow_mass_assignment => true + audited has_associated_audits class InvalidInvoiceStateException < RuntimeError; end diff --git a/app/models/line_item.rb b/app/models/line_item.rb index 0f7eee6fd2..c3f428c72a 100644 --- a/app/models/line_item.rb +++ b/app/models/line_item.rb @@ -5,7 +5,7 @@ class LineItem < ApplicationRecord belongs_to :contract, polymorphic: true belongs_to :metric, inverse_of: :line_items - audited associated_with: :invoice, allow_mass_assignment: true + audited associated_with: :invoice validates :name, :description, :type, :contract_type, length: { maximum: 255 } validates :type, inclusion: {in: [LineItem::PlanCost, LineItem::VariableCost].flat_map { |klass| [klass, klass.to_s]}, allow_blank: true} diff --git a/app/models/mail_dispatch_rule.rb b/app/models/mail_dispatch_rule.rb index 800933ada2..61a4239f6b 100644 --- a/app/models/mail_dispatch_rule.rb +++ b/app/models/mail_dispatch_rule.rb @@ -10,14 +10,15 @@ def self.fetch_with_retry!(options, &block) retries = 0 begin - MailDispatchRule.find_or_create_by!(options, &block) + # Use find_by + create! directly to avoid transaction wrapper from create_or_find_by! + # This preserves the old behavior from protected_attributes_continued gem + # where records created in the block are committed even if the outer create fails + find_by(options) || create!(options, &block) rescue ActiveRecord::RecordNotUnique - if retries > 10 - raise - else - retries += 1 - retry - end + raise if retries > 10 + + retries += 1 + retry end end end diff --git a/app/models/metric.rb b/app/models/metric.rb index 2e0c822d33..ff634d0dce 100644 --- a/app/models/metric.rb +++ b/app/models/metric.rb @@ -21,7 +21,7 @@ class Metric < ApplicationRecord has_many :proxy_rules, :dependent => :destroy, inverse_of: :metric - audited :allow_mass_assignment => true + audited has_system_name uniqueness_scope: %i[owner_type owner_id], human_name: :friendly_name acts_as_tree diff --git a/app/models/payment_detail.rb b/app/models/payment_detail.rb index 4600bd5f8d..d503b1a667 100644 --- a/app/models/payment_detail.rb +++ b/app/models/payment_detail.rb @@ -10,7 +10,7 @@ class PaymentDetail < ApplicationRecord belongs_to :account - audited allow_mass_assignment: true + audited validates :credit_card_partial_number, length: { :maximum => 4, :allow_blank => true, diff --git a/app/models/plan.rb b/app/models/plan.rb index 0a915e9d3a..bb4e86b48c 100644 --- a/app/models/plan.rb +++ b/app/models/plan.rb @@ -16,7 +16,7 @@ class PeriodRangeCalculationError < StandardError; end has_system_name :uniqueness_scope => [ :type, :issuer_id, :issuer_type ] - audited :allow_mass_assignment => true + audited acts_as_list scope: %i[issuer_id issuer_type] # There is a race condition here, the record gets deleted but the callbacks were not triggered yet diff --git a/app/models/pricing_rule.rb b/app/models/pricing_rule.rb index f8b631968f..23f84d777b 100644 --- a/app/models/pricing_rule.rb +++ b/app/models/pricing_rule.rb @@ -1,6 +1,6 @@ class PricingRule < ApplicationRecord DECIMALS = 4 - audited :allow_mass_assignment => true + audited belongs_to :plan belongs_to :metric diff --git a/app/models/provider_constraints.rb b/app/models/provider_constraints.rb index f7c602bf90..af72015db8 100644 --- a/app/models/provider_constraints.rb +++ b/app/models/provider_constraints.rb @@ -1,7 +1,7 @@ class ProviderConstraints < ApplicationRecord NO_LIMIT = Float::INFINITY - audited allow_mass_assignment: true + audited belongs_to :provider, class_name: 'Account' diff --git a/app/models/settings.rb b/app/models/settings.rb index 17ee8c8c3f..e765f88e26 100644 --- a/app/models/settings.rb +++ b/app/models/settings.rb @@ -2,7 +2,7 @@ class Settings < ApplicationRecord include Symbolize belongs_to :account, inverse_of: :settings - audited allow_mass_assignment: true + audited validates :product, inclusion: { in: %w[connect enterprise].freeze } validates :change_account_plan_permission, :change_service_plan_permission, inclusion: { in: %w[request none credit_card request_credit_card direct].freeze } @@ -37,8 +37,8 @@ def approval_required_disabled? not_custom_account_plans.size > 1 && account_plans_ui_visible? end - def assign_attributes(attrs, options = {}) - super(sanitize_attributes(attrs), options) + def assign_attributes(attrs) + super(sanitize_attributes(attrs)) end def update(attrs) diff --git a/app/services/finance/stripe_payment_intent_update_service.rb b/app/services/finance/stripe_payment_intent_update_service.rb index 91cf1416eb..3248b6892f 100644 --- a/app/services/finance/stripe_payment_intent_update_service.rb +++ b/app/services/finance/stripe_payment_intent_update_service.rb @@ -41,6 +41,6 @@ def create_payment_transaction params: source_object } - payment_transactions.create(attributes, without_protection: true) + payment_transactions.create(attributes) end end diff --git a/app/services/service_discovery/import_cluster_definitions_service.rb b/app/services/service_discovery/import_cluster_definitions_service.rb index 16ea305b08..940cf4ea73 100644 --- a/app/services/service_discovery/import_cluster_definitions_service.rb +++ b/app/services/service_discovery/import_cluster_definitions_service.rb @@ -91,7 +91,7 @@ def import_cluster_active_docs_to(service) end def build_api_doc_service(service) - service.api_docs_services.build({ name: cluster_service.name, published: true, discovered: true }, without_protection: true) + service.api_docs_services.build(name: cluster_service.name, published: true, discovered: true) end def valid_cluster_service_spec?(service) diff --git a/app/workers/audited_worker.rb b/app/workers/audited_worker.rb index 3c2fd8ff67..4a37c91618 100644 --- a/app/workers/audited_worker.rb +++ b/app/workers/audited_worker.rb @@ -3,7 +3,7 @@ class AuditedWorker def perform(attributes) audit = Audited.audit_class.new - audit.assign_attributes(attributes, without_protection: true) # Rails 4 can remove the option + audit.assign_attributes(attributes) audit.save! end end diff --git a/app/workers/backend_delete_service_worker.rb b/app/workers/backend_delete_service_worker.rb index 93e9e01ac4..e79752b767 100644 --- a/app/workers/backend_delete_service_worker.rb +++ b/app/workers/backend_delete_service_worker.rb @@ -11,7 +11,7 @@ def perform(event_id) event = EventStore::Repository.find_event!(event_id) service_id = event.service_id ThreeScale::Core::Service.delete_stats(service_id, {}) - service = Service.new({id: service_id}, without_protection: true) + service = Service.new(id: service_id) service.delete_backend_service rescue ActiveRecord::RecordNotFound => exception System::ErrorReporting.report_error(exception, parameters: {event_id: event_id}) diff --git a/config/application.rb b/config/application.rb index 98c52c41d3..bcca29a16a 100644 --- a/config/application.rb +++ b/config/application.rb @@ -131,8 +131,6 @@ def try_config_for(*args) config.logger = ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(STDOUT)) if ENV['RAILS_LOG_TO_STDOUT'].present? - config.active_record.whitelist_attributes = false - config.boot_time = Time.now # Configuration for the application, engines, and railties goes here. diff --git a/config/initializers/protected_attributes_hacks.rb b/config/initializers/protected_attributes_hacks.rb deleted file mode 100644 index 10159ab760..0000000000 --- a/config/initializers/protected_attributes_hacks.rb +++ /dev/null @@ -1,27 +0,0 @@ -# OMG!!!! Such bad citizen!!! -# https://github.com/westonganger/protected_attributes_continued/blob/master/lib/active_record/mass_assignment_security/relation.rb -# They just reopened Rails module instead of using their proper module ... -# Thus they totally short circuit all the other modules, not really good with integration with other gems like BabySqueel - -module ProtectedAttributesHacks - module QueryMethods - def sanitize_forbidden_attributes(attributes) - if model._uses_mass_assignment_security - # We just permit everything - super(attributes.respond_to?(:permit!) ? attributes.dup.permit! : attributes) - else - super - end - end - end -end - -ActiveSupport.on_load(:active_record) do - ActiveRecord::QueryMethods.module_eval do - remove_method :sanitize_forbidden_attributes - end - - ActiveRecord::Relation.class_eval do - include ProtectedAttributesHacks::QueryMethods - end -end diff --git a/config/initializers/state_machines_protected_attributes.rb b/config/initializers/state_machines_protected_attributes.rb deleted file mode 100644 index e8c02cbb6b..0000000000 --- a/config/initializers/state_machines_protected_attributes.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true -module StateMachines - module Integrations - module ActiveRecord - if defined?(ProtectedAttributes) - def define_state_initializer - define_helper :instance, <<-end_eval, __FILE__, __LINE__ + 1 - def initialize(attributes = nil, options = {}) - super(attributes, options) do |*args| - self.class.state_machines.initialize_states(self, {}, attributes || {}) - yield(*args) if block_given? - end - end - end_eval - end - end - end - end -end diff --git a/lib/developer_portal/test/dummy/config/application.rb b/lib/developer_portal/test/dummy/config/application.rb index c472c327d2..30c44519a0 100644 --- a/lib/developer_portal/test/dummy/config/application.rb +++ b/lib/developer_portal/test/dummy/config/application.rb @@ -43,12 +43,6 @@ class Application < Rails::Application # like if you have constraints or database-specific column types # config.active_record.schema_format = :sql - # Enforce whitelist mode for mass assignment. - # This will create an empty whitelist of attributes available for mass-assignment for all models - # in your app. As such, your models will need to explicitly whitelist or blacklist accessible - # parameters by using an attr_accessible or attr_protected declaration. - config.active_record.whitelist_attributes = true - # Enable the asset pipeline config.assets.enabled = true diff --git a/lib/developer_portal/test/dummy/config/environments/development.rb b/lib/developer_portal/test/dummy/config/environments/development.rb index 82c74d1541..4db6d3646f 100644 --- a/lib/developer_portal/test/dummy/config/environments/development.rb +++ b/lib/developer_portal/test/dummy/config/environments/development.rb @@ -22,9 +22,6 @@ # Only use best-standards-support built into browsers config.action_dispatch.best_standards_support = :builtin - # Raise exception on mass assignment protection for Active Record models - config.active_record.mass_assignment_sanitizer = :strict - # Log the query plan for queries taking more than this (works # with SQLite, MySQL, and PostgreSQL) config.active_record.auto_explain_threshold_in_seconds = 0.5 diff --git a/lib/developer_portal/test/dummy/config/environments/test.rb b/lib/developer_portal/test/dummy/config/environments/test.rb index f10c53c46d..70d1275f1d 100644 --- a/lib/developer_portal/test/dummy/config/environments/test.rb +++ b/lib/developer_portal/test/dummy/config/environments/test.rb @@ -29,9 +29,6 @@ # ActionMailer::Base.deliveries array. config.action_mailer.delivery_method = :test - # Raise exception on mass assignment protection for Active Record models - config.active_record.mass_assignment_sanitizer = :strict - # Print deprecation notices to the stderr config.active_support.deprecation = :stderr end diff --git a/test/integration/admin/api_docs/account_api_docs_controller_test.rb b/test/integration/admin/api_docs/account_api_docs_controller_test.rb index 57b0d88825..2bee96fc33 100644 --- a/test/integration/admin/api_docs/account_api_docs_controller_test.rb +++ b/test/integration/admin/api_docs/account_api_docs_controller_test.rb @@ -19,7 +19,7 @@ class ProviderLoggedInTest < Admin::ApiDocs::AccountApiDocsControllerTest get preview_admin_api_docs_service_path(api_docs_service) assert_account_active_docs_menus - api_docs_service.update({service_id: service.id}, without_protection: true) + api_docs_service.update({service_id: service.id}) get preview_admin_api_docs_service_path(api_docs_service) assert_redirected_to preview_admin_service_api_doc_path(service, api_docs_service) end @@ -28,7 +28,7 @@ class ProviderLoggedInTest < Admin::ApiDocs::AccountApiDocsControllerTest get edit_admin_api_docs_service_path(api_docs_service) assert_account_active_docs_menus - api_docs_service.update({service_id: service.id}, without_protection: true) + api_docs_service.update({service_id: service.id}) get edit_admin_api_docs_service_path(api_docs_service) assert_redirected_to edit_admin_service_api_doc_path(service, api_docs_service) end diff --git a/test/integration/services/finance/billing_service_integration_test.rb b/test/integration/services/finance/billing_service_integration_test.rb index b467f28aa8..db5a40e3bd 100644 --- a/test/integration/services/finance/billing_service_integration_test.rb +++ b/test/integration/services/finance/billing_service_integration_test.rb @@ -48,7 +48,7 @@ class ConcurrentBillingCalls < Finance::BillingServiceIntegrationTest Finance::BillingService.call!(buyer.id, provider_account_id: provider.id, now: invoice.due_on) assert_equal "unpaid", invoice.reload.state - invoice.update({ last_charging_retry: invoice.last_charging_retry - 4.days }, without_protection: true) + invoice.update(last_charging_retry: invoice.last_charging_retry - 4.days) Finance::BillingService.call!(buyer.id, provider_account_id: provider.id, now: invoice.due_on) assert_equal "unpaid", invoice.reload.state diff --git a/test/test_helpers/provider.rb b/test/test_helpers/provider.rb index 460143026a..7d0a9a32e5 100644 --- a/test/test_helpers/provider.rb +++ b/test/test_helpers/provider.rb @@ -80,15 +80,15 @@ def create_a_complete_provider FactoryBot.create(:keycloak_self_authentication_provider, account: provider) FactoryBot.create(:github_authentication_provider, account: provider) FactoryBot.create(:redhat_customer_portal_authentication_provider, account: provider) - provider.authentication_providers.create!({ type: "AuthenticationProvider::Custom", client_id: 'id', client_secret: 'secret', site: 'http://example.com', account: provider }, { without_protection: true }) - provider.authentication_providers.create!({ type: "AuthenticationProvider::ServiceDiscoveryProvider", client_id: 'id', client_secret: 'secret', site: 'http://example.com', account: provider }, {without_protection: true }) + provider.authentication_providers.create!(type: "AuthenticationProvider::Custom", client_id: 'id', client_secret: 'secret', site: 'http://example.com', account: provider) + provider.authentication_providers.create!(type: "AuthenticationProvider::ServiceDiscoveryProvider", client_id: 'id', client_secret: 'secret', site: 'http://example.com', account: provider) forum = FactoryBot.create(:forum, account: provider) topic = FactoryBot.create(:topic, user: provider.admin_user, forum:) FactoryBot.create(:topic_category, forum:) FactoryBot.create(:post, user: provider.admin_user, forum:, topic:) - UserTopic.create({user: provider.admin_user, topic:}, {without_protection: true}) - Moderatorship.create({user: provider.admin_user, forum:}, {without_protection: true}) + UserTopic.create(user: provider.admin_user, topic: topic) + Moderatorship.create(user: provider.admin_user, forum: forum) Configuration::Value.create(configurable: provider, name: "foo", value: "bar") @@ -106,7 +106,7 @@ def create_a_complete_provider LatestForumPostsPortlet.create!(provider:, portlet_type: 'LatestForumPostsPortlet', system_name: 'name', posts: forum.posts.count) TableOfContentsPortlet.create!(provider:, portlet_type: 'TableOfContentsPortlet', system_name: 'name', section_id: provider.provided_sections.first.id) CMS::Redirect.new.tap do |redirect| - redirect.assign_attributes({source: "a", target: "b", provider:}, {without_protection: true}) + redirect.assign_attributes(source: "a", target: "b", provider: provider) end.save! provider diff --git a/test/unit/api_docs/service_test.rb b/test/unit/api_docs/service_test.rb index 009363d00a..a864c70672 100644 --- a/test/unit/api_docs/service_test.rb +++ b/test/unit/api_docs/service_test.rb @@ -574,7 +574,7 @@ def teardown refute api_doc.valid? assert_includes api_doc.errors[:service], 'not found' - api_doc = account.api_docs_services.new(valid_attributes.merge({service_id: service.id + 1000}), without_protection: true) + api_doc = account.api_docs_services.new(valid_attributes.merge(service_id: service.id + 1000)) refute api_doc.valid? assert_includes api_doc.errors[:service], 'not found' end diff --git a/test/unit/backend_api_logic/service_extension_test.rb b/test/unit/backend_api_logic/service_extension_test.rb index a02199cfea..57b984e44c 100644 --- a/test/unit/backend_api_logic/service_extension_test.rb +++ b/test/unit/backend_api_logic/service_extension_test.rb @@ -36,5 +36,84 @@ class BackendApiProxy < ActiveSupport::TestCase assert_equal backend_api_config, service.backend_api_proxy.backend_api_config end + + test '#backend_api returns backend_api from the first backend_api_configs, if persisted' do + service = FactoryBot.create(:simple_service) + backend_api = FactoryBot.create(:backend_api, account: service.account) + another_backend_api = FactoryBot.create(:backend_api, account: service.account) + FactoryBot.create(:backend_api_config, service: service, backend_api: backend_api, path: '/one') + FactoryBot.create(:backend_api_config, service: service, backend_api: another_backend_api, path: '/two') + + assert_equal 2, service.reload.backend_api_configs.count + + first_result = service.backend_api + assert_equal backend_api, first_result + + # The second call returns the memoized instance + second_result = service.backend_api + assert_same first_result, second_result + end + + test '#update! saves backend_api_config and backend_api for the service' do + service = FactoryBot.create(:simple_service) + + # built on demand + backend_api = service.backend_api + assert_not backend_api.persisted? + assert_not service.backend_api_proxy.backend_api_config.persisted? + + service.backend_api_proxy.update!(private_endpoint: 'https://api.example.com', path: '/backend-path') + + # memoized instances, but now persisted + persisted_backend_api = service.backend_api + + assert persisted_backend_api.persisted? + assert service.backend_api_proxy.backend_api_config.persisted? + + assert_same backend_api, persisted_backend_api + end + + test '#backend_api builds and memoizes unpersisted backend_api' do + service = FactoryBot.create(:simple_service) + proxy = service.backend_api_proxy + + # First call builds unpersisted backend_api + first_call = proxy.backend_api + assert_not first_call.persisted? + assert_equal service.account, first_call.account + assert_equal service.system_name, first_call.system_name + + # Second call returns the same memoized unpersisted instance + second_call = proxy.backend_api + assert_same first_call, second_call + assert_not second_call.persisted? + end + + test '#backend_api_config memoizes across multiple calls' do + service = FactoryBot.create(:simple_service) + proxy = service.backend_api_proxy + + first_call = proxy.backend_api_config + second_call = proxy.backend_api_config + + assert_not first_call.persisted? + assert_not second_call.persisted? + assert_same first_call, second_call + end + + test '#backend_api does not pollute account.backend_apis association with unpersisted records' do + service = FactoryBot.create(:simple_service) + account = service.account + + initial_count = account.backend_apis.count + + # Call backend_api which builds an unpersisted backend_api + backend_api = service.backend_api_proxy.backend_api + assert_not backend_api.persisted? + + # Association should not include unpersisted record + assert_equal initial_count, account.backend_apis.count + assert_not_includes account.backend_apis, backend_api + end end end diff --git a/test/unit/cinstance_test.rb b/test/unit/cinstance_test.rb index 04abae07cb..6990986798 100644 --- a/test/unit/cinstance_test.rb +++ b/test/unit/cinstance_test.rb @@ -979,13 +979,13 @@ def setup end assert_no_difference(EventStore::Event.where(event_type: Applications::ApplicationUpdatedEvent.to_s).method(:count)) do - cinstance.update!({ first_traffic_at: 1.day.ago.round, first_daily_traffic_at: 1.day.ago.round }, without_protection: true) + cinstance.update!(first_traffic_at: 1.day.ago.round, first_daily_traffic_at: 1.day.ago.round) end end test 'ApplicationUpdatedEvent is created after an update when there are updated traffic and non-traffic attributes' do assert_difference(EventStore::Event.where(event_type: Applications::ApplicationUpdatedEvent.to_s).method(:count)) do - cinstance.update!({ name: 'mycinstance', first_traffic_at: 1.day.ago.round, first_daily_traffic_at: 1.day.ago.round }, without_protection: true) + cinstance.update!(name: 'mycinstance', first_traffic_at: 1.day.ago.round, first_daily_traffic_at: 1.day.ago.round) end end end diff --git a/test/unit/mail_dispatch_rule_test.rb b/test/unit/mail_dispatch_rule_test.rb index 1fbbcdeda3..4c05c1a73f 100644 --- a/test/unit/mail_dispatch_rule_test.rb +++ b/test/unit/mail_dispatch_rule_test.rb @@ -9,7 +9,7 @@ def test_fetch_with_retry expected.save! end - assert_equal expected , rule + assert_equal expected, rule end test 'is unique' do diff --git a/test/unit/models_test.rb b/test/unit/models_test.rb index ddd63c0141..c004e7bbde 100644 --- a/test/unit/models_test.rb +++ b/test/unit/models_test.rb @@ -72,7 +72,7 @@ class ModelsTest < ActiveSupport::TestCase next if column_sql_type.match(/\Acharacter varying\Z/) length = column_sql_type.match(/\(([\d]+)\)/)[1].to_i - object = model.new(options, without_protection: true) + object = model.new(options) object.send("#{column_name}=", 'a' * (length + 1)) object.valid? diff --git a/test/unit/tasks/services_test.rb b/test/unit/tasks/services_test.rb index 626e1e2fa5..8a904f3515 100644 --- a/test/unit/tasks/services_test.rb +++ b/test/unit/tasks/services_test.rb @@ -16,7 +16,7 @@ class ServicesTest < ActiveSupport::TestCase default_service = FactoryBot.create(:simple_service) account = default_service.account another_service = FactoryBot.create(:simple_service, account: account) - account.update!({default_service_id: default_service.id}, without_protection: true) + account.update!({default_service_id: default_service.id}) default_service.reload ThreeScale::Core::Service.expects(:make_default).with(another_service.backend_id) @@ -35,9 +35,9 @@ class ServicesTest < ActiveSupport::TestCase test 'destroy_service raises ActiveRecord::RecordNotFound or StateMachines::InvalidTransition when the Account does not have another active Service' do default_service = FactoryBot.create(:simple_service) - default_service.account.update!({default_service_id: default_service.id}, without_protection: true) + default_service.account.update!({default_service_id: default_service.id}) non_default_service = FactoryBot.create(:simple_service) - non_default_service.account.update!({default_service_id: nil}, without_protection: true) + non_default_service.account.update!({default_service_id: nil}) assert_raise(ActiveRecord::RecordNotFound) do execute_rake_task 'services.rake', 'services:destroy_service', default_service.account.id, default_service.id @@ -62,7 +62,7 @@ class ServicesTest < ActiveSupport::TestCase default_service = FactoryBot.create(:simple_service) account = default_service.account non_default_service = FactoryBot.create(:simple_service, account: account) - account.update!({default_service_id: default_service.id}, without_protection: true) + account.update!({default_service_id: default_service.id}) default_service.reload ThreeScale::Core::Service.expects(:make_default).never diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index a67325d51d..0bfcfab3ad 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -8,15 +8,6 @@ class UserTest < ActiveSupport::TestCase subject { @user || FactoryBot.create(:user) } should belong_to :account - should allow_mass_assignment_of :username - should allow_mass_assignment_of :email - should allow_mass_assignment_of :first_name - should allow_mass_assignment_of :last_name - should allow_mass_assignment_of :password - should allow_mass_assignment_of :password_confirmation - should allow_mass_assignment_of :conditions - should allow_mass_assignment_of :service_conditions - should allow_mass_assignment_of :job_role setup do ActionMailer::Base.deliveries = [] diff --git a/test/workers/delete_object_hierarchy_worker_test.rb b/test/workers/delete_object_hierarchy_worker_test.rb index aa9b1b847e..a0d054bc16 100644 --- a/test/workers/delete_object_hierarchy_worker_test.rb +++ b/test/workers/delete_object_hierarchy_worker_test.rb @@ -238,8 +238,8 @@ class DeletePlanTest < ActiveSupport::TestCase def perform_expectations DeleteObjectHierarchyWorker.stubs(:perform_later) - DeleteObjectHierarchyWorker.expects(:perform_later).with(Contract.new({ id: contract.id }, without_protection: true), anything, 'destroy') - DeleteObjectHierarchyWorker.expects(:perform_later).with(Plan.new({ id: customized_plan.id }, without_protection: true), anything, 'destroy') + DeleteObjectHierarchyWorker.expects(:perform_later).with(Contract.new(id: contract.id), anything, 'destroy') + DeleteObjectHierarchyWorker.expects(:perform_later).with(Plan.new(id: customized_plan.id), anything, 'destroy') end class AccountPlanTest < DeletePlanTest @@ -555,7 +555,7 @@ class DeleteCompleteAccountTest < ActiveSupport::TestCase buyer.save! topic = FactoryBot.create(:topic, user: buyer.admin_user, forum: provider.forum) before_objects << topic - topic_subscription = UserTopic.create({user: buyer.admin_user, topic:}, {without_protection: true}) + topic_subscription = UserTopic.create(user: buyer.admin_user, topic: topic) perform_enqueued_jobs(queue: "deletion") do DeleteObjectHierarchyWorker.delete_later buyer