diff --git a/CHANGELOG.md b/CHANGELOG.md index 5efe2150..a131d90d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] * Your contribution here +- [#17](https://github.com/numbata/grape-oas/pull/17): Support for nested rules and predicates in dry-schema introspection [@slbug](https://github.com/slbug) - [#20](https://github.com/numbata/grape-oas/pull/20): Use annotation for coverage report [@numbata](https://github.com/numbata) - [#18](https://github.com/numbata/grape-oas/pull/18): Support for range in size? predicate `required(:tags).value(:array, size?: 1..10).each(:string)` [@slbug](https://github.com/slbug) - [#19](https://github.com/numbata/grape-oas/pull/19): Temporary disable memory profiler workflow for PRs [@numbata](https://github.com/numbata). diff --git a/lib/grape_oas/introspectors/dry_introspector.rb b/lib/grape_oas/introspectors/dry_introspector.rb index c9e18647..9f47f5b6 100644 --- a/lib/grape_oas/introspectors/dry_introspector.rb +++ b/lib/grape_oas/introspectors/dry_introspector.rb @@ -4,6 +4,7 @@ require_relative "dry_introspector_support/contract_resolver" require_relative "dry_introspector_support/inheritance_handler" require_relative "dry_introspector_support/type_schema_builder" +require_relative "dry_introspector_support/rule_index" module GrapeOAS module Introspectors @@ -92,16 +93,30 @@ def cached_schema end def build_flat_schema - rule_constraints = DryIntrospectorSupport::ConstraintExtractor.extract(contract_resolver.contract_schema) + contract_schema = contract_resolver.contract_schema + + constraints_by_path, required_by_object_path = + DryIntrospectorSupport::RuleIndex.build(contract_schema) + + type_schema_builder.configure_path_aware_mode(constraints_by_path, required_by_object_path) + schema = ApiModel::Schema.new( type: Constants::SchemaTypes::OBJECT, canonical_name: contract_resolver.canonical_name, ) - contract_resolver.contract_schema.types.each do |name, dry_type| - constraints = rule_constraints[name] - prop_schema = type_schema_builder.build_schema_for_type(dry_type, constraints) - schema.add_property(name, prop_schema, required: type_schema_builder.required?(dry_type, constraints)) + root_required = required_by_object_path.fetch("", []) + + contract_schema.types.each do |name, dry_type| + name_s = name.to_s + prop_schema = nil + + type_schema_builder.with_path(name_s) do + prop_schema = type_schema_builder.build_schema_for_type(dry_type, + type_schema_builder.constraints_for_current_path,) + end + + schema.add_property(name, prop_schema, required: root_required.include?(name_s)) end # Use canonical_name as registry key for schema objects (they don't have unique classes), diff --git a/lib/grape_oas/introspectors/dry_introspector_support/inheritance_handler.rb b/lib/grape_oas/introspectors/dry_introspector_support/inheritance_handler.rb index 4be30f8d..ff82b43a 100644 --- a/lib/grape_oas/introspectors/dry_introspector_support/inheritance_handler.rb +++ b/lib/grape_oas/introspectors/dry_introspector_support/inheritance_handler.rb @@ -68,15 +68,27 @@ def parent_contract_types(parent_contract) def build_child_only_schema(parent_contract, type_schema_builder) child_schema = ApiModel::Schema.new(type: Constants::SchemaTypes::OBJECT) parent_keys = parent_contract_types(parent_contract) - rule_constraints = ConstraintExtractor.extract(@contract_resolver.contract_schema) + contract_schema = @contract_resolver.contract_schema - @contract_resolver.contract_schema.types.each do |name, dry_type| + constraints_by_path, required_by_object_path = + RuleIndex.build(contract_schema) + + type_schema_builder.configure_path_aware_mode(constraints_by_path, required_by_object_path) + root_required = required_by_object_path.fetch("", []) + + contract_schema.types.each do |name, dry_type| # Skip inherited properties next if parent_keys.include?(name.to_s) - constraints = rule_constraints[name] - prop_schema = type_schema_builder.build_schema_for_type(dry_type, constraints) - child_schema.add_property(name, prop_schema, required: type_schema_builder.required?(dry_type, constraints)) + name_s = name.to_s + prop_schema = nil + + type_schema_builder.with_path(name_s) do + prop_schema = type_schema_builder.build_schema_for_type(dry_type, + type_schema_builder.constraints_for_current_path,) + end + + child_schema.add_property(name, prop_schema, required: root_required.include?(name_s)) end child_schema diff --git a/lib/grape_oas/introspectors/dry_introspector_support/rule_index.rb b/lib/grape_oas/introspectors/dry_introspector_support/rule_index.rb new file mode 100644 index 00000000..9c8c0dd3 --- /dev/null +++ b/lib/grape_oas/introspectors/dry_introspector_support/rule_index.rb @@ -0,0 +1,196 @@ +# frozen_string_literal: true + +require_relative "ast_walker" +require_relative "constraint_extractor" +require_relative "constraint_merger" + +module GrapeOAS + module Introspectors + module DryIntrospectorSupport + # Builds path-aware constraint and required field indexes from dry-schema AST + class RuleIndex + def initialize(contract_schema) + @walker = AstWalker.new(ConstraintExtractor::ConstraintSet) + @merger = ConstraintMerger + @constraints_by_path = {} + @required_by_object_path = Hash.new { |h, k| h[k] = {} } + + build_indexes(contract_schema) + end + + def self.build(contract_schema) + new(contract_schema).to_a + end + + def to_a + [@constraints_by_path, @required_by_object_path.transform_values(&:keys)] + end + + private + + def build_indexes(contract_schema) + rules = contract_schema.respond_to?(:rules) ? contract_schema.rules : {} + rules.each_value do |rule| + ast = rule.respond_to?(:to_ast) ? rule.to_ast : rule + collect_constraints(ast, []) + collect_required(ast, [], in_implication_condition: false) + end + end + + def collect_constraints(ast, path) + return unless ast.is_a?(Array) + + case ast[0] + when :key + key_name, value_ast = parse_key_node(ast) + return unless key_name && value_ast.is_a?(Array) + + new_path = path + [key_name] + apply_node_constraints(value_ast, new_path) + collect_constraints(value_ast, new_path) + + when :each + child = ast[1] + return unless child.is_a?(Array) + + item_path = path + ["[]"] + + # Index constraints that apply to the item schema itself + apply_node_constraints(child, item_path) + + # Recurse so nested keys inside the item get their own paths + collect_constraints(child, item_path) + else + ast.each { |child| collect_constraints(child, path) if child.is_a?(Array) } + end + end + + def collect_required(ast, object_path, in_implication_condition:) + return unless ast.is_a?(Array) + + case ast[0] + when :implication + left, right = ast[1].is_a?(Array) ? ast[1] : [nil, nil] + collect_required(left, object_path, in_implication_condition: true) if left + collect_required(right, object_path, in_implication_condition: false) if right + + when :predicate + mark_required_if_key_predicate(ast[1], object_path) unless in_implication_condition + + when :key, :each + if ast[0] == :key + key_name, value_ast = parse_key_node(ast) + if key_name && value_ast.is_a?(Array) + collect_required(value_ast, object_path + [key_name], + in_implication_condition: in_implication_condition,) + end + elsif ast[1] # :each + collect_required(ast[1], object_path + ["[]"], + in_implication_condition: in_implication_condition,) + end + + else + ast.each do |child| + collect_required(child, object_path, in_implication_condition: in_implication_condition) if child.is_a?(Array) + end + end + end + + def parse_key_node(ast) + info = ast[1] + return [nil, nil] unless info.is_a?(Array) && info.any? + + key_name = info[0] + value_ast = info[1] || info[-1] + [key_name&.to_s, value_ast] + end + + def apply_node_constraints(value_ast, path) + pruned = prune_nested_validations(value_ast) + return unless pruned + + constraints = @walker.walk(pruned) + constraints.required = nil if constraints.respond_to?(:required=) + + path_key = path.join("/") + if @constraints_by_path.key?(path_key) + @merger.merge(@constraints_by_path[path_key], constraints) + else + @constraints_by_path[path_key] = constraints + end + end + + def prune_nested_validations(ast) + return ast unless ast.is_a?(Array) + + tag = ast[0] + return ast unless tag.is_a?(Symbol) + + case tag + when :each, :key + nil + + when :set + children, wrapped = extract_children(ast) + pruned = children.filter_map { |c| c.is_a?(Array) ? prune_nested_validations(c) : c } + return nil if pruned.empty? + + # rewrite set -> and, preserve wrapper style + wrapped ? [:and, pruned] : [:and, *pruned] + + when :and, :or, :rule + children, wrapped = extract_children(ast) + pruned = children.filter_map { |c| c.is_a?(Array) ? prune_nested_validations(c) : c } + return nil if pruned.empty? + + wrapped ? [tag, pruned] : [tag, *pruned] + + when :implication + pair = ast[1] + return ast unless pair.is_a?(Array) && pair.size >= 2 + + left = pair[0].is_a?(Array) ? prune_nested_validations(pair[0]) : pair[0] + right = pair[1].is_a?(Array) ? prune_nested_validations(pair[1]) : pair[1] + [:implication, [left, right]] + + when :not + child = ast[1] + child = prune_nested_validations(child) if child.is_a?(Array) + [:not, child] + + else + ast + end + end + + def extract_children(ast) + # handles both shapes: + # [:and, [node1, node2]] + # [:and, node1, node2] + payload = ast[1] + + if payload.is_a?(Array) && !payload.empty? && payload.all? { |x| x.is_a?(Array) && x[0].is_a?(Symbol) } + [payload, true] # wrapped list + else + [ast[1..], false] # splatted + end + end + + def mark_required_if_key_predicate(pred, object_path) + return unless pred.is_a?(Array) && pred[0] == :key? + + name = extract_key_name(pred) + @required_by_object_path[object_path.join("/")][name] = true if name + end + + def extract_key_name(pred_node) + args = pred_node[1] + return nil unless args.is_a?(Array) + + name_pair = args.find { |x| x.is_a?(Array) && x[0] == :name } + name_pair&.dig(1)&.to_s + end + end + end + end +end diff --git a/lib/grape_oas/introspectors/dry_introspector_support/type_schema_builder.rb b/lib/grape_oas/introspectors/dry_introspector_support/type_schema_builder.rb index 5cac7ca5..b8fddae1 100644 --- a/lib/grape_oas/introspectors/dry_introspector_support/type_schema_builder.rb +++ b/lib/grape_oas/introspectors/dry_introspector_support/type_schema_builder.rb @@ -11,7 +11,39 @@ class TypeSchemaBuilder ConstraintSet = ConstraintExtractor::ConstraintSet def initialize - # Stateless builder - no initialization needed + @path_stack = [] + @constraints_by_path = nil + @required_by_object_path = nil + end + + def configure_path_aware_mode(constraints_by_path, required_by_object_path) + @path_stack = [] + @constraints_by_path = constraints_by_path + @required_by_object_path = required_by_object_path + end + + def with_path(part) + @path_stack << part + yield + ensure + @path_stack.pop + end + + def current_object_path + @path_stack.join("/") + end + + def constraints_for_current_path + return nil unless @constraints_by_path + + @constraints_by_path[current_object_path] + end + + def required_keys_for_current_object + return nil unless @required_by_object_path + + # In path-aware mode we rely entirely on rule-index requiredness + @required_by_object_path[current_object_path] || [] end # Builds a schema for a Dry type. @@ -20,7 +52,7 @@ def initialize # @param constraints [ConstraintSet, nil] extracted constraints # @return [ApiModel::Schema] the built schema def build_schema_for_type(dry_type, constraints = nil) - constraints ||= ConstraintSet.new(unhandled_predicates: []) + constraints ||= constraints_for_current_path || ConstraintSet.new(unhandled_predicates: []) meta = dry_type.respond_to?(:meta) ? dry_type.meta : {} # Check for Sum type first (TypeA | TypeB) -> anyOf @@ -29,6 +61,10 @@ def build_schema_for_type(dry_type, constraints = nil) # Check for Hash schema type (nested schemas like .hash(SomeSchema)) return build_hash_schema(dry_type) if hash_schema_type?(dry_type) + # Check for object schema (unwrapped hash with keys) + unwrapped = TypeUnwrapper.unwrap(dry_type) + return build_object_schema(unwrapped) if unwrapped.respond_to?(:keys) + primitive, member = TypeUnwrapper.derive_primitive_and_member(dry_type) enum_vals = extract_enum_from_type(dry_type) @@ -59,6 +95,36 @@ def required?(dry_type, constraints = nil) private + def build_object_schema(unwrapped_schema_type) + schema = ApiModel::Schema.new(type: Constants::SchemaTypes::OBJECT) + required_keys = required_keys_for_current_object + + # Dry::Types::Schema does not have each_key, so we disable the cop here + unwrapped_schema_type.keys.each do |key| # rubocop:disable Style/HashEachMethods + key_name = key.respond_to?(:name) ? key.name.to_s : key.to_s + key_type = key.respond_to?(:type) ? key.type : nil + + prop_schema = nil + with_path(key_name) do + prop_schema = if key_type + build_schema_for_type(key_type, constraints_for_current_path) + else + default_string_schema + end + end + + is_required = if required_keys + required_keys.include?(key_name) + else + key.respond_to?(:required?) ? key.required? : false + end + + schema.add_property(key_name, prop_schema, required: is_required) + end + + schema + end + def build_any_of_schema(sum_type) types = TypeUnwrapper.extract_sum_types(sum_type) @@ -86,29 +152,35 @@ def hash_schema_type?(dry_type) end def build_hash_schema(dry_type) - schema = ApiModel::Schema.new(type: Constants::SchemaTypes::OBJECT) unwrapped = TypeUnwrapper.unwrap(dry_type) - return schema unless unwrapped.respond_to?(:keys) + # Delegate to the same path-aware logic as regular object schemas. + # This ensures nested rule constraints (e.g. max_size?, gteq?, format?) are applied + # to properties inside `.hash do ... end` blocks. + return ApiModel::Schema.new(type: Constants::SchemaTypes::OBJECT) unless unwrapped.respond_to?(:keys) - # Dry::Schema keys method returns an array of Key objects, not a Hash - schema_keys = unwrapped.keys - schema_keys.each do |key| - key_name = key.respond_to?(:name) ? key.name.to_s : key.to_s - key_type = key.respond_to?(:type) ? key.type : nil - - prop_schema = key_type ? build_schema_for_type(key_type) : default_string_schema - req = key.respond_to?(:required?) ? key.required? : true - schema.add_property(key_name, prop_schema, required: req) - end - - schema + build_object_schema(unwrapped) end def build_base_schema(primitive, member) if primitive == Array - items_schema = member ? build_schema_for_type(member) : default_string_schema + items_schema = nil + + with_path("[]") do + if member + unwrapped = TypeUnwrapper.unwrap(member) + items_schema = if unwrapped.respond_to?(:keys) + build_object_schema(unwrapped) + else + build_schema_for_type(member, constraints_for_current_path) + end + else + items_schema = default_string_schema + end + end + ApiModel::Schema.new(type: Constants::SchemaTypes::ARRAY, items: items_schema) + else build_schema_for_primitive(primitive) end diff --git a/test/e2e/generate_oas2_complex_test.rb b/test/e2e/generate_oas2_complex_test.rb index 86417af8..3b5417ef 100644 --- a/test/e2e/generate_oas2_complex_test.rb +++ b/test/e2e/generate_oas2_complex_test.rb @@ -53,7 +53,7 @@ class PageViewEventEntity < BaseEventEntity BasicContract = Dry::Schema.Params do required(:id).filled(:integer, gt?: 0) optional(:status).maybe(:string, included_in?: %w[draft active]) - optional(:tags).array(:string, min_size?: 1, max_size?: 3) + optional(:tags).value(:array, min_size?: 1, max_size?: 3).each(:string) optional(:code).maybe(:string, format?: /\A[A-Z]{3}\d{2}\z/) end diff --git a/test/e2e/generate_oas3_complex_test.rb b/test/e2e/generate_oas3_complex_test.rb index a2707168..ac13bafb 100644 --- a/test/e2e/generate_oas3_complex_test.rb +++ b/test/e2e/generate_oas3_complex_test.rb @@ -47,7 +47,7 @@ class ClickEventEntity < BaseEventEntity BasicContract = Dry::Schema.Params do required(:id).filled(:integer, gt?: 0) optional(:status).maybe(:string, included_in?: %w[draft active]) - optional(:tags).array(:string, min_size?: 1, max_size?: 3) + optional(:tags).value(:array, min_size?: 1, max_size?: 3).each(:string) optional(:code).maybe(:string, format?: /\A[A-Z]{3}\d{2}\z/) end diff --git a/test/grape_oas/api_model_builders/request_contract_dry_test.rb b/test/grape_oas/api_model_builders/request_contract_dry_test.rb index 8f1e7377..78143df4 100644 --- a/test/grape_oas/api_model_builders/request_contract_dry_test.rb +++ b/test/grape_oas/api_model_builders/request_contract_dry_test.rb @@ -17,7 +17,7 @@ def test_optional_enum_and_array_constraints contract = Dry::Schema.Params do required(:id).filled(:integer) optional(:status).maybe(:string, included_in?: %w[draft published]) - optional(:tags).array(:string, min_size?: 1, max_size?: 3) + optional(:tags).value(:array, min_size?: 1, max_size?: 3).each(:string) end operation = GrapeOAS::ApiModel::Operation.new(http_method: :post) @@ -90,7 +90,7 @@ def test_numeric_bounds_and_excluded def test_array_with_item_constraints_and_nullable contract = Dry::Schema.Params do - optional(:tags).array(:string, min_size?: 1, max_size?: 3) + optional(:tags).value(:array, min_size?: 1, max_size?: 3).each(:string) end operation = GrapeOAS::ApiModel::Operation.new(http_method: :post) diff --git a/test/grape_oas/introspectors/dry_introspector_test.rb b/test/grape_oas/introspectors/dry_introspector_test.rb index 0d52dd2a..fbfe4744 100644 --- a/test/grape_oas/introspectors/dry_introspector_test.rb +++ b/test/grape_oas/introspectors/dry_introspector_test.rb @@ -35,16 +35,126 @@ def test_or_branch_intersection_keeps_common_enum def test_each_array_predicates_apply_to_array_not_items contract = Dry::Schema.Params do + # WARN: THIS IS THE DIFFERENT MACRO USAGE, WITH DIFFERENT EFFECT required(:tags).array(:string, min_size?: 1, max_size?: 3) + required(:each_tags).value(:array, min_size?: 1, max_size?: 3).each(:string) end schema = processor.build(contract) tags = schema.properties["tags"] + each_tags = schema.properties["each_tags"] assert_equal "array", tags.type - assert_equal 1, tags.min_items - assert_equal 3, tags.max_items assert_equal "string", tags.items.type + assert_equal 1, tags.items.min_length + assert_equal 3, tags.items.max_length + assert_equal "array", each_tags.type + assert_equal 1, each_tags.min_items + assert_equal 3, each_tags.max_items + assert_equal "string", each_tags.items.type + end + + def test_inherited_child_nested_constraints + parent_contract = Class.new(Dry::Validation::Contract) do + params { required(:id).filled(:integer) } + end + + child_contract = Class.new(parent_contract) do + params do + required(:items).value(:array, size?: (2..8)).each(:hash) do + required(:code).filled(:string, min_size?: 3, max_size?: 5) + end + end + end + + schema = processor.build(child_contract).all_of.last + items_array = schema.properties["items"] + code = items_array.items.properties["code"] + + assert_equal 3, code.min_length + assert_equal 5, code.max_length + assert_equal 2, items_array.min_items + assert_equal 8, items_array.max_items + end + + def test_nested_array_constraints_no_bleeding + contract = Dry::Schema.Params do + optional(:deliveries).value(:array, max_size?: 2).each(:hash) do + optional(:addresses).array(:string, min_size?: 2, max_size?: 7) + end + + optional(:orders).array(:hash) do + required(:id).filled(:string, format?: /^ORD-\d+$/) + optional(:items).value(:array, min_size?: 1, max_size?: 10).each(:hash) do + required(:code).filled(:string, min_size?: 3, max_size?: 50) + required(:price).filled(:integer, gteq?: 0) + optional(:tags).value(:array, min_size?: 4).each(:string) + optional(:metadata).value(:array, min_size?: 3, max_size?: 8).each(:hash) do + required(:key).filled(:string, max_size?: 256) + optional(:value).filled(:string) + end + end + optional(:notes).array(:string) + end + end + + schema = processor.build(contract) + + orders = schema.properties["orders"] + + assert_equal "array", orders.type + + order_props = orders.items.properties + + assert_equal "string", order_props["id"].type + assert_equal "^ORD-\\d+$", order_props["id"].pattern + + items_array = order_props["items"] + + assert_equal "array", items_array.type + assert_equal 10, items_array.max_items + assert_equal 1, items_array.min_items + + notes_array = order_props["notes"] + + assert_equal "array", notes_array.type + assert_nil notes_array.min_items, "notes should not have min_items" + assert_nil notes_array.max_items, "notes should not have max_items" + + item_props = items_array.items.properties + + assert_equal "string", item_props["code"].type + assert_equal 50, item_props["code"].max_length + assert_equal 3, item_props["code"].min_length + assert_equal "integer", item_props["price"].type + assert_equal 0, item_props["price"].minimum + + tags_array = item_props["tags"] + + assert_equal "array", tags_array.type + assert_equal 4, tags_array.min_items + assert_equal "string", tags_array.items.type + + metadata_array = item_props["metadata"] + + assert_equal "array", metadata_array.type + assert_equal 8, metadata_array.max_items + assert_equal 3, metadata_array.min_items + metadata_props = metadata_array.items.properties + + assert_equal "string", metadata_props["key"].type + assert_equal 256, metadata_props["key"].max_length + assert_nil metadata_props["key"].min_length, "key should not have min_length" + assert_equal "string", metadata_props["value"].type + assert_nil metadata_props["value"].max_length, "value should not have max_length" + assert_nil metadata_props["value"].min_length, "value should not have min_length" + + assert_nil item_props["code"].minimum, "code should not have price minimum" + assert_nil item_props["code"].pattern, "code should not have id pattern" + assert_nil item_props["price"].max_length, "price should not have code max_length" + assert_nil item_props["price"].pattern, "price should not have id pattern" + assert_nil tags_array.items.minimum, "tag items should not have price minimum" + assert_nil tags_array.items.max_length, "tag items should not have code max_length" end def test_size_range_predicate_sets_min_and_max_size @@ -654,10 +764,7 @@ def test_nested_hash_schema_with_optional_field assert_equal "object", dimensions.type assert dimensions.properties.key?("width") assert dimensions.properties.key?("height") - # NOTE: Due to Dry::Types limitation, nested hash keys all report as not required - # The required array will be empty because key.required? returns false for all - assert_empty dimensions.required, - "Nested hash schema keys report as not required due to Dry::Types limitation" + assert_includes(dimensions.required, "width") end def test_deeply_nested_hash_schemas