smithy4s icon indicating copy to clipboard operation
smithy4s copied to clipboard

Collection tags are ignored in DynamicSchemaIndex

Open kubukoz opened this issue 1 year ago • 4 comments

//> 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 avatar Apr 02 '24 23:04 kubukoz

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.

kubukoz avatar Apr 02 '24 23:04 kubukoz

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.

Baccata avatar Apr 03 '24 07:04 Baccata

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 avatar Apr 03 '24 17:04 kubukoz

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?

kubukoz avatar Apr 03 '24 17:04 kubukoz