softwaremill / sttp-apispec

OpenAPI, AsyncAPI and JSON Schema Scala models.
Apache License 2.0
23 stars 10 forks source link

Schema comparator fails to detect addition of an optional field #170

Open seveneves opened 2 weeks ago

seveneves commented 2 weeks ago

I would like to use sttp.apispec.validation.SchemaComparator to check if two versions of schema are compatible. But it fails to identify addition of a new optional field.

The following application highlights the problem

import sttp.apispec.validation.{SchemaComparator, SchemaCompatibilityIssue}
import sttp.tapir.Schema
import sttp.tapir.docs.apispec.schema.TapirSchemaToJsonSchema

object TestSchemas extends App {

  object v1 {

    case class TestSchema(oldField: String)
    object TestSchema {
      implicit val Schema: Schema[TestSchema] = sttp.tapir.Schema.derived[TestSchema]
    }
  }

  object v2 {

    case class TestSchema(oldField: String, newField: Option[String] = None)
    object TestSchema {
      implicit val Schema: Schema[TestSchema] = sttp.tapir.Schema.derived[TestSchema]
    }
  }

  private def apiSpecSchema[T: Schema] = TapirSchemaToJsonSchema(
    implicitly[Schema[T]],
    markOptionsAsNullable = true,
  )

  val v1Schema = apiSpecSchema[v1.TestSchema]
  val v2Schema = apiSpecSchema[v2.TestSchema]
  val comparator = new SchemaComparator(Map.empty, Map.empty)
  val issues: List[SchemaCompatibilityIssue] = comparator.compare(v2Schema, v1Schema)
  issues.foreach(println)
  assert(issues.isEmpty)
}

it produces

schemas differ, but it was not possible to determine their compatibility

I read that SchemaComparator should support such cases

In practice, the comparator is designed to detect typical changes that may appear in schemas during API evolution, e.g. adding new fields, changing types, etc.

But also it has the following

In more complex situations, the comparator simply falls back to comparing schemas by plain equality, or returns a GeneralSchemaMismatch

Which seems to be producing my output.

So it is not clear if this is a complex case or it is a bug in SchemaComparator

seveneves commented 2 weeks ago

The problem seems to be caused by method isProductSchema which does the following check

    s.`type`.getOrElse(Nil).filter(_ != SchemaType.Null) == List(SchemaType.Object) &&
      s.properties.nonEmpty &&
      s == Schema(
        `type` = s.`type`,
        properties = s.properties,
        required = s.required,
        dependentRequired = s.dependentRequired,
      )

And in my case last check is not fulfilled. It is caused by having $schema set to http://json-schema.org/draft-04/schema#. So after removing it, the comparator works fine.

I think s == Schema should be replaced with a better check that avoids problems like the one I just described.

adamw commented 2 weeks ago

Would a fix be to ignore the value of $schema on nested schemas? The spec says that embedded schemas MAY specify it, and I suppose schemas with and without $schema are equivalent, as this only influences potential validation of the schema itself, not of the schema's semantics?

Maybe @ghik will have something to say as well :)

ghik commented 2 weeks ago

It might be the case that stripping $schema should be added to this method - I don't recall exactly why it wasn't so far.