Add configurable compatibility allowances to thrift audit

.github/workflows/build.yml

@@ -59,7 +59,7 @@ jobs:
         run: ./bootstrap.sh
 
       - name: Run configure
-        run: ./configure --disable-debug --disable-tests --disable-libs
+        run: ./configure --disable-debug --disable-libs
 
       - name: Run make
         run: make -j$(nproc)
@@ -70,6 +70,9 @@ jobs:
       - name: Run thrift version
         run: /usr/local/bin/thrift -version
 
+      - name: Run thrift audit tests
+        run: make -C test/audit check
+
       - name: Test Delphi UUIDv8 GUID determinism
         run: |
           # Run the Delphi generator twice on the same input and verify identical output.

CMakeLists.txt

@@ -84,6 +84,10 @@ elseif(EXISTS ${THRIFT_COMPILER})
     set_property(TARGET thrift-compiler PROPERTY IMPORTED_LOCATION ${THRIFT_COMPILER})
 endif()
 
+if(BUILD_TESTING AND TARGET thrift-compiler)
+    add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/test/audit)
+endif()
+
 if(BUILD_CPP)
     add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/lib/cpp)
     if(BUILD_TUTORIALS)

compiler/cpp/src/thrift/audit/t_audit.cpp

@@ -26,6 +26,8 @@
 extern int g_warn;
 extern std::string g_curpath;
 extern bool g_return_failure;
