open-api-spex / open_api_spex

Open API Specifications for Elixir Plug applications
Mozilla Public License 2.0
724 stars 187 forks source link

Casting schema with `allOf` and `properties` does not work with strings #641

Open xxdavid opened 2 weeks ago

xxdavid commented 2 weeks ago

The title may be a bit cryptic but here are a few examples

alias OpenApiSpex.Schema

%Schema{
  type: :object,
  allOf: [
    %Schema{
      type: :object,
      properties: %{a: %Schema{type: :integer}}
    }
  ],
}
|> then(&OpenApiSpex.cast_value(%{"a" => 1}, &1))
# => {:ok, %{a: 1}}

and

%Schema{
  type: :object,
  properties: %{b: %Schema{type: :integer}},
  required: [:b]
}
|> then(&OpenApiSpex.cast_value(%{"b" => 2}, &1))
# => {:ok, %{b: 2}}

but

%Schema{
  allOf: [
    %Schema{
      type: :object,
      properties: %{a: %Schema{type: :integer}}
    }
  ],
  type: :object,
  properties: %{b: %Schema{type: :integer}},
  required: [:b]
}
|> then(&OpenApiSpex.cast_value(%{"a" => 1, "b" => 2}, &1))
# => {:error,
# [
#   %OpenApiSpex.Cast.Error{
#     reason: :missing_field,
#     value: %{"a" => 1, "b" => 2},
#     format: nil,
#     type: nil,
#     name: :b,
#     path: [:b],
#     length: 0,
#     meta: %{}
#   }
# ]}

However, if I use an atom for b, everything works.

%Schema{
  allOf: [
    %Schema{
      type: :object,
      properties: %{a: %Schema{type: :integer}}
    }
  ],
  type: :object,
  properties: %{b: %Schema{type: :integer}},
  required: [:b]
}
|> then(&OpenApiSpex.cast_value(%{"a" => 1, :b => 2}, &1))
# => {:ok, %{a: 1, b: 2}}
msutkowski commented 1 week ago

@xxdavid I wouldn't expect those examples to work. Based on the spec, allOf should really only be evaluated by itself and your b schema should be the 2nd element in the list. E.g.

%Schema{
  allOf: [
    %Schema{
      type: :object,
      properties: %{a: %Schema{type: :integer}}
    },
    %Schema{
      type: :object,
      required: [:b],
      properties: %{b: %Schema{type: :integer}}
    }
  ],
}

References:

The behavior you're showing at the end there with an atom for b seems like a bug to me and I wouldn't expect any variation of that to work 🤔

xxdavid commented 1 week ago

Hi @msutkowski, can you please quote anything in the spec or docs that explicitly says that allOf cannot be combined with properties? I've found the spec and docs quite vague on this. Based on the fact that all examples in the docs used allOf without other keys, we can say that perhaps that is the intended use. However, using it with other keys is not forbidden either.

Nevertheless, I've found an example in the docs for JSON schema (which AFAIK Open API's schema is based on) which uses both allOf and properties: https://json-schema.org/understanding-json-schema/reference/object#extending (section Extending Closed Schemas). But I am not sure what's the exact relation between Open API's schema and JSON Schema and whether we can draw conclusions from this.

msutkowski commented 1 week ago

Hi @msutkowski, can you please quote anything in the spec or docs that explicitly says that allOf cannot be combined with properties? I've found the spec and docs quite vague on this. Based on the fact that all examples in the docs used allOf without other keys, we can say that perhaps that is the intended use. However, using it with other keys is not forbidden either.

Nevertheless, I've found an example in the docs for JSON schema (which AFAIK Open API's schema is based on) which uses both allOf and properties: https://json-schema.org/understanding-json-schema/reference/object#extending (section Extending Closed Schemas). But I am not sure what's the exact relation between Open API's schema and JSON Schema and whether we can draw conclusions from this.

Just what I linked already. From a general usability perspective, IMO it doesn't really make sense to merge a base object into a potential union type. But, even though its not explicitly stated that "you can't use base properties + anyOf/oneOf/allOf... it does say:

Use the anyOf keyword to validate the data against any amount of the given subschemas.

It's also not a part of the passing test scenarios, so I wouldn't bother trying to bend things - https://github.com/OAI/OpenAPI-Specification/blob/main/tests/v3.0/pass/petstore-expanded.yaml#L128