From 7bcfe60a768e5bb0864c18fdc449d7e98f0c3438 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 27 Feb 2024 09:16:37 +0800 Subject: [PATCH] DEV: Validate default value for `type: objects` theme settings (#25833) Why this change? This change adds validation for the default value for `type: objects` theme settings when a setting theme field is uploaded. This helps the theme author to ensure that the objects which they specifc in the default value adhere to the schema which they have declared. When an error is encountered in one of the objects, the error message will look something like: `"The property at JSON Pointer '/0/title' must be at least 5 characters long."` We use a JSON Pointer to reference the property in the object which is something most json-schema validator uses as well. What does this change do? 1. This commit once again changes the shape of hash returned by `ThemeSettingsObjectValidator.validate`. Instead of using the property name as the key previously, we have decided to avoid multiple levels of nesting and instead use a JSON Pointer as the key which helps to simplify the implementation. 2 Introduces `ThemeSettingsObjectValidator.validate_objects` which returns an array of validation error messages for all the objects passed to the method. --- config/locales/server.en.yml | 13 ++ lib/theme_settings_object_validator.rb | 80 +++++++-- lib/theme_settings_validator.rb | 4 + .../theme_settings/invalid_settings.yaml | 29 ++++ .../theme_settings_object_validator_spec.rb | 164 +++++++++++++++--- spec/models/theme_field_spec.rb | 8 + 6 files changed, 251 insertions(+), 47 deletions(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 59bb045e0fa..b5b74a7868b 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -137,18 +137,31 @@ en: string_value_not_valid_min: "Value must be at least %{min} characters long." string_value_not_valid_max: "Value must be at most %{max} characters long." objects: + humanize_required: "The property at JSON Pointer '%{property_json_pointer}' must be present." required: "must be present" + humanize_invalid_type: "The property at JSON Pointer '%{property_json_pointer}' must be of type %{type}." invalid_type: "%{type} is not a valid type" + humanize_not_valid_string_value: "The property at JSON Pointer '%{property_json_pointer}' must be a string." not_valid_string_value: "must be a string" + humanize_not_valid_integer_value: "The property at JSON Pointer '%{property_json_pointer}' must be an integer." not_valid_integer_value: "must be an integer" + humanize_not_valid_float_value: "The property at JSON Pointer '%{property_json_pointer}' must be a float." not_valid_float_value: "must be a float" + humanize_not_valid_boolean_value: "The property at JSON Pointer '%{property_json_pointer}' must be a boolean." not_valid_boolean_value: "must be a boolean" + humanize_not_valid_enum_value: "The property at JSON Pointer '%{property_json_pointer}' must be one of the following %{choices}." not_valid_enum_value: "must be one of the following: %{choices}" + humanize_not_valid_category_value: "The property at JSON Pointer '%{property_json_pointer}' must be a valid category id." not_valid_category_value: "must be a valid category id" + humanize_string_value_not_valid_min: "The property at JSON Pointer '%{property_json_pointer}' must be at least %{min} characters long." string_value_not_valid_min: "must be at least %{min} characters long" + humanize_string_value_not_valid_max: "The property at JSON Pointer '%{property_json_pointer}' must be at most %{max} characters long." string_value_not_valid_max: "must be at most %{max} characters long" + humanize_number_value_not_valid_min: "The property at JSON Pointer '%{property_json_pointer}' must be larger than or equal to %{min}." number_value_not_valid_min: "must be larger than or equal to %{min}" + humanize_number_value_not_valid_max: "The property at JSON Pointer '%{property_json_pointer}' must be smaller than or equal to %{max}." number_value_not_valid_max: "must be smaller than or equal to %{max}" + humanize_string_value_not_valid_url: "The property at JSON Pointer '%{property_json_pointer}' must be a valid URL." string_value_not_valid_url: "must be a valid URL" locale_errors: top_level_locale: "The top level key in a locale file must match the locale name" diff --git a/lib/theme_settings_object_validator.rb b/lib/theme_settings_object_validator.rb index 7ad36889e47..e641ba7f3b7 100644 --- a/lib/theme_settings_object_validator.rb +++ b/lib/theme_settings_object_validator.rb @@ -1,37 +1,72 @@ # frozen_string_literal: true class ThemeSettingsObjectValidator + class << self + def validate_objects(schema:, objects:) + error_messages = [] + + objects.each_with_index do |object, index| + humanize_error_messages( + self.new(schema: schema, object: object).validate, + index:, + error_messages:, + ) + end + + error_messages + end + + private + + def humanize_error_messages(errors, index:, error_messages:) + errors.each do |property_json_pointer, error_details| + error_messages.push(*error_details.humanize_messages("/#{index}#{property_json_pointer}")) + end + end + end + class ThemeSettingsObjectErrors def initialize @errors = [] end - def add_error(key, i18n_opts = {}) - @errors << ThemeSettingsObjectError.new(key, i18n_opts) + def add_error(error, i18n_opts = {}) + @errors << ThemeSettingsObjectError.new(error, i18n_opts) + end + + def humanize_messages(property_json_pointer) + @errors.map { |error| error.humanize_messages(property_json_pointer) } end def full_messages @errors.map(&:error_message) end end - class ThemeSettingsObjectError - def initialize(key, i18n_opts = {}) - @key = key + def initialize(error, i18n_opts = {}) + @error = error @i18n_opts = i18n_opts end + def humanize_messages(property_json_pointer) + I18n.t( + "themes.settings_errors.objects.humanize_#{@error}", + @i18n_opts.merge(property_json_pointer:), + ) + end + def error_message - I18n.t("themes.settings_errors.objects.#{@key}", @i18n_opts) + I18n.t("themes.settings_errors.objects.#{@error}", @i18n_opts) end end - def initialize(schema:, object:, valid_category_ids: nil) + def initialize(schema:, object:, valid_category_ids: nil, json_pointer_prefix: "", errors: {}) @object = object @schema_name = schema[:name] @properties = schema[:properties] - @errors = {} + @errors = errors @valid_category_ids = valid_category_ids + @json_pointer_prefix = json_pointer_prefix end def validate @@ -39,15 +74,17 @@ class ThemeSettingsObjectValidator @properties.each do |property_name, property_attributes| if property_attributes[:type] == "objects" - @object[property_name]&.each do |child_object| - @errors[property_name] ||= [] - - @errors[property_name].push( - self - .class - .new(schema: property_attributes[:schema], object: child_object, valid_category_ids:) - .validate, - ) + @object[property_name]&.each_with_index do |child_object, index| + self + .class + .new( + schema: property_attributes[:schema], + object: child_object, + valid_category_ids:, + json_pointer_prefix: "#{@json_pointer_prefix}#{property_name}/#{index}/", + errors: @errors, + ) + .validate end end end @@ -148,8 +185,13 @@ class ThemeSettingsObjectValidator end def add_error(property_name, key, i18n_opts = {}) - @errors[property_name] ||= ThemeSettingsObjectErrors.new - @errors[property_name].add_error(key, i18n_opts) + pointer = json_pointer(property_name) + @errors[pointer] ||= ThemeSettingsObjectErrors.new + @errors[pointer].add_error(key, i18n_opts) + end + + def json_pointer(property_name) + "/#{@json_pointer_prefix}#{property_name}" end def valid_category_ids diff --git a/lib/theme_settings_validator.rb b/lib/theme_settings_validator.rb index 19ad9d7f758..03385884376 100644 --- a/lib/theme_settings_validator.rb +++ b/lib/theme_settings_validator.rb @@ -51,6 +51,10 @@ class ThemeSettingsValidator errors:, translation_prefix: "string", ) + when types[:objects] + errors.push( + ThemeSettingsObjectValidator.validate_objects(schema: opts[:schema], objects: value), + ) end errors diff --git a/spec/fixtures/theme_settings/invalid_settings.yaml b/spec/fixtures/theme_settings/invalid_settings.yaml index 184f7e59691..68adfd1d8f3 100644 --- a/spec/fixtures/theme_settings/invalid_settings.yaml +++ b/spec/fixtures/theme_settings/invalid_settings.yaml @@ -17,3 +17,32 @@ default_out_of_range: string_default_out_of_range: default: "abcdefg" min: 20 + +invalid_default_objects_setting: + type: objects + default: + - min_5_chars_string: "12345" + children: + - required_integer: 1 + - required_string: "some string" + min_5_chars_string: "123" + children: + - non_existent: "string" + schema: + name: parent + properties: + required_string: + type: string + required: true + min_5_chars_string: + type: string + validations: + min_length: 5 + children: + type: objects + schema: + name: child + properties: + required_integer: + type: integer + required: true diff --git a/spec/lib/theme_settings_object_validator_spec.rb b/spec/lib/theme_settings_object_validator_spec.rb index f3c75f03b54..c0c4ae79606 100644 --- a/spec/lib/theme_settings_object_validator_spec.rb +++ b/spec/lib/theme_settings_object_validator_spec.rb @@ -1,6 +1,76 @@ # frozen_string_literal: true RSpec.describe ThemeSettingsObjectValidator do + describe ".validate_objects" do + it "should return the right array of humanized error messages for objects that are invalid" do + schema = { + name: "section", + properties: { + title: { + type: "string", + required: true, + validations: { + min_length: 5, + max_length: 10, + }, + }, + category_property: { + type: "category", + required: true, + }, + links: { + type: "objects", + schema: { + name: "link", + properties: { + position: { + type: "integer", + required: true, + }, + float: { + type: "float", + required: true, + validations: { + min: 5.5, + max: 11.5, + }, + }, + }, + }, + }, + }, + } + + category = Fabricate(:category) + + error_messages = + described_class.validate_objects( + schema: schema, + objects: [ + { + title: "1234", + category_property: category.id, + links: [{ position: 1, float: 4.5 }, { position: "string", float: 12 }], + }, + { title: "12345678910", category_property: 99_999_999, links: [{ float: 5 }] }, + ], + ) + + expect(error_messages).to eq( + [ + "The property at JSON Pointer '/0/title' must be at least 5 characters long.", + "The property at JSON Pointer '/0/links/0/float' must be larger than or equal to 5.5.", + "The property at JSON Pointer '/0/links/1/position' must be an integer.", + "The property at JSON Pointer '/0/links/1/float' must be smaller than or equal to 11.5.", + "The property at JSON Pointer '/1/title' must be at most 10 characters long.", + "The property at JSON Pointer '/1/category_property' must be a valid category id.", + "The property at JSON Pointer '/1/links/0/position' must be present.", + "The property at JSON Pointer '/1/links/0/float' must be larger than or equal to 5.5.", + ], + ) + end + end + describe "#validate" do it "should return the right hash of error messages when properties are required but missing" do schema = { @@ -46,8 +116,9 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema:, object: {}).validate - expect(errors[:description].full_messages).to contain_exactly("must be present") - expect(errors[:title].full_messages).to contain_exactly("must be present") + expect(errors.keys).to contain_exactly("/description", "/title") + expect(errors["/description"].full_messages).to contain_exactly("must be present") + expect(errors["/title"].full_messages).to contain_exactly("must be present") errors = described_class.new( @@ -57,19 +128,30 @@ RSpec.describe ThemeSettingsObjectValidator do }, ).validate - expect(errors[:title].full_messages).to contain_exactly("must be present") - expect(errors[:description].full_messages).to contain_exactly("must be present") - expect(errors[:links][0][:name].full_messages).to contain_exactly("must be present") + expect(errors.keys).to eq( + %w[ + /title + /description + /links/0/name + /links/0/child_links/0/title + /links/0/child_links/1/title + /links/1/name + ], + ) - expect(errors[:links][0][:child_links][0][:title].full_messages).to contain_exactly( + expect(errors["/title"].full_messages).to contain_exactly("must be present") + expect(errors["/description"].full_messages).to contain_exactly("must be present") + expect(errors["/links/0/name"].full_messages).to contain_exactly("must be present") + + expect(errors["/links/0/child_links/0/title"].full_messages).to contain_exactly( "must be present", ) - expect(errors[:links][0][:child_links][1][:title].full_messages).to contain_exactly( + expect(errors["/links/0/child_links/1/title"].full_messages).to contain_exactly( "must be present", ) - expect(errors[:links][1][:name].full_messages).to contain_exactly("must be present") + expect(errors["/links/1/name"].full_messages).to contain_exactly("must be present") end context "for enum properties" do @@ -95,7 +177,9 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { enum_property: "random_value" }).validate - expect(errors[:enum_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/enum_property"]) + + expect(errors["/enum_property"].full_messages).to contain_exactly( "must be one of the following: [\"choice 1\", 2, false]", ) end @@ -103,7 +187,9 @@ RSpec.describe ThemeSettingsObjectValidator do it "should return the right hash of error messages when enum property is not present" do errors = described_class.new(schema: schema, object: {}).validate - expect(errors[:enum_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/enum_property"]) + + expect(errors["/enum_property"].full_messages).to contain_exactly( "must be one of the following: [\"choice 1\", 2, false]", ) end @@ -126,7 +212,8 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { boolean_property: "string" }).validate - expect(errors[:boolean_property].full_messages).to contain_exactly("must be a boolean") + expect(errors.keys).to eq(["/boolean_property"]) + expect(errors["/boolean_property"].full_messages).to contain_exactly("must be a boolean") end end @@ -146,7 +233,8 @@ RSpec.describe ThemeSettingsObjectValidator do it "should return the right hash of error messages when value of property is not of type float" do errors = described_class.new(schema: schema, object: { float_property: "string" }).validate - expect(errors[:float_property].full_messages).to contain_exactly("must be a float") + expect(errors.keys).to eq(["/float_property"]) + expect(errors["/float_property"].full_messages).to contain_exactly("must be a float") end it "should return the right hash of error messages when integer property does not satisfy min or max validations" do @@ -165,13 +253,17 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { float_property: 4.5 }).validate - expect(errors[:float_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/float_property"]) + + expect(errors["/float_property"].full_messages).to contain_exactly( "must be larger than or equal to 5.5", ) errors = described_class.new(schema: schema, object: { float_property: 12.5 }).validate - expect(errors[:float_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/float_property"]) + + expect(errors["/float_property"].full_messages).to contain_exactly( "must be smaller than or equal to 11.5", ) end @@ -190,11 +282,13 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { integer_property: "string" }).validate - expect(errors[:integer_property].full_messages).to contain_exactly("must be an integer") + expect(errors.keys).to eq(["/integer_property"]) + expect(errors["/integer_property"].full_messages).to contain_exactly("must be an integer") errors = described_class.new(schema: schema, object: { integer_property: 1.0 }).validate - expect(errors[:integer_property].full_messages).to contain_exactly("must be an integer") + expect(errors.keys).to eq(["/integer_property"]) + expect(errors["/integer_property"].full_messages).to contain_exactly("must be an integer") end it "should not return any error messages when the value of the integer property satisfies min and max validations" do @@ -232,13 +326,17 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { integer_property: 4 }).validate - expect(errors[:integer_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/integer_property"]) + + expect(errors["/integer_property"].full_messages).to contain_exactly( "must be larger than or equal to 5", ) errors = described_class.new(schema: schema, object: { integer_property: 11 }).validate - expect(errors[:integer_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/integer_property"]) + + expect(errors["/integer_property"].full_messages).to contain_exactly( "must be smaller than or equal to 10", ) end @@ -257,7 +355,8 @@ RSpec.describe ThemeSettingsObjectValidator do schema = { name: "section", properties: { string_property: { type: "string" } } } errors = described_class.new(schema: schema, object: { string_property: 1 }).validate - expect(errors[:string_property].full_messages).to contain_exactly("must be a string") + expect(errors.keys).to eq(["/string_property"]) + expect(errors["/string_property"].full_messages).to contain_exactly("must be a string") end it "should return the right hash of error messages when string property does not statisfy url validation" do @@ -276,7 +375,8 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { string_property: "not a url" }).validate - expect(errors[:string_property].full_messages).to contain_exactly("must be a valid URL") + expect(errors.keys).to eq(["/string_property"]) + expect(errors["/string_property"].full_messages).to contain_exactly("must be a valid URL") end it "should not return any error messages when the value of the string property satisfies min_length and max_length validations" do @@ -314,14 +414,18 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { string_property: "1234" }).validate - expect(errors[:string_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/string_property"]) + + expect(errors["/string_property"].full_messages).to contain_exactly( "must be at least 5 characters long", ) errors = described_class.new(schema: schema, object: { string_property: "12345678910" }).validate - expect(errors[:string_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/string_property"]) + + expect(errors["/string_property"].full_messages).to contain_exactly( "must be at most 10 characters long", ) end @@ -344,7 +448,9 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { category_property: "string" }).validate - expect(errors[:category_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/category_property"]) + + expect(errors["/category_property"].full_messages).to contain_exactly( "must be a valid category id", ) end @@ -390,19 +496,21 @@ RSpec.describe ThemeSettingsObjectValidator do }, ).validate - expect(errors[:category_property].full_messages).to contain_exactly( + expect(errors.keys).to eq( + %w[/category_property /category_property_2 /child_categories/0/category_property_3], + ) + + expect(errors["/category_property"].full_messages).to contain_exactly( "must be a valid category id", ) - expect(errors[:category_property_2].full_messages).to contain_exactly( + expect(errors["/category_property_2"].full_messages).to contain_exactly( "must be a valid category id", ) expect( - errors[:child_categories][0][:category_property_3].full_messages, + errors["/child_categories/0/category_property_3"].full_messages, ).to contain_exactly("must be a valid category id") - - expect(errors[:child_categories][1]).to eq({}) end # only 1 SQL query should be executed to check if category ids are valid diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb index e9ec45c544d..d996ea7b1fc 100644 --- a/spec/models/theme_field_spec.rb +++ b/spec/models/theme_field_spec.rb @@ -412,6 +412,14 @@ HTML ) end + it "generates the right errors when setting of type objects have default values which does not matches the schema" do + field = create_yaml_field(get_fixture("invalid")) + + expect(field.error).to include( + "Setting `invalid_default_objects_setting` default value isn't valid. The property at JSON Pointer '/0/required_string' must be present. The property at JSON Pointer '/1/min_5_chars_string' must be at least 5 characters long. The property at JSON Pointer '/1/children/0/required_integer' must be present.", + ) + end + it "works correctly when valid yaml is provided" do field = create_yaml_field(get_fixture("valid")) expect(field.error).to be_nil