From: Bryan Duxbury Date: Wed, 17 Aug 2011 23:17:04 +0000 (+0000) Subject: THRIFT-1270. compiler: add --allow-neg-keys argument to allow X-Git-Tag: 0.8.0~144 X-Git-Url: https://source.supwisdom.com/gerrit/gitweb?a=commitdiff_plain;h=c7206a40117da7d1f4a8a98ea52099825bea3b45;p=common%2Fthrift.git 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 b2041436..b856a61d 100644 --- 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 3e4a8744..da86d0d8 100644 --- 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 0c3c2b1f..ac10d573 100644 --- 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 fda49cdd..f50c1e2f 100644 --- 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 TypeAnnotation %type Field -%type FieldIdentifier +%type FieldIdentifier %type FieldRequiredness %type FieldType %type 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: