Re: [3/3] thrift git commit: THRIFT-3221 Create a tool to audit network compatibility between two .thrift files Client: Compiler (general) Patch: Sanjay Poojary , Ben Craig , and Zach Hindes

Roger Meier Wed, 08 Jul 2015 22:15:07 -0700

Hi Ben

Nice stuff!
Just a few remarks from my side:
- do we really need perl here: test/audit/thrift_audit_test.pl
  The cross language test suite is python and I would prefer python here

- I would propose to move compiler/cpp/src/audit/readme.txt totest/audit/README.md


Thanks
roger


Quoting bencr...@apache.org:

THRIFT-3221 Create a tool to audit network compatibility between two
.thrift files
Client: Compiler (general)
Patch: Sanjay Poojary <sanjay.pooj...@ni.com>, Ben Craig
<bencr...@apache.org>, and Zach Hindes <zach.hin...@ni.com>

This closes #541


Project: http://git-wip-us.apache.org/repos/asf/thrift/repo
Commit: http://git-wip-us.apache.org/repos/asf/thrift/commit/262cfb41
Tree: http://git-wip-us.apache.org/repos/asf/thrift/tree/262cfb41
Diff: http://git-wip-us.apache.org/repos/asf/thrift/diff/262cfb41

Branch: refs/heads/master
Commit: 262cfb4189f3b347f472dfe8b754861ba481c433
Parents: 384f976
Author: Ben Craig <bencr...@apache.org>
Authored: Wed Jul 8 20:37:15 2015 -0500
Committer: Ben Craig <bencr...@apache.org>
Committed: Wed Jul 8 20:37:15 2015 -0500

----------------------------------------------------------------------
 compiler/cpp/CMakeLists.txt        |   1 +
 compiler/cpp/Makefile.am           |   1 +
 compiler/cpp/src/audit/readme.txt  |  32 +++
 compiler/cpp/src/audit/t_audit.cpp | 466 ++++++++++++++++++++++++++++++++
 compiler/cpp/src/audit/t_audit.h   |  11 +
 compiler/cpp/src/main.cc           | 199 ++++++++++----
 compiler/cpp/src/parse/t_program.h |   3 +
 test/audit/break1.thrift           | 188 +++++++++++++
 test/audit/break10.thrift          | 190 +++++++++++++
 test/audit/break11.thrift          | 190 +++++++++++++
 test/audit/break12.thrift          | 191 +++++++++++++
 test/audit/break13.thrift          | 191 +++++++++++++
 test/audit/break14.thrift          | 190 +++++++++++++
 test/audit/break15.thrift          | 190 +++++++++++++
 test/audit/break16.thrift          | 191 +++++++++++++
 test/audit/break17.thrift          | 191 +++++++++++++
 test/audit/break18.thrift          | 191 +++++++++++++
 test/audit/break19.thrift          | 191 +++++++++++++
 test/audit/break2.thrift           | 190 +++++++++++++
 test/audit/break20.thrift          | 190 +++++++++++++
 test/audit/break21.thrift          | 190 +++++++++++++
 test/audit/break22.thrift          | 190 +++++++++++++
 test/audit/break23.thrift          | 192 +++++++++++++
 test/audit/break24.thrift          | 191 +++++++++++++
 test/audit/break25.thrift          | 191 +++++++++++++
 test/audit/break26.thrift          | 191 +++++++++++++
 test/audit/break27.thrift          | 190 +++++++++++++
 test/audit/break28.thrift          | 190 +++++++++++++
 test/audit/break29.thrift          | 191 +++++++++++++
 test/audit/break3.thrift           | 191 +++++++++++++
 test/audit/break30.thrift          | 190 +++++++++++++
 test/audit/break31.thrift          | 191 +++++++++++++
 test/audit/break32.thrift          | 191 +++++++++++++
 test/audit/break33.thrift          | 191 +++++++++++++
 test/audit/break34.thrift          | 192 +++++++++++++
 test/audit/break4.thrift           | 190 +++++++++++++
 test/audit/break5.thrift           | 190 +++++++++++++
 test/audit/break6.thrift           | 191 +++++++++++++
 test/audit/break7.thrift           | 190 +++++++++++++
 test/audit/break8.thrift           | 191 +++++++++++++
 test/audit/break9.thrift           | 190 +++++++++++++
 test/audit/test.thrift             | 189 +++++++++++++
 test/audit/thrift_audit_test.pl    | 261 ++++++++++++++++++
 test/audit/warning.thrift          | 190 +++++++++++++
 44 files changed, 7785 insertions(+), 46 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/thrift/blob/262cfb41/compiler/cpp/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/compiler/cpp/CMakeLists.txt b/compiler/cpp/CMakeLists.txt
index 01e229d..bc6591c 100644
--- a/compiler/cpp/CMakeLists.txt
+++ b/compiler/cpp/CMakeLists.txt
@@ -56,6 +56,7 @@ set( thrift_SOURCES
     src/main.h
     src/platform.h
     src/md5.h
+    src/audit/t_audit.cpp
     src/parse/t_doc.h
     src/parse/t_type.h
     src/parse/t_base_type.h

http://git-wip-us.apache.org/repos/asf/thrift/blob/262cfb41/compiler/cpp/Makefile.am
----------------------------------------------------------------------
diff --git a/compiler/cpp/Makefile.am b/compiler/cpp/Makefile.am
index 559a839..f5514d9 100644
--- a/compiler/cpp/Makefile.am
+++ b/compiler/cpp/Makefile.am
@@ -40,6 +40,7 @@ thrift_SOURCES = src/main.cc \
                  src/platform.h \
                  src/logging.h \
                  src/md5.h \
+                 src/audit/t_audit.cpp \
                  src/parse/t_doc.h \
                  src/parse/t_type.h \
                  src/parse/t_base_type.h \

http://git-wip-us.apache.org/repos/asf/thrift/blob/262cfb41/compiler/cpp/src/audit/readme.txt
----------------------------------------------------------------------

diff --git a/compiler/cpp/src/audit/readme.txtb/compiler/cpp/src/audit/readme.txt

new file mode 100644
index 0000000..f1c53e3
--- /dev/null
+++ b/compiler/cpp/src/audit/readme.txt
@@ -0,0 +1,32 @@
+Typical usage:
+    thrift.exe --audit <oldFile> <newFile>
+Example run:
+    > thrift.exe --audit test.thrift break1.thrift

+ [Thrift Audit Failure:break1.thrift] New Thrift File hasmissing function base_function3

+    [Thrift Audit Warning:break1.thrift] Constant const3 has different value
+
+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
+    Adding a required struct field

+ Adding a struct field 'in the middle'. This usually indicatesan old ID has been recycled

+    Struct removed
+    Oneway-ness change
+    Return type change
+    Missing function
+    Missing service
+    Change in service inheritance
+Warnings
+    Removing a language namespace declaration
+    Changing a namespace
+    Changing an enum value's name
+    Removing an enum class
+    Default value changed
+    Struct field name change
+    Removed constant
+    Type of constant changed
+    Value of constant changed
+
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/thrift/blob/262cfb41/compiler/cpp/src/audit/t_audit.cpp
----------------------------------------------------------------------

diff --git a/compiler/cpp/src/audit/t_audit.cppb/compiler/cpp/src/audit/t_audit.cpp

new file mode 100644
index 0000000..afcbd5e
--- /dev/null
+++ b/compiler/cpp/src/audit/t_audit.cpp
@@ -0,0 +1,466 @@
+
+#include <cassert>
+#include <stdlib.h>
+#include <stdio.h>
+#include <stdarg.h>
+#include <time.h>
+#include <string>
+#include <algorithm>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <errno.h>
+#include <limits.h>
+
+// Careful: must include globals first for extern definitions
+#include "globals.h"
+
+#include "parse/t_program.h"
+#include "parse/t_scope.h"
+#include "parse/t_const.h"
+#include "parse/t_field.h"
+
+#include "version.h"
+
+#include "t_audit.h"
+
+extern int g_warn;
+extern std::string g_curpath;
+extern bool g_return_failure;
+
+void thrift_audit_warning(int level, const char* fmt, ...) {
+   if (g_warn < level) {
+      return;
+   }
+   va_list args;
+   printf("[Thrift Audit Warning:%s] ", g_curpath.c_str());
+   va_start(args, fmt);
+   vprintf(fmt, args);
+   va_end(args);
+   printf("\n");
+}
+
+void thrift_audit_failure(const char* fmt, ...) {
+  va_list args;
+  fprintf(stderr, "[Thrift Audit Failure:%s] ", g_curpath.c_str());
+  va_start(args, fmt);
+  vfprintf(stderr, fmt, args);
+  va_end(args);
+  fprintf(stderr, "\n");
+  g_return_failure = true;
+}
+
+void compare_namespace(t_program* newProgram, t_program* oldProgram)
+{

+ const std::map<std::string, std::string>& newNamespaceMap =newProgram->get_all_namespaces();+ const std::map<std::string, std::string>& oldNamespaceMap =oldProgram->get_all_namespaces();

+ for(std::map<std::string, std::string>::const_iteratoroldNamespaceMapIt = oldNamespaceMap.begin();

+         oldNamespaceMapIt != oldNamespaceMap.end();
+         oldNamespaceMapIt++)
+   {

+ std::map<std::string, std::string>::const_iteratornewNamespaceMapIt = newNamespaceMap.find(oldNamespaceMapIt->first);

+      if(newNamespaceMapIt == newNamespaceMap.end())
+      {

+ thrift_audit_warning(1, "Language %s not found in newthrift file\n", (oldNamespaceMapIt->first).c_str());

+      }
+      else if((newNamespaceMapIt->second) != oldNamespaceMapIt->second)
+      {

+ thrift_audit_warning(1, "Namespace %s changed in newthrift file\n", (oldNamespaceMapIt->second).c_str());

+      }
+   }
+}
+
+void compare_enum_values(t_enum* newEnum,t_enum* oldEnum)
+{

+ const std::vector<t_enum_value*>& oldEnumValues =oldEnum->get_constants();+ for(std::vector<t_enum_value*>::const_iterator oldEnumValuesIt =oldEnumValues.begin();

+         oldEnumValuesIt != oldEnumValues.end();
+         oldEnumValuesIt++)
+   {
+      int enumValue = (*oldEnumValuesIt)->get_value();

+ t_enum_value* newEnumValue =newEnum->get_constant_by_value(enumValue);

+      if(newEnumValue != NULL)
+      {
+         std::string enumName = (*oldEnumValuesIt)->get_name();
+         if(enumName != newEnumValue->get_name())
+         {

+ thrift_audit_warning(1, "Name of the value %d changedin enum %s\n", enumValue, oldEnum->get_name().c_str());

+         }
+      }
+      else
+      {

+ thrift_audit_failure("Enum value %d missing in %s\n",enumValue, oldEnum->get_name().c_str());

+      }
+
+   }
+}
+

+void compare_enums(const std::vector<t_enum*>& newEnumList, conststd::vector<t_enum*>& oldEnumList)

+{
+   std::map<std::string,t_enum*> newEnumMap;
+   std::vector<t_enum*>::const_iterator newEnumIt;

+ for(newEnumIt = newEnumList.begin(); newEnumIt !=newEnumList.end(); newEnumIt++)

+   {
+      newEnumMap[(*newEnumIt)->get_name()] = *newEnumIt;
+   }
+   std::vector<t_enum*>::const_iterator oldEnumIt;

+ for(oldEnumIt = oldEnumList.begin(); oldEnumIt !=oldEnumList.end(); oldEnumIt++)

+   {
+      std::map<std::string,t_enum*>::iterator newEnumMapIt;
+      newEnumMapIt = newEnumMap.find((*oldEnumIt)->get_name());
+
+      if(newEnumMapIt == newEnumMap.end())
+      {

+ thrift_audit_warning(1, "Enum %s not found in new thriftfile\n",(*oldEnumIt)->get_name().c_str());

+      }
+      else
+      {
+         compare_enum_values(newEnumMapIt->second, *oldEnumIt);
+      }
+   }
+}
+
+//This function returns 'true' if the two arguements are of same types.
+//Returns false if they are of different type
+bool compare_type(t_type* newType, t_type* oldType)
+{

+ //Comparing names of two types will work when the newType andoldType are basic types or structs or enums.+ //However, when they are containers, get_name() returns emptyfor which we have to compare the type of

+   //their elements as well.
+   if((newType->get_name()).empty() && (oldType->get_name()).empty())
+   {
+
+      if(newType->is_list() && oldType->is_list())
+      {
+         t_type* newElementType = ((t_list*)newType)->get_elem_type();
+         t_type* oldElementType = ((t_list*)oldType)->get_elem_type();
+         return compare_type(newElementType, oldElementType);
+      }
+      else if(newType->is_map() && oldType->is_map())
+      {
+         t_type* newKeyType = ((t_map*)newType)->get_key_type();
+         t_type* oldKeyType = ((t_map*)oldType)->get_key_type();
+
+         t_type* newValType = ((t_map*)newType)->get_val_type();
+         t_type* oldValType = ((t_map*)oldType)->get_val_type();
+

+ return (compare_type(newKeyType, oldKeyType) &&compare_type(newValType, oldValType));

+      }
+      else if(newType->is_set() && oldType->is_set())
+      {
+         t_type* newElementType = ((t_set*)newType)->get_elem_type();
+         t_type* oldElementType = ((t_set*)oldType)->get_elem_type();
+         return compare_type(newElementType, oldElementType);
+      }
+      else
+      {
+         return false;
+      }
+   }
+   else if(newType->get_name() == oldType->get_name())
+   {
+      return true;
+   }
+   else
+   {
+      return false;
+   }
+}
+

+bool compare_pair(std::pair<t_const_value*, t_const_value*>newMapPair, std::pair<t_const_value*, t_const_value*> oldMapPair)

+{

+ return compare_defaults(newMapPair.first, oldMapPair.first) &&compare_defaults(newMapPair.second, oldMapPair.second);

+}
+

+// This function returns 'true' if the default values are same.Returns false if they are different.+bool compare_defaults(t_const_value* newStructDefault,t_const_value* oldStructDefault)

+{
+   if(newStructDefault == NULL && oldStructDefault == NULL) return true;

+ else if(newStructDefault == NULL && oldStructDefault != NULL)return false;+ else if (newStructDefault != NULL && oldStructDefault == NULL)return false;

+
+   if(newStructDefault->get_type() != oldStructDefault->get_type())
+   {
+      return false;
+   }
+
+   switch(newStructDefault->get_type())
+   {
+      case t_const_value::CV_INTEGER:

+ return (newStructDefault->get_integer() ==oldStructDefault->get_integer());

+      case t_const_value::CV_DOUBLE:

+ return (newStructDefault->get_double() ==oldStructDefault->get_double());

+      case t_const_value::CV_STRING:

+ return (newStructDefault->get_string() ==oldStructDefault->get_string());

+      case t_const_value::CV_LIST:
+         {

+ const std::vector<t_const_value*>& oldDefaultList =oldStructDefault->get_list();+ const std::vector<t_const_value*>& newDefaultList =newStructDefault->get_list();+ bool defaultValuesCompare = (oldDefaultList.size() ==newDefaultList.size());

+ return defaultValuesCompare &&std::equal(newDefaultList.begin(), newDefaultList.end(),oldDefaultList.begin(), compare_defaults);

+         }
+      case t_const_value::CV_MAP:
+         {

+ const std::map<t_const_value*, t_const_value*> newMap =newStructDefault->get_map();+ const std::map<t_const_value*, t_const_value*> oldMap =oldStructDefault->get_map();

+
+            bool defaultValuesCompare = (oldMap.size() == newMap.size());
+

+ return defaultValuesCompare &&std::equal(newMap.begin(), newMap.end(), oldMap.begin(),compare_pair);

+         }
+      case t_const_value::CV_IDENTIFIER:

+ return (newStructDefault->get_identifier() ==oldStructDefault->get_identifier());

+      default:
+         return false;
+   }
+
+}
+

+void compare_struct_field(t_field* newField, t_field* oldField,std::string oldStructName)

+{
+   t_type* newFieldType = newField->get_type();
+   t_type* oldFieldType = oldField->get_type();
+   if(!compare_type(newFieldType, oldFieldType))
+   {

+ thrift_audit_failure("Struct Field Type Changed for Id = %din %s \n", newField->get_key(), oldStructName.c_str());

+   }
+

+ // A Struct member can be optional if it is mentionedexplicitly, or if it is assigned with default values.+ bool newStructFieldOptional = (newField->get_req() !=t_field::T_REQUIRED);+ bool oldStructFieldOptional = (oldField->get_req() !=t_field::T_REQUIRED);

+
+   if(newStructFieldOptional != oldStructFieldOptional)
+   {

+ thrift_audit_failure("Struct Field Requiredness Changed forId = %d in %s \n", newField->get_key(), oldStructName.c_str());

+   }
+   if(newStructFieldOptional || oldStructFieldOptional)
+   {
+      if(!compare_defaults(newField->get_value(), oldField->get_value()))
+      {

+ thrift_audit_warning(1, "Default value changed for Id = %din %s \n", newField->get_key(), oldStructName.c_str());

+      }
+   }
+
+   std::string fieldName = newField->get_name();
+   if(fieldName != oldField->get_name())
+   {

+ thrift_audit_warning(1, "Struct field name changed for Id =%d in %s\n", newField->get_key(), oldStructName.c_str());

+   }
+
+}
+

+void compare_single_struct(t_struct* newStruct, t_struct*oldStruct, const std::string& oldStructName = std::string())

+{

+ std::string structName = oldStructName.empty() ?oldStruct->get_name() : oldStructName;+ const std::vector<t_field*>& oldStructMembersInIdOrder =oldStruct->get_sorted_members();+ const std::vector<t_field*>& newStructMembersInIdOrder =newStruct->get_sorted_members();+ std::vector<t_field*>::const_iterator oldStructMemberIt =oldStructMembersInIdOrder.begin();+ std::vector<t_field*>::const_iterator newStructMemberIt =newStructMembersInIdOrder.begin();

+ // Since we have the struct members in their ID order, comparingtheir IDs can be done by traversing the two member

+   // lists together.

+ while(!(oldStructMemberIt == oldStructMembersInIdOrder.end() &&newStructMemberIt == newStructMembersInIdOrder.end()))

+   {

+ 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());

+         oldStructMemberIt++;
+      }

+ else if(newStructMemberIt != newStructMembersInIdOrder.end()&& oldStructMemberIt == oldStructMembersInIdOrder.end())

+      {
+         //New field ID has been added to the end.
+         if((*newStructMemberIt)->get_req() == t_field::T_REQUIRED)
+         {

+ thrift_audit_failure("Required Struct Field Added forId = %d in %s \n", (*newStructMemberIt)->get_key(),structName.c_str());

+         }
+         newStructMemberIt++;
+      }

+ else if((*newStructMemberIt)->get_key() ==(*oldStructMemberIt)->get_key())

+      {

+ //Field ID found in both structs. Compare field types,default values.+ compare_struct_field(*newStructMemberIt,*oldStructMemberIt, structName);

+
+         newStructMemberIt++;
+         oldStructMemberIt++;
+      }

+ else if((*newStructMemberIt)->get_key() <(*oldStructMemberIt)->get_key())

+      {
+         //New Field Id is inserted in between

+ //Adding fields to struct is fine, but adding them in themiddle is suspicious. Error!!+ thrift_audit_failure("Struct field is added in the middlewith Id = %d in %s\n", (*newStructMemberIt)->get_key(),structName.c_str());

+         newStructMemberIt++;
+      }

+ 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());

