diff --git a/tools/src/main/scala/proteus/Change.scala b/tools/src/main/scala/proteus/Change.scala index bbbf536..f57b642 100644 --- a/tools/src/main/scala/proteus/Change.scala +++ b/tools/src/main/scala/proteus/Change.scala @@ -90,7 +90,7 @@ enum Change { case EnumRemoved(_, name) => s"enum '$name' removed" case EnumRenamed(_, oldName, newName) => s"enum renamed from '$oldName' to '$newName'" case EnumValueAdded(_, name, _) => s"enum value '$name' added" - case EnumValueRemoved(_, name, _, reserved) => s"enum value '$name' removed${if (reserved) " (number reserved)" else ""}" + case EnumValueRemoved(_, name, _, reserved) => s"enum value '$name' removed${if (reserved) " (number retained)" else ""}" case EnumValueNumberChanged(_, name, oldNum, newNum) => s"enum value '$name' number changed from $oldNum to $newNum" case EnumValueRenamed(_, _, oldName, newName) => s"enum value '$oldName' renamed to '$newName'" case ReservedAdded(_, reserved) => s"reserved ${Renderer.renderReservedValue(reserved)} added" diff --git a/tools/src/main/scala/proteus/ProtoDiff.scala b/tools/src/main/scala/proteus/ProtoDiff.scala index 889da62..98b3d96 100644 --- a/tools/src/main/scala/proteus/ProtoDiff.scala +++ b/tools/src/main/scala/proteus/ProtoDiff.scala @@ -604,8 +604,10 @@ object ProtoDiff { val rem3New = rem2New.filterNot(v => numKeys.contains(v.intValue)) // Pass 4: unmatched - val removed = rem3Old.map(v => EnumValueRemoved(path, v.name, v.intValue, isNumberReserved(v.intValue, newReserved))) - val added = rem3New.map(v => EnumValueAdded(path, v.name, v.intValue)) + val newNumbers = newValues.map(_.intValue).toSet + val removed = + rem3Old.map(v => EnumValueRemoved(path, v.name, v.intValue, isNumberReserved(v.intValue, newReserved) || newNumbers.contains(v.intValue))) + val added = rem3New.map(v => EnumValueAdded(path, v.name, v.intValue)) exactDiffs ++ nameDiffs ++ numDiffs ++ removed ++ added } @@ -630,12 +632,54 @@ object ProtoDiff { (changes, oldItems.filterNot(renamedOld.contains), newItems.filterNot(renamedNew.contains)) } + private def reservedNumberIntervals(res: List[Reserved]): List[(Int, Int)] = { + val ranges = res + .collect { + case Reserved.Number(n) => (n, n) + case Reserved.Range(s, e) => (s, e) + } + .sortBy(_._1) + ranges + .foldLeft(List.empty[(Int, Int)]) { case (acc, (s, e)) => + acc match { + case (ps, pe) :: tail if s.toLong <= pe.toLong + 1 => (ps, math.max(pe, e)) :: tail + case _ => (s, e) :: acc + } + } + .reverse + } + + /** + * Coverage of `minuend` intervals not covered by `subtrahend`, as canonical Int intervals. + */ + private def subtractIntervals(minuend: List[(Int, Int)], subtrahend: List[(Int, Int)]): List[(Int, Int)] = + minuend.flatMap { case (s, e) => + subtrahend + .foldLeft(List((s.toLong, e.toLong))) { (segments, b) => + val (bs, be) = (b._1.toLong, b._2.toLong) + segments.flatMap { case (ls, le) => + if (be < ls || bs > le) List((ls, le)) + else (if (ls < bs) List((ls, bs - 1)) else Nil) ++ (if (le > be) List((be + 1, le)) else Nil) + } + } + .map { case (ls, le) => (ls.toInt, le.toInt) } + } + + private def intervalToReserved(iv: (Int, Int)): Reserved = + if (iv._1 == iv._2) Reserved.Number(iv._1) else Reserved.Range(iv._1, iv._2) + private def diffReserved(oldRes: List[Reserved], newRes: List[Reserved], path: List[String]): List[Change] = { - val oldSet = oldRes.toSet - val newSet = newRes.toSet - val removed = (oldSet -- newSet).toList.map(r => ReservedRemoved(path, r)) - val added = (newSet -- oldSet).toList.map(r => ReservedAdded(path, r)) - removed ++ added + val oldIv = reservedNumberIntervals(oldRes) + val newIv = reservedNumberIntervals(newRes) + val numberChanges = + subtractIntervals(oldIv, newIv).map(iv => ReservedRemoved(path, intervalToReserved(iv))) ++ + subtractIntervals(newIv, oldIv).map(iv => ReservedAdded(path, intervalToReserved(iv))) + val oldNames = oldRes.collect { case r: Reserved.Name => r }.toSet + val newNames = newRes.collect { case r: Reserved.Name => r }.toSet + val nameChanges = + (oldNames -- newNames).toList.map(r => ReservedRemoved(path, r)) ++ + (newNames -- oldNames).toList.map(r => ReservedAdded(path, r)) + numberChanges ++ nameChanges } private def diffServices(oldSvcs: List[Service], newSvcs: List[Service], path: List[String]): List[Change] = { diff --git a/tools/src/test/scala/proteus/ProtoDiffSpec.scala b/tools/src/test/scala/proteus/ProtoDiffSpec.scala index eae3a17..4f6a866 100644 --- a/tools/src/test/scala/proteus/ProtoDiffSpec.scala +++ b/tools/src/test/scala/proteus/ProtoDiffSpec.scala @@ -333,6 +333,15 @@ object ProtoDiffSpec extends ZIOSpecDefault { val old = parse("""syntax = "proto3"; enum Status { UNKNOWN = 0; ACTIVE = 1; }""") val nw = parse("""syntax = "proto3"; enum Status { UNKNOWN = 0; ENABLED = 1; }""") assertTrue(ProtoDiff.diff(old, nw) == List(EnumValueRenamed(List("Status"), 1, "ACTIVE", "ENABLED"))) + }, + test("removing an enum alias whose number is still used is wire-compatible") { + val old = parse("""syntax = "proto3"; enum E { option allow_alias = true; A = 0; B = 1; C = 1; }""") + val nw = parse("""syntax = "proto3"; enum E { option allow_alias = true; A = 0; B = 1; }""") + val changes = ProtoDiff.diff(old, nw) + assertTrue( + changes == List(EnumValueRemoved(List("E"), "C", 1, numberReserved = true)), + changes.forall(c => ProtoDiff.severity(c, CompatMode.Wire) == Severity.Info) + ) } ), suite("Nested messages")( @@ -440,6 +449,26 @@ object ProtoDiffSpec extends ZIOSpecDefault { val old = parse("""syntax = "proto3"; message Foo { string name = 1; reserved 2; }""") val nw = parse("""syntax = "proto3"; message Foo { string name = 1; }""") assertTrue(ProtoDiff.diff(old, nw) == List(ReservedRemoved(List("Foo"), Reserved.Number(2)))) + }, + test("reformatting a reserved range as individual numbers is not a change") { + val old = parse("""syntax = "proto3"; message Foo { reserved 2 to 4; }""") + val nw = parse("""syntax = "proto3"; message Foo { reserved 2, 3, 4; }""") + assertTrue(ProtoDiff.diff(old, nw) == Nil) + }, + test("a reserved-name change alongside a range reformat only reports the name change") { + val old = parse("""syntax = "proto3"; message Foo { reserved 2 to 4; }""") + val nw = parse("""syntax = "proto3"; message Foo { reserved 2, 3, 4; reserved "foo"; }""") + assertTrue(ProtoDiff.diff(old, nw) == List(ReservedAdded(List("Foo"), Reserved.Name("foo")))) + }, + test("extending a reserved range while reformatting reports only the added number") { + val old = parse("""syntax = "proto3"; message Foo { reserved 2 to 4; }""") + val nw = parse("""syntax = "proto3"; message Foo { reserved 2, 3, 4, 5; }""") + assertTrue(ProtoDiff.diff(old, nw) == List(ReservedAdded(List("Foo"), Reserved.Number(5)))) + }, + test("dropping one number from a reserved range reports only that number") { + val old = parse("""syntax = "proto3"; message Foo { reserved 2 to 4; }""") + val nw = parse("""syntax = "proto3"; message Foo { reserved 2, 4; }""") + assertTrue(ProtoDiff.diff(old, nw) == List(ReservedRemoved(List("Foo"), Reserved.Number(3)))) } ), suite("Option changes")(