+extern bool g_audit_allow_optional_field_removal;
+extern bool g_audit_allow_required_field_to_default;
 
 void thrift_audit_warning(int level, const char* fmt, ...) {
    if (g_warn < level) {
@@ -49,6 +51,19 @@ void thrift_audit_failure(const char* fmt, ...) {
   g_return_failure = true;
 }
 
+static bool is_allowed_field_removal(const t_field* oldField) {
+  return g_audit_allow_optional_field_removal
+         && oldField->get_req() == t_field::T_OPTIONAL;
+}
+
+static void report_field_removal(const t_field* oldField, const std::string& structName) {
+  if (!is_allowed_field_removal(oldField)) {
+    thrift_audit_failure("Struct Field removed for Id = %d in %s \n",
+                         oldField->get_key(),
+                         structName.c_str());
+  }
+}
+
 void compare_namespace(t_program* newProgram, t_program* oldProgram)
 {
    const std::map<std::string, std::string>& newNamespaceMap = newProgram->get_all_namespaces();
@@ -226,7 +241,12 @@ void compare_struct_field(t_field* newField, t_field* oldField, std::string oldS
    bool newStructFieldOptional = (newField->get_req() != t_field::T_REQUIRED);
    bool oldStructFieldOptional = (oldField->get_req() != t_field::T_REQUIRED);
 
-   if(newStructFieldOptional != oldStructFieldOptional)
+   bool requiredToDefaultAllowed =
+       g_audit_allow_required_field_to_default
+       && oldField->get_req() == t_field::T_REQUIRED
+       && newField->get_req() == t_field::T_OPT_IN_REQ_OUT;
+
+   if(newStructFieldOptional != oldStructFieldOptional && !requiredToDefaultAllowed)
    {
       thrift_audit_failure("Struct Field Requiredness Changed for Id = %d in %s \n", newField->get_key(), oldStructName.c_str());
    }
@@ -261,7 +281,7 @@ void compare_single_struct(t_struct* newStruct, t_struct* oldStruct, const std::
       if(newStructMemberIt == newStructMembersInIdOrder.end() && oldStructMemberIt != oldStructMembersInIdOrder.end())
       {
          // A field ID has been removed from the end.
-         thrift_audit_failure("Struct Field removed for Id = %d in %s \n", (*oldStructMemberIt)->get_key(), structName.c_str());
+         report_field_removal(*oldStructMemberIt, structName);
          oldStructMemberIt++;
       }
       else if(newStructMemberIt != newStructMembersInIdOrder.end() && oldStructMemberIt == oldStructMembersInIdOrder.end())
@@ -291,7 +311,7 @@ void compare_single_struct(t_struct* newStruct, t_struct* oldStruct, const std::
       else if((*newStructMemberIt)->get_key() > (*oldStructMemberIt)->get_key())
       {
          //A field is deleted in newStruct.
-         thrift_audit_failure("Struct Field removed for Id = %d in %s \n",  (*oldStructMemberIt)->get_key(), structName.c_str());
+         report_field_removal(*oldStructMemberIt, structName);
          oldStructMemberIt++;
       }
 

compiler/cpp/src/thrift/main.cc

@@ -153,6 +153,8 @@ bool gen_recurse = false;
  * Flags to control thrift audit
  */
 bool g_audit = false;
+bool g_audit_allow_optional_field_removal = false;
+bool g_audit_allow_required_field_to_default = false;
 
 /**
  * Flag to control return status
@@ -712,6 +714,11 @@ void help() {
   fprintf(stderr, "\n");
   fprintf(stderr, "Options related to audit operation\n");
   fprintf(stderr, "   --audit OldFile   Old Thrift file to be audited with 'file'\n");
+  fprintf(stderr, "   --audit-allow-optional-field-removal\n");
+  fprintf(stderr, "                Allow explicitly optional fields to be removed\n");
+  fprintf(stderr, "   --audit-allow-required-field-to-default\n");
+  fprintf(stderr, "                Allow required fields to use default requiredness\n");
+  fprintf(stderr, "                Binding-dependent; includes service method arguments\n");
   fprintf(stderr, "  -Iold dir    Add a directory to the list of directories\n");
   fprintf(stderr, "                searched for include directives for old thrift file\n");
   fprintf(stderr, "  -Inew dir    Add a directory to the list of directories\n");
@@ -1189,6 +1196,10 @@ int main(int argc, char** argv) {
         old_input_file = string(old_thrift_file_rp);
       } else if (strcmp(arg, "-audit-nofatal") == 0) {
         g_audit_fatal = false;
+      } else if (strcmp(arg, "-audit-allow-optional-field-removal") == 0) {
+        g_audit_allow_optional_field_removal = true;
+      } else if (strcmp(arg, "-audit-allow-required-field-to-default") == 0) {
+        g_audit_allow_required_field_to_default = true;
       } else if (strcmp(arg, "-Iold") == 0) {
         arg = argv[++i];
         if (arg == nullptr) {

compiler/cpp/tests/thrift_test_globals.cc

@@ -43,3 +43,5 @@ std::string g_curpath;
 std::vector<std::string> g_incl_searchpath;
 
 bool g_return_failure = false;
+bool g_audit_allow_optional_field_removal = false;
+bool g_audit_allow_required_field_to_default = false;

configure.ac

@@ -286,6 +286,8 @@ AM_CONDITIONAL(WITH_LUA, [test "$have_lua" = "yes"])
 
 # Find python regardless of with_python value, because it's needed by make cross
 AM_PATH_PYTHON(2.6,, :)
+AC_PATH_PROG([PERL_FOR_BUILD], [perl])
+AM_CONDITIONAL([HAVE_PERL_FOR_BUILD], [test -n "$PERL_FOR_BUILD"])
 AX_THRIFT_LIB(python, [Python], yes)
 if test "$with_python" = "yes";  then
   if test -n "$PYTHON"; then
@@ -809,6 +811,7 @@ AC_CONFIG_FILES([
   lib/xml/Makefile
   lib/xml/test/Makefile
   test/Makefile
+  test/audit/Makefile
   test/features/Makefile
   test/c_glib/Makefile
   test/cl/Makefile

test/Makefile.am

@@ -17,7 +17,7 @@
 # under the License.
 #
 
-SUBDIRS = features
+SUBDIRS = audit features
 PRECROSS_TARGET =
 
 if WITH_C_GLIB
@@ -121,7 +121,6 @@ distdir:
 	$(MAKE) $(AM_MAKEFLAGS) distdir-am
 
 EXTRA_DIST = \
-	audit \
 	c_glib \
 	cl \
 	cpp \

test/audit/CMakeLists.txt

@@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+find_package(Perl QUIET)
+
+if(PERL_FOUND)
+    add_test(
+        NAME ThriftAuditTest
+        COMMAND ${PERL_EXECUTABLE}
+                ${CMAKE_CURRENT_SOURCE_DIR}/thrift_audit_test.pl
+                -f ${CMAKE_CURRENT_SOURCE_DIR}
+                -t $<TARGET_FILE:thrift-compiler>
+    )
+else()
+    message(WARNING "Skipping ThriftAuditTest because no Perl interpreter was found.")
+endif()

test/audit/Makefile.am

@@ -0,0 +1,80 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+AUTOMAKE_OPTIONS = serial-tests
+
+if HAVE_PERL_FOR_BUILD
+TESTS = thrift_audit_test.pl
+TESTS_ENVIRONMENT = \
+	THRIFT_AUDIT_TEST_COMPILER='$(abs_top_builddir)/compiler/cpp/thrift'; \
+	export THRIFT_AUDIT_TEST_COMPILER; \
+	THRIFT_AUDIT_TEST_FIXTURES='$(abs_srcdir)'; \
+	export THRIFT_AUDIT_TEST_FIXTURES; \
+	$(PERL_FOR_BUILD)
+endif
+
+EXTRA_DIST = \
+	CMakeLists.txt \
+	README.md \
+	thrift_audit_test.pl \
+	break1.thrift \
+	break2.thrift \
+	break3.thrift \
+	break4.thrift \
+	break5.thrift \
+	break6.thrift \
+	break7.thrift \
+	break8.thrift \
+	break9.thrift \
+	break10.thrift \
+	break11.thrift \
+	break12.thrift \
+	break13.thrift \
+	break14.thrift \
+	break15.thrift \
+	break16.thrift \
+	break17.thrift \
+	break18.thrift \
+	break19.thrift \
+	break20.thrift \
+	break21.thrift \
+	break22.thrift \
+	break23.thrift \
+	break24.thrift \
+	break25.thrift \
+	break26.thrift \
+	break27.thrift \
+	break28.thrift \
+	break29.thrift \
+	break30.thrift \
+	break31.thrift \
+	break32.thrift \
+	break33.thrift \
+	break34.thrift \
+	default_field_old.thrift \
+	optional_field_middle_old.thrift \
+	optional_field_middle_removed.thrift \
+	optional_field_old.thrift \
+	optional_field_removed.thrift \
+	required_argument_default.thrift \
+	required_argument_old.thrift \
+	required_field_old.thrift \
+	required_to_default_new.thrift \
+	required_to_default_old.thrift \
+	required_to_optional_new.thrift \
+	test.thrift \
+	warning.thrift

test/audit/README.md

@@ -3,6 +3,38 @@ Typical usage
 ```
 thrift.exe --audit <oldFile> <newFile>
 ```
+
+Compatibility options
+=====================
+The audit remains strict by default. These options suppress specific audit
+errors; callers are responsible for verifying that the selected change is safe
+for every language binding in use.
+
+* `--audit-allow-optional-field-removal` allows removal of fields declared
+  `optional`. It does not allow removal of fields with default or required
+  requiredness. Removing an explicitly optional field is wire-compatible.
+* `--audit-allow-required-field-to-default` allows a field declared `required`
+  to change to default requiredness. It does not allow changing that field to
+  `optional`, or changing a default field to `required`. The option also applies
+  to service method arguments. Explicit `required` is illegal in a `throws`
+  clause and is normalized to default requiredness by the parser, so `throws`
+  clauses are not affected by this option.
+
+Changing `required` to default requiredness is binding- and application-
+dependent, and is not universally wire-compatible. For example, the standard
+C++ generator writes ordinary default-requiredness values, including default-
+constructed strings, containers, and nested structs. In contrast, the Java
+generator may omit a default-requiredness field when its value is `null`. An
+older reader generated from an explicitly `required` field rejects that omitted
+field. Other generators may behave differently, and C++ exception-typed fields
+and generated result structs also have separate set-state handling.
+
+Before using `--audit-allow-required-field-to-default`, verify the write
+behavior of every language binding and field type in use.
+Upgrade every reader away from explicit `required` before deploying any writer
+that may omit the field. For a service method argument, an older server can
+reject the request before invoking the handler.
+
 Example run
 ===========
 ```
@@ -16,8 +48,8 @@ Problems that the audit tool can catch
 Errors
 * Removing an enum value
 * Changing the type of a struct field
-* Changing the required-ness of a struct field
-* Removing a struct field
+* Changing the required-ness of a struct field (unless explicitly allowed)
+* Removing a struct field (unless explicitly allowed)
 * Adding a required struct field
 * Adding a struct field 'in the middle'.  This usually indicates an old ID has been recycled
 * Struct removed
@@ -37,4 +69,3 @@ Warnings
 * Removed constant
 * Type of constant changed
 * Value of constant changed
-

test/audit/default_field_old.thrift

@@ -0,0 +1,21 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+struct Record {
+  1: i32 retained
+  2: string removed
+}

test/audit/optional_field_middle_old.thrift

@@ -0,0 +1,22 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+struct Record {
+  1: i32 retained_before
+  2: optional string removed
+  3: i64 retained_after
+}

test/audit/optional_field_middle_removed.thrift

@@ -0,0 +1,21 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+struct Record {
+  1: i32 retained_before
+  3: i64 retained_after
+}