+         oldStructMemberIt++;
+      }
+
+   }
+}
+

+void compare_structs(const std::vector<t_struct*>& newStructList,const std::vector<t_struct*>& oldStructList)

+{
+   std::map<std::string,t_struct*> newStructMap;
+   std::vector<t_struct*>::const_iterator newStructListIt;

+ for(newStructListIt = newStructList.begin(); newStructListIt !=newStructList.end(); newStructListIt++)

+   {
+      newStructMap[(*newStructListIt)->get_name()] = *newStructListIt;
+   }
+
+   std::vector<t_struct*>::const_iterator oldStructListIt;

+ for(oldStructListIt = oldStructList.begin(); oldStructListIt !=oldStructList.end(); oldStructListIt++)

+   {
+      std::map<std::string, t_struct*>::iterator newStructMapIt;
+      newStructMapIt = newStructMap.find((*oldStructListIt)->get_name());
+      if(newStructMapIt == newStructMap.end())
+      {

+ thrift_audit_failure("Struct %s not found in new thriftfile\n", (*oldStructListIt)->get_name().c_str());

+      }
+      else
+      {
+         compare_single_struct(newStructMapIt->second, *oldStructListIt);
+      }
+   }
+
+}
+

+void compare_single_function(t_function* newFunction, t_function*oldFunction)

