From 0cee545248ebc61632da3040efad67441be22a50 Mon Sep 17 00:00:00 2001 From: ulferts Date: Thu, 2 Sep 2021 13:58:24 +0200 Subject: [PATCH] adapt the json_validator from activerecord_json_validator The error messages need to be more fine grained than what can be achieved with the original gem --- Gemfile | 2 +- Gemfile.lock | 5 +- .../user_preferences/update_contract.rb | 9 +- app/models/user_preference.rb | 8 +- app/models/user_preferences/schema.rb | 4 +- app/validators/json_validator.rb | 134 ++++++++++++++ config/locales/en.yml | 8 + .../schemas/api/user_preferences.schema.json | 9 +- ...210825183540_make_user_preferences_json.rb | 2 + .../user_preferences/update_contract_spec.rb | 169 +++++++++++++++++- spec/models/user_preference_spec.rb | 9 + 11 files changed, 341 insertions(+), 18 deletions(-) create mode 100644 app/validators/json_validator.rb diff --git a/Gemfile b/Gemfile index aa45ab6a5a0..5234acb9811 100644 --- a/Gemfile +++ b/Gemfile @@ -97,8 +97,8 @@ gem 'svg-graph', '~> 2.2.0' gem 'date_validator', '~> 0.12.0' gem 'email_validator', '~> 2.2.3' +gem 'json_schemer', '~> 0.2.18' gem 'ruby-duration', '~> 3.2.0' -gem 'activerecord_json_validator', '~> 2.0.0' # provide compatible filesystem information for available storage gem 'sys-filesystem', '~> 1.4.0', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 4fa9f088c83..949f94598ca 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -246,9 +246,6 @@ GEM multi_json (~> 1.11, >= 1.11.2) rack (>= 2.0.8, < 3) railties (>= 5.2.4.1) - activerecord_json_validator (2.0.1) - activerecord (>= 4.2.0, < 7) - json_schemer (~> 0.2.18) activestorage (6.1.4.1) actionpack (= 6.1.4.1) activejob (= 6.1.4.1) @@ -973,7 +970,6 @@ DEPENDENCIES activerecord-import (~> 1.2.0) activerecord-nulldb-adapter (~> 0.7.0) activerecord-session_store (~> 2.0.0) - activerecord_json_validator (~> 2.0.0) acts_as_list (~> 1.0.1) acts_as_tree (~> 2.9.0) addressable (~> 2.8.0) @@ -1023,6 +1019,7 @@ DEPENDENCIES html-pipeline (~> 2.14.0) htmldiff i18n-js (~> 3.9.0) + json_schemer (~> 0.2.18) json_spec (~> 1.1.4) ladle launchy (~> 2.5.0) diff --git a/app/contracts/user_preferences/update_contract.rb b/app/contracts/user_preferences/update_contract.rb index fc785e0090a..55665568143 100644 --- a/app/contracts/user_preferences/update_contract.rb +++ b/app/contracts/user_preferences/update_contract.rb @@ -32,15 +32,16 @@ module UserPreferences class UpdateContract < BaseContract property :settings - USER_PREFERENCE_SCHEMA = Rails.root.join('config/schemas/api/user_preferences.schema.json') - validates :settings, - presence: true, + exclusion: { in: [{}] }, json: { message: ->(errors) do errors.map { |error| JSONSchemer::Errors.pretty(error) } end, - schema: USER_PREFERENCE_SCHEMA + schema: ->(*) { + UserPreferences::Schema.schema + }, + if: -> { model.settings.present? } } end end diff --git a/app/models/user_preference.rb b/app/models/user_preference.rb index 9b660b90b38..8b07444f306 100644 --- a/app/models/user_preference.rb +++ b/app/models/user_preference.rb @@ -41,6 +41,8 @@ class UserPreference < ApplicationRecord # as boolean with ? suffix def method_missing(method_name, *args) key = method_name.to_s + return super unless supported_settings_method?(key) + action = key[-1] case action @@ -57,7 +59,7 @@ class UserPreference < ApplicationRecord # We respond to all methods as we retrieve # the key from settings def respond_to_missing?(method_name, include_private = false) - UserPreferences::Schema.properties.include?(method_name.to_s.gsub(/\?|=\z/, '')) || super + supported_settings_method?(method_name) || super end def [](attr_name) @@ -138,4 +140,8 @@ class UserPreference < ApplicationRecord def time_zone_correctness errors.add(:time_zone, :inclusion) if time_zone.present? && canonical_time_zone.nil? end + + def supported_settings_method?(method_name) + UserPreferences::Schema.properties.include?(method_name.to_s.gsub(/\?|=\z/, '')) + end end diff --git a/app/models/user_preferences/schema.rb b/app/models/user_preferences/schema.rb index d47ca35e98a..9f19ee0e194 100644 --- a/app/models/user_preferences/schema.rb +++ b/app/models/user_preferences/schema.rb @@ -32,8 +32,10 @@ class UserPreferences::Schema class << self PATH = Rails.root.join('config/schemas/api/user_preferences.schema.json') + # TODO: check if Time zones can be verified within the schema + def schema - @schema ||= JSON::load(PATH) + @schema ||= JSON::parse(File.read(PATH)) end def properties diff --git a/app/validators/json_validator.rb b/app/validators/json_validator.rb new file mode 100644 index 00000000000..7d5b8e42347 --- /dev/null +++ b/app/validators/json_validator.rb @@ -0,0 +1,134 @@ +# The code in here, with the exception of the error handling has been copied from the +# activerecord_json_validator gem. + +# frozen_string_literal: true + +class JsonValidator < ActiveModel::EachValidator + def initialize(options) + options.reverse_merge!(schema: nil) + options.reverse_merge!(options: {}) + @attributes = options[:attributes] + + super + + inject_setter_method(options[:class], @attributes) + end + + # Validate the JSON value with a JSON schema path or String + def validate_each(record, attribute, value) + # Validate value with JSON Schemer + errors = JSONSchemer.schema(schema(record), **options.fetch(:options)).validate(value).to_a + + # Everything is good if we don’t have any errors and we got valid JSON value + return if errors.empty? && record.send(:"#{attribute}_invalid_json").blank? + + # Add error message to the attribute + errors.each do |error| + add_error(record, error) + end + end + + protected + + # Redefine the setter method for the attributes, since we want to + # catch JSON parsing errors. + def inject_setter_method(klass, attributes) + attributes.each do |attribute| + # rubocop:disable Style/DocumentDynamicEvalDefinition + klass.class_eval <<-RUBY, __FILE__, __LINE__ + 1 + attr_reader :"#{attribute}_invalid_json" + define_method "#{attribute}=" do |args| + begin + @#{attribute}_invalid_json = nil + args = ::ActiveSupport::JSON.decode(args) if args.is_a?(::String) + super(args) + rescue ActiveSupport::JSON.parse_error + @#{attribute}_invalid_json = args + super({}) + end + end + RUBY + # rubocop:enable Style/DocumentDynamicEvalDefinition + end + end + + # Return a valid schema, recursively calling + # itself until it gets a non-Proc/non-Symbol value. + def schema(record, schema = nil) + schema ||= options.fetch(:schema) + + case schema + when Proc then schema(record, record.instance_exec(&schema)) + when Symbol then schema(record, record.send(schema)) + else schema + end + end + + def add_error(record, error) + data_pointer, type, schema = error.values_at('data_pointer', 'type', 'schema') + path = data_pointer.split('/', 3)[1..] + + case type + when 'required' + add_blank_error(record, error, path) + when 'null', 'string', 'boolean', 'integer', 'number', 'array', 'object' + add_type_mismatch_error(record, path, type) + when 'schema' + add_schema_violated_error(record, path) + when 'format' + add_format_error(record, path, schema.fetch('format')) + when 'enum' + add_enum_error(record, path) + else + add_invalid_error(record, path) + end + end + + def add_blank_error(record, error, path) + keys = error.dig('details', 'missing_keys') + + keys.each do |key| + if path.nil? + record.errors.add(key, :blank) + else + record.errors.add(path[0], :blank_nested, property: (path[1..] + [key]).join('/')) + end + end + end + + def add_type_mismatch_error(record, path, type) + if path.length == 1 + record.errors.add(path[0], :type_mismatch, type: type) + else + record.errors.add(path[0], :type_mismatch_nested, type: type, path: path[1]) + end + end + + def add_schema_violated_error(record, path) + if path.length == 1 + record.errors.add(path[0], :unknown_property) + else + record.errors.add(path[0], :unknown_property_nested, path: path[1]) + end + end + + def add_format_error(record, path, expected) + if path.length == 1 + record.errors.add(path[0], :format, expected: expected) + else + record.errors.add(path[0], :format_nested, expected: expected, path: path[1]) + end + end + + def add_enum_error(record, path) + if path.length == 1 + record.errors.add(path[0], :inclusion) + else + record.errors.add(path[0], :inclusion_nested, path: path[1]) + end + end + + def add_invalid_error(record, path) + record.errors.add(path[0], :invalid) + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index cfb1d58a62e..a54c998d3eb 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -594,6 +594,7 @@ en: before: "must be before %{date}." before_or_equal_to: "must be before or equal to %{date}." blank: "can't be blank." + blank_nested: "needs to have the property '%{property}' set." cant_link_a_work_package_with_a_descendant: "A work package cannot be linked to one of its subtasks." circular_dependency: "This relation would create a circular dependency." confirmation: "doesn't match %{attribute}." @@ -607,11 +608,14 @@ en: even: "must be even." exclusion: "is reserved." file_too_large: "is too large (maximum size is %{count} Bytes)." + format: "does not match the expected format '%{expected}'." + format_nested: "does not match the expected format '%{expected}' at path '%{path}'." greater_than: "must be greater than %{count}." greater_than_or_equal_to: "must be greater than or equal to %{count}." greater_than_or_equal_to_start_date: "must be greater than or equal to the start date." greater_than_start_date: "must be greater than the start date." inclusion: "is not set to one of the allowed values." + inclusion_nested: "is not set to one of the allowed values at path '%{path}'." invalid: "is invalid." invalid_url: 'is not a valid URL.' invalid_url_scheme: 'is not a supported protocol (allowed: %{allowed_schemes}).' @@ -632,7 +636,11 @@ en: taken: "has already been taken." too_long: "is too long (maximum is %{count} characters)." too_short: "is too short (minimum is %{count} characters)." + type_mismatch: "is not of type '%{type}'" + type_mismatch_nested: "is not of type '%{type}' at path '%{path}'" unchangeable: "cannot be changed." + unknown_property: "is not a known property." + unknown_property_nested: "has the unknown path '%{path}'." unremovable: "cannot be removed." wrong_length: "is the wrong length (should be %{count} characters)." models: diff --git a/config/schemas/api/user_preferences.schema.json b/config/schemas/api/user_preferences.schema.json index 81594fc5790..e6bee480a9b 100644 --- a/config/schemas/api/user_preferences.schema.json +++ b/config/schemas/api/user_preferences.schema.json @@ -16,14 +16,12 @@ "type": "boolean" }, "comments_sorting": { - "type": "string" + "type": "string", + "enum": ["asc", "desc"] }, "auto_hide_popups": { "type": "boolean" }, - "self_notified": { - "type": "boolean" - }, "daily_reminders": { "$ref": "#/definitions/DailyReminders" } @@ -33,11 +31,10 @@ "comments_sorting", "daily_reminders", "hide_mail", - "self_notified", "time_zone", "warn_on_leaving_unsaved" ], - "title": "Welcome" + "title": "UserPreferences" }, "DailyReminders": { "type": "object", diff --git a/db/migrate/20210825183540_make_user_preferences_json.rb b/db/migrate/20210825183540_make_user_preferences_json.rb index 1505a6bb2b6..6a356931614 100644 --- a/db/migrate/20210825183540_make_user_preferences_json.rb +++ b/db/migrate/20210825183540_make_user_preferences_json.rb @@ -3,6 +3,8 @@ require_relative './migration_utils/utils' class MakeUserPreferencesJson < ActiveRecord::Migration[6.1] include ::Migration::Utils + # TODO: remove self_notified and switch type of warn_on_leaving_unsaved in DB to boolean (currently is 1/0) + class UserPreferenceWithOthers < ::UserPreference self.table_name = 'user_preferences' serialize :others, Hash diff --git a/spec/contracts/user_preferences/update_contract_spec.rb b/spec/contracts/user_preferences/update_contract_spec.rb index b54f36270e3..b24fb5542c8 100644 --- a/spec/contracts/user_preferences/update_contract_spec.rb +++ b/spec/contracts/user_preferences/update_contract_spec.rb @@ -35,7 +35,24 @@ describe UserPreferences::UpdateContract do let(:current_user) { FactoryBot.build_stubbed(:user) } let(:user) { FactoryBot.build_stubbed :user } - let(:user_preference) { FactoryBot.build_stubbed(:user_preference, user: user) } + let(:user_preference) do + FactoryBot.build_stubbed(:user_preference, + user: user, + settings: settings&.with_indifferent_access) + end + let(:settings) do + { + hide_mail: true, + auto_hide_popups: true, + comments_sorting: 'desc', + daily_reminders: { + enabled: true, + times: %w[08:00:00+00:00 12:00:00+00:00] + }, + time_zone: 'Brasilia', + warn_on_leaving_unsaved: true + } + end let(:contract) { described_class.new(user_preference, current_user) } context 'when current_user is admin' do @@ -78,4 +95,154 @@ describe UserPreferences::UpdateContract do context 'when current_user is a regular user' do it_behaves_like 'contract user is unauthorized' end + + context 'when the settings are nil' do + let(:settings) { nil } + + it_behaves_like 'contract is invalid', settings: :exclusion + end + + context 'without a hide_mail value' do + let(:settings) do + { + auto_hide_popups: true, + comments_sorting: 'desc', + daily_reminders: { + enabled: true, + times: %w[08:00:00+00:00 12:00:00+00:00] + }, + time_zone: 'Brasilia', + warn_on_leaving_unsaved: true + } + end + + it_behaves_like 'contract is invalid', hide_mail: :blank + end + + context 'with a string for hide_mail' do + let(:settings) do + { + auto_hide_popups: true, + comments_sorting: 'desc', + daily_reminders: { + enabled: true, + times: %w[08:00:00+00:00 12:00:00+00:00] + }, + time_zone: 'Brasilia', + warn_on_leaving_unsaved: true, + hide_mail: 'yes please' + } + end + + it_behaves_like 'contract is invalid', hide_mail: :type_mismatch + end + + context 'with a field within the daily_reminders having the wrong type' do + let(:settings) do + { + auto_hide_popups: true, + comments_sorting: 'desc', + daily_reminders: { + enabled: 'sure', + times: %w[08:00:00+00:00 12:00:00+00:00] + }, + time_zone: 'Brasilia', + warn_on_leaving_unsaved: true, + hide_mail: true + } + end + + it_behaves_like 'contract is invalid', daily_reminders: :type_mismatch_nested + end + + context 'with a field within the daily_reminders missing' do + let(:settings) do + { + auto_hide_popups: true, + comments_sorting: 'desc', + daily_reminders: { + times: %w[08:00:00+00:00 12:00:00+00:00] + }, + time_zone: 'Brasilia', + warn_on_leaving_unsaved: true, + hide_mail: true + } + end + + it_behaves_like 'contract is invalid', daily_reminders: :blank_nested + end + + context 'with an extra property' do + let(:settings) do + { + auto_hide_popups: true, + comments_sorting: 'desc', + daily_reminders: { + enabled: true, + times: %w[08:00:00+00:00 12:00:00+00:00] + }, + time_zone: 'Brasilia', + warn_on_leaving_unsaved: true, + hide_mail: true, + foo: true + } + end + + it_behaves_like 'contract is invalid', foo: :unknown_property + end + + context 'with an extra property within the daily_reminders' do + let(:settings) do + { + auto_hide_popups: true, + comments_sorting: 'desc', + daily_reminders: { + enabled: true, + times: %w[08:00:00+00:00 12:00:00+00:00], + foo: true + }, + time_zone: 'Brasilia', + warn_on_leaving_unsaved: true, + hide_mail: true + } + end + + it_behaves_like 'contract is invalid', daily_reminders: :unknown_property_nested + end + + context 'with an invalid time for the daily_reminders' do + let(:settings) do + { + auto_hide_popups: true, + comments_sorting: 'desc', + daily_reminders: { + enabled: true, + times: %w[abc 12:00:00+00:00] + }, + time_zone: 'Brasilia', + warn_on_leaving_unsaved: true, + hide_mail: true + } + end + + it_behaves_like 'contract is invalid', daily_reminders: :format_nested + end + + context 'with an invalid order for comments_sorting' do + let(:settings) do + { + auto_hide_popups: true, + comments_sorting: 'up', + daily_reminders: { + enabled: true, + times: %w[08:00:00+00:00 12:00:00+00:00] + }, + time_zone: 'Brasilia', + warn_on_leaving_unsaved: true, + hide_mail: true + } + end + + it_behaves_like 'contract is invalid', comments_sorting: :inclusion + end end diff --git a/spec/models/user_preference_spec.rb b/spec/models/user_preference_spec.rb index 364377fe7dc..8cf0f4a358f 100644 --- a/spec/models/user_preference_spec.rb +++ b/spec/models/user_preference_spec.rb @@ -82,6 +82,15 @@ describe UserPreference do end end + describe 'an unsupported method' do + context 'for created_at (key not in the schema)' do + it 'raises an error' do + expect { preference.created_at } + .to raise_error NoMethodError + end + end + end + describe 'sort order' do it_behaves_like 'accepts real and false booleans', :comments_in_reverse_order=,