diff --git a/Gemfile b/Gemfile index b22eb288d..a42b47a30 100644 --- a/Gemfile +++ b/Gemfile @@ -2,6 +2,9 @@ source "https://rubygems.org" gemspec +# While https://github.com/brandonhilkert/sucker_punch/pull/253 is outstanding +gem "sucker_punch", git: "https://github.com/pact-foundation/sucker_punch.git", ref: "fix/rename-is-singleton-class-method-2" + gem "rake", "~>12.3.3" gem "sqlite3", "~>1.3" gem "conventional-changelog", "~>1.3" diff --git a/lib/pact_broker/api.rb b/lib/pact_broker/api.rb index d5a66b2d2..56c635dad 100644 --- a/lib/pact_broker/api.rb +++ b/lib/pact_broker/api.rb @@ -2,6 +2,7 @@ require "pact_broker/db/models" require "pact_broker/api/resources" require "pact_broker/api/decorators" +require "pact_broker/api/contracts" require "pact_broker/application_context" require "pact_broker/feature_toggle" diff --git a/lib/pact_broker/api/contracts.rb b/lib/pact_broker/api/contracts.rb new file mode 100644 index 000000000..c62ee7cd2 --- /dev/null +++ b/lib/pact_broker/api/contracts.rb @@ -0,0 +1,3 @@ +Dir.glob(File.expand_path(File.join(__FILE__, "..", "contracts", "*.rb"))).sort.each do | path | + require path +end diff --git a/lib/pact_broker/api/contracts/README.md b/lib/pact_broker/api/contracts/README.md new file mode 100644 index 000000000..77fcc6c0e --- /dev/null +++ b/lib/pact_broker/api/contracts/README.md @@ -0,0 +1,21 @@ +# Using dry-validation + +## Things you need to know about dry-validation + +* It's better than it used to be. +* There are two parts to a Dry Validation contract. + * Schemas: are for defining TYPES and presence of values. They come in 3 different flavours - json, string params, and vanilla https://dry-rb.org/gems/dry-validation/1.8/schemas/. We generally use the JSON one. + * Rules: are for applying VALIDATION RULES. Don't try and put validation rules in the schema, or you'll end up wanting to punch something. +* The schema is applied first, and only if it passes are the rules evaluated for that particular parameter. +* The rules do NOT stop evaluating when one of them fails. Beth thinks this is less than ideal. +* Macros allow you to apply rules to fields in a declarative way. https://dry-rb.org/gems/dry-validation/1.8/macros/ We have some [here](lib/pact_broker/api/contracts/dry_validation_macros.rb) +* The docs are brief, but worth skimming through + * https://dry-rb.org/gems/dry-validation/1.8/ + * https://dry-rb.org/gems/dry-schema/1.10/ +* You can include modules and define methods inside the Contract classes. +* the "filled?" predicate in dry-schema will fail for "" but pass for " " + +## dry-validation modifications for Pact Broker + +* I cannot find any native support for Contract composition (there's schema composition, but I can't find Contract composition). The macros `validate_with_contract` and `validate_each_with_contract` in lib/pact_broker/api/contracts/dry_validation_macros.rb allow a child item or array of child items to be validated by a contract that is defined in a separate file, merging the results into the parent's results. There's an example in lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema.rb +* The format that dry-validation returns the error messages for arrays didn't work with the original Pact Broker error format, so the errors are formatted in lib/pact_broker/api/contracts/dry_validation_errors_formatter.rb (one day, the errors should be returned directly to the resource, and the error decorator can either format them in problem json, or raw hash) diff --git a/lib/pact_broker/api/contracts/base_contract.rb b/lib/pact_broker/api/contracts/base_contract.rb index 891deeef5..945fcbc9f 100644 --- a/lib/pact_broker/api/contracts/base_contract.rb +++ b/lib/pact_broker/api/contracts/base_contract.rb @@ -1,14 +1,29 @@ -require "reform" -require "reform/form/dry" - -Reform::Form.class_eval do - feature Reform::Form::Dry -end +require "dry-validation" +require "pact_broker/api/contracts/dry_validation_macros" +require "pact_broker/api/contracts/dry_validation_methods" +require "pact_broker/api/contracts/dry_validation_errors_formatter" +require "pact_broker/messages" +require "pact_broker/hash_refinements" module PactBroker module Api module Contracts - class BaseContract < Reform::Form + class BaseContract < Dry::Validation::Contract + include DryValidationMethods + extend DryValidationErrorsFormatter + + using PactBroker::HashRefinements + + # The entry method for all the Dry::Validation::Contract classes + # eg. MyContract.call(params) + # It takes the params (doesn't matter if they're string or symbol keys) + # executes the dry-validation validation, and formats the errors into the Pactflow format. + # + # @param [Hash] the parameters to validate + # @return [Hash] the validation errors to display to the user + def self.call(params) + format_errors(new.call(params&.symbolize_keys).errors) + end end end end diff --git a/lib/pact_broker/api/contracts/can_i_deploy_query_schema.rb b/lib/pact_broker/api/contracts/can_i_deploy_query_schema.rb new file mode 100644 index 000000000..47d708d67 --- /dev/null +++ b/lib/pact_broker/api/contracts/can_i_deploy_query_schema.rb @@ -0,0 +1,34 @@ +require "dry-validation" +require "pact_broker/messages" +require "pact_broker/project_root" +require "pact_broker/string_refinements" + +module PactBroker + module Api + module Contracts + class CanIDeployQuerySchema < BaseContract + using PactBroker::StringRefinements + + params do + required(:pacticipant).filled(:string) + required(:version).filled(:string) + optional(:to).filled(:string) + optional(:environment).filled(:string) + end + + rule(:pacticipant).validate(:pacticipant_with_name_exists) + rule(:environment).validate(:environment_with_name_exists) + + rule(:to, :environment) do + if provided?(values[:to]) && provided?(values[:environment]) + base.failure(PactBroker::Messages.message("errors.validation.cannot_specify_tag_and_environment")) + end + + if not_provided?(values[:to]) && not_provided?(values[:environment]) + base.failure(PactBroker::Messages.message("errors.validation.must_specify_environment_or_tag")) + end + end + end + end + end +end diff --git a/lib/pact_broker/api/contracts/consumer_version_selector_contract.rb b/lib/pact_broker/api/contracts/consumer_version_selector_contract.rb new file mode 100644 index 000000000..dda334b59 --- /dev/null +++ b/lib/pact_broker/api/contracts/consumer_version_selector_contract.rb @@ -0,0 +1,140 @@ +require "pact_broker/api/contracts/base_contract" + +module PactBroker + module Api + module Contracts + class ConsumerVersionSelectorContract < BaseContract + option :parent # the parent hash in which the ConsumerVersionSelector is embedded + + BRANCH_KEYS = [:latest, :tag, :fallbackTag, :branch, :fallbackBranch, :matchingBranch, :mainBranch] + ENVIRONMENT_KEYS = [:environment, :deployed, :released, :deployedOrReleased] + ALL_KEYS = BRANCH_KEYS + ENVIRONMENT_KEYS + [:consumer] + + json do + optional(:mainBranch).filled(included_in?: [true]) + optional(:tag).filled(:str?) + optional(:branch).filled(:str?) + optional(:matchingBranch).filled(included_in?: [true]) + optional(:latest).filled(included_in?: [true, false]) + optional(:fallbackTag).filled(:str?) + optional(:fallbackBranch).filled(:str?) + optional(:consumer).filled(:str?) + optional(:deployed).filled(included_in?: [true]) + optional(:released).filled(included_in?: [true]) + optional(:deployedOrReleased).filled(included_in?: [true]) + optional(:environment).filled(:str?) + end + + rule(:consumer).validate(:not_blank_if_present) + + # has minimum required params + rule(*ALL_KEYS) do + if not_provided?(values[:mainBranch]) && + not_provided?(values[:tag]) && + not_provided?(values[:branch]) && + not_provided?(values[:environment]) && + values[:matchingBranch] != true && + values[:deployed] != true && + values[:released] != true && + values[:deployedOrReleased] != true && + values[:latest] != true + + base.failure(validation_message("pacts_for_verification_selector_required_params_missing")) + end + end + + # mainBranch + rule(*ALL_KEYS) do + if values[:mainBranch] && values.slice(*ALL_KEYS - [:consumer, :mainBranch, :latest]).any? + base.failure(validation_message("pacts_for_verification_selector_main_branch_with_other_param_disallowed")) + end + end + + # mainBranch/latest + rule(:mainBranch, :latest) do + if values[:mainBranch] && values[:latest] == false + base.failure(validation_message("pacts_for_verification_selector_main_branch_and_latest_false_disallowed")) + end + end + + # matchingBranch + rule(*ALL_KEYS) do + if values[:matchingBranch] && values.slice(*ALL_KEYS - [:consumer, :matchingBranch]).any? + base.failure(validation_message("pacts_for_verification_selector_matching_branch_with_other_param_disallowed")) + end + + if values[:matchingBranch] && not_provided?(parent[:providerVersionBranch]) + base.failure(validation_message("pacts_for_verification_selector_matching_branch_requires_provider_version_branch")) + end + end + + # tag and branch + rule(:tag, :branch) do + if values[:tag] && values[:branch] + base.failure(validation_message("pacts_for_verification_selector_tag_and_branch_disallowed")) + end + end + + # branch/environment keys + rule(*ALL_KEYS) do + non_environment_fields = values.slice(*BRANCH_KEYS).keys.sort + environment_related_fields = values.slice(*ENVIRONMENT_KEYS).keys.sort + + if (non_environment_fields.any? && environment_related_fields.any?) + base.failure("cannot specify the #{PactBroker::Messages.pluralize("field", non_environment_fields.count)} #{non_environment_fields.join("/")} with the #{PactBroker::Messages.pluralize("field", environment_related_fields.count)} #{environment_related_fields.join("/")}") + end + end + + # fallbackTag + rule(:fallbackTag, :tag, :latest) do + if values[:fallbackTag] && !values[:latest] + base.failure(validation_message("pacts_for_verification_selector_fallback_tag")) + end + + if values[:fallbackTag] && !values[:tag] + base.failure(validation_message("pacts_for_verification_selector_fallback_tag_requires_tag")) + end + end + + # fallbackBranch + rule(:fallbackBranch, :branch, :latest) do + if values[:fallbackBranch] && !values[:branch] + base.failure(validation_message("pacts_for_verification_selector_fallback_branch_requires_branch")) + end + + + if values[:fallbackBranch] && values[:latest] == false + base.failure(validation_message("pacts_for_verification_selector_fallback_branch_and_latest_false_disallowed")) + end + end + + # branch/latest + rule(:branch, :latest) do + if values[:branch] && values[:latest] == false + base.failure(validation_message("pacts_for_verification_selector_branch_and_latest_false_disallowed")) + end + end + + # environment + rule(:environment) do + validate_environment_with_name_exists(value, key) if provided?(value) + end + + # deployed, released, deployedOrReleased + rule(:deployed, :released, :deployedOrReleased) do + if values[:deployed] && values[:released] + base.failure(validation_message("pacts_for_verification_selector_deployed_and_released_disallowed")) + end + + if values[:deployed] && values[:deployedOrReleased] + base.failure(validation_message("pacts_for_verification_selector_deployed_and_deployed_or_released_disallowed")) + end + + if values[:released] && values[:deployedOrReleased] + base.failure(validation_message("pacts_for_verification_selector_released_and_deployed_or_released_disallowed")) + end + end + end + end + end +end \ No newline at end of file diff --git a/lib/pact_broker/api/contracts/dry_validation_errors_formatter.rb b/lib/pact_broker/api/contracts/dry_validation_errors_formatter.rb new file mode 100644 index 000000000..7a3002e31 --- /dev/null +++ b/lib/pact_broker/api/contracts/dry_validation_errors_formatter.rb @@ -0,0 +1,50 @@ +require "pact_broker/error" + +module PactBroker + module Api + module Contracts + module DryValidationErrorsFormatter + + # Formats the dry validation errors in the expected PactBroker error format of { :key => ["errors"] } + # where there are no nested hashes. + # @param [Dry::Validation::MessageSet] errors + # @return [Hash] + def format_errors(errors) + errors.each_with_object({}) do | error, errors_hash | + integers = error.path.select{ | k | k.is_a?(Integer) } + + if integers.size > 1 + raise PactBroker::Error, "Cannot currently format an error message with more than one index" + end + + if integers.empty? + key = error.path == [nil] ? nil : error.path.join(".").to_sym + add_error(errors_hash, key, error.text) + else + add_indexed_error(errors_hash, error) + end + end + end + + # @private + def add_error(errors_hash, key, text) + errors_hash[key] ||= [] + errors_hash[key] << text + end + + # @private + def add_indexed_error(errors_hash, error) + error_path_classes = error.path.collect(&:class) + if error_path_classes == [Symbol, Integer, Symbol] + add_error(errors_hash, error.path.first, "#{error.path.last} #{error.text} (at index #{error.path[1]})") + elsif error_path_classes == [Symbol, Integer] + add_error(errors_hash, error.path.first, "#{error.text} (at index #{error.path[1]})") + else + # Don't have any usecases for this - will deal with it when it happens + raise PactBroker::Error, "Cannot currently format an error message with path classes #{error_path_classes}" + end + end + end + end + end +end diff --git a/lib/pact_broker/api/contracts/dry_validation_macros.rb b/lib/pact_broker/api/contracts/dry_validation_macros.rb new file mode 100644 index 000000000..26dde0aa3 --- /dev/null +++ b/lib/pact_broker/api/contracts/dry_validation_macros.rb @@ -0,0 +1,79 @@ +require "pact_broker/api/contracts/dry_validation_methods" + +Dry::Validation.register_macro(:not_multiple_lines) do + PactBroker::Api::Contracts::DryValidationMethods.validate_not_multiple_lines(value, key) +end + +Dry::Validation.register_macro(:no_spaces_if_present) do + PactBroker::Api::Contracts::DryValidationMethods.validate_no_spaces_if_present(value, key) +end + +Dry::Validation.register_macro(:not_blank_if_present) do + PactBroker::Api::Contracts::DryValidationMethods.validate_not_blank_if_present(value, key) +end + +Dry::Validation.register_macro(:array_values_not_blank_if_any) do + value&.each_with_index do | item, index | + PactBroker::Api::Contracts::DryValidationMethods.validate_not_blank_if_present(item, key(path.keys + [index])) + end +end + +Dry::Validation.register_macro(:valid_url_if_present) do + PactBroker::Api::Contracts::DryValidationMethods.validate_valid_url(value, key) +end + +Dry::Validation.register_macro(:valid_version_number) do + PactBroker::Api::Contracts::DryValidationMethods.validate_version_number(value, key) +end + +Dry::Validation.register_macro(:pacticipant_with_name_exists) do + PactBroker::Api::Contracts::DryValidationMethods.validate_pacticipant_with_name_exists(value, key) +end + +Dry::Validation.register_macro(:environment_with_name_exists) do + PactBroker::Api::Contracts::DryValidationMethods.validate_environment_with_name_exists(value, key) +end + +# Validate each object in an array with the specified contract, +# and merge the errors into the appropriate path in the parent +# validation results. +# eg. +# rule(:myChildArray).validate(validate_each_with_contract: MyChildContract) +# +# If the child contract defines a option called `parent` then it can access the parent +# hash for validation rules that need to work across the levels. +# eg. ConsumerVersionSelectorContract for the matchingBranch rule +# +# Will not add any errors if the array is nil +Dry::Validation.register_macro(:validate_each_with_contract) do |macro:| + value&.each_with_index do | item, index | + child_contract_class = macro.args[0] + messages = child_contract_class.new(parent: values).call(item).errors(full: true).to_hash.values.flatten + messages.each do | message | + key(path.keys + [index]).failure(message) + end + end +end + +# Validate a child node with the specified contract, +# and merge the errors into the appropriate path in the parent +# validation results. +# eg. +# rule(:myChildHash).validate(validate_with_contract: MyChildContract) +# +# If the child contract defines a option called `parent` then it can access the parent +# hash for validation rules that need to work across the levels. +# eg. ConsumerVersionSelectorContract for the matchingBranch rule +# +# Will not add any errors if the value is nil +Dry::Validation.register_macro(:validate_with_contract) do |macro:| + if value + child_contract_class = macro.args[0] + errors = child_contract_class.new(parent: values).call(value).errors.to_hash + errors.each do | key, messages | + messages.each do | message | + key(path.keys + [key]).failure(message) + end + end + end +end diff --git a/lib/pact_broker/api/contracts/dry_validation_methods.rb b/lib/pact_broker/api/contracts/dry_validation_methods.rb new file mode 100644 index 000000000..fafc883d2 --- /dev/null +++ b/lib/pact_broker/api/contracts/dry_validation_methods.rb @@ -0,0 +1,71 @@ +require "pact_broker/api/contracts/validation_helpers" + +module PactBroker + module Api + module Contracts + module DryValidationMethods + extend self + + def validation_message(key, params = {}) + PactBroker::Messages.validation_message(key, params) + end + + def provided?(value) + ValidationHelpers.provided?(value) + end + + def not_provided?(value) + ValidationHelpers.not_provided?(value) + end + + def validate_version_number(value, key) + if !PactBroker::Api::Contracts::ValidationHelpers.valid_version_number?(value) + key.failure(PactBroker::Messages.validation_message("invalid_version_number", value: value)) + end + end + + def validate_url(value, key) + if PactBroker::Api::Contracts::ValidationHelpers.valid_url?(value) + key.failure(PactBroker::Messages.validation_message("invalid_url")) + end + end + + def validate_pacticipant_with_name_exists(value, key) + if ValidationHelpers.provided?(value) && !ValidationHelpers.pacticipant_with_name_exists?(value) + key.failure(PactBroker::Messages.validation_message("pacticipant_with_name_not_found")) + end + end + + def validate_environment_with_name_exists(value, key) + if ValidationHelpers.provided?(value) && !ValidationHelpers.environment_with_name_exists?(value) + key.failure(PactBroker::Messages.validation_message("environment_not_found", value: value)) + end + end + + def validate_not_blank_if_present(value, key) + if value && ValidationHelpers.blank?(value) + key.failure(PactBroker::Messages.validation_message("blank")) + end + end + + def validate_no_spaces_if_present(value, key) + if value && ValidationHelpers.includes_space?(value) + key.failure(PactBroker::Messages.validation_message("no_spaces")) + end + end + + def validate_not_multiple_lines(value, key) + if value && ValidationHelpers.multiple_lines?(value) + key.failure(PactBroker::Messages.validation_message("single_line")) + end + end + + def validate_valid_url(value, key) + if value && !ValidationHelpers.valid_url?(value) + key.failure(PactBroker::Messages.validation_message("invalid_url")) + end + end + end + end + end +end diff --git a/lib/pact_broker/api/contracts/dry_validation_predicates.rb b/lib/pact_broker/api/contracts/dry_validation_predicates.rb deleted file mode 100644 index 4cbb87e86..000000000 --- a/lib/pact_broker/api/contracts/dry_validation_predicates.rb +++ /dev/null @@ -1,36 +0,0 @@ -require "dry-validation" - -module PactBroker - module Api - module Contracts - module DryValidationPredicates - include Dry::Logic::Predicates - - predicate(:date?) do |value| - DateTime.parse(value) rescue false - end - - predicate(:base64?) do |value| - Base64.strict_decode64(value) rescue false - end - - predicate(:not_blank?) do | value | - value && value.is_a?(String) && value.strip.size > 0 - end - - predicate(:single_line?) do | value | - value && value.is_a?(String) && !value.include?("\n") - end - - predicate(:no_spaces?) do | value | - value && value.is_a?(String) && !value.include?(" ") - end - - predicate(:environment_with_name_exists?) do | value | - require "pact_broker/deployments/environment_service" - !!PactBroker::Deployments::EnvironmentService.find_by_name(value) - end - end - end - end -end diff --git a/lib/pact_broker/api/contracts/dry_validation_workarounds.rb b/lib/pact_broker/api/contracts/dry_validation_workarounds.rb deleted file mode 100644 index c1a77a1d2..000000000 --- a/lib/pact_broker/api/contracts/dry_validation_workarounds.rb +++ /dev/null @@ -1,39 +0,0 @@ -module PactBroker - module Api - module Contracts - module DryValidationWorkarounds - extend self - - # I just cannot seem to get the validation to stop on the first error. - # If one rule fails, they all come back failed, and it's driving me nuts. - # Why on earth would I want that behaviour? - def select_first_message(messages) - messages.each_with_object({}) do | (key, value), new_messages | - new_messages[key] = [value.first] - end - end - - def flatten_array_of_hashes(array_of_hashes) - array_of_hashes.collect do | index, hash_or_array | - array = hash_or_array.is_a?(Hash) ? hash_or_array.values.flatten : hash_or_array - array.collect { | text | "#{text} at index #{index}"} - end.flatten - end - - def flatten_indexed_messages(messages) - if messages.values.any?{ | value | is_indexed_structure?(value) } - messages.each_with_object({}) do | (key, value), new_messages | - new_messages[key] = is_indexed_structure?(value) ? flatten_array_of_hashes(value) : value - end - else - messages - end - end - - def is_indexed_structure?(thing) - thing.is_a?(Hash) && thing.keys.first.is_a?(Integer) - end - end - end - end -end diff --git a/lib/pact_broker/api/contracts/environment_schema.rb b/lib/pact_broker/api/contracts/environment_schema.rb index a994063f3..3d6866765 100644 --- a/lib/pact_broker/api/contracts/environment_schema.rb +++ b/lib/pact_broker/api/contracts/environment_schema.rb @@ -1,48 +1,34 @@ -require "dry-validation" -require "pact_broker/api/contracts/dry_validation_workarounds" -require "pact_broker/api/contracts/dry_validation_predicates" -require "pact_broker/messages" +require "pact_broker/api/contracts/base_contract" module PactBroker module Api module Contracts - class EnvironmentSchema - extend DryValidationWorkarounds - extend PactBroker::Messages - using PactBroker::HashRefinements - - SCHEMA = Dry::Validation.Schema do - configure do - predicates(DryValidationPredicates) - config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) - end - required(:name).filled(:str?, :single_line?, :no_spaces?) - optional(:displayName).maybe(:str?, :single_line?) + class EnvironmentSchema < BaseContract + json do + optional(:uuid) + required(:name).filled(:string) + optional(:displayName).maybe(:string) required(:production).filled(included_in?: [true, false]) - optional(:contacts).each do - schema do - required(:name).filled(:str?, :single_line?) - optional(:details).schema do - end - end + optional(:contacts).array(:hash) do + required(:name).filled(:string) + optional(:details).hash end end - def self.call(params_with_string_keys) - params = params_with_string_keys&.symbolize_keys - results = select_first_message(flatten_indexed_messages(SCHEMA.call(params).messages(full: true))) - validate_name(params, results) - results - end + rule(:name).validate(:not_multiple_lines, :no_spaces_if_present) + rule(:displayName).validate(:not_multiple_lines) - def self.validate_name(params, results) - if (environment_with_same_name = PactBroker::Deployments::EnvironmentService.find_by_name(params[:name])) - if environment_with_same_name.uuid != params[:uuid] - results[:name] ||= [] - results[:name] << message("errors.validation.environment_name_must_be_unique", name: params[:name]) + rule(:name, :uuid) do + if (environment_with_same_name = PactBroker::Deployments::EnvironmentService.find_by_name(values[:name])) + if environment_with_same_name.uuid != values[:uuid] + key.failure(validation_message("environment_name_must_be_unique", name: values[:name])) end end end + + rule(:contacts).each do + validate_not_multiple_lines(value[:name], key(path.keys + [:name])) + end end end end diff --git a/lib/pact_broker/api/contracts/pacticipant_create_schema.rb b/lib/pact_broker/api/contracts/pacticipant_create_schema.rb index b2ae305c9..229476a4b 100644 --- a/lib/pact_broker/api/contracts/pacticipant_create_schema.rb +++ b/lib/pact_broker/api/contracts/pacticipant_create_schema.rb @@ -3,25 +3,12 @@ module PactBroker module Api module Contracts - class PacticipantCreateSchema - extend DryValidationWorkarounds - extend PactBroker::Messages - using PactBroker::HashRefinements - - SCHEMA = Dry::Validation.Schema do - configure do - predicates(DryValidationPredicates) - config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) - end - required(:name).filled(:str?, :single_line?) + class PacticipantCreateSchema < PactBroker::Api::Contracts::PacticipantSchema + json do + required(:name).filled(:string) end - def self.call(params_with_string_keys) - params = params_with_string_keys&.symbolize_keys - update_errors = PacticipantSchema::SCHEMA.call(params).messages(full: true) - create_errors = SCHEMA.call(params).messages(full: true) - select_first_message(flatten_indexed_messages(update_errors.merge(create_errors))) - end + rule(:name).validate(:not_blank_if_present, :not_multiple_lines) end end end diff --git a/lib/pact_broker/api/contracts/pacticipant_name_contract.rb b/lib/pact_broker/api/contracts/pacticipant_name_contract.rb deleted file mode 100644 index a406eb46d..000000000 --- a/lib/pact_broker/api/contracts/pacticipant_name_contract.rb +++ /dev/null @@ -1,24 +0,0 @@ -require "pact_broker/api/contracts/base_contract" - -module PactBroker - module Api - module Contracts - class PacticipantNameContract < BaseContract - property :name - property :name_in_pact - property :pacticipant - property :message_args - - include PactBroker::Messages - - def blank? string - string && string.strip.empty? - end - - def empty? string - string.nil? || blank?(string) - end - end - end - end -end diff --git a/lib/pact_broker/api/contracts/pacticipant_name_validation.rb b/lib/pact_broker/api/contracts/pacticipant_name_validation.rb deleted file mode 100644 index f02ab97fe..000000000 --- a/lib/pact_broker/api/contracts/pacticipant_name_validation.rb +++ /dev/null @@ -1,30 +0,0 @@ -module PactBroker - module Api - module Contracts - module PacticipantNameValidation - - include PactBroker::Messages - - def name_in_pact_present - unless name_in_pact - errors.add(:'name', validation_message("pact_missing_pacticipant_name", pacticipant: pacticipant)) - end - end - - def name_not_blank - if blank? name - errors.add(:'name', validation_message("blank")) - end - end - - def blank? string - string && string.strip.empty? - end - - def empty? string - string.nil? || blank?(string) - end - end - end - end -end diff --git a/lib/pact_broker/api/contracts/pacticipant_schema.rb b/lib/pact_broker/api/contracts/pacticipant_schema.rb index 2ba239e89..1c2307276 100644 --- a/lib/pact_broker/api/contracts/pacticipant_schema.rb +++ b/lib/pact_broker/api/contracts/pacticipant_schema.rb @@ -1,33 +1,24 @@ -require "dry-validation" -require "pact_broker/api/contracts/dry_validation_workarounds" -require "pact_broker/api/contracts/dry_validation_predicates" -require "pact_broker/messages" +require "pact_broker/api/contracts/base_contract" module PactBroker module Api module Contracts - class PacticipantSchema - extend DryValidationWorkarounds - extend PactBroker::Messages - using PactBroker::HashRefinements - - SCHEMA = Dry::Validation.Schema do - configure do - predicates(DryValidationPredicates) - config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) - end - optional(:name).filled(:str?, :single_line?) - optional(:displayName).maybe(:str?, :single_line?, :not_blank?) - optional(:mainBranch).maybe(:str?, :single_line?, :no_spaces?) - optional(:repositoryUrl).maybe(:str?, :single_line?) - optional(:repositoryName).maybe(:str?, :single_line?) - optional(:repositoryNamespace).maybe(:str?, :single_line?) + class PacticipantSchema < BaseContract + json do + optional(:name).filled(:string) + optional(:displayName).maybe(:string) + optional(:mainBranch).maybe(:string) + optional(:repositoryUrl).maybe(:string) + optional(:repositoryName).maybe(:string) + optional(:repositoryNamespace).maybe(:string) end - def self.call(params_with_string_keys) - params = params_with_string_keys&.symbolize_keys - select_first_message(flatten_indexed_messages(SCHEMA.call(params).messages(full: true))) - end + rule(:name).validate(:not_multiple_lines, :not_blank_if_present) + rule(:displayName).validate(:not_multiple_lines, :not_blank_if_present) + rule(:mainBranch).validate(:not_multiple_lines, :no_spaces_if_present, :not_blank_if_present) + rule(:repositoryUrl).validate(:not_multiple_lines) + rule(:repositoryName).validate(:not_multiple_lines) + rule(:repositoryNamespace).validate(:not_multiple_lines) end end end diff --git a/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema.rb b/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema.rb index e90b433cf..563436324 100644 --- a/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema.rb +++ b/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema.rb @@ -1,161 +1,55 @@ -require "dry-validation" -require "pact_broker/hash_refinements" -require "pact_broker/string_refinements" -require "pact_broker/api/contracts/dry_validation_workarounds" -require "pact_broker/api/contracts/dry_validation_predicates" -require "pact_broker/messages" +require "pact_broker/api/contracts/base_contract" +require "pact_broker/api/contracts/consumer_version_selector_contract" +require "pact_broker/logging" module PactBroker module Api module Contracts - class PactsForVerificationJSONQuerySchema - extend DryValidationWorkarounds - extend PactBroker::Messages + class PactsForVerificationJSONQuerySchema < BaseContract + include PactBroker::Logging - using PactBroker::HashRefinements - using PactBroker::StringRefinements - - BRANCH_KEYS = [:latest, :tag, :fallbackTag, :branch, :fallbackBranch, :matchingBranch] - ENVIRONMENT_KEYS = [:environment, :deployed, :released, :deployedOrReleased] - ALL_KEYS = BRANCH_KEYS + ENVIRONMENT_KEYS - - SCHEMA = Dry::Validation.Schema do - configure do - predicates(DryValidationPredicates) - config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) - end + json do + optional(:providerVersionBranch).maybe(:string) optional(:providerVersionTags).maybe(:array?) - optional(:consumerVersionSelectors).each do - schema do - # configure do - # def self.messages - # super.merge(en: { errors: { fallbackTagMustBeForLatest: 'can only be set if latest=true' }}) - # end - # end - - optional(:mainBranch).filled(included_in?: [true]) - optional(:tag).filled(:str?) - optional(:branch).filled(:str?) - optional(:matchingBranch).filled(included_in?: [true]) - optional(:latest).filled(included_in?: [true, false]) - optional(:fallbackTag).filled(:str?) - optional(:fallbackBranch).filled(:str?) - optional(:consumer).filled(:str?, :not_blank?) - optional(:deployed).filled(included_in?: [true]) - optional(:released).filled(included_in?: [true]) - optional(:deployedOrReleased).filled(included_in?: [true]) - optional(:environment).filled(:str?, :environment_with_name_exists?) - - # rule(fallbackTagMustBeForLatest: [:fallbackTag, :latest]) do | fallback_tag, latest | - # fallback_tag.filled?.then(latest.eql?(true)) - # end - end - end + optional(:consumerVersionSelectors).array(:hash) optional(:includePendingStatus).filled(included_in?: [true, false]) - optional(:includeWipPactsSince).filled(:date?) - end - - def self.call(params) - symbolized_params = params&.symbolize_keys - results = select_first_message(flatten_indexed_messages(SCHEMA.call(symbolized_params).messages(full: true))) - add_cross_field_validation_errors(symbolized_params, results) - results + optional(:includeWipPactsSince).filled(:date) end - def self.add_cross_field_validation_errors(params, results) - # This is a ducking joke. Need to get rid of dry-validation - if params[:consumerVersionSelectors].is_a?(Array) - errors = params[:consumerVersionSelectors].each_with_index.flat_map do | selector, index | - validate_consumer_version_selector(selector, index, params) + # The original implementation of pacts-for-verification unfortunately went out without any validation on the + # providerVersionBranch at all (most likely unintentionally.) + # When we added + # optional(:providerVersionBranch).filled(:string) + # during the dry-validation upgrade, we discovered that many users/pact clients were requesting pacts for verification + # with an empty string in the providerVersionBranch + # This complicated logic tries to not break those customers as much as possible, while still raising an error + # when the blank string is most likely a configuration error + # (eg. when the request performs logic that uses the provider version branch) + # It allows the providerVersionBranch to be unspecified/nil, as that most likely means the user did not + # specify the branch at all. + rule(:providerVersionBranch, :providerVersionTags, :includePendingStatus, :includeWipPactsSince) do + branch = values[:providerVersionBranch] + + # a space is a clear user error - don't bother checking further + if branch && branch.size > 0 + validate_not_blank_if_present(branch, key) + end + + if !rule_error? + tags = values[:providerVersionTags] + include_pending = values[:includePendingStatus] + wip = values[:includeWipPactsSince] + + # There are no tags, the user specified wip or pending, and they set a branch, but the branch is an empty or blank string... + if !tags&.any? && (wip || include_pending) && branch && ValidationHelpers.blank?(branch) + # most likely a user error - return a message + #key.failure(validation_message("pacts_for_verification_selector_provider_version_branch_empty")) + logger.info("pacts_for_verification_empty_provider_version", values.data) end - if errors.any? - results[:consumerVersionSelectors] ||= [] - results[:consumerVersionSelectors].concat(errors) - end - end - end - - def self.not_provided?(value) - value.nil? || value.blank? - end - - # when setting a tag, latest=true and latest=false are both valid - # when setting the branch, it doesn't make sense to verify all pacts for a branch, - # so latest is not required, but cannot be set to false - # rubocop: disable Metrics/CyclomaticComplexity, Metrics/MethodLength - def self.validate_consumer_version_selector(selector, index, params) - errors = [] - - if selector[:fallbackTag] && !selector[:latest] - errors << "fallbackTag can only be set if latest is true (at index #{index})" - end - - if selector[:fallbackBranch] && selector[:latest] == false - errors << "fallbackBranch can only be set if latest is true (at index #{index})" - end - - if not_provided?(selector[:mainBranch]) && - not_provided?(selector[:tag]) && - not_provided?(selector[:branch]) && - not_provided?(selector[:environment]) && - selector[:matchingBranch] != true && - selector[:deployed] != true && - selector[:released] != true && - selector[:deployedOrReleased] != true && - selector[:latest] != true - errors << "must specify a value for environment or tag or branch, or specify mainBranch=true, matchingBranch=true, latest=true, deployed=true, released=true or deployedOrReleased=true (at index #{index})" - end - - if selector[:mainBranch] && selector.slice(*ALL_KEYS - [:consumer, :mainBranch]).any? - errors << "cannot specify mainBranch=true with any other criteria apart from consumer (at index #{index})" - end - - if selector[:matchingBranch] && selector.slice(*ALL_KEYS - [:consumer, :matchingBranch]).any? - errors << "cannot specify matchingBranch=true with any other criteria apart from consumer (at index #{index})" - end - - if selector[:tag] && selector[:branch] - errors << "cannot specify both a tag and a branch (at index #{index})" - end - - if selector[:fallbackBranch] && !selector[:branch] - errors << "a branch must be specified when a fallbackBranch is specified (at index #{index})" - end - - if selector[:fallbackTag] && !selector[:tag] - errors << "a tag must be specified when a fallbackTag is specified (at index #{index})" - end - - if selector[:branch] && selector[:latest] == false - errors << "cannot specify a branch with latest=false (at index #{index})" - end - - if selector[:deployed] && selector[:released] - errors << "cannot specify both deployed=true and released=true (at index #{index})" - end - - if selector[:deployed] && selector[:deployedOrReleased] - errors << "cannot specify both deployed=true and deployedOrReleased=true (at index #{index})" - end - - if selector[:released] && selector[:deployedOrReleased] - errors << "cannot specify both released=true and deployedOrReleased=true (at index #{index})" - end - - if selector[:matchingBranch] && not_provided?(params[:providerVersionBranch]) - errors << "the providerVersionBranch must be specified when the selector matchingBranch=true is used (at index #{index})" - end - - non_environment_fields = selector.slice(*BRANCH_KEYS).keys.sort - environment_related_fields = selector.slice(*ENVIRONMENT_KEYS).keys.sort - - if (non_environment_fields.any? && environment_related_fields.any?) - errors << "cannot specify the #{pluralize("field", non_environment_fields.count)} #{non_environment_fields.join("/")} with the #{pluralize("field", environment_related_fields.count)} #{environment_related_fields.join("/")} (at index #{index})" end - errors end - # rubocop: enable Metrics/CyclomaticComplexity, Metrics/MethodLength + rule(:consumerVersionSelectors).validate(validate_each_with_contract: ConsumerVersionSelectorContract) end end end diff --git a/lib/pact_broker/api/contracts/pacts_for_verification_query_string_schema.rb b/lib/pact_broker/api/contracts/pacts_for_verification_query_string_schema.rb index 2ed78c4c4..17c4d0a83 100644 --- a/lib/pact_broker/api/contracts/pacts_for_verification_query_string_schema.rb +++ b/lib/pact_broker/api/contracts/pacts_for_verification_query_string_schema.rb @@ -1,34 +1,21 @@ -require "dry-validation" -require "pact_broker/api/contracts/dry_validation_workarounds" -require "pact_broker/api/contracts/dry_validation_predicates" +require "pact_broker/api/contracts/base_contract" module PactBroker module Api module Contracts - class PactsForVerificationQueryStringSchema - extend DryValidationWorkarounds - using PactBroker::HashRefinements - - SCHEMA = Dry::Validation.Schema do - configure do - predicates(DryValidationPredicates) - config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) - end + class PactsForVerificationQueryStringSchema < BaseContract + params do optional(:provider_version_tags).maybe(:array?) optional(:consumer_version_selectors).each do schema do - required(:tag).filled(:str?) + required(:tag).filled(:string) optional(:latest).filled(included_in?: ["true", "false"]) - optional(:fallback_tag).filled(:str?) - optional(:consumer).filled(:str?, :not_blank?) + optional(:fallback_tag).filled(:string) + optional(:consumer).filled(:string) end end optional(:include_pending_status).filled(included_in?: ["true", "false"]) - optional(:include_wip_pacts_since).filled(:date?) - end - - def self.call(params) - select_first_message(flatten_indexed_messages(SCHEMA.call(params&.symbolize_keys).messages(full: true))) + optional(:include_wip_pacts_since).filled(:date) end end end diff --git a/lib/pact_broker/api/contracts/publish_contracts_contract_contract.rb b/lib/pact_broker/api/contracts/publish_contracts_contract_contract.rb new file mode 100644 index 000000000..8196c2bca --- /dev/null +++ b/lib/pact_broker/api/contracts/publish_contracts_contract_contract.rb @@ -0,0 +1,62 @@ +require "pact_broker/api/contracts/base_contract" +require "pact_broker/api/contracts/utf_8_validation" + +# The contract for the contract object in the publish contracts request +module PactBroker + module Api + module Contracts + class PublishContractsContractContract < BaseContract + json do + required(:consumerName).filled(:string) + required(:providerName).filled(:string) + required(:content).filled(:string) + required(:contentType).filled(included_in?: ["application/json"]) + required(:specification).filled(included_in?: ["pact"]) + optional(:onConflict).filled(included_in?:["overwrite", "merge"]) + optional(:decodedParsedContent) # set in the resource + optional(:decodedContent) # set in the resource + end + + rule(:consumerName).validate(:not_blank_if_present) + rule(:providerName).validate(:not_blank_if_present) + + # validate_consumer_name_in_content + rule(:decodedParsedContent, :consumerName, :specification) do + consumer_name_in_content = values.dig(:decodedParsedContent, :consumer, :name) + if consumer_name_in_content && consumer_name_in_content != values[:consumerName] + base.failure(validation_message("consumer_name_in_content_mismatch", { consumer_name_in_content: consumer_name_in_content, consumer_name: values[:consumerName] })) + end + end + + # validate_provider_name_in_content + rule(:decodedParsedContent, :providerName) do + provider_name_in_content = values.dig(:decodedParsedContent, :provider, :name) + if provider_name_in_content && provider_name_in_content != values[:providerName] + base.failure(validation_message("provider_name_in_content_mismatch", { provider_name_in_content: provider_name_in_content, provider_name: values[:providerName] })) + end + end + + # validate_encoding + rule(:decodedContent) do + if value.nil? + base.failure(validation_message("base64")) + end + + if value + char_number, fragment = PactBroker::Api::Contracts::UTF8Validation.fragment_before_invalid_utf_8_char(value) + if char_number + base.failure(validation_message("non_utf_8_char_in_contract", char_number: char_number, fragment: fragment)) + end + end + end + + # validate_content_matches_content_type + rule(:decodedParsedContent, :contentType) do + if values[:decodedParsedContent].nil? && values[:contentType] + base.failure(validation_message("invalid_content_for_content_type", { content_type: values[:contentType] })) + end + end + end + end + end +end diff --git a/lib/pact_broker/api/contracts/publish_contracts_schema.rb b/lib/pact_broker/api/contracts/publish_contracts_schema.rb index a8f7f883c..201804517 100644 --- a/lib/pact_broker/api/contracts/publish_contracts_schema.rb +++ b/lib/pact_broker/api/contracts/publish_contracts_schema.rb @@ -1,134 +1,35 @@ -require "dry-validation" -require "pact_broker/api/contracts/dry_validation_workarounds" -require "pact_broker/api/contracts/dry_validation_predicates" -require "pact_broker/messages" -require "pact_broker/api/contracts/utf_8_validation" +require "pact_broker/api/contracts/base_contract" +require "pact_broker/api/contracts/publish_contracts_contract_contract" module PactBroker module Api module Contracts - class PublishContractsSchema - extend DryValidationWorkarounds - using PactBroker::HashRefinements - extend PactBroker::Messages - - class << self - include PactBroker::Api::Contracts::UTF8Validation - end - - SCHEMA = Dry::Validation.Schema do - configure do - predicates(DryValidationPredicates) - config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) - end - - required(:pacticipantName).filled(:str?, :not_blank?) - required(:pacticipantVersionNumber).filled(:not_blank?, :single_line?) - optional(:tags).each(:not_blank?, :single_line?) - optional(:branch).maybe(:not_blank?, :single_line?) - optional(:buildUrl).maybe(:single_line?) - - required(:contracts).each do - required(:consumerName).filled(:str?, :not_blank?) - required(:providerName).filled(:str?, :not_blank?) - required(:content).filled(:str?) - required(:contentType).filled(included_in?: ["application/json"]) - required(:specification).filled(included_in?: ["pact"]) - optional(:onConflict).filled(included_in?:["overwrite", "merge"]) - end - end - - def self.call(params) - dry_results = SCHEMA.call(params&.symbolize_keys).messages(full: true) - dry_results.then do | results | - add_cross_field_validation_errors(params&.symbolize_keys, results) - end.then do | results | - select_first_message(results) - end.then do | results | - flatten_indexed_messages(results) - end - end - - def self.add_cross_field_validation_errors(params, errors) - if params[:contracts].is_a?(Array) - params[:contracts].each_with_index do | contract, i | - if contract.is_a?(Hash) - validate_consumer_name(params, contract, i, errors) - validate_consumer_name_in_content(params, contract, i, errors) - validate_provider_name_in_content(contract, i, errors) - validate_encoding(contract, i, errors) - validate_content_matches_content_type(contract, i, errors) - end - end - end - errors - end - - def self.validate_consumer_name(params, contract, i, errors) - if params[:pacticipantName] && contract[:consumerName] && (contract[:consumerName] != params[:pacticipantName]) - add_contract_error(:consumerName, validation_message("consumer_name_in_contract_mismatch_pacticipant_name", { consumer_name_in_contract: contract[:consumerName], pacticipant_name: params[:pacticipantName] } ), i, errors) - end - end - - def self.validate_consumer_name_in_content(params, contract, i, errors) - consumer_name_in_content = contract.dig(:decodedParsedContent, :consumer, :name) - if consumer_name_in_content && consumer_name_in_content != params[:pacticipantName] - add_contract_error(:consumerName, validation_message("consumer_name_in_content_mismatch_pacticipant_name", { consumer_name_in_content: consumer_name_in_content, pacticipant_name: params[:pacticipantName] } ), i, errors) - end - end - - def self.validate_provider_name_in_content(contract, i, errors) - provider_name_in_content = contract.dig(:decodedParsedContent, :provider, :name) - if provider_name_in_content && provider_name_in_content != contract[:providerName] - add_contract_error(:providerName, validation_message("provider_name_in_content_mismatch", { provider_name_in_content: provider_name_in_content, provider_name: contract[:providerName] } ), i, errors) - end - end - - def self.validate_encoding(contract, i, errors) - if contract[:decodedContent].nil? - add_contract_error(:content, message("errors.base64?", scope: nil), i, errors) - end - - if contract[:decodedContent] - char_number, fragment = fragment_before_invalid_utf_8_char(contract[:decodedContent]) - if char_number - error_message = message("errors.non_utf_8_char_in_contract", char_number: char_number, fragment: fragment) - add_contract_error(:content, error_message, i, errors) + class PublishContractsSchema < BaseContract + json do + required(:pacticipantName).filled(:string) + required(:pacticipantVersionNumber).filled(:string) + optional(:tags).maybe{ array? & each { filled? } } + optional(:branch).maybe(:string) + optional(:buildUrl).maybe(:string) + required(:contracts).array(:hash) + end + + rule(:pacticipantName).validate(:not_blank_if_present) + rule(:pacticipantVersionNumber).validate(:not_blank_if_present, :not_multiple_lines) + rule(:branch).validate(:not_blank_if_present, :not_multiple_lines) + rule(:buildUrl).validate(:not_multiple_lines) + rule(:tags).validate(:array_values_not_blank_if_any) + + rule(:contracts).validate(validate_each_with_contract: PublishContractsContractContract) + + # validate_consumer_name_matches_pacticipant_name + rule(:contracts, :pacticipantName) do + values[:contracts]&.each_with_index do | contract, index | + if values[:pacticipantName] && contract[:consumerName] && (contract[:consumerName] != values[:pacticipantName]) + key([:contracts, index]).failure(validation_message("consumer_name_in_contract_mismatch_pacticipant_name", { consumer_name_in_contract: contract[:consumerName], pacticipant_name: values[:pacticipantName] })) end end end - - def self.validate_content_matches_content_type(contract, i, errors) - if contract[:decodedParsedContent].nil? && contract[:contentType] - add_contract_error(:content, validation_message("invalid_content_for_content_type", { content_type: contract[:contentType]}), i, errors) - end - end - - def self.add_contract_error(field, message, i, errors) - errors[:contracts] ||= {} - errors[:contracts][i] ||= {} - errors[:contracts][i][field] ||= [] - errors[:contracts][i][field] << message - errors - end - - # Need to fix this whole dry-validation eff up - def self.select_first_message(results) - case results - when Hash then results.each_with_object({}) { |(key, value), new_hash| new_hash[key] = select_first_message(value) } - when Array then select_first_message_from_array(results) - else - results - end - end - - def self.select_first_message_from_array(results) - if results.all?{ |value| value.is_a?(String) } - results[0...1] - else - results.collect { |value| select_first_message(value) } - end - end end end end diff --git a/lib/pact_broker/api/contracts/put_pact_params_contract.rb b/lib/pact_broker/api/contracts/put_pact_params_contract.rb index 1678ed7fe..01d40945c 100644 --- a/lib/pact_broker/api/contracts/put_pact_params_contract.rb +++ b/lib/pact_broker/api/contracts/put_pact_params_contract.rb @@ -1,45 +1,40 @@ require "pact_broker/api/contracts/base_contract" +require "pact_broker/api/contracts/validation_helpers" module PactBroker module Api module Contracts class PutPacticipantNameContract < BaseContract - property :name - property :name_in_pact - property :pacticipant - property :message_args + json do + required(:name).maybe(:string) + required(:name_in_pact).maybe(:string) + end - validation do - configure do - config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) + rule(:name, :name_in_pact) do + if name_in_pact_does_not_match_name_in_url_path?(values) + key.failure(validation_message("pact_name_in_path_mismatch_name_in_pact", name_in_pact: values[:name_in_pact], name_in_path: values[:name])) end + end - required(:name).maybe - required(:name_in_pact).maybe - - rule(name_in_path_matches_name_in_pact?: [:name, :name_in_pact]) do |name, name_in_pact| - name_in_pact.filled?.then(name.eql?(value(:name_in_pact))) - end + def name_in_pact_does_not_match_name_in_url_path?(values) + provided?(values[:name_in_pact]) && values[:name] != values[:name_in_pact] end end class PutPactParamsContract < BaseContract - property :consumer_version_number - property :consumer, form: PutPacticipantNameContract - property :provider, form: PutPacticipantNameContract + json do + required(:consumer).filled(:hash) + required(:provider).filled(:hash) + required(:consumer_version_number).filled(:string) + end - validation do - configure do - config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) + rule(:consumer).validate(validate_with_contract: PutPacticipantNameContract) + rule(:provider).validate(validate_with_contract: PutPacticipantNameContract) - def valid_consumer_version_number?(value) - return true if PactBroker.configuration.order_versions_by_date - parsed_version_number = PactBroker.configuration.version_parser.call(value) - !parsed_version_number.nil? - end - end + rule(:consumer_version_number).validate(:not_blank_if_present) - required(:consumer_version_number).filled(:valid_consumer_version_number?) + rule(:consumer_version_number) do + validate_version_number(value, key) if !rule_error?(:consumer_version_number) end end end diff --git a/lib/pact_broker/api/contracts/request_validations.rb b/lib/pact_broker/api/contracts/request_validations.rb deleted file mode 100644 index 58d67379d..000000000 --- a/lib/pact_broker/api/contracts/request_validations.rb +++ /dev/null @@ -1,33 +0,0 @@ -require "net/http" -require "uri" - -module PactBroker - module Api - module Contracts - - module RequestValidations - def method_is_valid - http_method && !valid_method? - end - - def valid_method? - Net::HTTP.const_defined?(http_method.capitalize) - end - - def url_is_valid - url && !url_valid? - end - - def url_valid? - uri && uri.scheme && uri.host - end - - def uri - URI(url) - rescue URI::InvalidURIError, ArgumentError - nil - end - end - end - end -end diff --git a/lib/pact_broker/api/contracts/utf_8_validation.rb b/lib/pact_broker/api/contracts/utf_8_validation.rb index b1b95d829..63757d51d 100644 --- a/lib/pact_broker/api/contracts/utf_8_validation.rb +++ b/lib/pact_broker/api/contracts/utf_8_validation.rb @@ -2,6 +2,8 @@ module PactBroker module Api module Contracts module UTF8Validation + extend self + def fragment_before_invalid_utf_8_char(string) string.force_encoding("UTF-8").each_char.with_index do | char, index | if !char.valid_encoding? diff --git a/lib/pact_broker/api/contracts/validation_helpers.rb b/lib/pact_broker/api/contracts/validation_helpers.rb new file mode 100644 index 000000000..962d0f3c3 --- /dev/null +++ b/lib/pact_broker/api/contracts/validation_helpers.rb @@ -0,0 +1,71 @@ +require "pact_broker/services" +require "pact_broker/string_refinements" +require "pact_broker/configuration" +require "uri" + +module PactBroker + module Api + module Contracts + module ValidationHelpers + extend self + using PactBroker::StringRefinements + + def multiple_lines?(value) + value && value.is_a?(String) && value.include?("\n") + end + + def includes_space?(value) + value && value.is_a?(String) && value.include?(" ") + end + + # @return true if there is a value present, and it only contains whitespace + def blank?(value) + value&.blank? + end + + # The tins gem has screwed up the present? method by not using refinements + # Return true if the object is not nil, and if a String, is not blank. + # @param [Object] + def provided?(value) + if value.is_a?(String) + value.strip.size > 0 + else + !value.nil? + end + end + + def not_provided?(value) + !provided?(value) + end + + def valid_url?(url) + URI(url) + rescue URI::InvalidURIError, ArgumentError + false + end + + def valid_http_method?(http_method) + Net::HTTP.const_defined?(http_method.capitalize) + rescue StandardError + false + end + + def pacticipant_with_name_exists?(value) + PactBroker::Services.pacticipant_service.find_pacticipant_by_name(value) + end + + def environment_with_name_exists?(value) + PactBroker::Services.environment_service.find_by_name(value) + end + + def valid_version_number?(value) + if PactBroker.configuration.order_versions_by_date + true + else + !!PactBroker.configuration.version_parser.call(value) + end + end + end + end + end +end \ No newline at end of file diff --git a/lib/pact_broker/api/contracts/verification_contract.rb b/lib/pact_broker/api/contracts/verification_contract.rb index fd857944c..a0a37e32d 100644 --- a/lib/pact_broker/api/contracts/verification_contract.rb +++ b/lib/pact_broker/api/contracts/verification_contract.rb @@ -1,41 +1,22 @@ require "pact_broker/api/contracts/base_contract" -require "uri" module PactBroker module Api module Contracts class VerificationContract < BaseContract + json do + required(:success).filled(:bool) + required(:providerApplicationVersion).filled(:string) + optional(:buildUrl).maybe(:string) + end - property :success - property :provider_version, as: :providerApplicationVersion - property :build_url, as: :buildUrl - - validation do - configure do - config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) - - def not_blank? value - value && value.to_s.strip.size > 0 - end - - def valid_url? url - URI(url) - rescue URI::InvalidURIError, ArgumentError - nil - end - - def valid_version_number?(value) - return true if PactBroker.configuration.order_versions_by_date - - parsed_version_number = PactBroker.configuration.version_parser.call(value) - !!parsed_version_number - end - end + rule(:providerApplicationVersion).validate(:not_blank_if_present) - required(:success).filled(:bool?) - required(:provider_version) { not_blank? & valid_version_number? } - optional(:build_url).maybe(:valid_url?) + rule(:providerApplicationVersion) do + validate_version_number(value, key) unless rule_error?(:providerApplicationVersion) end + + rule(:buildUrl).validate(:valid_url_if_present) end end end diff --git a/lib/pact_broker/api/contracts/webhook_contract.rb b/lib/pact_broker/api/contracts/webhook_contract.rb index 7a7215f37..401b567be 100644 --- a/lib/pact_broker/api/contracts/webhook_contract.rb +++ b/lib/pact_broker/api/contracts/webhook_contract.rb @@ -1,7 +1,6 @@ require "pact_broker/api/contracts/base_contract" -require "pact_broker/webhooks/check_host_whitelist" -require "pact_broker/webhooks/render" -require "pact_broker/pacticipants/service" +require "pact_broker/api/contracts/webhook_request_contract" +require "pact_broker/api/contracts/webhook_pacticipant_contract" require "pact_broker/webhooks/webhook_event" module PactBroker @@ -9,182 +8,31 @@ module Api module Contracts class WebhookContract < BaseContract - def validate(*) - result = super - # I just cannot seem to get the validation to stop on the first error. - # If one rule fails, they all come back failed, and it's driving me nuts. - # Why on earth would I want that behaviour? - # I cannot believe I have to do this shit. - @first_errors = errors - @first_errors.messages.keys.each do | key | - @first_errors.messages[key] = @first_errors.messages[key][0...1] - end - - # rubocop: disable Lint/NestedMethodDefinition - def self.errors; @first_errors end - # rubocop: enable Lint/NestedMethodDefinition - - result - end - - validation do - configure do - config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) - end - - optional(:consumer) - optional(:provider) - required(:request).filled - optional(:events).maybe(min_size?: 1) - end - - property :consumer do - property :name - property :label - - validation do - configure do - config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) - - def pacticipant_exists?(name) - !!PactBroker::Pacticipants::Service.find_pacticipant_by_name(name) - end - end - - optional(:name) - .maybe(:pacticipant_exists?) - .when(:none?) { value(:label).filled? } - - optional(:label) - .maybe(:str?) - .when(:none?) { value(:name).filled? } - - rule(label: [:name, :label]) do |name, label| - (name.filled? & label.filled?) > label.none? - end - end - end - - property :provider do - property :name - property :label - - validation do - configure do - config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) - - def pacticipant_exists?(name) - !!PactBroker::Pacticipants::Service.find_pacticipant_by_name(name) - end - end + UUID_REGEX = /^[A-Za-z0-9_\-]{16,}$/ - optional(:name) - .maybe(:pacticipant_exists?) - .when(:none?) { value(:label).filled? } - - optional(:label) - .maybe(:str?) - .when(:none?) { value(:name).filled? } - - rule(label: [:name, :label]) do |name, label| - (name.filled? & label.filled?) > label.none? - end + class EventContract < BaseContract + json do + required(:name).filled(included_in?: PactBroker::Webhooks::WebhookEvent::EVENT_NAMES) end end - property :request do - property :url - property :http_method - - validation do - configure do - config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) - - def self.messages - super.merge( - en: { - errors: { - allowed_webhook_method?: http_method_error_message, - allowed_webhook_scheme?: scheme_error_message, - allowed_webhook_host?: host_error_message - } - } - ) - end - - def self.http_method_error_message - if PactBroker.configuration.webhook_http_method_whitelist.size == 1 - "must be #{PactBroker.configuration.webhook_http_method_whitelist.first}. See /doc/webhooks#whitelist for more information." - else - "must be one of #{PactBroker.configuration.webhook_http_method_whitelist.join(", ")}. See /doc/webhooks#whitelist for more information." - end - end - - def self.scheme_error_message - "scheme must be #{PactBroker.configuration.webhook_scheme_whitelist.join(", ")}. See /doc/webhooks#whitelist for more information." - end - - def self.host_error_message - "host must be in the whitelist #{PactBroker.configuration.webhook_host_whitelist.join(",")}. See /doc/webhooks#whitelist for more information." - end - - def valid_method?(http_method) - Net::HTTP.const_defined?(http_method.capitalize) - rescue StandardError - false - end - - def valid_url?(url) - uri = parse_uri(url) - uri.scheme && uri.host - rescue URI::InvalidURIError, ArgumentError - nil - end - - def allowed_webhook_method?(http_method) - PactBroker.configuration.webhook_http_method_whitelist.any? do | allowed_method | - http_method.downcase == allowed_method.downcase - end - end - - def allowed_webhook_scheme?(url) - scheme = parse_uri(url).scheme - PactBroker.configuration.webhook_scheme_whitelist.any? do | allowed_scheme | - scheme.downcase == allowed_scheme.downcase - end - end - - def allowed_webhook_host?(url) - if host_whitelist.any? - PactBroker::Webhooks::CheckHostWhitelist.call(parse_uri(url).host, host_whitelist).any? - else - true - end - end - - def non_templated_host?(url) - parse_uri(url).host == parse_uri(url, "differentplaceholder").host - end - - def host_whitelist - PactBroker.configuration.webhook_host_whitelist - end - - def parse_uri(uri_string, placeholder = "placeholder") - URI(PactBroker::Webhooks::Render.render_with_placeholder(uri_string, placeholder)) - end - end - - required(:http_method).filled(:valid_method?, :allowed_webhook_method?) - required(:url).filled(:valid_url?, :allowed_webhook_scheme?, :allowed_webhook_host?, :non_templated_host?) - end + json do + optional(:uuid).maybe(:string) # set in resource class from the path info - doesn't come in in the request body + optional(:consumer).maybe(:hash) + optional(:provider).maybe(:hash) + required(:request).filled(:hash) + optional(:events).maybe(min_size?: 1) + optional(:enabled).filled(:bool) end - collection :events do - property :name + rule(:consumer).validate(validate_with_contract: WebhookPacticipantContract) + rule(:provider).validate(validate_with_contract: WebhookPacticipantContract) + rule(:request).validate(validate_with_contract: WebhookRequestContract) + rule(:events).validate(validate_each_with_contract: EventContract) - validation do - required(:name).filled(included_in?: PactBroker::Webhooks::WebhookEvent::EVENT_NAMES) + rule(:uuid) do + if value && !(value =~ UUID_REGEX) + key.failure(validation_message("invalid_webhook_uuid")) end end end diff --git a/lib/pact_broker/api/contracts/webhook_pacticipant_contract.rb b/lib/pact_broker/api/contracts/webhook_pacticipant_contract.rb new file mode 100644 index 000000000..045a01cfe --- /dev/null +++ b/lib/pact_broker/api/contracts/webhook_pacticipant_contract.rb @@ -0,0 +1,33 @@ +require "pact_broker/api/contracts/base_contract" + +module PactBroker + module Api + module Contracts + class WebhookPacticipantContract < BaseContract + json do + optional(:name).maybe(:string) + optional(:label).maybe(:string) + end + + register_macro(:name_or_label_required) do + if !provided?(values[:name]) && !provided?(values[:label]) + key(path.keys).failure(validation_message("blank")) + end + end + + register_macro(:name_and_label_exclusive) do + if provided?(values[:name]) && provided?(values[:label]) + key([:label]).failure(validation_message("cannot_be_provided_at_same_time", name_1: "name", name_2: "label")) + end + end + + rule(:name, :label).validate(:name_or_label_required) + rule(:name, :label).validate(:name_and_label_exclusive) + + rule(:name) do + validate_pacticipant_with_name_exists(value, key) if provided?(value) + end + end + end + end +end diff --git a/lib/pact_broker/api/contracts/webhook_request_contract.rb b/lib/pact_broker/api/contracts/webhook_request_contract.rb new file mode 100644 index 000000000..2b54dd3a5 --- /dev/null +++ b/lib/pact_broker/api/contracts/webhook_request_contract.rb @@ -0,0 +1,125 @@ +require "pact_broker/api/contracts/base_contract" +require "pact_broker/webhooks/render" +require "pact_broker/webhooks/check_host_whitelist" +require "pact_broker/api/contracts/validation_helpers" + +module PactBroker + module Api + module Contracts + class WebhookRequestContract < BaseContract + include ValidationHelpers + + json do + required(:method).filled(:string) + required(:url).filled(:string) + end + + rule(:url) do + if !valid_webhook_url?(value) + key.failure(validation_message("invalid_url")) + end + end + + rule(:method) do + if !valid_http_method?(value) + key.failure(validation_message("invalid_http_method")) + end + end + + rule(:method) do + if_still_valid(self) do + if !allowed_webhook_method?(value) + key.failure(http_method_error_message) + end + end + end + + rule(:url) do + if_still_valid(self) do + if templated_host?(value) + key.failure(validation_message("webhook_templated_host_not_allowed")) + end + end + end + + rule(:url) do + if_still_valid(self) do + if !allowed_webhook_scheme?(value) + key.failure(scheme_error_message) + end + end + end + + rule(:url) do + if_still_valid(self) do + if !allowed_webhook_host?(value) + key.failure(host_error_message) + end + end + end + + def allowed_webhook_method?(http_method) + PactBroker.configuration.webhook_http_method_whitelist.any? do | allowed_method | + http_method.downcase == allowed_method.downcase + end + end + + def http_method_error_message + if PactBroker.configuration.webhook_http_method_whitelist.size == 1 + "must be #{PactBroker.configuration.webhook_http_method_whitelist.first}. See /doc/webhooks#whitelist for more information." + else + "must be one of #{PactBroker.configuration.webhook_http_method_whitelist.join(", ")}. See /doc/webhooks#whitelist for more information." + end + end + + def allowed_webhook_scheme?(url) + scheme = parse_uri(url).scheme + PactBroker.configuration.webhook_scheme_whitelist.any? do | allowed_scheme | + scheme.downcase == allowed_scheme.downcase + end + end + + def scheme_error_message + "scheme must be #{PactBroker.configuration.webhook_scheme_whitelist.join(", ")}. See /doc/webhooks#whitelist for more information." + end + + def parse_uri(uri_string, placeholder = "placeholder") + URI(PactBroker::Webhooks::Render.render_with_placeholder(uri_string, placeholder)) + end + + def allowed_webhook_host?(url) + if host_whitelist.any? + PactBroker::Webhooks::CheckHostWhitelist.call(parse_uri(url).host, host_whitelist).any? + else + true + end + end + + def host_whitelist + PactBroker.configuration.webhook_host_whitelist + end + + def host_error_message + "host must be in the whitelist #{PactBroker.configuration.webhook_host_whitelist.collect(&:inspect).join(", ")}. See /doc/webhooks#whitelist for more information." + end + + def valid_webhook_url?(url) + uri = parse_uri(url) + uri.scheme && uri.host + rescue URI::InvalidURIError, ArgumentError + nil + end + + def templated_host?(url) + parse_uri(url).host != parse_uri(url, "differentplaceholder").host + end + + def if_still_valid(context) + if !context.rule_error?(context.path.keys) + yield + end + end + end + end + end +end diff --git a/lib/pact_broker/api/decorators/pacts_for_verification_query_decorator.rb b/lib/pact_broker/api/decorators/pacts_for_verification_query_decorator.rb index 3679c9420..c21e8bdbb 100644 --- a/lib/pact_broker/api/decorators/pacts_for_verification_query_decorator.rb +++ b/lib/pact_broker/api/decorators/pacts_for_verification_query_decorator.rb @@ -15,7 +15,10 @@ class PactsForVerificationQueryDecorator < BaseDecorator property :provider_version_branch collection :consumer_version_selectors, default: PactBroker::Pacts::Selectors.new, class: PactBroker::Pacts::Selector do - property :main_branch + property :main_branch, setter: -> (fragment:, represented:, **) { + represented.main_branch = fragment + represented.latest = true + } property :tag property :branch, setter: -> (fragment:, represented:, **) { represented.branch = fragment diff --git a/lib/pact_broker/api/resources/all_webhooks.rb b/lib/pact_broker/api/resources/all_webhooks.rb index a8124fa9c..95b492bee 100644 --- a/lib/pact_broker/api/resources/all_webhooks.rb +++ b/lib/pact_broker/api/resources/all_webhooks.rb @@ -30,7 +30,7 @@ def post_is_create? end def malformed_request? - super || (request.post? && validation_errors?(webhook)) + super || (request.post? && validation_errors_for_schema?) end def to_json @@ -56,14 +56,8 @@ def policy_record private - def validation_errors? webhook - errors = webhook_service.errors(webhook) - - unless errors.empty? - set_json_validation_error_messages(errors.messages) - end - - !errors.empty? + def schema + api_contract_class(:webhook_contract) end def consumer diff --git a/lib/pact_broker/api/resources/base_resource.rb b/lib/pact_broker/api/resources/base_resource.rb index ed94eb0d0..261111461 100644 --- a/lib/pact_broker/api/resources/base_resource.rb +++ b/lib/pact_broker/api/resources/base_resource.rb @@ -203,22 +203,6 @@ def invalid_json? end end - def validation_errors? model - if (errors = model.validate).any? - set_json_validation_error_messages errors - true - else - false - end - end - - def contract_validation_errors? contract, params - if (invalid = !contract.validate(params)) - set_json_validation_error_messages contract.errors.messages - end - invalid - end - def find_pacticipant name, role pacticipant_service.find_pacticipant_by_name(name).tap do | pacticipant | if pacticipant.nil? diff --git a/lib/pact_broker/api/resources/can_i_deploy.rb b/lib/pact_broker/api/resources/can_i_deploy.rb index 02326fac2..aeb5c818a 100644 --- a/lib/pact_broker/api/resources/can_i_deploy.rb +++ b/lib/pact_broker/api/resources/can_i_deploy.rb @@ -1,5 +1,5 @@ require "pact_broker/api/resources/matrix" -require "pact_broker/matrix/can_i_deploy_query_schema" +require "pact_broker/api/contracts/can_i_deploy_query_schema" require "pact_broker/matrix/parse_can_i_deploy_query" require "pact_broker/messages" @@ -9,16 +9,9 @@ module Resources class CanIDeploy < Matrix include PactBroker::Messages + # Can't call super because it will execute the Matrix validation, not the BaseResource validation def malformed_request? - if (errors = query_schema.call(query_params)).any? - set_json_validation_error_messages(errors) - true - elsif !pacticipant - set_json_validation_error_messages(pacticipant: [message("errors.validation.pacticipant_not_found", name: pacticipant_name)]) - true - else - false - end + request.get? && validation_errors_for_schema?(schema, request.query) end def policy_name @@ -27,18 +20,10 @@ def policy_name private - def query_schema + def schema PactBroker::Api::Contracts::CanIDeployQuerySchema end - def pacticipant - @pacticipant ||= pacticipant_service.find_pacticipant_by_name(pacticipant_name) - end - - def pacticipant_name - selectors.first.pacticipant_name - end - def parsed_query @parsed_query ||= PactBroker::Matrix::ParseCanIDeployQuery.call(query_params) end diff --git a/lib/pact_broker/api/resources/can_i_deploy_pacticipant_version_by_branch_to_environment.rb b/lib/pact_broker/api/resources/can_i_deploy_pacticipant_version_by_branch_to_environment.rb index c88dac161..57ded2211 100644 --- a/lib/pact_broker/api/resources/can_i_deploy_pacticipant_version_by_branch_to_environment.rb +++ b/lib/pact_broker/api/resources/can_i_deploy_pacticipant_version_by_branch_to_environment.rb @@ -1,8 +1,5 @@ require "pact_broker/api/resources/matrix" -require "pact_broker/matrix/can_i_deploy_query_schema" -require "pact_broker/matrix/parse_can_i_deploy_query" require "pact_broker/api/decorators/matrix_decorator" -require "pact_broker/api/decorators/matrix_text_decorator" module PactBroker module Api diff --git a/lib/pact_broker/api/resources/can_i_deploy_pacticipant_version_by_tag_to_tag.rb b/lib/pact_broker/api/resources/can_i_deploy_pacticipant_version_by_tag_to_tag.rb index 34d944210..f4f2033b0 100644 --- a/lib/pact_broker/api/resources/can_i_deploy_pacticipant_version_by_tag_to_tag.rb +++ b/lib/pact_broker/api/resources/can_i_deploy_pacticipant_version_by_tag_to_tag.rb @@ -1,6 +1,4 @@ require "pact_broker/api/resources/matrix" -require "pact_broker/matrix/can_i_deploy_query_schema" -require "pact_broker/matrix/parse_can_i_deploy_query" module PactBroker module Api diff --git a/lib/pact_broker/api/resources/can_i_deploy_pacticipant_version_by_tag_to_tag_badge.rb b/lib/pact_broker/api/resources/can_i_deploy_pacticipant_version_by_tag_to_tag_badge.rb index 487b06981..fe0d51ff1 100644 --- a/lib/pact_broker/api/resources/can_i_deploy_pacticipant_version_by_tag_to_tag_badge.rb +++ b/lib/pact_broker/api/resources/can_i_deploy_pacticipant_version_by_tag_to_tag_badge.rb @@ -1,5 +1,4 @@ -require "pact_broker/matrix/can_i_deploy_query_schema" -require "pact_broker/matrix/parse_can_i_deploy_query" +require "pact_broker/api/resources/can_i_deploy_pacticipant_version_by_tag_to_tag" require "pact_broker/api/resources/badge_methods" module PactBroker diff --git a/lib/pact_broker/api/resources/pact.rb b/lib/pact_broker/api/resources/pact.rb index e49fab053..441ed1496 100644 --- a/lib/pact_broker/api/resources/pact.rb +++ b/lib/pact_broker/api/resources/pact.rb @@ -54,9 +54,8 @@ def is_conflict? potential_duplicate_pacticipants?(pact_params.pacticipant_names) || merge_conflict || disallowed_modification? end - def malformed_request? - super || ((request.patch? || request.really_put?) && contract_validation_errors?(Contracts::PutPactParamsContract.new(pact_params), pact_params)) + super || ((request.patch? || request.really_put?) && validation_errors_for_schema?(schema, pact_params.to_hash_for_validation)) end def resource_exists? @@ -118,6 +117,10 @@ def disallowed_modification? false end end + + def schema + api_contract_class(:put_pact_params_contract) + end end end end diff --git a/lib/pact_broker/api/resources/pact_webhooks.rb b/lib/pact_broker/api/resources/pact_webhooks.rb index 9f7f3108c..261f1de6c 100644 --- a/lib/pact_broker/api/resources/pact_webhooks.rb +++ b/lib/pact_broker/api/resources/pact_webhooks.rb @@ -25,18 +25,7 @@ def resource_exists? end def malformed_request? - super || (request.post? && validation_errors?(webhook)) - end - - def validation_errors? webhook - errors = webhook_service.errors(webhook) - - unless errors.empty? - response.headers["Content-Type"] = "application/hal+json;charset=utf-8" - response.body = { errors: errors.messages }.to_json - end - - !errors.empty? + super || (request.post? && validation_errors_for_schema?) end def create_path @@ -62,6 +51,10 @@ def policy_name private + def schema + api_contract_class(:webhook_contract) + end + def webhooks @webhooks ||= webhook_service.find_by_consumer_and_provider consumer, provider end diff --git a/lib/pact_broker/api/resources/pacticipant_webhooks.rb b/lib/pact_broker/api/resources/pacticipant_webhooks.rb index 146e9c6cd..aaac060da 100644 --- a/lib/pact_broker/api/resources/pacticipant_webhooks.rb +++ b/lib/pact_broker/api/resources/pacticipant_webhooks.rb @@ -2,13 +2,11 @@ require "pact_broker/api/decorators/webhook_decorator" require "pact_broker/api/decorators/webhooks_decorator" require "pact_broker/api/contracts/webhook_contract" -require "pact_broker/api/resources/webhook_resource_methods" module PactBroker module Api module Resources class PacticipantWebhooks < BaseResource - include WebhookResourceMethods def allowed_methods ["POST", "GET", "OPTIONS"] @@ -27,7 +25,7 @@ def resource_exists? end def malformed_request? - super || (request.post? && webhook_validation_errors?(webhook)) + super || (request.post? && validation_errors_for_schema?) end def create_path @@ -57,6 +55,10 @@ def policy_record private + def schema + api_contract_class(:webhook_contract) + end + def webhooks webhook_service.find_by_consumer_and_provider(consumer, provider) end diff --git a/lib/pact_broker/api/resources/verifications.rb b/lib/pact_broker/api/resources/verifications.rb index 5ff71b101..ac743214f 100644 --- a/lib/pact_broker/api/resources/verifications.rb +++ b/lib/pact_broker/api/resources/verifications.rb @@ -34,7 +34,7 @@ def resource_exists? end def malformed_request? - super || (request.post? && any_validation_errors?) + super || (request.post? && validation_errors_for_schema?) end def create_path @@ -84,10 +84,8 @@ def verification_params params(symbolize_names: false).merge("wip" => wip?, "pending" => pending?) end - def any_validation_errors? - errors = verification_service.errors(params) - set_json_validation_error_messages(errors.messages) if !errors.empty? - !errors.empty? + def schema + PactBroker::Api::Contracts::VerificationContract end end end diff --git a/lib/pact_broker/api/resources/webhook.rb b/lib/pact_broker/api/resources/webhook.rb index 50d8a6d54..96e7df320 100644 --- a/lib/pact_broker/api/resources/webhook.rb +++ b/lib/pact_broker/api/resources/webhook.rb @@ -1,15 +1,12 @@ require "pact_broker/api/resources/base_resource" require "pact_broker/services" require "pact_broker/api/decorators/webhook_decorator" -require "pact_broker/api/resources/webhook_resource_methods" +require "pact_broker/api/contracts/webhook_contract" module PactBroker module Api module Resources class Webhook < BaseResource - - include WebhookResourceMethods - def content_types_accepted [["application/json", :from_json]] end @@ -31,7 +28,7 @@ def resource_exists? end def malformed_request? - super || (request.put? && webhook_validation_errors?(parsed_webhook, uuid)) + super || (request.put? && validation_errors_for_schema?(schema, { uuid: uuid }.compact.merge(params))) end def from_json @@ -80,6 +77,10 @@ def parsed_webhook @parsed_webhook ||= decorator_class(:webhook_decorator).new(PactBroker::Domain::Webhook.new).from_json(request_body) end + def schema + api_contract_class(:webhook_contract) + end + def uuid identifier_from_path[:uuid] end diff --git a/lib/pact_broker/api/resources/webhook_execution.rb b/lib/pact_broker/api/resources/webhook_execution.rb index 133160797..2a30fa0e8 100644 --- a/lib/pact_broker/api/resources/webhook_execution.rb +++ b/lib/pact_broker/api/resources/webhook_execution.rb @@ -1,7 +1,6 @@ require "pact_broker/api/resources/base_resource" require "pact_broker/services" require "pact_broker/api/decorators/webhook_execution_result_decorator" -require "pact_broker/api/resources/webhook_resource_methods" require "pact_broker/constants" require "pact_broker/webhooks/execution_configuration" require "pact_broker/api/resources/webhook_execution_methods" @@ -10,7 +9,6 @@ module PactBroker module Api module Resources class WebhookExecution < BaseResource - include WebhookResourceMethods include WebhookExecutionMethods def content_types_provided @@ -39,7 +37,7 @@ def malformed_request? if uuid false else - webhook_validation_errors?(webhook) + validation_errors_for_schema? end else false @@ -88,6 +86,10 @@ def decorator_options def build_unsaved_webhook decorator_class(:webhook_decorator).new(PactBroker::Domain::Webhook.new).from_json(request_body) end + + def schema + api_contract_class(:webhook_contract) + end end end end diff --git a/lib/pact_broker/api/resources/webhook_resource_methods.rb b/lib/pact_broker/api/resources/webhook_resource_methods.rb deleted file mode 100644 index 4cc1191cc..000000000 --- a/lib/pact_broker/api/resources/webhook_resource_methods.rb +++ /dev/null @@ -1,17 +0,0 @@ -module PactBroker - module Api - module Resources - module WebhookResourceMethods - def webhook_validation_errors?(webhook, uuid = nil) - errors = webhook_service.errors(webhook, uuid) - if !errors.empty? - set_json_validation_error_messages(errors.messages) - true - else - false - end - end - end - end - end -end diff --git a/lib/pact_broker/locale/en.yml b/lib/pact_broker/locale/en.yml index d54f84eb9..8855e2350 100644 --- a/lib/pact_broker/locale/en.yml +++ b/lib/pact_broker/locale/en.yml @@ -2,17 +2,10 @@ en: errors: not_blank?: "can't be blank" filled?: "can't be blank" - valid_method?: "is not a recognised HTTP method" valid_url?: "is not a valid URL eg. http://example.org" base64?: "is not valid Base64" - valid_version_number?: "Version number '%{value}' cannot be parsed to a version number. The expected format (unless this configuration has been overridden) is a semantic version. eg. 1.3.0 or 2.0.4.rc1" - name_in_path_matches_name_in_pact?: "does not match %{left} name in path ('%{right}')." - valid_consumer_version_number?: "Consumer version number '%{value}' cannot be parsed to a version number. The expected format (unless this configuration has been overridden) is a semantic version. eg. 1.3.0 or 2.0.4.rc1" - non_templated_host?: "cannot have a template parameter in the host" - pacticipant_exists?: "does not match an existing pacticipant" single_line?: "cannot contain multiple lines" no_spaces?: "cannot contain spaces" - environment_with_name_exists?: "with name '%{value}' does not exist" none?: "cannot be provided" pact_broker: @@ -68,7 +61,12 @@ en: version_branch: Configure the version branch to be the value of your repository branch. errors: validation: - blank: "cannot be blank." + no_spaces: "cannot contain spaces" + single_line: "cannot contain multiple lines" + environment_not_found: "with name '%{value}' does not exist" + blank: "cannot be blank" + cannot_be_provided_at_same_time: "%{name_1} and %{name_2} cannot be provided at the same time" + blank_with_name: "%{name} cannot be blank" attribute_missing: "Missing required attribute '%{attribute}'" invalid_http_method: "Invalid HTTP method '%{method}'" invalid_url: "Invalid URL '%{url}'. Expected format: http://example.org" @@ -76,25 +74,48 @@ en: pact_content_modification_not_allowed: "Cannot change the content of the pact for %{consumer_name} version %{consumer_version_number} and provider %{provider_name}, as race conditions will cause unreliable results for can-i-deploy. Each pact must be published with a unique consumer version number. Some Pact libraries generate random data when a concrete value for a type matcher is not specified, and this can cause the contract to mutate - ensure you have given example values for all type matchers. For more information see https://docs.pact.io/go/versioning" consumer_version_number_missing: "Please specify the consumer version number by setting the X-Pact-Consumer-Version header." consumer_version_number_header_invalid: "X-Pact-Consumer-Version '%{consumer_version_number}' cannot be parsed to a version number. The expected format (unless this configuration has been overridden) is a semantic version. eg. 1.3.0 or 2.0.4.rc1" - consumer_version_number_invalid: "Consumer version number '%{consumer_version_number}' cannot be parsed to a version number. The expected format (unless this configuration has been overridden) is a semantic version. eg. 1.3.0 or 2.0.4.rc1" pacticipant_name_mismatch: "in pact ('%{name_in_pact}') does not match %{pacticipant} name in path ('%{name}')." - consumer_name_in_content_mismatch_pacticipant_name: "consumer name in contract content ('%{consumer_name_in_content}') must match pacticipantName ('%{pacticipant_name}')" consumer_name_in_contract_mismatch_pacticipant_name: "consumerName ('%{consumer_name_in_contract}') must match pacticipantName ('%{pacticipant_name}')" - provider_name_in_content_mismatch: "provider name in contract content ('%{provider_name_in_content}') must match providerName ('%{provider_name}') in contracts" + consumer_name_in_content_mismatch: "consumer name in contract content ('%{consumer_name_in_content}') must match consumerName in contract params ('%{consumer_name}')" + provider_name_in_content_mismatch: "provider name in contract content ('%{provider_name_in_content}') must match providerName in contract params ('%{provider_name}')" connection_encoding_not_utf8: "The Sequel connection encoding (%{encoding}) is strongly recommended to be \"utf8\". If you need to set it to %{encoding} for some particular reason, then disable this check by setting config.validate_database_connection_config = false" invalid_webhook_uuid: The UUID can only contain the characters A-Z, a-z, 0-9, _ and -, and must be 16 or more characters. pacticipant_not_found: No pacticipant with name '%{name}' found - environment_name_must_be_unique: Another environment with name '%{name}' already exists. + environment_name_must_be_unique: "name '%{name}' is already used by an existing environment." must_specify_environment_or_tag: Must specify either an environment or a 'to' tag. cannot_specify_tag_and_environment: Cannot specify both a 'to' tag and an environment. cannot_specify_latest_and_environment: Cannot specify both latest=true and an environment. cannot_specify_more_than_one_destination_identifier: Cannot specify more than one of tag, environment and mainBranch. environment_with_name_not_found: "Environment with name '%{name}' does not exist" cannot_modify_version_branch: "The branch for a pacticipant version cannot be changed once set (currently '%{old_branch}', proposed value '%{new_branch}'). Your pact publication/verification publication configurations may be in conflict with each other if you are seeing this error. If the current branch value is incorrect, you must delete the pacticipant version resource at %{version_url} and publish the pacts/verification results again with a consistent branch name." - invalid_content_for_content_type: "The content could not be parsed as %{content_type}" + invalid_content_for_content_type: "content could not be parsed as %{content_type}" cannot_set_currently_deployed_true: The currentlyDeployed property cannot be set back to true. Please record a new deployment. cannot_set_currently_supported_true: The currentlySupported property cannot be set back to true. Please record a new deployment. invalid_limit: The limit must be 1 or greater. + pacts_for_verification_selector_fallback_tag: "fallbackTag can only be set if latest is true" + pacts_for_verification_selector_matching_branch_requires_provider_version_branch: the providerVersionBranch must be specified when the selector matchingBranch=true is used + pacts_for_verification_selector_tag_and_branch_disallowed: "cannot specify both a tag and a branch" + pacts_for_verification_selector_fallback_tag_requires_tag: "a tag must be specified when a fallbackTag is specified" + pacts_for_verification_selector_fallback_branch_requires_branch: "a branch must be specified when a fallbackBranch is specified" + pacts_for_verification_selector_branch_and_latest_false_disallowed: cannot specify a branch with latest=false + pacts_for_verification_selector_main_branch_and_latest_false_disallowed: cannot specify mainBranch=true with latest=false + pacts_for_verification_selector_fallback_branch_and_latest_false_disallowed: fallbackBranch can only be set if latest is true + pacts_for_verification_selector_required_params_missing: must specify a value for environment or tag or branch, or specify mainBranch=true, matchingBranch=true, latest=true, deployed=true, released=true or deployedOrReleased=true + pacts_for_verification_selector_main_branch_with_other_param_disallowed: cannot specify mainBranch=true with any other criteria apart from consumer + pacts_for_verification_selector_matching_branch_with_other_param_disallowed: cannot specify matchingBranch=true with any other criteria apart from consumer + pacts_for_verification_selector_deployed_and_released_disallowed: cannot specify both deployed=true and released=true + pacts_for_verification_selector_deployed_and_deployed_or_released_disallowed: cannot specify both deployed=true and deployedOrReleased=true + pacts_for_verification_selector_released_and_deployed_or_released_disallowed: cannot specify both released=true and deployedOrReleased=true + pacts_for_verification_selector_provider_version_branch_empty: when pending or WIP pacts are enabled and there are no tags provided, the provider version branch must not be an empty string (as this suggests it is a user error in configuration) - a value must be provided (recommended), or it must not be set at all + pact_name_in_path_mismatch_name_in_pact: "name in pact '%{name_in_pact}' does not match name in URL path '%{name_in_path}'." + base64: "is not valid Base64" + non_utf_8_char_in_contract: "contract content has a non UTF-8 character at char %{char_number} and cannot be parsed as JSON. Fragment preceding invalid character is: '%{fragment}'" + invalid_consumer_version_number: "Consumer version number '%{value}' cannot be parsed to a version number. The expected format (unless this configuration has been overridden) is a semantic version. eg. 1.3.0 or 2.0.4.rc1" + invalid_version_number: "Version number '%{value}' cannot be parsed to a version number. The expected format (unless this configuration has been overridden) is a semantic version. eg. 1.3.0 or 2.0.4.rc1" + invalid_url: "is not a valid URL eg. http://example.org" + pacticipant_with_name_not_found: "does not match an existing pacticipant" + invalid_http_method: "is not a recognised HTTP method" + webhook_templated_host_not_allowed: "cannot have a template parameter in the host" duplicate_pacticipant: | This is the first time a pact has been published for "%{new_name}". The name "%{new_name}" is very similar to the following existing pacticipants: diff --git a/lib/pact_broker/matrix/can_i_deploy_query_schema.rb b/lib/pact_broker/matrix/can_i_deploy_query_schema.rb deleted file mode 100644 index 04065385f..000000000 --- a/lib/pact_broker/matrix/can_i_deploy_query_schema.rb +++ /dev/null @@ -1,46 +0,0 @@ -require "dry-validation" -require "pact_broker/messages" -require "pact_broker/api/contracts/dry_validation_predicates" -require "pact_broker/project_root" -require "pact_broker/string_refinements" - -module PactBroker - module Api - module Contracts - class CanIDeployQuerySchema - extend PactBroker::Messages - using PactBroker::StringRefinements - - SCHEMA = Dry::Validation.Schema do - configure do - predicates(DryValidationPredicates) - config.messages_file = PactBroker.project_root.join("lib", "pact_broker", "locale", "en.yml") - end - required(:pacticipant).filled(:str?) - required(:version).filled(:str?) - optional(:to).filled(:str?) - optional(:environment).filled(:str?, :environment_with_name_exists?) - end - - def self.call(params) - result = select_first_message(SCHEMA.call(params).messages(full: true)) - if params[:to] && params[:environment] - result[:to] ||= [] - result[:to] << message("errors.validation.cannot_specify_tag_and_environment") - end - if params[:to].blank? && params[:environment].blank? - result[:environment] ||= [] - result[:environment] << message("errors.validation.must_specify_environment_or_tag") - end - result - end - - def self.select_first_message(messages) - messages.each_with_object({}) do | (key, value), new_messages | - new_messages[key] = [value.first] - end - end - end - end - end -end diff --git a/lib/pact_broker/matrix/service.rb b/lib/pact_broker/matrix/service.rb index 52759f058..398b6c81c 100644 --- a/lib/pact_broker/matrix/service.rb +++ b/lib/pact_broker/matrix/service.rb @@ -52,6 +52,7 @@ def find_for_consumer_and_provider_with_tags params end end + # TODO create a proper contract for this # rubocop: disable Metrics/CyclomaticComplexity def validate_selectors selectors, options = {} error_messages = [] diff --git a/lib/pact_broker/messages.rb b/lib/pact_broker/messages.rb index 223871574..a05fc9c5a 100644 --- a/lib/pact_broker/messages.rb +++ b/lib/pact_broker/messages.rb @@ -23,6 +23,10 @@ def validation_message key, options = {} message("errors.validation." + key, options) end + def validation_message_at_index key, index, options = {} + message("errors.validation." + key, options).chomp(".") + " (at index #{index})" + end + def pluralize(word, count) if count == 1 word diff --git a/lib/pact_broker/pacts/pact_params.rb b/lib/pact_broker/pacts/pact_params.rb index 95a00d8c9..e7f75723c 100644 --- a/lib/pact_broker/pacts/pact_params.rb +++ b/lib/pact_broker/pacts/pact_params.rb @@ -81,24 +81,13 @@ def provider_name_in_pact self[:provider_name_in_pact] end - def consumer - PacticipantName.new(consumer_name, consumer_name_in_pact, "consumer") + def to_hash_for_validation + { + consumer_version_number: consumer_version_number, + consumer: { name: consumer_name, name_in_pact: consumer_name_in_pact }, + provider: { name: provider_name, name_in_pact: provider_name_in_pact } + } end - - def provider - PacticipantName.new(provider_name, provider_name_in_pact, "provider") - end - - class PacticipantName < Struct.new(:name, :name_in_pact, :pacticipant) - def message_args - { - name: name, - name_in_pact: name_in_pact, - pacticipant: pacticipant - } - end - end - end end end diff --git a/lib/pact_broker/verifications/service.rb b/lib/pact_broker/verifications/service.rb index b8824b9e4..0b7dd6dce 100644 --- a/lib/pact_broker/verifications/service.rb +++ b/lib/pact_broker/verifications/service.rb @@ -57,12 +57,6 @@ def delete(verification) verification_repository.delete(verification.id) end - def errors params - contract = PactBroker::Api::Contracts::VerificationContract.new(PactBroker::Domain::Verification.new) - contract.validate(params) - contract.errors - end - def find params verification_repository.find(params.fetch(:consumer_name), params.fetch(:provider_name), params.fetch(:pact_version_sha), params.fetch(:verification_number)) end diff --git a/lib/pact_broker/webhooks/service.rb b/lib/pact_broker/webhooks/service.rb index 0d634cdf3..011a6ade0 100644 --- a/lib/pact_broker/webhooks/service.rb +++ b/lib/pact_broker/webhooks/service.rb @@ -42,27 +42,11 @@ module Service :delete_by_uuid ] => :webhook_repository - # Not actually a UUID. Ah well. - def valid_uuid_format?(uuid) - !!(uuid =~ /^[A-Za-z0-9_\-]{16,}$/) - end def next_uuid SecureRandom.urlsafe_base64 end - def errors webhook, uuid = nil - contract = PactBroker::Api::Contracts::WebhookContract.new(webhook) - contract.validate(webhook.attributes) - messages = contract.errors.messages - - if uuid && !valid_uuid_format?(uuid) - messages["uuid"] = [message("errors.validation.invalid_webhook_uuid")] - end - - OpenStruct.new(messages: messages, empty?: messages.empty?, any?: messages.any?) - end - def update_by_uuid uuid, params webhook = webhook_repository.find_by_uuid(uuid) maintain_redacted_params(webhook, params) diff --git a/pact_broker.gemspec b/pact_broker.gemspec index c344f1931..eef527746 100644 --- a/pact_broker.gemspec +++ b/pact_broker.gemspec @@ -20,7 +20,7 @@ Gem::Specification.new do |gem| if Dir.exist?(".git") Dir.chdir(File.expand_path(__dir__)) do include_patterns = %w[lib/**/* db/**/* docs/**/*.md public/**/* vendor/**/* LICENSE.txt README.md CHANGELOG.md Gemfile pact_broker.gemspec] - exclude_patterns = %w[db/test/**/*] + exclude_patterns = %w[db/test/**/* lib/**/*/README.md] include_list = include_patterns.flat_map{ | pattern | Dir.glob(pattern) } - exclude_patterns.flat_map{ | pattern | Dir.glob(pattern) } `git ls-files -z`.split("\x0") & include_list @@ -52,18 +52,8 @@ Gem::Specification.new do |gem| gem.add_runtime_dependency "json", "~> 2.3" gem.add_runtime_dependency "psych", "~> 4.0" # TODO identify breaking changes and see if we can use 5 gem.add_runtime_dependency "roar", "~> 1.1" - gem.add_runtime_dependency "reform", "~> 2.3.3" - gem.add_runtime_dependency "dry-validation", "~> 0.10.5" - # if dry-container is not locked, get this error: - # An error occurred while loading spec_helper. - # Failure/Error: require "reform/form/dry" - # NoMethodError: - # undefined method `call' for nil:NilClass - # # /Users/beth.skurrie/.gem/ruby/2.7.5/gems/dry-container-0.10.0/lib/dry/container/mixin.rb:151:in `register' - gem.add_runtime_dependency "dry-container", "0.8.0" - # /usr/local/bundle/gems/dry-validation-0.10.7/lib/dry/validation/schema/class_interface.rb:7:in `' [dry-configurable] default value as positional argument to settings is deprecated and will be removed in the next major version - # Provide a `default:` keyword argument instead - gem.add_runtime_dependency "dry-configurable", "0.12.1" + gem.add_runtime_dependency "dry-validation", "~> 1.8" + gem.add_runtime_dependency "reform", "~> 2.3" gem.add_runtime_dependency "sequel", "~> 5.28" gem.add_runtime_dependency "webmachine", "1.6.0" gem.add_runtime_dependency "semver2", "~> 3.4.2" @@ -75,8 +65,6 @@ Gem::Specification.new do |gem| gem.add_runtime_dependency "haml", "~>5.0" gem.add_runtime_dependency "sucker_punch", "~>2.0" gem.add_runtime_dependency "rack-protection", ">= 2.0.8.1", "< 3.0" - gem.add_runtime_dependency "dry-types", "~> 0.10.3" # https://travis-ci.org/pact-foundation/pact_broker/jobs/249448621 - gem.add_runtime_dependency "dry-logic", "0.4.2" # Later version cases ArgumentError: wrong number of arguments gem.add_runtime_dependency "table_print", "~> 1.5" gem.add_runtime_dependency "semantic_logger", "~> 4.11" gem.add_runtime_dependency "sanitize", "~> 6.0" diff --git a/spec/fixtures/approvals/publish_contract_with_validation_error.approved.json b/spec/fixtures/approvals/publish_contract_with_validation_error.approved.json index e1d0be5f1..ec0227be3 100644 --- a/spec/fixtures/approvals/publish_contract_with_validation_error.approved.json +++ b/spec/fixtures/approvals/publish_contract_with_validation_error.approved.json @@ -33,7 +33,7 @@ "body": { "errors": { "pacticipantVersionNumber": [ - "pacticipantVersionNumber is missing" + "is missing" ] } } diff --git a/spec/lib/pact_broker/matrix/can_i_deploy_query_schema_spec.rb b/spec/lib/pact_broker/api/contracts/can_i_deploy_query_schema_spec.rb similarity index 59% rename from spec/lib/pact_broker/matrix/can_i_deploy_query_schema_spec.rb rename to spec/lib/pact_broker/api/contracts/can_i_deploy_query_schema_spec.rb index 0917548c6..b811a81c5 100644 --- a/spec/lib/pact_broker/matrix/can_i_deploy_query_schema_spec.rb +++ b/spec/lib/pact_broker/api/contracts/can_i_deploy_query_schema_spec.rb @@ -1,9 +1,15 @@ -require "pact_broker/matrix/can_i_deploy_query_schema" +require "pact_broker/api/contracts/can_i_deploy_query_schema" module PactBroker module Api module Contracts describe CanIDeployQuerySchema do + before do + allow(PactBroker::Pacticipants::Service).to receive(:find_pacticipant_by_name).and_return(pacticipant) + end + + let(:pacticipant) { double("pacticipant") } + subject { CanIDeployQuerySchema.call(params) } context "with valid params" do @@ -18,6 +24,32 @@ module Contracts it { is_expected.to be_empty } end + context "with missing params" do + let(:params) do + { + pacticipant: nil, + version: nil, + to: nil + } + end + + it { is_expected.to_not be_empty } + end + + context "with the wrong types" do + let(:params) do + { + pacticipant: 1, + version: 1, + to: 1 + } + end + + its([:pacticipant]) { is_expected.to eq ["must be a string"] } + its([:version]) { is_expected.to eq ["must be a string"] } + its([:to]) { is_expected.to eq ["must be a string"] } + end + context "with a to tag and an environment specified" do before do allow(PactBroker::Deployments::EnvironmentService).to receive(:find_by_name).and_return(double("environment")) @@ -32,7 +64,7 @@ module Contracts } end - it { is_expected.to_not be_empty } + its([nil, 0]) { is_expected.to include("both") } end context "with neither a to tag or an environment specified" do @@ -47,7 +79,7 @@ module Contracts } end - it { is_expected.to_not be_empty } + its([nil, 0]) { is_expected.to include("either") } end context "when the environment does exist" do @@ -79,7 +111,20 @@ module Contracts } end - its([:environment, 0]) { is_expected.to eq "environment with name 'prod' does not exist" } + its([:environment, 0]) { is_expected.to eq "with name 'prod' does not exist" } + end + + context "when the pacticipant does not exist" do + let(:pacticipant) { nil } + + let(:params) do + { + pacticipant: "foo", + version: "1" + } + end + + its([:pacticipant, 0]) { is_expected.to eq "does not match an existing pacticipant" } end end end diff --git a/spec/lib/pact_broker/api/contracts/environment_schema_spec.rb b/spec/lib/pact_broker/api/contracts/environment_schema_spec.rb index e23da9829..4c8be99ee 100644 --- a/spec/lib/pact_broker/api/contracts/environment_schema_spec.rb +++ b/spec/lib/pact_broker/api/contracts/environment_schema_spec.rb @@ -48,7 +48,7 @@ module Contracts context "when there is another environment with the same name but a different uuid" do let(:existing_environment) { instance_double("PactBroker::Deployments::Environment", uuid: "5678")} - its([:name]) { is_expected.to eq ["Another environment with name 'test' already exists."] } + its([:name]) { is_expected.to eq ["name 'test' is already used by an existing environment."] } end context "when there is another environment with the same name and same uuid" do @@ -72,7 +72,17 @@ module Contracts }] end - its([:contacts, 0]) { is_expected.to eq "name is missing at index 0" } + its([:contacts, 0]) { is_expected.to eq "name is missing (at index 0)" } + end + + context "with a multi line owner name" do + let(:contacts) do + [{ + name: "foo\nbar" + }] + end + + its([:contacts, 0]) { is_expected.to eq "name cannot contain multiple lines (at index 0)" } end context "with string contact details" do @@ -83,7 +93,7 @@ module Contracts }] end - its([:contacts, 0]) { is_expected.to eq "details must be a hash at index 0" } + its([:contacts, 0]) { is_expected.to eq "details must be a hash (at index 0)" } end end end diff --git a/spec/lib/pact_broker/api/contracts/pacticipant_create_schema_spec.rb b/spec/lib/pact_broker/api/contracts/pacticipant_create_schema_spec.rb index 8927c9db4..4bf5a8e6c 100644 --- a/spec/lib/pact_broker/api/contracts/pacticipant_create_schema_spec.rb +++ b/spec/lib/pact_broker/api/contracts/pacticipant_create_schema_spec.rb @@ -1,4 +1,4 @@ -require "pact_broker/api/contracts/pacticipant_schema" +require "pact_broker/api/contracts/pacticipant_create_schema" module PactBroker module Api @@ -6,7 +6,7 @@ module Contracts describe PacticipantCreateSchema do let(:params) do { - name: "pact-broker", + name: name, displayName: "Pact Broker", mainBranch: main_branch, repositoryUrl: "https://github.com/pact-foundation/pact_broker", @@ -15,6 +15,8 @@ module Contracts } end + let(:name) { "pact-broker" } + let(:main_branch) { "main" } subject { PacticipantCreateSchema.call(params) } @@ -23,6 +25,24 @@ module Contracts it { is_expected.to be_empty } end + context "with an empty name" do + let(:name) { "" } + + it { is_expected.to_not be_empty } + end + + context "with a blank name" do + let(:name) { " " } + + it { is_expected.to_not be_empty } + end + + context "with a branch that has a space" do + let(:main_branch) { "origin main" } + + its([:mainBranch, 0]) { is_expected.to eq "cannot contain spaces" } + end + context "with empty params" do let(:params) do { @@ -32,7 +52,7 @@ module Contracts } end - its([:name, 0]) { is_expected.to include "name is missing" } + its([:name, 0]) { is_expected.to include "is missing" } end end end diff --git a/spec/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema_combinations_spec.rb b/spec/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema_combinations_spec.rb index 0231180cc..4d79a264c 100644 --- a/spec/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema_combinations_spec.rb +++ b/spec/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema_combinations_spec.rb @@ -21,6 +21,7 @@ module Contracts VALID_KEY_COMBINATIONS = [ [:mainBranch], + [:mainBranch, :latest], [:matchingBranch], [:tag], [:tag, :latest], diff --git a/spec/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema_spec.rb b/spec/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema_spec.rb index dd95871fe..80b36d572 100644 --- a/spec/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema_spec.rb +++ b/spec/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema_spec.rb @@ -4,11 +4,6 @@ module PactBroker module Api module Contracts describe PactsForVerificationJSONQuerySchema do - before do - allow(PactBroker::Deployments::EnvironmentService).to receive(:find_by_name).and_return(environment) - end - let(:environment) { double("environment") } - let(:params) do { providerVersionTags: provider_version_tags, @@ -63,7 +58,7 @@ module Contracts context "when there are multiple errors" do let(:consumer_version_selectors) do [{ - consumer: "", + consumer: " ", tag: "feat-x", fallbackTag: "master" }] @@ -158,12 +153,12 @@ module Contracts let(:consumer_version_selectors) do [{ tag: "feat-x", - consumer: "" + consumer: " " }] end it "has an error" do - expect(subject[:consumerVersionSelectors].first).to include "blank" + expect(subject[:consumerVersionSelectors].first).to include "consumer cannot be blank (at index 0)" end end @@ -201,6 +196,10 @@ module Contracts context "when the environment is specified" do + before do + allow(PactBroker::Deployments::EnvironmentService).to receive(:find_by_name).and_return(double("environment")) + end + let(:consumer_version_selectors) do [{ environment: "test" @@ -211,6 +210,10 @@ module Contracts end context "when deployed with an environment is specified" do + before do + allow(PactBroker::Deployments::EnvironmentService).to receive(:find_by_name).and_return(double("environment")) + end + let(:consumer_version_selectors) do [{ environment: "feat", @@ -222,6 +225,10 @@ module Contracts end context "when deployed=false with an environment is specified" do + before do + allow(PactBroker::Deployments::EnvironmentService).to receive(:find_by_name).and_return(double("environment")) + end + let(:consumer_version_selectors) do [{ environment: "feat", @@ -229,7 +236,7 @@ module Contracts }] end - its([:consumerVersionSelectors, 0]) { is_expected.to eq "deployed must be one of: true at index 0" } + its([:consumerVersionSelectors, 0]) { is_expected.to eq "deployed must be one of: true (at index 0)" } end context "when the environment is specified and deployed is nil" do @@ -240,7 +247,7 @@ module Contracts }] end - its([:consumerVersionSelectors, 0]) { is_expected.to eq "deployed can't be blank at index 0" } + its([:consumerVersionSelectors, 0]) { is_expected.to eq "deployed must be filled (at index 0)" } end context "when deployed is nil" do @@ -250,7 +257,7 @@ module Contracts }] end - its([:consumerVersionSelectors, 0]) { is_expected.to eq "deployed can't be blank at index 0" } + its([:consumerVersionSelectors, 0]) { is_expected.to eq "deployed must be filled (at index 0)" } end context "when latest=true and an environment is specified" do @@ -340,7 +347,20 @@ module Contracts [{ environment: "prod" }] end - its([:consumerVersionSelectors, 0]) { is_expected.to eq "environment with name 'prod' does not exist at index 0" } + its([:consumerVersionSelectors, 0]) { is_expected.to eq "environment with name 'prod' does not exist (at index 0)" } + end + + context "when the environment name does not pass schema validation" do + let(:environment) { nil } + + let(:consumer_version_selectors) do + [{ environment: 1 }] + end + + it "does not attempt to look up the environment" do + expect(PactBroker::Deployments::EnvironmentService).to_not receive(:find_by_name) + subject + end end context "when matchingBranch is true, but the providerVersionBranch is not set" do @@ -352,6 +372,25 @@ module Contracts its([:consumerVersionSelectors, 0]) { is_expected.to include "the providerVersionBranch must be specified"} end + + context "when the providerVersionBranch is a space" do + let(:provider_version_branch) { " " } + + let(:params) do + { + providerVersionBranch: provider_version_branch, + consumerVersionSelectors: consumer_version_selectors + } + end + + let(:consumer_version_selectors) do + [{ + matchingBranch: true + }] + end + + its([:providerVersionBranch, 0]) { is_expected.to include "cannot be blank"} + end end end end diff --git a/spec/lib/pact_broker/api/contracts/pacts_for_verification_query_string_schema_spec.rb b/spec/lib/pact_broker/api/contracts/pacts_for_verification_query_string_schema_spec.rb index 38186cf36..3d300b1fd 100644 --- a/spec/lib/pact_broker/api/contracts/pacts_for_verification_query_string_schema_spec.rb +++ b/spec/lib/pact_broker/api/contracts/pacts_for_verification_query_string_schema_spec.rb @@ -40,7 +40,7 @@ module Contracts end it "flattens the messages" do - expect(subject[:consumer_version_selectors].first).to eq "tag is missing at index 0" + expect(subject[:consumer_version_selectors].first).to eq "tag is missing (at index 0)" end end @@ -88,7 +88,7 @@ module Contracts end it "has an error" do - expect(subject[:consumer_version_selectors].first).to include "blank" + expect(subject[:consumer_version_selectors].first).to include "filled" end end end diff --git a/spec/lib/pact_broker/api/contracts/publish_contracts_schema_spec.rb b/spec/lib/pact_broker/api/contracts/publish_contracts_schema_spec.rb index 727708c47..decc5fd24 100644 --- a/spec/lib/pact_broker/api/contracts/publish_contracts_schema_spec.rb +++ b/spec/lib/pact_broker/api/contracts/publish_contracts_schema_spec.rb @@ -11,22 +11,26 @@ module Contracts :tags => tags, :branch => branch, :buildUrl => build_url, - :contracts => [ - { - :consumerName => consumer_name, - :providerName => "Bar", - :specification => "pact", - :contentType => content_type, - :content => encoded_contract, - :decodedContent => decoded_content, - :decodedParsedContent => decoded_parsed_content - } - ] + :contracts => contracts } end + let(:contracts) do + [ + { + :consumerName => consumer_name, + :providerName => provider_name, + :specification => "pact", + :contentType => content_type, + :content => encoded_contract, + :decodedContent => decoded_content, + :decodedParsedContent => decoded_parsed_content + } + ] + end let(:pacticipant_name) { "Foo" } let(:consumer_name) { pacticipant_name } + let(:provider_name) { "Bar" } let(:version_number) { "34" } let(:tags) { ["a", "b"] } let(:branch) { "main" } @@ -46,6 +50,12 @@ module Contracts context "with an empty tag" do let(:tags) { [""] } + its([:tags, 0]) { is_expected.to include "filled" } + end + + context "with a blank tag" do + let(:tags) { [" "] } + its([:tags, 0]) { is_expected.to include "blank" } end @@ -76,19 +86,19 @@ module Contracts context "when the decoded parsed content is nil" do let(:decoded_parsed_content) { nil } - its([:contracts, 0]) { is_expected.to include "The content could not be parsed as application/json" } + its([:contracts, 0]) { is_expected.to include "content could not be parsed as application/json" } context "when the content type is also nil" do let(:content_type) { nil } - its([:contracts, 0]) { is_expected.to include "contentType can't be blank at index 0" } + its([:contracts, 0]) { is_expected.to include "contentType must be filled (at index 0)" } end end - context "when the consumer name in the content does not match the pacticipant name" do + context "when the consumer name in the content does not match the consumerName in the contract params" do let(:contract_hash) { { "consumer" => { "name" => "WRONG" }, "provider" => { "name" => "Bar" }, "interactions" => [] } } - its([:contracts, 0]) { is_expected.to include "consumer name in contract content ('WRONG') must match pacticipantName ('Foo') at index 0" } + its([:contracts, 0]) { is_expected.to include "consumer name in contract content ('WRONG') must match consumerName in contract params ('Foo') (at index 0)" } end context "when there is no consumer name in the content" do @@ -100,13 +110,19 @@ module Contracts context "when the consumer name in the contract node does not match the pacticipant name" do let(:consumer_name) { "WRONG" } - its([:contracts, 0]) { is_expected.to include "consumerName ('WRONG') must match pacticipantName ('Foo') at index 0" } + its([:contracts]) { is_expected.to include(match("consumerName ('WRONG') must match pacticipantName ('Foo') (at index 0)")) } end context "when the providerName in the contract node does not match the provider name in the contract content" do let(:contract_hash) { { "consumer" => { "name" => "Foo" }, "provider" => { "name" => "WRONG" }, "interactions" => [] } } - its([:contracts, 0]) { is_expected.to include "provider name in contract content ('WRONG') must match providerName ('Bar') in contracts at index 0" } + its([:contracts, 0]) { is_expected.to include "provider name in contract content ('WRONG') must match providerName in contract params ('Bar') (at index 0)" } + end + + context "when the providerName in the contract node is a space" do + let(:provider_name) { " " } + + its([:contracts, 0]) { is_expected.to include "blank" } end context "when the consumer name is missing and there is a validation error with the content" do @@ -115,10 +131,10 @@ module Contracts end it "handles multiple errors" do - expect(subject[:contracts]).to include "consumerName is missing at index 0" - expect(subject[:contracts]).to include "providerName is missing at index 0" - expect(subject[:contracts]).to include "contentType is missing at index 0" - expect(subject[:contracts]).to include "specification is missing at index 0" + expect(subject[:contracts]).to include "consumerName is missing (at index 0)" + expect(subject[:contracts]).to include "providerName is missing (at index 0)" + expect(subject[:contracts]).to include "contentType is missing (at index 0)" + expect(subject[:contracts]).to include "specification is missing (at index 0)" end end @@ -131,6 +147,18 @@ module Contracts expect(subject[:contracts].first).to include "UTF-8 character at char 17" end end + + context "when the contracts array does not contain hashes" do + let(:contracts) { ["string" ]} + + it { is_expected.to_not be_empty } + end + + context "when the contracts is not an array" do + let(:contracts) { "string" } + + it { is_expected.to_not be_empty } + end end end end diff --git a/spec/lib/pact_broker/api/contracts/put_pact_params_contract_spec.rb b/spec/lib/pact_broker/api/contracts/put_pact_params_contract_spec.rb index 05f644a99..0d98ec2c4 100644 --- a/spec/lib/pact_broker/api/contracts/put_pact_params_contract_spec.rb +++ b/spec/lib/pact_broker/api/contracts/put_pact_params_contract_spec.rb @@ -1,4 +1,5 @@ require "pact_broker/api/contracts/put_pact_params_contract" +require "pact_broker/pacts/pact_params" module PactBroker module Api @@ -9,7 +10,7 @@ module Contracts end let(:json_content) { {"some" => "json" }.to_json } - let(:pact_params) { Pacts::PactParams.new(attributes) } + let(:pact_params) { Pacts::PactParams.new(attributes).to_hash_for_validation } let(:order_versions_by_date) { false } let(:valid_attributes) do @@ -23,29 +24,25 @@ module Contracts } end - subject { PutPactParamsContract.new(pact_params) } + subject { PutPactParamsContract.call(pact_params) } describe "errors" do let(:attributes) { valid_attributes } - before do - subject.validate(attributes) - end - context "with valid params" do it "is empty" do - expect(subject.errors).to be_empty + expect(subject).to be_empty end end - context "with a nil consumer version number" do + context "with a blank consumer version number" do let(:attributes) do - valid_attributes.merge(consumer_version_number: nil) + valid_attributes.merge(consumer_version_number: " ") end it "returns an error" do - expect(subject.errors[:consumer_version_number]).to include("can't be blank") + expect(subject[:consumer_version_number].first).to include("blank") end end @@ -55,7 +52,7 @@ module Contracts end it "returns an error" do - expect(subject.errors[:consumer_version_number]).to include("can't be blank") + expect(subject[:consumer_version_number].first).to include("filled") end end @@ -65,7 +62,7 @@ module Contracts end it "returns an error" do - expect(subject.errors[:consumer_version_number]).to include(/Consumer version number 'blah' cannot be parsed to a version number/) + expect(subject[:consumer_version_number]).to include(/Version number 'blah' cannot be parsed to a version number/) end end @@ -78,7 +75,7 @@ module Contracts end it "does not return an error" do - expect(subject.errors[:consumer_version_number]).to be_empty + expect(subject).to be_empty end end end @@ -89,7 +86,7 @@ module Contracts end it "returns an error" do - expect(subject.errors[:'consumer.name']).to include("does not match consumer name in path ('another consumer').") + expect(subject[:'consumer.name']).to include("name in pact 'consumer' does not match name in URL path 'another consumer'.") end end @@ -99,7 +96,7 @@ module Contracts end it "returns an error" do - expect(subject.errors[:'provider.name']).to include("does not match provider name in path ('another provider').") + expect(subject[:'provider.name']).to include("name in pact 'provider' does not match name in URL path 'another provider'.") end end @@ -111,7 +108,7 @@ module Contracts end it "returns no error because I don't want to stop a different CDC from being published" do - expect(subject.errors).to be_empty + expect(subject).to be_empty end end @@ -123,7 +120,7 @@ module Contracts end it "returns no error because I don't want to stop a different CDC from being published" do - expect(subject.errors).to be_empty + expect(subject).to be_empty end end end diff --git a/spec/lib/pact_broker/api/contracts/verification_contract_spec.rb b/spec/lib/pact_broker/api/contracts/verification_contract_spec.rb index 331638dcc..3969c4c04 100644 --- a/spec/lib/pact_broker/api/contracts/verification_contract_spec.rb +++ b/spec/lib/pact_broker/api/contracts/verification_contract_spec.rb @@ -7,9 +7,9 @@ module Api module Contracts describe VerificationContract do - subject { VerificationContract.new(verification) } + subject { VerificationContract.call(params) } let(:verification) { PactBroker::Domain::Verification.new } - let(:valid_params) { {success: success, providerApplicationVersion: provider_version, buildUrl: build_url} } + let(:valid_params) { { success: success, providerApplicationVersion: provider_version, buildUrl: build_url } } let(:params) { valid_params } let(:success) { true } @@ -26,64 +26,69 @@ def modify hash, options before do allow(PactBroker.configuration).to receive(:order_versions_by_date).and_return(order_versions_by_date) - subject.validate(params) end context "with valid fields" do - its(:errors) { is_expected.to be_empty } + it { is_expected.to be_empty } end context "with no success property" do let(:success) { nil } it "has an error" do - expect(subject.errors[:success]).to include(match("blank")) + expect(subject[:success]).to include(match("boolean")) end end context "when success is a non-boolean string" do let(:success) { "foo" } + it "has an error" do - expect(subject.errors[:success]).to include(match("boolean")) + expect(subject[:success]).to include(match("boolean")) end end context "when buildUrl is not a URL" do let(:build_url) { "foo bar" } + it "has an error" do - expect(subject.errors[:build_url]).to include(match("URL")) + expect(subject[:buildUrl]).to include(match("URL")) end end context "when buildUrl is nil" do let(:build_url) { nil } - its(:errors) { is_expected.to be_empty } + + it { is_expected.to be_empty } end context "when the buildURL is not present" do let(:params) { modify valid_params, without: :buildUrl } - its(:errors) { is_expected.to be_empty } + + it { is_expected.to be_empty } end context "when the buildURL is not stringable" do let(:build_url) { {} } it "has an error" do - expect(subject.errors[:build_url]).to include(match("URL")) + expect(subject[:buildUrl]).to include(match("string")) end end context "when the providerApplicationVersion is not present" do let(:params) { modify valid_params, without: :providerApplicationVersion } + it "has an error" do - expect(subject.errors[:provider_version]).to include(match("can't be blank")) + expect(subject[:providerApplicationVersion]).to include(match("missing")) end end context "when the providerApplicationVersion is blank" do let(:provider_version) { " " } + it "has an error" do - expect(subject.errors[:provider_version]).to include(match("can't be blank")) + expect(subject[:providerApplicationVersion]).to contain_exactly(match("blank")) end end @@ -92,7 +97,8 @@ def modify hash, options context "when the providerApplicationVersion is not a semantic version" do let(:provider_version) { "#" } - its(:errors) { is_expected.to be_empty } + + it { is_expected.to be_empty } end end @@ -101,7 +107,7 @@ def modify hash, options let(:provider_version) { "#" } it "has an error" do - expect(subject.errors[:provider_version]).to include(match("#.*cannot be parsed")) + expect(subject[:providerApplicationVersion]).to include(match("#.*cannot be parsed")) end end end diff --git a/spec/lib/pact_broker/api/contracts/webhook_contract_spec.rb b/spec/lib/pact_broker/api/contracts/webhook_contract_spec.rb index 9003e730c..84db7b1b2 100644 --- a/spec/lib/pact_broker/api/contracts/webhook_contract_spec.rb +++ b/spec/lib/pact_broker/api/contracts/webhook_contract_spec.rb @@ -8,11 +8,12 @@ module Contracts let(:json) { load_fixture "webhook_valid_with_pacticipants.json" } let(:hash) { JSON.parse(json) } let(:webhook) { PactBroker::Api::Decorators::WebhookDecorator.new(Domain::Webhook.new).from_json(json) } - let(:subject) { WebhookContract.new(webhook) } let(:matching_hosts) { ["foo"] } let(:consumer) { double("consumer") } let(:provider) { double("provider") } + subject { WebhookContract.call(hash) } + def valid_webhook_with hash = load_json_fixture "webhook_valid_with_pacticipants.json" yield hash @@ -26,7 +27,6 @@ def valid_webhook_with allow(PactBroker::Webhooks::CheckHostWhitelist).to receive(:call).and_return(whitelist_matches) allow(PactBroker::Pacticipants::Service).to receive(:find_pacticipant_by_name).with("Foo").and_return(consumer) allow(PactBroker::Pacticipants::Service).to receive(:find_pacticipant_by_name).with("Bar").and_return(provider) - subject.validate(hash) end let(:webhook_http_method_whitelist) { ["POST"] } @@ -35,7 +35,7 @@ def valid_webhook_with context "with valid fields" do it "is empty" do - expect(subject.errors).to be_empty + expect(subject).to be_empty end end @@ -47,7 +47,7 @@ def valid_webhook_with end it "contains an error" do - expect(subject.errors[:'consumer.name']).to eq ["can't be blank"] + expect(subject[:'consumer.name']).to eq ["cannot be blank"] end end @@ -61,7 +61,7 @@ def valid_webhook_with # I'd prefer this to be "is missing". Isn't the whole point of dry validation # that you can distingush between keys being missing and values being missing? FFS. it "contains an error" do - expect(subject.errors[:'consumer.name']).to eq ["can't be blank"] + expect(subject[:'consumer.name']).to eq ["cannot be blank"] end end @@ -73,7 +73,7 @@ def valid_webhook_with end it "contains no errors" do - expect(subject.errors).to be_empty + expect(subject).to be_empty end end @@ -81,7 +81,7 @@ def valid_webhook_with let(:consumer) { nil } it "contains no errors" do - expect(subject.errors[:'consumer.name']).to eq ["does not match an existing pacticipant"] + expect(subject[:'consumer.name']).to eq ["does not match an existing pacticipant"] end end @@ -94,7 +94,7 @@ def valid_webhook_with end it "contains no errors" do - expect(subject.errors).to be_empty + expect(subject).to be_empty end end @@ -106,7 +106,7 @@ def valid_webhook_with end it "contains consumer.label error" do - expect(subject.errors.messages).to eq({:'consumer.label' => ["cannot be provided"]}) + expect(subject[:'consumer.label']).to contain_exactly(match("name and label cannot be provided at the same time")) end end @@ -118,7 +118,7 @@ def valid_webhook_with end it "contains an error" do - expect(subject.errors[:'provider.name']).to eq ["can't be blank"] + expect(subject[:'provider.name']).to eq ["cannot be blank"] end end @@ -132,7 +132,7 @@ def valid_webhook_with # I'd prefer this to be "is missing". Isn't the whole point of dry validation # that you can distingush between keys being missing and values being missing? FFS. it "contains an error" do - expect(subject.errors[:'provider.name']).to eq ["can't be blank"] + expect(subject[:'provider.name']).to eq ["cannot be blank"] end end @@ -144,7 +144,7 @@ def valid_webhook_with end it "contains no errors" do - expect(subject.errors).to be_empty + expect(subject).to be_empty end end @@ -152,7 +152,7 @@ def valid_webhook_with let(:provider) { nil } it "contains no errors" do - expect(subject.errors[:'provider.name']).to eq ["does not match an existing pacticipant"] + expect(subject[:'provider.name']).to eq ["does not match an existing pacticipant"] end end @@ -165,7 +165,7 @@ def valid_webhook_with end it "contains no errors" do - expect(subject.errors).to be_empty + expect(subject).to be_empty end end @@ -177,7 +177,7 @@ def valid_webhook_with end it "contains provider.label error" do - expect(subject.errors.messages).to eq({:'provider.label' => ["cannot be provided"]}) + expect(subject[:'provider.label']).to contain_exactly(match("name and label cannot be provided at the same time")) end end @@ -185,7 +185,7 @@ def valid_webhook_with let(:json) { {}.to_json } it "contains an error for missing request" do - expect(subject.errors[:request]).to eq ["can't be blank"] + expect(subject[:request]).to contain_exactly(match("is missing")) end end @@ -193,7 +193,7 @@ def valid_webhook_with let(:json) { {}.to_json } it "does not contain an error for missing event, as it will be defaulted" do - expect(subject.errors.messages[:events]).to be nil + expect(subject[:events]).to be nil end end @@ -205,7 +205,7 @@ def valid_webhook_with end it "contains an error for missing request" do - expect(subject.errors[:events]).to eq ["size cannot be less than 1"] + expect(subject[:events]).to eq ["size cannot be less than 1"] end end @@ -217,7 +217,7 @@ def valid_webhook_with end it "contains an error" do - expect(subject.errors[:'events.name'].first).to include "must be one of" + expect(subject[:events]).to contain_exactly(match("must be one of")) end end @@ -229,7 +229,7 @@ def valid_webhook_with end it "contains an error for missing method" do - expect(subject.errors[:"request.http_method"]).to include "can't be blank" + expect(subject[:"request.method"]).to contain_exactly(match("is missing")) end end @@ -241,7 +241,7 @@ def valid_webhook_with end it "contains an error for invalid method" do - expect(subject.errors[:"request.http_method"]).to include "is not a recognised HTTP method" + expect(subject[:"request.method"]).to include "is not a recognised HTTP method" end end @@ -253,7 +253,7 @@ def valid_webhook_with end it "contains an error for the URL" do - expect(subject.errors[:"request.url"]).to include "scheme must be https. See /doc/webhooks#whitelist for more information." + expect(subject[:"request.url"]).to include "scheme must be https. See /doc/webhooks#whitelist for more information." end end @@ -266,22 +266,23 @@ def valid_webhook_with context "when there is only one allowed HTTP method" do it "contains an error for invalid method" do - expect(subject.errors[:"request.http_method"]).to include "must be POST. See /doc/webhooks#whitelist for more information." + expect(subject[:"request.method"]).to include "must be POST. See /doc/webhooks#whitelist for more information." end end - context "when there is more than one allowed HTTP method", pending: "need to work out how to dynamically create this message" do + context "when there is more than one allowed HTTP method" do let(:webhook_http_method_whitelist) { ["POST", "GET"] } it "contains an error for invalid method" do - expect(subject.errors[:"request.http_method"]).to include "must be one of POST, GET" + expect(subject[:"request.method"]).to contain_exactly(match("must be one of POST, GET")) end end end context "when the host whitelist is empty" do it "does not attempt to validate the host" do - expect(PactBroker::Webhooks::CheckHostWhitelist).to_not have_received(:call) + expect(PactBroker::Webhooks::CheckHostWhitelist).to_not receive(:call) + subject end end @@ -289,14 +290,15 @@ def valid_webhook_with let(:webhook_host_whitelist) { [/foo/, "bar"] } it "validates the host" do - expect(PactBroker::Webhooks::CheckHostWhitelist).to have_received(:call).with("some.url", webhook_host_whitelist) + expect(PactBroker::Webhooks::CheckHostWhitelist).to receive(:call).with("some.url", webhook_host_whitelist) + subject end context "when the host does not match the whitelist" do let(:whitelist_matches) { [] } - it "contains an error", pending: "need to work out how to do dynamic messages" do - expect(subject.errors[:"request.url"]).to include "host must be in the whitelist /foo/, \"bar\" . See /doc/webhooks#whitelist for more information." + it "contains an error" do + expect(subject[:"request.url"]).to eq ["host must be in the whitelist /foo/, \"bar\". See /doc/webhooks#whitelist for more information."] end end end @@ -309,7 +311,7 @@ def valid_webhook_with end it "contains an error for missing URL" do - expect(subject.errors[:"request.url"]).to include "can't be blank" + expect(subject[:"request.url"]).to contain_exactly(match("missing")) end end @@ -321,7 +323,7 @@ def valid_webhook_with end it "contains an error for invalid URL" do - expect(subject.errors[:"request.url"]).to eq ["is not a valid URL eg. http://example.org"] + expect(subject[:"request.url"]).to eq ["is not a valid URL eg. http://example.org"] end end @@ -333,11 +335,11 @@ def valid_webhook_with end it "contains an error for invalid URL" do - expect(subject.errors[:"request.url"]).to eq ["is not a valid URL eg. http://example.org"] + expect(subject[:"request.url"]).to eq ["is not a valid URL eg. http://example.org"] end end - context "with a URL that has templated parameters in it" do + context "with a URL that has templated parameters in the path" do let(:json) do valid_webhook_with do |hash| hash["request"]["url"] = "https://foo/commits/${pactbroker.consumerVersionNumber}" @@ -345,7 +347,7 @@ def valid_webhook_with end it "is empty" do - expect(subject.errors).to be_empty + expect(subject).to be_empty end end @@ -357,19 +359,19 @@ def valid_webhook_with end it "contains an error" do - expect(subject.errors[:"request.url"]).to eq ["cannot have a template parameter in the host"] + expect(subject[:"request.url"]).to eq ["cannot have a template parameter in the host"] end end - context "when enabled is not a boolean", pending: "I can't work out why this doesn't work" do + context "with enabled that is not a boolean" do let(:json) do valid_webhook_with do |hash| hash["enabled"] = "foo" end end - it "contains an error" do - expect(subject.errors[:enabled]).to eq ["cannot have a template parameter in the host"] + it "contains an error for invalid URL" do + expect(subject[:enabled]).to eq ["must be boolean"] end end @@ -381,7 +383,29 @@ def valid_webhook_with end it "contains an error" do - expect(subject.errors[:"request.http_method"]).to eq ["is not a recognised HTTP method"] + expect(subject[:"request.method"]).to eq ["is not a recognised HTTP method"] + end + end + + context "with a valid uuid" do + let(:json) do + valid_webhook_with do |hash| + hash["uuid"] = "28f40dc2-1e46-4b08-ba7c-1184892fac13" + end + end + + it { is_expected.to be_empty } + end + + ["HIJKLMNOP", "abcdefghigHIJKLMNOP\\", "abcdefghigHIJKLMNOP#", "abcdefghigHIJKLMNOP$"].each do | invalid_uuid | + context "with an invalid uuid #{invalid_uuid}" do + let(:json) do + valid_webhook_with do |hash| + hash["uuid"] = invalid_uuid + end + end + + its([:uuid, 0]) { is_expected.to match "can only contain" } end end end diff --git a/spec/lib/pact_broker/api/resources/all_webhooks_spec.rb b/spec/lib/pact_broker/api/resources/all_webhooks_spec.rb index 6bdb3745d..f6a6932a9 100644 --- a/spec/lib/pact_broker/api/resources/all_webhooks_spec.rb +++ b/spec/lib/pact_broker/api/resources/all_webhooks_spec.rb @@ -26,7 +26,7 @@ module Resources before do allow(webhook_service).to receive(:create).and_return(saved_webhook) allow(webhook_service).to receive(:next_uuid).and_return(next_uuid) - allow(webhook_service).to receive(:errors).and_return(errors) + allow(PactBroker::Api::Contracts::WebhookContract).to receive(:call).and_return(errors) allow(PactBroker::Domain::Webhook).to receive(:new).and_return(webhook) end @@ -37,8 +37,7 @@ module Resources end let(:next_uuid) { "123k2nvkkwjrwk34" } - let(:valid) { true } - let(:errors) { double("errors", empty?: valid, messages: ["messages"]) } + let(:errors) { {} } subject { post path, webhook_json, headers } @@ -52,7 +51,7 @@ module Resources end context "with invalid attributes" do - let(:valid) { false } + let(:errors) { { some: ["messages"] } } it "returns a 400" do subject @@ -66,7 +65,7 @@ module Resources it "returns the validation errors" do subject - expect(JSON.parse(last_response.body, symbolize_names: true)).to eq errors: ["messages"] + expect(JSON.parse(last_response.body, symbolize_names: true)).to eq errors: { some: ["messages"] } end end diff --git a/spec/lib/pact_broker/api/resources/can_i_deploy_spec.rb b/spec/lib/pact_broker/api/resources/can_i_deploy_spec.rb index b45d6fd8d..15b800432 100644 --- a/spec/lib/pact_broker/api/resources/can_i_deploy_spec.rb +++ b/spec/lib/pact_broker/api/resources/can_i_deploy_spec.rb @@ -7,7 +7,6 @@ module Resources include_context "stubbed services" before do - allow(pacticipant_service).to receive(:find_pacticipant_by_name).and_return(pacticipant) allow(matrix_service).to receive(:can_i_deploy).and_return(results) allow_any_instance_of(described_class).to receive(:matrix_service).and_return(matrix_service) allow(PactBroker::Api::Decorators::MatrixDecorator).to receive(:new).and_return(decorator) @@ -36,15 +35,6 @@ module Resources expect(JSON.parse(subject.body)["errors"]["pacticipant"].first).to_not be_empty end end - - context "when the pacticipant does not exist" do - let(:pacticipant) { nil } - - it "returns a 400" do - expect(subject.status).to eq 400 - expect(JSON.parse(subject.body)["errors"]["pacticipant"].first).to match(/Foo.*found/) - end - end end end end diff --git a/spec/lib/pact_broker/api/resources/pact_spec.rb b/spec/lib/pact_broker/api/resources/pact_spec.rb index 3ce94e304..8c7098d9e 100644 --- a/spec/lib/pact_broker/api/resources/pact_spec.rb +++ b/spec/lib/pact_broker/api/resources/pact_spec.rb @@ -80,11 +80,10 @@ module Resources end context "with validation errors" do - let(:errors) { double(:errors, messages: ["messages"]) } + let(:errors) { { messages: ["messages"] } } before do - allow_any_instance_of(Contracts::PutPactParamsContract).to receive(:validate).and_return(false) - allow_any_instance_of(Contracts::PutPactParamsContract).to receive(:errors).and_return(errors) + allow(Contracts::PutPactParamsContract).to receive(:call).and_return(errors) end it "returns a 400 error" do diff --git a/spec/lib/pact_broker/api/resources/pacticipant_webhooks_spec.rb b/spec/lib/pact_broker/api/resources/pacticipant_webhooks_spec.rb index d6c611cbf..cc8c94e31 100644 --- a/spec/lib/pact_broker/api/resources/pacticipant_webhooks_spec.rb +++ b/spec/lib/pact_broker/api/resources/pacticipant_webhooks_spec.rb @@ -30,7 +30,7 @@ module Resources allow(Decorators::WebhooksDecorator).to receive(:new).and_return(decorator) end - subject { get path } + subject { get(path) } describe "for webhooks with a consumer and provider" do it "returns a 200 HAL JSON response" do @@ -113,13 +113,12 @@ module Resources end let(:next_uuid) { "123k2nvkkwjrwk34" } - let(:valid) { true } - let(:errors) { double("errors", empty?: valid, messages: ["messages"]) } + let(:errors) { {} } before do allow(webhook_service).to receive(:create).and_return(saved_webhook) allow(webhook_service).to receive(:next_uuid).and_return(next_uuid) - allow(webhook_service).to receive(:errors).and_return(errors) + allow(PactBroker::Api::Contracts::WebhookContract).to receive(:call).and_return(errors) allow(PactBroker::Domain::Webhook).to receive(:new).and_return(webhook) end @@ -172,8 +171,7 @@ module Resources end context "with invalid attributes" do - - let(:valid) { false } + let(:errors) { { some: ["messages"] } } it "returns a 400" do subject @@ -187,7 +185,7 @@ module Resources it "returns the validation errors" do subject - expect(JSON.parse(last_response.body, symbolize_names: true)).to eq errors: ["messages"] + expect(JSON.parse(last_response.body, symbolize_names: true)).to eq errors: errors end end diff --git a/spec/lib/pact_broker/api/resources/verifications_spec.rb b/spec/lib/pact_broker/api/resources/verifications_spec.rb index 2d34afe3b..7f12b8c25 100644 --- a/spec/lib/pact_broker/api/resources/verifications_spec.rb +++ b/spec/lib/pact_broker/api/resources/verifications_spec.rb @@ -15,15 +15,15 @@ module Resources let(:response_body) { JSON.parse(subject.body, symbolize_names: true) } let(:database_connector) { double("database_connector" )} let(:verification) { double(PactBroker::Domain::Verification) } - let(:errors_empty) { true } let(:parsed_metadata) { { the: "metadata", consumer_version_number: "2", pending: true } } let(:base_url) { "http://example.org" } let(:webhook_execution_configuration) { instance_double(PactBroker::Webhooks::ExecutionConfiguration) } + let(:errors) { {} } before do allow_any_instance_of(Verifications).to receive(:handle_webhook_events) { |&block| block.call } allow(PactBroker::Verifications::Service).to receive(:create).and_return(verification) - allow(PactBroker::Verifications::Service).to receive(:errors).and_return(double(:errors, messages: ["errors"], empty?: errors_empty)) + allow(PactBroker::Api::Contracts::VerificationContract).to receive(:call).and_return(errors) allow(PactBrokerUrls).to receive(:decode_pact_metadata).and_return(parsed_metadata) end @@ -118,14 +118,14 @@ module Resources end context "when invalid parameters are used" do - let(:errors_empty) { false } + let(:errors) { { some: ["errors"]} } it "returns a 400 status" do expect(subject.status).to eq 400 end it "sets errors on the response" do - expect(response_body[:errors]).to eq ["errors"] + expect(response_body[:errors]).to eq errors end end end diff --git a/spec/lib/pact_broker/api/resources/webhook_spec.rb b/spec/lib/pact_broker/api/resources/webhook_spec.rb index 0450576c1..49571f608 100644 --- a/spec/lib/pact_broker/api/resources/webhook_spec.rb +++ b/spec/lib/pact_broker/api/resources/webhook_spec.rb @@ -57,9 +57,10 @@ module Resources allow(PactBroker::Webhooks::Service).to receive(:create).and_return(created_webhook) allow_any_instance_of(Webhook).to receive(:consumer).and_return(consumer) allow_any_instance_of(Webhook).to receive(:provider).and_return(provider) - allow_any_instance_of(Webhook).to receive(:webhook_validation_errors?).and_return(false) + allow(PactBroker::Api::Contracts::WebhookContract).to receive(:call).and_return(errors) end + let(:consumer) { double("consumer") } let(:provider) { double("provider") } let(:webhook) { double("webhook") } @@ -71,11 +72,12 @@ module Resources let(:webhook) { nil } let(:webhook_json) { load_fixture("webhook_valid.json") } let(:uuid) { "some-uuid" } + let(:errors) { {} } subject { put("/webhooks/#{uuid}", webhook_json, "CONTENT_TYPE" => "application/json") } it "validates the UUID" do - expect_any_instance_of(Webhook).to receive(:webhook_validation_errors?).with(parsed_webhook, uuid) + allow(PactBroker::Api::Contracts::WebhookContract).to receive(:call).with(hash_including(uuid: uuid)) subject end diff --git a/spec/lib/pact_broker/verifications/service_spec.rb b/spec/lib/pact_broker/verifications/service_spec.rb index e3ec7c707..306b934cc 100644 --- a/spec/lib/pact_broker/verifications/service_spec.rb +++ b/spec/lib/pact_broker/verifications/service_spec.rb @@ -249,18 +249,6 @@ module Verifications end end end - - describe "#errors" do - let(:params) { {} } - - it "returns errors" do - expect(subject.errors(params)).to_not be_empty - end - - it "returns something that responds to :messages" do - expect(subject.errors(params).messages).to_not be_empty - end - end end end end diff --git a/spec/lib/pact_broker/webhooks/service_spec.rb b/spec/lib/pact_broker/webhooks/service_spec.rb index 2526f78e9..f8a9a6661 100644 --- a/spec/lib/pact_broker/webhooks/service_spec.rb +++ b/spec/lib/pact_broker/webhooks/service_spec.rb @@ -15,36 +15,6 @@ module Webhooks let(:logger) { double("logger").as_null_object } - describe "validate - integration test" do - let(:invalid_webhook) { PactBroker::Domain::Webhook.new } - - context "with no uuid" do - subject { Service.errors(invalid_webhook) } - - it "does not contain an error for the uuid" do - expect(subject.messages).to_not have_key("uuid") - end - end - - context "with a uuid" do - subject { Service.errors(invalid_webhook, "") } - - it "merges the uuid errors with the webhook errors" do - expect(subject.messages["uuid"].first).to include "can only contain" - end - end - end - - describe ".valid_uuid_format?" do - it "does something" do - expect(Service.valid_uuid_format?("_-bcdefghigHIJKLMNOP")).to be true - expect(Service.valid_uuid_format?("HIJKLMNOP")).to be false - expect(Service.valid_uuid_format?("abcdefghigHIJKLMNOP\\")).to be false - expect(Service.valid_uuid_format?("abcdefghigHIJKLMNOP#")).to be false - expect(Service.valid_uuid_format?("abcdefghigHIJKLMNOP$")).to be false - end - end - describe ".delete_by_uuid" do before do td.create_pact_with_hierarchy