Add a WIRE_COMPATIBLE compatibility checker level (#966)

Adding new enumration values has special consideration in the compatibility checker. Even though such changes are compatible on the wire level (don't break deser for old clients), they are still considered backwards incompatible by the checker. The documentation explains why:
`However, it’s still not possible to guarantee backward compatibility, even with the “$UNKNOWN” symbol available. It’s possible that clients did not handle the “$UNKNOWN” symbol in the best possible way, and even if they did it may be that they cannot do anything other than fail if they encounter a enum symbol they do not recognize.`

This patch adds a new paramater to the compatibility checker to distinguish this special scenario. Adding new enumeration values will still fail under the BACKWARDS and EQUIVALENT levels, but will be allowed under the WIRE_COMPATIBLE level.

---------

Co-authored-by: Szymon Gizecki <[email protected]>

CHANGELOG.md

@@ -14,6 +14,9 @@ and what APIs have changed, if applicable.
 
 ## [Unreleased]
 
+## [29.49.8] - 2024-01-19
+- add WIRE_COMPATIBLE compatility checker mode.
+
 ## [29.49.7] - 2024-01-18
 adjust dual read monitoring data match logic and log rate limiter
 
@@ -5615,7 +5618,8 @@ patch operations can re-use these classes for generating patch messages.
 
 ## [0.14.1]
 
-[Unreleased]: https://github.com/linkedin/rest.li/compare/v29.49.7...master
+[Unreleased]: https://github.com/linkedin/rest.li/compare/v29.49.8...master
+[29.49.8]: https://github.com/linkedin/rest.li/compare/v29.49.7...v29.49.8
 [29.49.7]: https://github.com/linkedin/rest.li/compare/v29.49.6...v29.49.7
 [29.49.6]: https://github.com/linkedin/rest.li/compare/v29.49.5...v29.49.6
 [29.49.5]: https://github.com/linkedin/rest.li/compare/v29.49.4...v29.49.5

data/src/main/java/com/linkedin/data/schema/compatibility/CompatibilityChecker.java

@@ -553,7 +553,7 @@ private void checkEnum(EnumDataSchema older, EnumDataSchema newer)
 
     if (!newerOnlySymbols.isEmpty())
     {
-      appendMessage(CompatibilityMessage.Impact.BREAKS_OLD_READER,
+      appendMessage(CompatibilityMessage.Impact.ENUM_VALUE_ADDED,
                     "new enum added symbols %s",
                     newerOnlySymbols);
     }

data/src/main/java/com/linkedin/data/schema/compatibility/CompatibilityMessage.java

@@ -73,7 +73,11 @@ public enum Impact
     /**
      * Enum symbol order changed, which is a compatible change.
      */
-    ENUM_SYMBOLS_ORDER_CHANGE(false);
+    ENUM_SYMBOLS_ORDER_CHANGE(false),
+    /**
+     * New enum value added, which is wire compatible change. However, old readers may not be able to handle it.
+     */
+    ENUM_VALUE_ADDED(false);
 
     private final boolean _error;
 

data/src/test/java/com/linkedin/data/schema/compatibility/TestCompatibilityChecker.java

@@ -491,15 +491,13 @@ public void testUnion() throws IOException
         "[ { \"type\" : \"enum\", \"name\" : \"a.b.Enum\", \"symbols\" : [ \"B\", \"D\", \"E\" ] } ]",
         _dataAndSchema,
         true,
-        "ERROR :: BREAKS_OLD_READER :: /union/a.b.Enum/symbols :: new enum added symbols E",
         "ERROR :: BREAKS_NEW_READER :: /union/a.b.Enum/symbols :: new enum removed symbols A, C"
       },
       {
         "[ \"int\", { \"type\" : \"enum\", \"name\" : \"a.b.Enum\", \"symbols\" : [ \"A\", \"B\", \"C\", \"D\" ] }, \"string\" ]",
         "[ \"string\", { \"type\" : \"enum\", \"name\" : \"a.b.Enum\", \"symbols\" : [ \"B\", \"D\", \"E\" ] }, \"int\" ]",
         _dataAndSchema,
         true,
-        "ERROR :: BREAKS_OLD_READER :: /union/a.b.Enum/symbols :: new enum added symbols E",
         "ERROR :: BREAKS_NEW_READER :: /union/a.b.Enum/symbols :: new enum removed symbols A, C"
       },
       {
@@ -787,7 +785,6 @@ public void testEnum() throws IOException
         "{ \"type\" : \"enum\", \"name\" : \"a.b.Enum\", \"symbols\" : [ \"B\", \"D\", \"E\" ] }",
         _dataAndSchema,
         true,
-        "ERROR :: BREAKS_OLD_READER :: /a.b.Enum/symbols :: new enum added symbols E",
         "ERROR :: BREAKS_NEW_READER :: /a.b.Enum/symbols :: new enum removed symbols A, C"
       },
       {

gradle.properties

@@ -1,4 +1,4 @@
-version=29.49.7
+version=29.49.8
 group=com.linkedin.pegasus
 org.gradle.configureondemand=true
 org.gradle.parallel=true

restli-tools/src/main/java/com/linkedin/restli/tools/compatibility/CompatibilityInfoMap.java

@@ -42,9 +42,11 @@ public class CompatibilityInfoMap
   public CompatibilityInfoMap()
   {
     _restSpecMap.put(CompatibilityInfo.Level.INCOMPATIBLE, new ArrayList<>());
+    _restSpecMap.put(CompatibilityInfo.Level.WIRE_COMPATIBLE, new ArrayList<>());
     _restSpecMap.put(CompatibilityInfo.Level.COMPATIBLE, new ArrayList<>());
 
     _modelMap.put(CompatibilityInfo.Level.INCOMPATIBLE, new ArrayList<>());
+    _modelMap.put(CompatibilityInfo.Level.WIRE_COMPATIBLE, new ArrayList<>());
     _modelMap.put(CompatibilityInfo.Level.COMPATIBLE, new ArrayList<>());
 
     _annotationMap.put(CompatibilityInfo.Level.INCOMPATIBLE, new ArrayList<>());
@@ -128,7 +130,11 @@ public void addModelInfo(CompatibilityMessage message)
     }
     else
     {
-      infoType = CompatibilityInfo.Type.TYPE_INFO;
+      if (message.getImpact() == CompatibilityMessage.Impact.ENUM_VALUE_ADDED) {
+        infoType = CompatibilityInfo.Type.ENUM_VALUE_ADDED;
+      } else {
+        infoType = CompatibilityInfo.Type.TYPE_INFO;
+      }
     }
     info = new CompatibilityInfo(Arrays.asList(message.getPath()), infoType, infoMessage);
     _modelMap.get(infoType.getLevel()).add(info);
@@ -194,30 +200,33 @@ private static void createSummaryForInfo(Collection<CompatibilityInfo> info,
   public boolean isCompatible(CompatibilityLevel level)
   {
     final Collection<CompatibilityInfo> incompatibles = getIncompatibles();
+    final Collection<CompatibilityInfo> wireCompatibles = getWireCompatibles();
     final Collection<CompatibilityInfo> compatibles = getCompatibles();
 
-    return isCompatible(incompatibles, compatibles, level);
+    return isCompatible(incompatibles, wireCompatibles, compatibles, level);
   }
 
   public boolean isRestSpecCompatible(CompatibilityLevel level)
   {
     final Collection<CompatibilityInfo> incompatibles = getRestSpecIncompatibles();
     final Collection<CompatibilityInfo> compatibles = getRestSpecCompatibles();
 
-    return isCompatible(incompatibles, compatibles, level);
+    return isCompatible(incompatibles, new ArrayList<>(), compatibles, level);
   }
 
   public boolean isModelCompatible(CompatibilityLevel level)
   {
     final Collection<CompatibilityInfo> incompatibles = getModelIncompatibles();
+    final Collection<CompatibilityInfo> wireCompatibles = getModelWireCompatibles();
     final Collection<CompatibilityInfo> compatibles = getModelCompatibles();
 
-    return isCompatible(incompatibles, compatibles, level);
+    return isCompatible(incompatibles, wireCompatibles, compatibles, level);
   }
 
-  private boolean isCompatible(Collection<CompatibilityInfo> incompatibles, Collection<CompatibilityInfo> compatibles, CompatibilityLevel level)
+  private boolean isCompatible(Collection<CompatibilityInfo> incompatibles, Collection<CompatibilityInfo> wireCompatibles, Collection<CompatibilityInfo> compatibles, CompatibilityLevel level)
   {
-    return ((incompatibles.isEmpty() || level.ordinal() < CompatibilityLevel.BACKWARDS.ordinal()) &&
+    return ((incompatibles.isEmpty() || level.ordinal() < CompatibilityLevel.WIRE_COMPATIBLE.ordinal()) &&
+            (wireCompatibles.isEmpty() || level.ordinal() < CompatibilityLevel.BACKWARDS.ordinal()) &&
             (compatibles.isEmpty()   || level.ordinal() < CompatibilityLevel.EQUIVALENT.ordinal()));
   }
 
@@ -254,6 +263,15 @@ public Collection<CompatibilityInfo> getCompatibles()
     return get(CompatibilityInfo.Level.COMPATIBLE);
   }
 
+  /**
+   * @return check results in the backwards wire compatibility category.
+   *         empty collection if called before checking any files
+   */
+  public Collection<CompatibilityInfo> getWireCompatibles()
+  {
+    return get(CompatibilityInfo.Level.WIRE_COMPATIBLE);
+  }
+
   public Collection<CompatibilityInfo> getRestSpecIncompatibles()
   {
     return getRestSpecInfo(CompatibilityInfo.Level.INCOMPATIBLE);
@@ -274,6 +292,11 @@ public Collection<CompatibilityInfo> getModelCompatibles()
     return getModelInfo(CompatibilityInfo.Level.COMPATIBLE);
   }
 
+  public Collection<CompatibilityInfo> getModelWireCompatibles()
+  {
+    return getModelInfo(CompatibilityInfo.Level.WIRE_COMPATIBLE);
+  }
+
   public Collection<CompatibilityInfo> get(CompatibilityInfo.Level level)
   {
     Collection<CompatibilityInfo> infos = new ArrayList<>(getRestSpecInfo(level));
@@ -347,7 +370,7 @@ public boolean isAnnotationCompatible()
    */
   public boolean isAnnotationCompatible(CompatibilityLevel level)
   {
-    return isCompatible(_annotationMap.get(CompatibilityInfo.Level.INCOMPATIBLE),
+    return isCompatible(_annotationMap.get(CompatibilityInfo.Level.INCOMPATIBLE), new ArrayList<>(),
             _annotationMap.get(CompatibilityInfo.Level.COMPATIBLE), level);
   }
 

restli-tools/src/main/java/com/linkedin/restli/tools/compatibility/CompatibilityReport.java

@@ -26,7 +26,7 @@ public CompatibilityReport(CompatibilityInfoMap infoMap, CompatibilityLevel comp
    *   <li>[RS-C] - String describing a restspec change that is backward compatible.</li>
    *   <li>[RS-I] - String describing a restspec change that is backward incompatible.</li>
    *   <li>[MD-C] - String describing a model(PDSC) change that is backward compatible.</li>
-   *   <li>[MD-C] - String describing a model(PDSC) change that is backward incompatible.</li>
+   *   <li>[MD-I] - String describing a model(PDSC) change that is backward incompatible.</li>
    *   <li>[RS-COMPAT] - Boolean indicating if the full compatibility check was restspec backward compatible for the provided compatibilityLevel</li>
    *   <li>[MD-COMPAT] - Boolean indicating if the full compatibility check was model backward compatible for the provided compatibilityLevel</li>
    * </ul>

restli-tools/src/main/java/com/linkedin/restli/tools/idlcheck/CompatibilityInfo.java

@@ -28,7 +28,13 @@ public class CompatibilityInfo
   public enum Level
   {
     INCOMPATIBLE,
-    COMPATIBLE
+    COMPATIBLE,
+
+    /**
+     * Old readers can deserialize changes serialized by new writers, but may not be able to handle them correctly.
+     * Currently only used for adding new enum values.
+     **/
+    WIRE_COMPATIBLE
   }
 
   public enum Type
@@ -48,6 +54,7 @@ public enum Type
     TYPE_UNKNOWN(Level.INCOMPATIBLE, "Type cannot be resolved: %s"),
     VALUE_NOT_EQUAL(Level.INCOMPATIBLE, "Current value \"%2$s\" does not match the previous value \"%1$s\""),
     VALUE_WRONG_OPTIONALITY(Level.INCOMPATIBLE, "\"%s\" may not be removed because it exists in the previous version"),
+    ENUM_VALUE_ADDED(Level.WIRE_COMPATIBLE, "%s, new enum value may break old readers"),
     TYPE_BREAKS_OLD_READER(Level.INCOMPATIBLE, "%s, breaks old readers"),
     TYPE_BREAKS_NEW_READER(Level.INCOMPATIBLE, "%s, breaks new readers"),
     TYPE_BREAKS_NEW_AND_OLD_READERS(Level.INCOMPATIBLE, "%s, breaks new and old readers"),

restli-tools/src/main/java/com/linkedin/restli/tools/idlcheck/CompatibilityLevel.java

@@ -28,6 +28,7 @@ public enum CompatibilityLevel
 {
   OFF,
   IGNORE,
+  WIRE_COMPATIBLE,
   BACKWARDS,
   EQUIVALENT;
 

restli-tools/src/test/java/com/linkedin/restli/tools/snapshot/check/TestPegasusSchemaSnapshotCompatibilityChecker.java

@@ -48,7 +48,8 @@ public void testCompatiblePegasusSchemaSnapshot(String prevSchema, String currSc
 
   @Test(dataProvider = "incompatibleInputFiles")
   public void testIncompatiblePegasusSchemaSnapshot(String prevSchema, String currSchema,
-      Collection<CompatibilityInfo> expectedIncompatibilityErrors, Collection<CompatibilityInfo> expectedCompatibilityDiffs )
+      Collection<CompatibilityInfo> expectedIncompatibilityErrors, Collection<CompatibilityInfo> expectedWireCompatibilityDiffs,
+      Collection<CompatibilityInfo> expectedCompatibilityDiffs )
   {
     PegasusSchemaSnapshotCompatibilityChecker checker = new PegasusSchemaSnapshotCompatibilityChecker();
     CompatibilityInfoMap infoMap = checker.checkPegasusSchemaCompatibility(snapshotDir + FS + prevSchema, snapshotDir + FS + currSchema,
@@ -58,20 +59,27 @@ public void testIncompatiblePegasusSchemaSnapshot(String prevSchema, String curr
     Assert.assertTrue(infoMap.isModelCompatible(CompatibilityLevel.IGNORE));
 
     final Collection<CompatibilityInfo> modelIncompatibles = infoMap.getModelIncompatibles();
+    final Collection<CompatibilityInfo> modelWireCompatibles = infoMap.getModelWireCompatibles();
     final Collection<CompatibilityInfo> modelCompatibles = infoMap.getModelCompatibles();
 
     for (CompatibilityInfo error : expectedIncompatibilityErrors)
     {
       Assert.assertTrue(modelIncompatibles.contains(error), "Reported model incompatibles should contain: " + error.toString());
       modelIncompatibles.remove(error);
     }
+    for (CompatibilityInfo diff : expectedWireCompatibilityDiffs)
+    {
+      Assert.assertTrue(modelWireCompatibles.contains(diff), "Reported model wireCompatibles should contain: " + diff.toString());
+      modelWireCompatibles.remove(diff);
+    }
     for (CompatibilityInfo diff : expectedCompatibilityDiffs)
     {
       Assert.assertTrue(modelCompatibles.contains(diff), "Reported model compatibles should contain: " + diff.toString());
       modelCompatibles.remove(diff);
     }
 
     Assert.assertTrue(modelIncompatibles.isEmpty());
+    Assert.assertTrue(modelWireCompatibles.isEmpty());
     Assert.assertTrue(modelCompatibles.isEmpty());
   }
 
@@ -145,8 +153,9 @@ private Object[][] compatibleInputFiles()
   {
     return new Object[][]
         {
-            { "BirthInfo.pdl", "compatibleSchemaSnapshot/BirthInfo.pdl", CompatibilityLevel.EQUIVALENT, CompatibilityOptions.Mode.DATA },
+            { "Date.pdl", "compatibleSchemaSnapshot/Date.pdl", CompatibilityLevel.EQUIVALENT, CompatibilityOptions.Mode.DATA },
             { "Foo.pdl", "compatibleSchemaSnapshot/Foo.pdl", CompatibilityLevel.BACKWARDS, CompatibilityOptions.Mode.EXTENSION },
+            { "BirthInfo.pdl", "compatibleSchemaSnapshot/BirthInfo.pdl", CompatibilityLevel.WIRE_COMPATIBLE, CompatibilityOptions.Mode.DATA },
         };
   }
 
@@ -169,8 +178,16 @@ private Object[][] incompatibleInputFiles()
             {   "BirthInfo.pdl",
                 "incompatibleSchemaSnapshot/BirthInfo.pdl",
                 incompatibilityErrors,
+                new HashSet<>(),
                 compatibilityDiffs
-            }
+            },
+            {   "BirthInfo.pdl",
+                "compatibleSchemaSnapshot/BirthInfo.pdl", // This is combination is compatible with WIRE_COMPATIBLE, but not with BACKWARDS
+                new HashSet<>(),
+                new HashSet<>(Arrays.asList(new CompatibilityInfo(Arrays.<Object>asList("BirthInfo", "eyeColor", "Color", "symbols"),
+                    CompatibilityInfo.Type.ENUM_VALUE_ADDED, "new enum added symbols GREEN"))),
+                new HashSet<>()
+            },
         };
   }
 }

restli-tools/src/test/pegasus/com/linkedin/restli/tools/pegasusSchemaSnapshotTest/BirthInfo.pdl

@@ -6,4 +6,10 @@ record BirthInfo {
   year: int
 
   location: optional Location
+
+  eyeColor: enum Color {
+    BLUE
+    BROWN
+    OTHER
+  }
 }

restli-tools/src/test/pegasusSchemaSnapshot/BirthInfo.pdl

@@ -7,4 +7,9 @@ record BirthInfo {
     longitude: float
     name: optional string
   }
+  eyeColor: enum Color {
+    BLUE
+    BROWN
+    OTHER
+  }
 }

restli-tools/src/test/pegasusSchemaSnapshot/compatibleSchemaSnapshot/BirthInfo.pdl

@@ -11,4 +11,10 @@ record BirthInfo {
     longitude: float
     name: optional string
   }
+  eyeColor: enum Color {
+    BLUE
+    BROWN
+    GREEN
+    OTHER
+  }
 }

restli-tools/src/test/pegasusSchemaSnapshot/compatibleSchemaSnapshot/Date.pdl

@@ -0,0 +1,5 @@
+record Date {
+  day: int
+  month: int
+  year: int
+}

restli-tools/src/test/pegasusSchemaSnapshot/incompatibleSchemaSnapshot/BirthInfo.pdl

@@ -12,4 +12,9 @@ record BirthInfo {
     name: optional string
   }
   name: string
+  eyeColor: enum Color {
+    BLUE
+    BROWN
+    OTHER
+  }
 }