Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix oneOf/allOf/anyOf and schema module in Discriminator mapping #455

Merged
merged 34 commits into from
Oct 19, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
024e7d2
fix: cast discriminator when value has atom keys
igormq Aug 16, 2021
bfba2b9
chore: addressing pr comments
Aug 23, 2021
53490dd
fix: add new logic for one of and any of cast
igormq Nov 30, 2021
876bf78
chore: fix format
igormq Nov 30, 2021
5c33b97
chore: fix format
igormq Nov 30, 2021
44de9df
fix: all of with required
igormq Dec 15, 2021
1d83763
fix: small fix on the allOf function
robpmarques Dec 16, 2021
44867b7
fix: fixing required fields outside all_of, one_of and any_of
robpmarques Dec 16, 2021
f4ea70b
chore: adding put_properties to inherit properties on all of, any of …
robpmarques Dec 16, 2021
13a97b2
fix: adding put_properties
robpmarques Dec 16, 2021
b87cf1c
fix: removing put_properties
robpmarques Dec 19, 2021
53f56ea
chore: make a better approach to test for errors in required fields
igormq Dec 23, 2021
36467a5
chore: one of should return errors
igormq Jan 24, 2022
f853eb7
chore: expose all errors
igormq Jan 25, 2022
58388d0
fix: anyOf and allOf casting with additionalProperties
albertored Mar 30, 2022
20bdf66
fix format
albertored Mar 30, 2022
10d5d29
fix: correctly handle write/readOnly on checking required properties
albertored Mar 30, 2022
4d33362
fix: it should be possible to cast a null value when nullabe true in …
albertored Mar 30, 2022
85cfd24
fix: no special treatment for allOf of arrays
albertored Mar 30, 2022
cc1a6c3
fix: nil value when schema has xxxOf
albertored Mar 30, 2022
3a10680
Merge branch 'master' into fix-xxxOf
lucacorti May 23, 2022
ad7547b
Don't prepend discriminator propertyName to path
lucacorti Jun 6, 2022
ed48cc9
Fix tests
lucacorti Jun 6, 2022
5cf10bd
Merge branch 'master' into fix-xxxOf
lucacorti Jul 5, 2022
071d7c5
Fix merge
lucacorti Jul 7, 2022
9599e57
Format and fix tests
lucacorti Jul 7, 2022
55e56f2
Merge branch 'master' into fix-xxxOf
lucacorti Jul 21, 2022
489e93a
Merge branch 'master' into fix-xxxOf
lucacorti Jul 28, 2022
895e2f5
Merge branch 'open-api-spex:master' into fix-xxxOf
lucacorti Jul 31, 2022
75db236
Merge branch 'open-api-spex:master' into fix-xxxOf
lucacorti Aug 3, 2022
7eb02e4
Merge branch 'open-api-spex:master' into fix-xxxOf
lucacorti Sep 26, 2022
33ec112
Address review remarks
lucacorti Sep 26, 2022
d2402d3
Remove casting from atom-keyed maps
mbuhot Oct 19, 2022
cd94ab5
chore: formatting
mbuhot Oct 19, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/open_api_spex/cast/discriminator.ex
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ defmodule OpenApiSpex.Cast.Discriminator do
defp cast_discriminator(%_{value: value, schema: schema} = ctx) do
{discriminator_property, mappings} = discriminator_details(schema)

case value["#{discriminator_property}"] || value[discriminator_property] do
case value["#{discriminator_property}"] do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbuhot I don't remember if I was the one that originally added this line but I think this was needed to support discriminators where the property is an atom, it is indirectly tested in the test you removed in this same commit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a scenario where the discriminator property is an atom?
The test was casting a map where all keys were already atoms, I'm not sure when that would be required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are use cases but I don't have a particular one in mind.

My reasoning was that since it is possible to cast an object with atom keys it should be possible also when the discriminator property is an atom

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igormq Is there a scenario you have where you need to cast data that already has atom keys?
I'm open to including the functionality, I just want to make sure it's not a hack to fix some other bug we might have :)

024e7d2

v when v in ["", nil] ->
error(:no_value_for_discriminator, ctx)

Expand Down
20 changes: 4 additions & 16 deletions lib/open_api_spex/cast/utils.ex
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,10 @@ defmodule OpenApiSpex.Cast.Utils do
# Merge 2 maps considering as equal keys that are atom or string representation
# of that atom. Atom keys takes precedence over string ones.
def merge_maps(map1, map2) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not implemented this and am not sure what the functions achieves. @albertored can you take a look?

l1 = Enum.map(map1, fn {key, val} -> {to_string(key), key, val} end)
l2 = Enum.map(map2, fn {key, val} -> {to_string(key), key, val} end)

l1
|> Kernel.++(l2)
|> Enum.group_by(fn {ks, _, _} -> ks end, fn {_, k, v} -> {k, v} end)
|> Map.new(fn
{_ks, [{k, v}]} ->
{k, v}

{_ks, [{k1, v1}, {k2, v2}]} ->
cond do
is_atom(k2) -> {k2, v2}
is_atom(k1) -> {k1, v1}
true -> {k2, v2}
end
result = Map.merge(map1, map2)
Enum.reduce(result, result, fn
{k, _v}, result when is_atom(k) -> Map.delete(result, to_string(k))
_, result -> result
end)
end

Expand Down
18 changes: 0 additions & 18 deletions test/cast/discriminator_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -195,24 +195,6 @@ defmodule OpenApiSpex.CastDiscriminatorTest do
assert expected == cast_cast(value: input_value, schema: discriminator_schema)
end

test "nested, atom map success", %{schemas: %{dog: dog, cat: cat}} do
# "animal_type" is the discriminator and the keys need to be strings.
input_value = %{data: %{"#{@discriminator}": "Dog", breed: "Corgi", age: 1}}
# Nested schema to better simulate use with JSON API (real world)
discriminator_schema = %Schema{
title: "Nested Skemuh",
type: :object,
properties: %{
data: build_discriminator_schema([dog, cat], :anyOf, String.to_atom(@discriminator), nil)
}
}

# Note: We're expecting to getting atoms back, not strings
expected = {:ok, %{data: %{age: 1, breed: "Corgi", animal_type: "Dog"}}}

assert expected == cast_cast(value: input_value, schema: discriminator_schema)
end

test "nested, with invalid property on discriminator schema", %{
schemas: %{dog: dog, wolf: wolf}
} do
Expand Down