disneystreaming / smithy4s

https://disneystreaming.github.io/smithy4s/
Other
348 stars 70 forks source link

Collection tags are ignored in DynamicSchemaIndex #1465

Open kubukoz opened 6 months ago

kubukoz commented 6 months ago
//> using scala "3.4.1"
//> using dep "com.disneystreaming.smithy4s::smithy4s-json:0.18.15"
//> using dep "com.disneystreaming.smithy4s::smithy4s-dynamic:0.18.15"
import smithy4s.schema.Schema
import smithy4s.Blob
import smithy4s.json.Json
import smithy4s.ShapeId
import software.amazon.smithy.model.Model
import smithy4s.dynamic.DynamicSchemaIndex

val input =
  """
$version: "2"

namespace demo

structure Hello {
  @required name: String
}

@uniqueItems
list Hellos { member: Hello }
""".stripMargin

val dsi = DynamicSchemaIndex.loadModel(
  Model.assembler().addUnparsedModel("foo.smithy", input).assemble().unwrap()
)

val schema = dsi.getSchema(ShapeId.parse("demo#Hellos").get).get

println(schema.asInstanceOf[Schema.CollectionSchema[?, ?]].tag)

prints ListTag.

Is this intentional? It seems similar to how we (don't) handle refinements like @length, but it's inconsistent with how we do handle enum tags (including open enums).

kubukoz commented 6 months ago

Separate question, although one that follows from the above: if we were to capture collection tags as something more than hints, Dynamic will also fail to deconflict set items that are more complex than simple shapes - because of array equality (used for pretty much any aggregate shape in Dynamic). That pretty much makes Dynamic unfeasible without extra pre-processing before passing the schemas to standard codecs like Json / Document.

Baccata commented 6 months ago

I don't think collection tags should be captured by Dynamic, as collection tags are performance related, not behaviour related. I think defaulting to lists is likely wrong, and defaulting to vectors may be better. Generally speaking, hints from smithy4s.meta should be disregarded by dynamic, if anything to save us some rabbit holes.

I do think our treatment of uniqueItems is off, as hinted in your other issue. It should be treated as a validation constraint, not as something dictating the collection type.

kubukoz commented 6 months ago

so the other issue (#1466 for other readers) would touch the general treatment of uniqueItems. I created #1469 (nice) for the vector default as per your suggestion.

I don't think we've addressed the array equality problem. If we consider uniqueItems similar to other constraint traits (which it may very well become after `#1469), it may make sense that we leave Dynamic out of it. However, if someone were to try and make Dynamic aware of such a constraint (for example, Playground does this for other constraints), how would they do that then? And I mean in existing codecs provided by smithy4s, not in custom schema compilers.

kubukoz commented 6 months ago

Exposing the "non-implicit, parameterized" refinements might help:

https://github.com/disneystreaming/smithy4s/blob/3a69c3755dc56efad3c3bf45860855e259608343/modules/core/src/smithy4s/RefinementProvider.scala#L53-L54

but in this case I think you'd want sth like

def uniqueItemsRefinementProvider[A](mkEqualityCheck: Schema[A] => Eq[A]): Simple[UniqueItems, List[A], List[A]]

so that the equality check for a given type could be derived using, say, Document.Encoder. Does this sound reasonable?