+{
+   t_type* newFunctionReturnType = newFunction->get_returntype();
+
+   if(newFunction->is_oneway() != oldFunction->is_oneway())
+   {

+ thrift_audit_failure("Oneway attribute changed for function%s\n",oldFunction->get_name().c_str());

+   }
+   if(!compare_type(newFunctionReturnType, oldFunction->get_returntype()))
+   {

+ thrift_audit_failure("Return type changed for function%s\n",oldFunction->get_name().c_str());

+   }
+
+   //Compare function arguments.

+ compare_single_struct(newFunction->get_arglist(),oldFunction->get_arglist());

+   std::string exceptionName = oldFunction->get_name();
+   exceptionName += "_exception";

+ compare_single_struct(newFunction->get_xceptions(),oldFunction->get_xceptions(), exceptionName);

+}
+

+void compare_functions(const std::vector<t_function*>&newFunctionList, const std::vector<t_function*>& oldFunctionList)

+{
+   std::map<std::string, t_function*> newFunctionMap;
+   std::map<std::string, t_function*>::iterator newFunctionMapIt;

+ for(std::vector<t_function*>::const_iterator newFunctionIt =newFunctionList.begin();

+         newFunctionIt != newFunctionList.end();
+         newFunctionIt++)
+   {
+      newFunctionMap[(*newFunctionIt)->get_name()] = *newFunctionIt;
+   }
+

+ for(std::vector<t_function*>::const_iterator oldFunctionIt =oldFunctionList.begin();

+         oldFunctionIt != oldFunctionList.end();
+         oldFunctionIt++)
+   {
+      newFunctionMapIt = newFunctionMap.find((*oldFunctionIt)->get_name());
+      if(newFunctionMapIt == newFunctionMap.end())
+      {

+ thrift_audit_failure("New Thrift File has missing function%s\n",(*oldFunctionIt)->get_name().c_str());

+         continue;
+      }
+      else
+      {

+ //Function is found in both thrift files. Compare returntype and argument list

+         compare_single_function(newFunctionMapIt->second, *oldFunctionIt);
+      }
+   }
+
+}
+

