Bump zio-schema to 1.0.1 and remove snapshot resolver #2730
Conversation
guersam
requested review from
jdegoes,
vigoo,
adamgfraser and
987Nabil
as code owners
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2730 +/- ##
==========================================
- Coverage 64.56% 64.38% -0.18%
==========================================
Files 148 148
Lines 8649 8668 +19
Branches 1573 1589 +16
==========================================
- Hits 5584 5581 -3
- Misses 3065 3087 +22 ☔ View full report in Codecov by Sentry. |
guersam
force-pushed
the
stable-zio-schema
branch
from
cf462c2
to
0419bd7
Compare
| if (fullDecode) | ||
| AnyOfSchema(Chunk(leftSchema.root, rightSchema.root)) | ||
| else | ||
| OneOfSchema(Chunk(leftSchema.root, rightSchema.root)) |
There was a problem hiding this comment.
I wonder if I understood Schema.Fallback.fullDecode correctly here.
There was a problem hiding this comment.
I think this is wrong. I took a look how it is handled in schema and fullDecode will return a left and a right.
If not set, it will return either a left or right. But in both cases, we can't say if left contais right in their schema or the other way around. And one of says they don't, but any of they say may. Since I don't see a reason to enforce a possible "one of" by checking the schema structure of the two schemas, I think we should just always use any of.
There was a problem hiding this comment.
@987Nabil Thanks for clarifying. I assumed Schema.Fallback was equivalent to Schema.Either when fullDecode is off, using oneOf.
Just double-checking: is anyOf(left, right) sufficient, or should we consider adding a variant for fullDecode, like oneOf(left, right, allOf(left, right))?
There was a problem hiding this comment.
That's a good question. Thinking about it the idea of oneOf(left, right, allOf(left, right)) is more precise. And I don't see a reason not to do it.
|
Forgot to ping after update: @987Nabil |
| ), | ||
| ) | ||
| case Schema.Fallback(left, right, _, _) => | ||
| AnyOfSchema(Chunk(fromZSchema(left, refType), fromZSchema(right, refType))) |
There was a problem hiding this comment.
I am not sure this should be any of. Since the flag for full decode is false here, we expect exactly one value.
That would fit more to one of. It is true, that we don't know, if left contains right or the other way around.
But this would also make this API unusable.
Imagine we have a case class User(name: String) and case class Admin(name: String, roles:List[String], if you hand over a json with name and roles, which one will be deserialized (additional fields are allowed by default)? We have here no discriminator. Only structure. If we use one of and one contains the other, at least the error is visible.
There was a problem hiding this comment.
Ok, I'll change this.
Off the topic, but one thing I'm worried of is the spec doesn't guarantee that the order of elements in oneOf or anyOf matters, so we can't be 100% sure that it exactly expresses the purpose of Fallback.
|
@987Nabil can a new RC be published, current RC5 is broken because of this |
|
There is more that needs to be fixed. But I'll do it soonish |
Fix #2726