From 737cd077372e637bbea8992649f0a01d16b8689c Mon Sep 17 00:00:00 2001
From: Chet Murthy <chetsky@gmail.com>
Date: Mon, 18 Dec 2017 11:49:30 -0800
Subject: [PATCH] THRIFT-4399 plugin.thrift t_const_value is not used as a
 union in C++ code -- fix this

plugin.thrift defines t_const_value as a union.  But in plugin_output.cc
and plugin.cc, the converters clearly either (a) SET NEITHER of
identifier_val & enum_val, or (b) SET BOTH.  But these are two different
fields in the union.  So clearly, the type t_const_value isn't being
treated as a union.

I think we need to fix Thrift's treatment of unions, but independently,
the plugin should use Thrift's type system in a correct manner.  This is
easy-to-fix, but since the current plugin relies on a bug, the fix will
be a breaking change.
---
 compiler/cpp/src/thrift/plugin/plugin.cc        | 12 +++++-------
 compiler/cpp/src/thrift/plugin/plugin.thrift    |  9 +++++++--
 compiler/cpp/src/thrift/plugin/plugin_output.cc |  9 +++++++--
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/compiler/cpp/src/thrift/plugin/plugin.cc b/compiler/cpp/src/thrift/plugin/plugin.cc
index 0bac1352b98..414b2cbe7cd 100644
--- a/compiler/cpp/src/thrift/plugin/plugin.cc
+++ b/compiler/cpp/src/thrift/plugin/plugin.cc
@@ -258,11 +258,11 @@ THRIFT_CONVERSION(t_const_value, ) {
     T_CONST_VALUE_CASE(string);
   else T_CONST_VALUE_CASE(integer);
   else T_CONST_VALUE_CASE(double);
-  else {
-    T_CONST_VALUE_CASE(identifier);
-    if (from.__isset.enum_val)
-      to->set_enum(resolve_type< ::t_enum>(from.enum_val));
+  else if (from.__isset.const_identifier_val) {
+    to->set_identifier(from.const_identifier_val.identifier_val) ;
+    to->set_enum(resolve_type< ::t_enum>(from.const_identifier_val.enum_val)) ;
   }
+
 #undef T_CONST_VALUE_CASE
 }
 THRIFT_CONVERSION(t_field, resolve_type< ::t_type>(from.type), from.name, from.key) {
@@ -458,9 +458,7 @@ ::t_const_value::t_const_value_type const_value_case(const t_const_value& v) {
     return ::t_const_value::CV_INTEGER;
   if (v.__isset.double_val)
     return ::t_const_value::CV_DOUBLE;
-  if (v.__isset.identifier_val)
-    return ::t_const_value::CV_IDENTIFIER;
-  if (v.__isset.enum_val)
+  if (v.__isset.const_identifier_val)
     return ::t_const_value::CV_IDENTIFIER;
   throw ThriftPluginError("Unknown const value type");
 }
diff --git a/compiler/cpp/src/thrift/plugin/plugin.thrift b/compiler/cpp/src/thrift/plugin/plugin.thrift
index 1e51310d861..6d98f99558c 100644
--- a/compiler/cpp/src/thrift/plugin/plugin.thrift
+++ b/compiler/cpp/src/thrift/plugin/plugin.thrift
@@ -105,15 +105,20 @@ enum Requiredness {
   T_OPT_IN_REQ_OUT = 2
 }
 
+struct t_const_identifier_value {
+  1: required string identifier_val
+  2: required t_type_id enum_val
+}
+
 union t_const_value {
   1: optional map<t_const_value, t_const_value> map_val
   2: optional list<t_const_value> list_val
   3: optional string string_val
   4: optional i64 integer_val
   5: optional double double_val
-  6: optional string identifier_val
-  7: optional t_type_id enum_val
+  8: optional t_const_identifier_value const_identifier_val
 }
+
 struct t_const {
   1: required string name
   2: required t_type_id type
diff --git a/compiler/cpp/src/thrift/plugin/plugin_output.cc b/compiler/cpp/src/thrift/plugin/plugin_output.cc
index 75725a1c034..01e996a0a6d 100644
--- a/compiler/cpp/src/thrift/plugin/plugin_output.cc
+++ b/compiler/cpp/src/thrift/plugin/plugin_output.cc
@@ -195,8 +195,13 @@ THRIFT_CONVERSION(t_const_value) {
     THRIFT_ASSIGN_N(get_string(), string_val, );
     break;
   case t_const_value::CV_IDENTIFIER:
-    THRIFT_ASSIGN_ID_N(t_type, enum_, enum_val);
-    THRIFT_ASSIGN_N(get_identifier(), identifier_val, );
+    if (from) {
+      apache::thrift::plugin::t_const_identifier_value cidval ;
+      if (from->enum_)
+	cidval.__set_enum_val(store_type<t_type>(from->enum_));
+      cidval.__set_identifier_val(from->get_identifier());
+      to.__set_const_identifier_val(cidval) ;
+    }
     break;
   case t_const_value::CV_MAP:
     to.__isset.map_val = true;