+void compare_services(const std::vector<t_service*>& newServices,const std::vector<t_service*>& oldServices)

+{
+   std::vector<t_service*>::const_iterator oldServiceIt;
+
+   std::map<std::string, t_service*> newServiceMap;

+ for(std::vector<t_service*>::const_iterator newServiceIt =newServices.begin();

+         newServiceIt != newServices.end();
+         newServiceIt++)
+   {
+      newServiceMap[(*newServiceIt)->get_name()] = *newServiceIt;
+   }
+
+

+ for(oldServiceIt = oldServices.begin(); oldServiceIt !=oldServices.end(); oldServiceIt++)

+   {
+      const std::string oldServiceName = (*oldServiceIt)->get_name();

+ std::map<std::string, t_service*>::iterator newServiceMapIt =newServiceMap.find(oldServiceName);

+
+      if(newServiceMapIt == newServiceMap.end())
+      {

+ thrift_audit_failure("New Thrift file is missing a service%s\n", oldServiceName.c_str());

+      }
+      else
+      {
+         t_service* oldServiceExtends = (*oldServiceIt)->get_extends();

+ t_service* newServiceExtends =(newServiceMapIt->second)->get_extends();

+
+         if(oldServiceExtends == NULL)
+         {

+ // It is fine to add extends. So if service in olderthrift did not have any extends, we are fine.

+            // DO Nothing
+         }
+         else if(oldServiceExtends != NULL && newServiceExtends == NULL)
+         {

+ thrift_audit_failure("Change in Service inheritance for%s\n", oldServiceName.c_str());

+         }
+         else
+         {
+            std::string oldExtendsName = oldServiceExtends->get_name();
+            std::string newExtendsName = newServiceExtends->get_name();
+
+            if( newExtendsName != oldExtendsName)
+            {

+ thrift_audit_failure("Change in Service inheritancefor %s\n", oldServiceName.c_str());

+            }
+         }
+

+compare_functions((newServiceMapIt->second)->get_functions(),(*oldServiceIt)->get_functions());

+      }
+
+   }
+
+}
+

+void compare_consts(const std::vector<t_const*>& newConst, conststd::vector<t_const*>& oldConst)

+{
+   std::vector<t_const*>::const_iterator newConstIt;
+   std::vector<t_const*>::const_iterator oldConstIt;
+
+   std::map<std::string, t_const*> newConstMap;
+

+ for(newConstIt = newConst.begin(); newConstIt != newConst.end();newConstIt++)

+   {
+      newConstMap[(*newConstIt)->get_name()] = *newConstIt;
+   }
+
+   std::map<std::string, t_const*>::const_iterator newConstMapIt;

+ for(oldConstIt = oldConst.begin(); oldConstIt != oldConst.end();oldConstIt++)

+   {
+      newConstMapIt = newConstMap.find((*oldConstIt)->get_name());
+      if(newConstMapIt == newConstMap.end())
+      {

+ thrift_audit_warning(1, "Constants Missing %s \n",((*oldConstIt)->get_name()).c_str());

+      }

+ else if(!compare_type((newConstMapIt->second)->get_type(),(*oldConstIt)->get_type()))

+      {

+ thrift_audit_warning(1, "Constant %s is of different type\n", ((*oldConstIt)->get_name()).c_str());

+      }

+ elseif(!compare_defaults((newConstMapIt->second)->get_value(),(*oldConstIt)->get_value()))

+      {

+ thrift_audit_warning(1, "Constant %s has differentvalue\n", ((*oldConstIt)->get_name()).c_str());

+      }
+   }
+}
+
+

http://git-wip-us.apache.org/repos/asf/thrift/blob/262cfb41/compiler/cpp/src/audit/t_audit.h
----------------------------------------------------------------------

diff --git a/compiler/cpp/src/audit/t_audit.hb/compiler/cpp/src/audit/t_audit.h

