Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Alan Guo Xiang Tan 2024-02-27 09:16:37 +08:00 committed by GitHub
parent 4df4584396
commit 7bcfe60a76
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 251 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
config/locales/server.en.yml
View File

@ -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"

80
lib/theme_settings_object_validator.rb
View File

@ -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

4
lib/theme_settings_validator.rb
View File

@ -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

29
spec/fixtures/theme_settings/invalid_settings.yaml vendored
View File

@ -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

164
spec/lib/theme_settings_object_validator_spec.rb
View File

@ -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

8
spec/models/theme_field_spec.rb
View File

@ -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