ernestoittig / xod

Parsing and schema validation library for Elixir
MIT License
21 stars 2 forks source link

Way to mark key pure optional? #1

Open dvv opened 2 months ago

dvv commented 2 months ago

Hi!

How do I denote that a map key may be either missing or be of an exact type? This is slightly different from key being nullable.

TIA

dvv commented 2 months ago

I addressed the issue here https://github.com/ernestoittig/xod/compare/main...dvv:xod:main Please consider applying

ernestoittig commented 2 months ago

Hi! Thanks for showing interest in my little library! I'm currently away from my computer and won't be back until late tomorrow. However I did take a cursory look and I now remember why I didn't include this feature when I wrote this a year ago. The way I see it there are three options:

Either way this is an expected feature so I should choose one option in the end. I'll need some time to think about it

dvv commented 2 months ago

I see. I'm not quite content with own solution as well -- if I do not put Optional as the last modifier yo a key it won't be seen by the map parser. However it works and gives us time to think it over.

As for remaining options I'd prefer the second as imo optional keys maintenance is the task for the map itself. The third looks kinda rather academic to me and would complicate things imo.

Thank you for an excellent peace of software!

ernestoittig commented 2 months ago

From playing around I have decided on an implementation of 2. Until now if a key's schema was nilable, it would match if the key was missing; this is also how Default works. In that case the key would (obviously) be included in the result. I don't want to break this behaviour.

The way I see it there are five outcomes, in pseudo-Elixir:

case {hasKey, keySchemaMatched, keyIsOptional} do
  {true, true, _} ->
    {:ok, Map.put(result, key, parseResult)} # value from key's schema included
  {true, false, _} ->
    {:error, parseError} # error from parsing with key's schema
  {false, true, _} -> # key is not present but key's schema is nilable
    {:ok, Map.put(result, key, parseResult)} # value is included in the result, even if it's nil
  {false, false, true} -> # key's schema failed but key optional
    {:ok, result} # key is skipped and not present in result
  {false, false, false} ->
    {:error, error_key_missing(key)} # error with :missing_key type
end

I think this would work for your use case, and is quite elegant. The only unexpected side effect I can think of would be that you could receive an odd :missing_key error in some complex schemata that admit nil. E.g. piping the result of Default into a custom email schema, but not making the default value be a valid email address. But I think that's a very unlikely edge case and it's probably fine.


Thank you for an excellent peace of software!

Thanks but Colin McDonnell deserves all the praise for creating Zod and inspiring this whole thing

dvv commented 2 months ago

I would argue against

  {false, false, true} -> # key's schema failed but key optional
    {:ok, result} # key is skipped and not present in result

IMO optional means either completely missing or completely valid.

ernestoittig commented 2 months ago

I think I may have made this more confusing than it needed to be. In order for branch you highlighted to match, the following things need to all be true:

Now the important thing here is that the key's schema failed because the key is missing and the schema doesn't accept nil values.

As an example:

    my_map = Xod.map(%{my_key: Xod.string()}, optional: [:my_key])

    assert Xod.parse(my_map, %{my_key: "This is valid"}) == {:ok, %{my_key: "This is valid"}}
    assert_raise(Xod.XodError, "Expected string, got number (in path [:my_key])", fn ->
      Xod.parse!(my_map, %{my_key: 28})
    end)
    assert_raise(Xod.XodError, "Expected string, got nil (in path [:my_key])", fn ->
      Xod.parse!(my_map, %{my_key: nil})
    end)
    assert Xod.parse(my_map, %{}) == {:ok, %{}}

That is from a test that is passing.

This whole calling-the-schema-on-the-nil-value thing is so that the following holds

assert Xod.map(%{x: Xod.number() |> Xod.default(0)}) |> Xod.parse!(%{}) == %{x: 0}
assert Xod.map(%{x: Xod.union([Xod.number(), Xod.literal(nil)])}) |> Xod.parse!(%{}) == %{x: nil}