new file mode 100644
index 0000000..fd0013a
--- /dev/null
+++ b/compiler/cpp/src/audit/t_audit.h
@@ -0,0 +1,11 @@
+#ifndef T_AUDIT_H
+#define T_AUDIT_H
+
+void compare_namespace(t_program* newProgram, t_program* oldProgram);

+void compare_enums(const std::vector<t_enum*>& newEnumList, conststd::vector<t_enum*>& oldEnumList);+bool compare_defaults(t_const_value* newStructDefault,t_const_value* oldStructDefault);+void compare_structs(const std::vector<t_struct*>& newStructList,const std::vector<t_struct*>& oldStructList);+void compare_services(const std::vector<t_service*>& newServices,const std::vector<t_service*>& oldServices);+void compare_consts(const std::vector<t_const*>& newConst, conststd::vector<t_const*>& oldConst);

+
+#endif

http://git-wip-us.apache.org/repos/asf/thrift/blob/262cfb41/compiler/cpp/src/main.cc
----------------------------------------------------------------------
diff --git a/compiler/cpp/src/main.cc b/compiler/cpp/src/main.cc
index 97d523e..a337cc6 100644
--- a/compiler/cpp/src/main.cc
+++ b/compiler/cpp/src/main.cc
@@ -51,6 +51,7 @@
 #include "parse/t_program.h"
 #include "parse/t_scope.h"
 #include "generate/t_generator.h"
+#include "audit/t_audit.h"

 #include "version.h"

@@ -169,6 +170,17 @@ int g_allow_64bit_consts = 0;
 bool gen_recurse = false;

 /**
+ * Flags to control thrift audit
+ */
+bool g_audit = false;
+
+/**
+ * Flag to control return status
+ */
+bool g_return_failure = false;
+bool g_audit_fatal = true;
+
+/**
  * Win32 doesn't have realpath, so use fallback implementation in that case,
  * otherwise this just calls through to realpath
  */
@@ -711,6 +723,13 @@ void help() {

fprintf(stderr, " Keys and values are optionspassed to the generator.\n");fprintf(stderr, " Many options will not requirevalues.\n");

   fprintf(stderr, "\n");
+  fprintf(stderr, "Options related to audit operation\n");

+ fprintf(stderr, " --audit OldFile Old Thrift file to beaudited with 'file'\n");+ fprintf(stderr, " -Iold dir Add a directory to the list ofdirectories\n");+ fprintf(stderr, " searched for include directivesfor old thrift file\n");+ fprintf(stderr, " -Inew dir Add a directory to the list ofdirectories\n");+ fprintf(stderr, " searched for include directivesfor new thrift file\n");

+  fprintf(stderr, "\n");
   fprintf(stderr, "Available generators (and options):\n");

t_generator_registry::gen_map_t gen_map =t_generator_registry::get_generator_map();@@ -1029,6 +1048,30 @@ void generate(t_program* program, constvector<string>& generator_strings) {

   }
 }

+void audit(t_program* new_program, t_program* old_program, stringnew_thrift_include_path, string old_thrift_include_path)

