THRIFT-1270. compiler: add --allow-neg-keys argument to allow

This switch allows the user to specify explicit negative field IDs in Thrift IDL in order to avoid breaking wire protocol.

Patch: Dave Watson

git-svn-id: https://svn.apache.org/repos/asf/thrift/trunk@1158967 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/compiler/cpp/src/globals.h b/compiler/cpp/src/globals.h
index b204143..b856a61 100644 (file)
--- a/compiler/cpp/src/globals.h
+++ b/compiler/cpp/src/globals.h
@@ -114,4 +114,19 @@ extern char* g_doctext;
  */
 extern int g_doctext_lineno;
 
+/**
+ * Whether or not negative field keys are accepted.
+ *
+ * When a field does not have a user-specified key, thrift automatically
+ * assigns a negative value.  However, this is fragile since changes to the
+ * file may unintentionally change the key numbering, resulting in a new
+ * protocol that is not backwards compatible.
+ *
+ * When g_allow_neg_field_keys is enabled, users can explicitly specify
+ * negative keys.  This way they can write a .thrift file with explicitly
+ * specified keys that is still backwards compatible with older .thrift files
+ * that did not specify key values.
+ */
+extern int g_allow_neg_field_keys;
+
 #endif

diff --git a/compiler/cpp/src/main.cc b/compiler/cpp/src/main.cc
index 3e4a874..da86d0d 100644 (file)
--- a/compiler/cpp/src/main.cc
+++ b/compiler/cpp/src/main.cc
@@ -151,6 +151,11 @@ char* g_doctext;
  */
 int g_doctext_lineno;
 
+/**
+ * Whether or not negative field keys are accepted.
+ */
+int g_allow_neg_field_keys;
+
 /**
  * Flags to control code generation
  */
@@ -639,6 +644,9 @@ void usage() {
   fprintf(stderr, "  -v[erbose]  Verbose mode\n");
   fprintf(stderr, "  -r[ecurse]  Also generate included files\n");
   fprintf(stderr, "  -debug      Parse debug trace to stdout\n");
+  fprintf(stderr, "  --allow-neg-keys  Allow negative field keys (Used to "
+          "preserve protocol\n");
+  fprintf(stderr, "                compatibility with older .thrift files)\n");
   fprintf(stderr, "  --gen STR   Generate code with a dynamically-registered generator.\n");
   fprintf(stderr, "                STR has the form language[:key1=val1[,key2,[key3=val3]]].\n");
   fprintf(stderr, "                Keys and values are options passed to the generator.\n");
@@ -970,6 +978,8 @@ int main(int argc, char** argv) {
         g_verbose = 1;
       } else if (strcmp(arg, "-r") == 0 || strcmp(arg, "-recurse") == 0 ) {
         gen_recurse = true;
+      } else if (strcmp(arg, "-allow-neg-keys") == 0) {
+        g_allow_neg_field_keys = true;
       } else if (strcmp(arg, "-gen") == 0) {
         arg = argv[++i];
         if (arg == NULL) {

diff --git a/compiler/cpp/src/parse/t_field.h b/compiler/cpp/src/parse/t_field.h
index 0c3c2b1..ac10d57 100644 (file)
--- a/compiler/cpp/src/parse/t_field.h
+++ b/compiler/cpp/src/parse/t_field.h
@@ -150,4 +150,13 @@ class t_field : public t_doc {
 
 };
 
+/**
+ * A simple struct for the parser to use to store a field ID, and whether or
+ * not it was specified by the user or automatically chosen.
+ */
+struct t_field_id {
+  int64_t value;
+  bool auto_assigned;
+};
+
 #endif

diff --git a/compiler/cpp/src/thrifty.yy b/compiler/cpp/src/thrifty.yy
index fda49cd..f50c1e2 100644 (file)
--- a/compiler/cpp/src/thrifty.yy
+++ b/compiler/cpp/src/thrifty.yy
@@ -71,6 +71,7 @@ const int struct_is_union = 1;
   char*          dtext;
   t_field::e_req ereq;
   t_annotation*  tannot;
+  t_field_id     tfieldid;
 }
 
 /**
@@ -174,7 +175,7 @@ const int struct_is_union = 1;
 %type<tannot>    TypeAnnotation
 
 %type<tfield>    Field
-%type<iconst>    FieldIdentifier
+%type<tfieldid>  FieldIdentifier
 %type<ereq>      FieldRequiredness
 %type<ttype>     FieldType
 %type<tconstv>   FieldValue
@@ -870,14 +871,14 @@ Field:
   CaptureDocText FieldIdentifier FieldRequiredness FieldType tok_identifier FieldValue XsdOptional XsdNillable XsdAttributes TypeAnnotations CommaOrSemicolonOptional
     {
       pdebug("tok_int_constant : Field -> FieldType tok_identifier");
-      if ($2 < 0) {
+      if ($2.auto_assigned) {
         pwarning(1, "No field key specified for %s, resulting protocol may have conflicts or not be backwards compatible!\n", $5);
         if (g_strict >= 192) {
           yyerror("Implicit field keys are deprecated and not allowed with -strict");
           exit(1);
         }
       }
-      $$ = new t_field($4, $5, $2);
+      $$ = new t_field($4, $5, $2.value);
       $$->set_req($3);
       if ($6 != NULL) {
         g_scope->resolve_const_value($6, $4);
@@ -902,14 +903,42 @@ FieldIdentifier:
   tok_int_constant ':'
     {
       if ($1 <= 0) {
-        pwarning(1, "Nonpositive value (%d) not allowed as a field key.\n", $1);
-        $1 = y_field_val--;
+        if (g_allow_neg_field_keys) {
+          /*
+           * g_allow_neg_field_keys exists to allow users to add explicitly
+           * specified key values to old .thrift files without breaking
+           * protocol compatibility.
+           */
+          if ($1 != y_field_val) {
+            /*
+             * warn if the user-specified negative value isn't what
+             * thrift would have auto-assigned.
+             */
+            pwarning(1, "Negative field key (%d) differs from what would be "
+                     "auto-assigned by thrift (%d).\n", $1, y_field_val);
+          }
+          /*
+           * Leave $1 as-is, and update y_field_val to be one less than $1.
+           * The FieldList parsing will catch any duplicate key values.
+           */
+          y_field_val = $1 - 1;
+          $$.value = $1;
+          $$.auto_assigned = false;
+        } else {
+          pwarning(1, "Nonpositive value (%d) not allowed as a field key.\n",
+                   $1);
+          $$.value = y_field_val--;
+          $$.auto_assigned = true;
+        }
+      } else {
+        $$.value = $1;
+        $$.auto_assigned = false;
       }
-      $$ = $1;
     }
 |
     {
-      $$ = y_field_val--;
+      $$.value = y_field_val--;
+      $$.auto_assigned = true;
     }
 
 FieldRequiredness: