From e1da17587b10df8ae3b29d7583c3a4881a377f97 Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Tue, 21 May 2024 17:00:10 +0100 Subject: [PATCH 1/2] Fix missing and duplicated data in docblock --- lib/govuk_schemas/random_example.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/govuk_schemas/random_example.rb b/lib/govuk_schemas/random_example.rb index 1131217..7e6200a 100644 --- a/lib/govuk_schemas/random_example.rb +++ b/lib/govuk_schemas/random_example.rb @@ -29,7 +29,8 @@ class RandomExample # GovukSchemas::RandomExample.new(schema: schema, seed: 777).payload # GovukSchemas::RandomExample.new(schema: schema, seed: 777).payload # returns same as above # - # @param [Hash] schema A JSON schema. + # @param [Hash] schema A JSON schema. + # @param [Integer, nil] seed A random number seed for deterministic results # @return [GovukSchemas::RandomExample] def initialize(schema:, seed: nil) @schema = schema @@ -54,8 +55,6 @@ def initialize(schema:, seed: nil) # @param [Block] the base payload is passed inton the block, with the block result then becoming # the new payload. The new payload is then validated. (optional) # @return [GovukSchemas::RandomExample] - # @param [Block] the base payload is passed inton the block, with the block result then becoming - # the new payload. The new payload is then validated. (optional) def self.for_schema(schema_key_value, &block) schema = GovukSchemas::Schema.find(schema_key_value) GovukSchemas::RandomExample.new(schema:).payload(&block) From 3a7e16baa0a28381b95250e07971756e30dc7d0d Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Tue, 21 May 2024 19:45:20 +0100 Subject: [PATCH 2/2] Avoid double validation of payloads The call to validate a schema is quite expensive: ``` [3] pry(#)> puts Benchmark.measure { 100.times { JSON::Validator.fully_validate(@schema, item, errors_as_objects: true) } } 7.614167 1.532485 9.146652 ( 10.468765) ``` This updates the code to avoid making two calls to the validator when a payload is generated with a block if the customised version is valid. Before: ``` irb(main):010:0> puts Benchmark.measure { 100.times { GovukSchemas::RandomExample.for_schema(frontend_schema: "news_artic le") { |schema| schema["title"] = "Custom"; schema } } } 13.308641 3.087679 16.396320 ( 18.977005) ``` After: ``` irb(main):002:0> puts Benchmark.measure { 100.times { GovukSchemas::RandomExample.for_schema(frontend_schema: "news_artic le") { |schema| schema["title"] = "Custom"; schema } } } 7.654303 1.561620 9.215923 ( 10.604802) ``` The motivation for this was that the GOV.UK Chat test suite makes quite heavy use of random schemas with customisations for checking supported schemas. On a MBP with GOV.UK Docker this reduces the time of the running the test suite from around ~55s to around ~35s. --- CHANGELOG.md | 4 +++ lib/govuk_schemas/random_example.rb | 34 ++++++++++++++----- spec/lib/random_example_spec.rb | 52 +++++++++++++++++------------ 3 files changed, 60 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e4e4f2..52bff70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# Unreleased + +* Improve speed of random schema generation for customised content items + # 5.0.0 * BREAKING: Drop support for Ruby 3.0. The minimum required Ruby version is now 3.1.4. diff --git a/lib/govuk_schemas/random_example.rb b/lib/govuk_schemas/random_example.rb index 7e6200a..f3d0a7a 100644 --- a/lib/govuk_schemas/random_example.rb +++ b/lib/govuk_schemas/random_example.rb @@ -79,24 +79,40 @@ def self.for_schema(schema_key_value, &block) # the new payload. The new payload is then validated. (optional) # @return [Hash] A content item # @raise [GovukSchemas::InvalidContentGenerated] - def payload + def payload(&block) payload = @random_generator.payload - # ensure the base payload is valid + + return customise_payload(payload, &block) if block + errors = validation_errors_for(payload) raise InvalidContentGenerated, error_message(payload, errors) if errors.any? - if block_given? - payload = yield(payload) - # check the payload again after customisation - errors = validation_errors_for(payload) - raise InvalidContentGenerated, error_message(payload, errors, customised: true) if errors.any? - end - payload end private + def customise_payload(payload) + original_payload = payload + customised_payload = yield(payload) + customised_errors = validation_errors_for(customised_payload) + + if customised_errors.any? + # Check if the original payload had errors and report those over + # any from customisation. This is not done prior to generating the + # customised payload because validation is time expensive and we + # want to avoid it if possible. + original_errors = validation_errors_for(original_payload) + errors = original_errors.any? ? original_errors : customised_errors + payload = original_errors.any? ? original_payload : customised_payload + message = error_message(payload, errors, customised: original_errors.empty?) + + raise InvalidContentGenerated, message + end + + customised_payload + end + def validation_errors_for(item) JSON::Validator.fully_validate(@schema, item, errors_as_objects: true) end diff --git a/spec/lib/random_example_spec.rb b/spec/lib/random_example_spec.rb index 48e4316..e2e4e32 100644 --- a/spec/lib/random_example_spec.rb +++ b/spec/lib/random_example_spec.rb @@ -32,34 +32,44 @@ expect(first_payload).to eql(second_payload) end - it "can customise the payload" do - schema = GovukSchemas::Schema.random_schema(schema_type: "frontend") - - example = GovukSchemas::RandomExample.new(schema:).payload do |hash| - hash.merge("base_path" => "/some-base-path") - end + it "raises an error if the generated schema is invalid" do + generator = instance_double(GovukSchemas::RandomSchemaGenerator, payload: {}) + allow(GovukSchemas::RandomSchemaGenerator).to receive(:new).and_return(generator) - expect(example["base_path"]).to eql("/some-base-path") + schema = GovukSchemas::Schema.random_schema(schema_type: "frontend") + expect { GovukSchemas::RandomExample.new(schema:).payload } + .to raise_error(GovukSchemas::InvalidContentGenerated) end - it "failes when attempting to edit the hash in place" do - schema = GovukSchemas::Schema.random_schema(schema_type: "frontend") + context "when the payload is customised" do + it "returns the customised payload" do + schema = GovukSchemas::Schema.random_schema(schema_type: "frontend") - expect { - GovukSchemas::RandomExample.new(schema:).payload do |hash| - hash["base_path"] = "/some-base-path" + example = GovukSchemas::RandomExample.new(schema:).payload do |hash| + hash.merge("base_path" => "/some-base-path") end - }.to raise_error(GovukSchemas::InvalidContentGenerated) - end - it "raises if the resulting content item won't be valid" do - schema = GovukSchemas::Schema.random_schema(schema_type: "frontend") + expect(example["base_path"]).to eql("/some-base-path") + end - expect { - GovukSchemas::RandomExample.new(schema:).payload do |hash| - hash.merge("base_path" => nil) - end - }.to raise_error(GovukSchemas::InvalidContentGenerated) + it "raises if the resulting content item won't be valid" do + schema = GovukSchemas::Schema.random_schema(schema_type: "frontend") + + expect { + GovukSchemas::RandomExample.new(schema:).payload do |hash| + hash.merge("base_path" => nil) + end + }.to raise_error(GovukSchemas::InvalidContentGenerated, /The item was valid before being customised/) + end + + it "raises if the non-customised content item was invalid" do + generator = instance_double(GovukSchemas::RandomSchemaGenerator, payload: {}) + allow(GovukSchemas::RandomSchemaGenerator).to receive(:new).and_return(generator) + + schema = GovukSchemas::Schema.random_schema(schema_type: "frontend") + expect { GovukSchemas::RandomExample.new(schema:).payload { |h| h.merge("base_path" => "/test") } } + .to raise_error(GovukSchemas::InvalidContentGenerated, /An invalid content item was generated/) + end end end end