+{
+  vector<string> temp_incl_searchpath = g_incl_searchpath;
+  if(!old_thrift_include_path.empty()) {
+    g_incl_searchpath.push_back(old_thrift_include_path);
+  }
+
+  parse(old_program, NULL);
+
+  g_incl_searchpath = temp_incl_searchpath;
+  if(!new_thrift_include_path.empty()) {
+    g_incl_searchpath.push_back(new_thrift_include_path);
+  }
+
+  parse(new_program, NULL);
+
+  compare_namespace(new_program, old_program);

+ compare_services(new_program->get_services(),old_program->get_services());

+  compare_enums(new_program->get_enums(), old_program->get_enums());
+  compare_structs(new_program->get_structs(), old_program->get_structs());

+ compare_structs(new_program->get_xceptions(),old_program->get_xceptions());

+  compare_consts(new_program->get_consts(), old_program->get_consts());
+}
+
 /**

* Parse it up.. then spit it back out, in pretty much everylanguage. Alright

  * not that many languages, but the cool ones that we care about.
@@ -1049,6 +1092,9 @@ int main(int argc, char** argv) {
   }

   vector<string> generator_strings;
+  string old_thrift_include_path;
+  string new_thrift_include_path;
+  string old_input_file;

   // Set the current path to a dummy value to make warning messages clearer.
   g_curpath = "arguments";
@@ -1118,6 +1164,35 @@ int main(int argc, char** argv) {
 #endif
         if (!check_is_directory(out_path.c_str()))
           return -1;
+      } else if (strcmp(arg, "-audit") == 0) {
+        g_audit = true;
+        arg = argv[++i];
+        if (arg == NULL) {

+ fprintf(stderr, "Missing old thrift file name for auditoperation\n");

+          usage();
+        }
+        char old_thrift_file_rp[THRIFT_PATH_MAX];
+
+        if (saferealpath(arg, old_thrift_file_rp) == NULL) {
+          failure("Could not open input file with realpath: %s", arg);
+        }
+        old_input_file = string(old_thrift_file_rp);
+      } else if(strcmp(arg, "-audit-nofatal") == 0){
+        g_audit_fatal = false;
+      } else if (strcmp(arg, "-Iold") == 0) {
+        arg = argv[++i];
+        if (arg == NULL) {

+ fprintf(stderr, "Missing Include directory for old thriftfile\n");

+          usage();
+        }
+        old_thrift_include_path = string(arg);
+      } else if (strcmp(arg, "-Inew") == 0) {
+        arg = argv[++i];
+        if(arg == NULL) {
+        fprintf(stderr, "Missing Include directory for new thrift file\n");
+        usage();
+        }
+        new_thrift_include_path = string(arg);
       } else {
         fprintf(stderr, "Unrecognized option: %s\n", arg);
         usage();
@@ -1139,41 +1214,6 @@ int main(int argc, char** argv) {
     exit(0);
   }

-  // You gotta generate something!
-  if (generator_strings.empty()) {
-    fprintf(stderr, "No output language(s) specified\n");
-    usage();
-  }
-
-  // Real-pathify it
-  char rp[THRIFT_PATH_MAX];
-  if (argv[i] == NULL) {
-    fprintf(stderr, "Missing file name\n");
-    usage();
-  }
-  if (saferealpath(argv[i], rp) == NULL) {
-    failure("Could not open input file with realpath: %s", argv[i]);
-  }
-  string input_file(rp);
-
-  // Instance of the global parse tree
-  t_program* program = new t_program(input_file);
-  if (out_path.size()) {
-    program->set_out_path(out_path, out_path_is_absolute);
-  }
-
-  // Compute the cpp include prefix.
-  // infer this from the filename passed in
-  string input_filename = argv[i];
-  string include_prefix;
-
-  string::size_type last_slash = string::npos;
-  if ((last_slash = input_filename.rfind("/")) != string::npos) {
-    include_prefix = input_filename.substr(0, last_slash);
-  }
-
-  program->set_include_prefix(include_prefix);
-
   // Initialize global types
   g_type_void = new t_base_type("void", t_base_type::TYPE_VOID);
   g_type_string = new t_base_type("string", t_base_type::TYPE_STRING);
@@ -1188,24 +1228,87 @@ int main(int argc, char** argv) {
   g_type_i64 = new t_base_type("i64", t_base_type::TYPE_I64);
   g_type_double = new t_base_type("double", t_base_type::TYPE_DOUBLE);

-  // Parse it!
-  parse(program, NULL);
+  if(g_audit)
+  {
+    // Audit operation

-  // The current path is not really relevant when we are doing generation.
-  // Reset the variable to make warning messages clearer.
-  g_curpath = "generation";
-  // Reset yylineno for the heck of it.  Use 1 instead of 0 because
-  // That is what shows up during argument parsing.
-  yylineno = 1;
+    if (old_input_file.empty()) {
+      fprintf(stderr, "Missing file name of old thrift file for audit\n");
+      usage();
+    }

-  // Generate it!
-  generate(program, generator_strings);
+    char new_thrift_file_rp[THRIFT_PATH_MAX];
+    if (argv[i] == NULL) {
+      fprintf(stderr, "Missing file name of new thrift file for audit\n");
+      usage();
+    }
+    if (saferealpath(argv[i], new_thrift_file_rp) == NULL) {
+      failure("Could not open input file with realpath: %s", argv[i]);
+    }
+    string new_input_file(new_thrift_file_rp);
+
+    t_program new_program(new_input_file);
+    t_program old_program(old_input_file);
+

+ audit(&new_program, &old_program, new_thrift_include_path,old_thrift_include_path);

+
+  } else {
+    // Generate options
+
+    // You gotta generate something!
+    if (generator_strings.empty()) {
+      fprintf(stderr, "No output language(s) specified\n");
+      usage();
+    }
+
+    // Real-pathify it
+    char rp[THRIFT_PATH_MAX];
+    if (argv[i] == NULL) {
+      fprintf(stderr, "Missing file name\n");
+      usage();
+    }
+    if (saferealpath(argv[i], rp) == NULL) {
+      failure("Could not open input file with realpath: %s", argv[i]);
+    }
+    string input_file(rp);
+
+    // Instance of the global parse tree
+    t_program* program = new t_program(input_file);
+    if (out_path.size()) {
+      program->set_out_path(out_path, out_path_is_absolute);
+    }
+
+    // Compute the cpp include prefix.
+    // infer this from the filename passed in
+    string input_filename = argv[i];
+    string include_prefix;
+
+    string::size_type last_slash = string::npos;
+    if ((last_slash = input_filename.rfind("/")) != string::npos) {
+      include_prefix = input_filename.substr(0, last_slash);
+    }
+
+    program->set_include_prefix(include_prefix);
+
+    // Parse it!
+    parse(program, NULL);
+
+    // The current path is not really relevant when we are doing generation.
+    // Reset the variable to make warning messages clearer.
+    g_curpath = "generation";
+    // Reset yylineno for the heck of it.  Use 1 instead of 0 because
+    // That is what shows up during argument parsing.
+    yylineno = 1;
+
+    // Generate it!
+    generate(program, generator_strings);
+    delete program;
+  }

   // Clean up. Who am I kidding... this program probably orphans heap memory
   // all over the place, but who cares because it is about to exit and it is
   // all referenced and used by this wacky parse tree up until now anyways.

-  delete program;
   delete g_type_void;
   delete g_type_string;
   delete g_type_bool;
@@ -1216,5 +1319,9 @@ int main(int argc, char** argv) {
   delete g_type_double;

   // Finished
+  if (g_return_failure && g_audit_fatal) {
+    exit(2);
+  }
+  // Finished
   return 0;
 }

http://git-wip-us.apache.org/repos/asf/thrift/blob/262cfb41/compiler/cpp/src/parse/t_program.h
----------------------------------------------------------------------

diff --git a/compiler/cpp/src/parse/t_program.hb/compiler/cpp/src/parse/t_program.h

index cfab691..556ee6c 100644
--- a/compiler/cpp/src/parse/t_program.h
+++ b/compiler/cpp/src/parse/t_program.h
@@ -321,6 +321,9 @@ public:
     return std::string();
   }

+  const std::map<std::string, std::string>& get_all_namespaces(){
+     return namespaces_;
+  }
   // Language specific namespace / packaging

   void add_cpp_include(std::string path) { cpp_includes_.push_back(path); }

http://git-wip-us.apache.org/repos/asf/thrift/blob/262cfb41/test/audit/break1.thrift
----------------------------------------------------------------------
diff --git a/test/audit/break1.thrift b/test/audit/break1.thrift
new file mode 100644
index 0000000..f77f672
--- /dev/null
+++ b/test/audit/break1.thrift
@@ -0,0 +1,188 @@
+/*
+ * 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.
+ */
+
+//Thrift Method removed from service base.
+
+namespace cpp test
+
+//constants
+const i32 const1 = 123;
+const double const2 = 23.3;
+const map<string,string> const3 = {"hello":"world", "thrift":"audit"};
+
+//Exception
+exception test_exception1 {
+    1: i32 code;
+    2: string json;
+}
+exception test_exception2 {
+    1: i32 code;
+    2: string json;
+}
+
+//Enums
+
+enum test_enum1 {
+    enum1_value0 = 0,
+    enum1_value1 = 1,
+    enum1_value2 = 2,
+    enum1_value5 = 5,
+    enum1_value7 = 7,
+    enum1_value8 = 8
+}
+
+enum test_enum2 {
+    enum2_value0 = 0,
+    enum2_value1 = 1,
+    enum2_value2 = 2,
+    enum2_value3 = 3
+}
+
+enum test_enum3 {
+    enum3_value1 = 0,
+    enum3_value2 = 1
+}
+
+struct test_struct1 {
+    1: i16 struct1_member1,
+    2: i32 struct1_member2,
+    3: i64 struct1_member3,
+    4: double struct1_member4 = 2.5,
+    5: string struct1_member5 = "Audit test",
+    6: bool struct1_member6,
+    7: byte struct1_member7,
+    8: binary struct1_member8,
+    9: test_enum1 struct1_member9
+}
+
+struct test_struct2 {
+    1: list<i16> struct2_member1,
+    2: list<i32> struct2_member2,
+    3: list<i64> struct2_member3= [23, 32],
+    4: list<double> struct2_member4,
+    5: list<string> struct2_member5,
+    6: list<bool> struct2_member6,
+    7: list<byte> struct2_member7,
+    8: list<binary> struct2_member8,
+    9: list<test_enum1> struct2_member9
+}
+
+struct test_struct3 {
+    1: map<i16, i32> struct3_member1 = {1:2, 3:4},
+    2: map<i64, double> struct3_member2 = {10:1.1, 20:2.1},
+    3: map<string, bool> struct3_member3,
+    4: map<byte, test_enum1> struct3_member4,
+    5: map<test_enum2, test_enum3 > struct3_member5,
+    7: map<double, string> struct3_member7
+}
+
+struct test_struct4 {
+    1: i32 struct4_member1,
+    2: optional i32 struct4_member2
+}
+
+struct test_struct5{
+    1: double struct5_member1,
+    2: string struct5_member2 = "Thrift Audit Test"
+}
+struct test_struct6 {
+    1: i32 struct6_member1,
+    2: required i32 struct6_member2
+}
+
+service base {
+    oneway void base_oneway(
+        1: i32 arg1),
+
+    void base_function1(
+        1: i16 function1_arg1,
+        2: i32 function1_arg2,
+        3: i64 function1_arg3,
+        4: double function1_arg4,
+        5: string function1_arg5,
+        6: bool function1_arg6,
+        7: test_enum1 function1_arg7,
+        8: test_struct1 function1_arg8),
+
+    void base_function2(
+        1: list<i16> function2_arg1,
+        2: list<i32> function2_arg2,
+        3: list<i64> function2_arg3,
+        4: list<double> function2_arg4,
+        5: list<string> function2_arg5,
+        6: list<bool> function2_arg6,
+        7: list<byte> function2_arg7,
+        8: list<test_enum1> function2_arg8,
+        9: list<test_struct1> function2_arg9) throws (1:test_exception2 e),
+
+}
+
+service derived1 extends base {
+
+    test_enum1 derived1_function1(
+        1: i64 function1_arg1,
+        2: double function1_arg2,
+        3: test_enum1 function1_arg3) throws (1:test_exception2 e),
+
+    i64 derived1_function2(
+        1: list<i64> function2_arg1,
+        2: list<double> function2_arg2,
+        3: list<string> function2_arg3,
+        4: list<byte> function2_arg4,
+        5: list<test_enum1> function2_arg5) throws (1:test_exception2 e),
+
+    double derived1_function3(
+        1: string function3_arg1,
+        2: bool function3_arg2) throws (1:test_exception2 e),
+
+    string derived1_function4(
+        1: string function4_arg1,
+        2: bool function4_arg2) throws (1:test_exception2 e),
+
+
+    bool derived1_function5(
+        1: map<i64, double> function5_arg1,
+        2: map<string, bool> function5_arg2,