AllOf cast returns a map, but I expected a struct

[angelikatyborska]

Hi!

I have a schema that includes another nested schema. Something like this:

defmodule MyApp.CreateUserResponse do
  OpenApiSpex.schema(
    %{
      title: "CreateUserResponse",
      type: :object,
      properties: %{
        user: MyApp.UserSchema.schema()
      }
    },
    derive?: false
  )
end

defmodule MyApp.UserSchema do
  OpenApiSpex.schema(%{
    title: "UserSchema",
    allOf: [
      SomeOtherSchema1.schema(),
      SomeOtherSchema.schema()
    ]
  })
end

When I call something like OpenApiSpex.cast_value(%{user: %{}}, MyApp.CreateUserResponse.schema()), I get this result:

%MyApp.CreateUserResponse{
  user: %{}
}

but I expected to get:

%MyApp.CreateUserResponse{
  user: %MyApp.UserSchema{}
}

(All this code is pseudo-code, I modified my real app code to serve as a simple example - please let me know if you need me to create a reproduction repo with code that can actually run)

After some debugging, I found this PR: https://github.com/open-api-spex/open_api_spex/pull/455 That PR removed calls of struct(acc, module) from OpenApiSpex.Cast.AllOf, but not from OpenApiSpex.Cast.OneOf and OpenApiSpex.Cast.AnyOf. I am guessing that might have been a mistake. I was able to modify OpenApiSpex.Cast.AllOf in a way that made my code work. It's this: https://github.com/open-api-spex/open_api_spex/compare/master...angelikatyborska:open_api_spex:all-of-struct

If my assumptions and my changes are correct, I can open a PR. I'm opening an issue first according to the contributing guides.

[zorbash]

Hi @angelikatyborska thanks for reporting this well-written issue.

I was also curious about this inconsistency in the past and I thought it was because the code wasn't smart enough to know how to accumulate the properties of the constituent schemas to make them the properties of the parent schema.

Turns out it is smart enough (see: https://github.com/open-api-spex/open_api_spex/blob/master/lib/open_api_spex/schema.ex#L337-L339) which explains why the tests pass on your fork.

I'm positive about turning this into a PR. This will be a backwards incompatible change though, so we'll have to think whether to bump a major version number, or to make it an opt-in behaviour through configuration.

[angelikatyborska]

Sorry for the delayed response!

This will be a backwards incompatible change though, so we'll have to think whether to bump a major version number, or to make it an opt-in behaviour through configuration.

I would want to argue that this is a bug fix, maybe a minor change. It is also consistent with how every other cast work, so I think returning a struct from a AllOf cast should be the default behavior.

In version v3.13.0 and below AllOf cast used to return a struct. Only from version v3.14.0 and up, unexpectedly, it started returning maps. I verified this by checking out both tags and running this modified test:

--- a/test/cast/all_of_test.exs
+++ b/test/cast/all_of_test.exs
@@ -102,6 +102,6 @@ defmodule OpenApiSpex.CastAllOfTest do

   test "with schema having x-type" do
     value = %{fur: true, meow: true}
-    assert {:ok, _} = cast(value: value, schema: CatSchema.schema())
+    assert {:ok, %CatSchema{}} = cast(value: value, schema: CatSchema.schema())
   end
 end

In v3.13.0 the test passes, in v3.14.0 it fails.

I will open a PR now.

[zorbash]

Sorry for the delayed response!

This will be a backwards incompatible change though, so we'll have to think whether to bump a major version number, or to make it an opt-in behaviour through configuration.

I would want to argue that this is a bug fix, maybe a minor change. It is also consistent with how every other cast work, so I think returning a struct from a AllOf cast should be the default behavior.

In version v3.13.0 and below AllOf cast used to return a struct. Only from version v3.14.0 and up, unexpectedly, it started returning maps. I verified this by checking out both tags and running this modified test:

In v3.13.0 the test passes, in v3.14.0 it fails.

Makes sense, we'll release this as a bugfix.