Model loading - determinism

[kubukoz]

Hi! My team has struggled with some false positives for ModelDiff saying there are changes, even when the input files haven't changed between two versions of the model.

We've narrowed it down to a difference in metadata merged from multiple files. It seems like, depending on the location on disk of the files being parsed (even if they're in the same relative directories, they may have a different base directory - e.g. in CI runs this may differ from run to run).

You can see this behavior here (runnable online on Scastie):

//> using scala "2.13.10"
//> using lib "software.amazon.smithy:smithy-model:1.28.0"
//> using lib "software.amazon.smithy:smithy-diff:1.28.0"
import software.amazon.smithy.model.Model
import scala.jdk.CollectionConverters._
import scala.util.Random
import software.amazon.smithy.diff.ModelDiff

def loadModel(prefix: String) = {
  val assembler = Model
    .assembler()

  assembler.addUnparsedModel(
    s"$prefix/smithy/first/a.smithy",
    """metadata items = ["item-2"]""",
  )
  assembler.addUnparsedModel(
    s"$prefix/smithy/second/b.smithy",
    """metadata items = ["item-3"]""",
  )
  assembler.addUnparsedModel(
    s"$prefix/smithy/a.smithy",
    """metadata items = ["item-1"]""",
  )

  assembler.assemble().unwrap()
}

def metas(model: Model) =
  model
    .getMetadata()
    .asScala
    .apply("items")
    .expectArrayNode()
    .getElements()
    .asScala
    .map(elem => elem.expectStringNode().getValue())
    .toList

ModelDiff.compare(loadModel("/Users/kubukoz/projects/demos"), loadModel("")).asScala.toList.head
// res0: software.amazon.smithy.model.validation.ValidationEvent = [WARNING] -: Metadata key `items` was changed from [
//     "item-1",
//     "item-2",
//     "item-3"
// ] to [
//     "item-3",
//     "item-2",
//     "item-1"
// ] | ChangedMetadata /smithy/a.smithy:1:18

I'm wondering if it's something that should be expected.

I suppose users shouldn't rely on metadata having any specific order - is that true?
If that's the case, should ModelDiff be adjusted to ignore changes in ordering of metadata on the top level, or should I adjust that on my side? Thanks!

[mtdowling]

I think we just need to make this a LinkedHashMap: https://github.com/awslabs/smithy/blob/main/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java#L96

[daddykotex]

might be unrelated but I also thought about this one when I saw this issue: #1576