Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds an accumulating decode API for DynamicValue that can collect multiple decode errors (rather than failing fast) and extends test coverage to validate success-path parity and multi-error accumulation.
Changes:
- Introduce
DynamicValue.toTypedValueAccumulatingbacked by an accumulatingValidation-based decoder. - Accumulate decode errors across tuples, sequences, sets, maps, and record structures.
- Add
DynamicValueSpectests for parity on successful decodes and for multi-error accumulation scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| zio-schema/shared/src/main/scala/zio/schema/DynamicValue.scala | Adds accumulating decode API and record-structure accumulating decoder. |
| tests/shared/src/test/scala/zio/schema/DynamicValueSpec.scala | Adds focused tests for accumulating behavior and success-path parity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def decodeStructureAccumulating( | ||
| values: ListMap[String, DynamicValue], | ||
| structure: Chunk[Schema.Field[_, _]] | ||
| ): Validation[DecodeError, ListMap[String, _]] = { | ||
| val keys = values.keySet | ||
| keys.foldLeft[Validation[DecodeError, ListMap[String, Any]]](Validation.succeed(ListMap.empty)) { | ||
| case (acc, key) => | ||
| (structure.find(_.name == key), values.get(key)) match { | ||
| case (Some(field), Some(value)) => | ||
| acc.zipWithPar( | ||
| value.toTypedValueAccumulatingLazyError(field.schema) | ||
| )((record, value) => record + (key -> value)) | ||
| case _ => | ||
| acc.zipWithPar(Validation.fail(DecodeError.IncompatibleShape(values, structure)))((record, _) => record) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
decodeStructureAccumulating folds over values.keySet, which can (a) miss schema fields that are absent from values (so missing-field / incompatible-shape errors won’t be accumulated), and (b) produce a record map in an order unrelated to structure. This is particularly problematic for Schema.Record[A] decoding, where s.construct(Chunk.fromIterable(m.values)) is typically positional and expects schema-field ordering. A concrete fix is to drive decoding primarily from structure (preserving field order and detecting missing values), and separately detect/accumulate errors for unexpected extra keys in values.
| .foldLeft[Validation[DecodeError, Chunk[t]]](Validation.succeed(Chunk.empty)) { (acc, value) => | ||
| acc.zipWithPar( | ||
| value.toTypedValueAccumulatingLazyError(schema.elementSchema) | ||
| )(_ :+ _) | ||
| } | ||
| .map(schema.fromChunk) |
There was a problem hiding this comment.
Chunk appends via (_ :+ _) can lead to quadratic behavior for large sequences because repeated appends may copy increasingly large chunks. Prefer accumulating into a builder/collection with efficient append (or accumulate in reverse with prepend-then-reverse) and convert once at the end, while still using Validation to accumulate errors.
| .foldLeft[Validation[DecodeError, Chunk[t]]](Validation.succeed(Chunk.empty)) { (acc, value) => | |
| acc.zipWithPar( | |
| value.toTypedValueAccumulatingLazyError(schema.elementSchema) | |
| )(_ :+ _) | |
| } | |
| .map(schema.fromChunk) | |
| .foldLeft[Validation[DecodeError, List[t]]](Validation.succeed(List.empty[t])) { (acc, value) => | |
| acc.zipWithPar( | |
| value.toTypedValueAccumulatingLazyError(schema.elementSchema) | |
| )((list, elem) => elem :: list) | |
| } | |
| .map { reversedList => | |
| schema.fromChunk(Chunk.fromIterable(reversedList.reverse)) | |
| } |
| result match { | ||
| case ZValidation.Success(_, value) => assertTrue(value == person) | ||
| case ZValidation.Failure(_, _) => assertTrue(false) | ||
| } |
There was a problem hiding this comment.
Using assertTrue(false) on the failure branch discards useful failure information and makes debugging harder. Prefer asserting directly on the Validation shape/value using ZIO Test assertions (e.g., assert that it is Success with the expected value), so failures report the collected errors.
| case (DynamicValue.SetValue(values), schema: Schema.Set[t]) => | ||
| values.foldLeft[Validation[DecodeError, Set[t]]](Validation.succeed(Set.empty[t])) { (acc, value) => | ||
| acc.zipWithPar( | ||
| value.toTypedValueAccumulatingLazyError(schema.elementSchema) | ||
| )(_ + _) | ||
| } |
There was a problem hiding this comment.
This PR adds new accumulating behavior for SetValue and Dictionary element decoding, but the new DynamicValueSpec cases only cover record/tuple/sequence accumulation. Add tests that demonstrate multiple independent failures are accumulated for sets and maps as well (e.g., multiple bad keys/values and multiple bad set elements), to validate the newly introduced branches.
| case (DynamicValue.Dictionary(entries), schema: Schema.Map[k, v]) => | ||
| entries.foldLeft[Validation[DecodeError, Map[k, v]]](Validation.succeed(Map.empty[k, v])) { (acc, entry) => | ||
| val typedKey = entry._1.toTypedValueAccumulatingLazyError(schema.keySchema) | ||
| val typedValue = entry._2.toTypedValueAccumulatingLazyError(schema.valueSchema) | ||
| acc.zipWithPar( | ||
| typedKey.zipWithPar(typedValue)(_ -> _) | ||
| )(_ + _) | ||
| } |
There was a problem hiding this comment.
This PR adds new accumulating behavior for SetValue and Dictionary element decoding, but the new DynamicValueSpec cases only cover record/tuple/sequence accumulation. Add tests that demonstrate multiple independent failures are accumulated for sets and maps as well (e.g., multiple bad keys/values and multiple bad set elements), to validate the newly introduced branches.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #480
Summary
DynamicValue.toTypedValueAccumulatingas a backward-compatible accumulating APIDynamicValueSpeccoverage for success-path parity and multi-error accumulationValidation
testsJVM/testOnly zio.schema.DynamicValueSpeczioSchemaJVM/scalafmtChecktestsJVM/scalafmtCheck/claim #480