THRIFT-3122 Javascript struct constructor should properly initialize …

…struct and container members from plain js arguments Patch: Igor Tkach This closes apache#519
paddyesch · Jun 25, 2015 · 15d9042 · 15d9042
1 parent 0b8132d
commit 15d9042
Show file tree

Hide file tree

Showing 13 changed files with 737 additions and 10 deletions.
diff --git a/compiler/cpp/src/generate/t_js_generator.cc b/compiler/cpp/src/generate/t_js_generator.cc
@@ -41,6 +41,7 @@ static const string endl = "\n"; // avoid ostream << std::endl flushes
 
 #include "t_oop_generator.h"
 
+
 /**
  * JS code generator.
  */
@@ -181,6 +182,8 @@ class t_js_generator : public t_oop_generator {
            + "//\n";
   }
 
+  t_type* get_contained_type(t_type* t);
+
   std::vector<std::string> js_namespace_pieces(t_program* p) {
     std::string ns = p->get_namespace("js");
 
@@ -600,6 +603,22 @@ void t_js_generator::generate_js_struct(t_struct* tstruct, bool is_exception) {
   generate_js_struct_definition(f_types_, tstruct, is_exception);
 }
 
+/**
+ * Return type of contained elements for a container type. For maps
+ * this is type of value (keys are always strings in js)
+ */
+t_type* t_js_generator::get_contained_type(t_type* t) {
+  t_type* etype;
+  if (t->is_list()) {
+    etype = ((t_list*)t)->get_elem_type();
+  } else if (t->is_set()) {
+    etype = ((t_set*)t)->get_elem_type();
+  } else {
+    etype = ((t_map*)t)->get_val_type();
+  }
+  return etype;
+}
+
 /**
  * Generates a struct definition for a thrift data type. This is nothing in JS
  * where the objects are all just associative arrays (unless of course we
@@ -685,9 +704,47 @@ void t_js_generator::generate_js_struct_definition(ofstream& out,
     }
 
     for (m_iter = members.begin(); m_iter != members.end(); ++m_iter) {
+      t_type* t = get_true_type((*m_iter)->get_type());
       out << indent() << indent() << "if (args." << (*m_iter)->get_name() << " !== undefined) {"
-          << endl << indent() << indent() << indent() << "this." << (*m_iter)->get_name()
-          << " = args." << (*m_iter)->get_name() << ";" << endl;
+          << endl << indent() << indent() << indent() << "this." << (*m_iter)->get_name();
+
+      if (t->is_struct()) {
+        out << (" = new " + js_type_namespace(t->get_program()) + t->get_name() +
+                "(args."+(*m_iter)->get_name() +");");
+        out << endl;
+      } else if (t->is_container()) {
+        t_type* etype = get_contained_type(t);
+        string copyFunc = t->is_map() ? "Thrift.copyMap" : "Thrift.copyList";
+        string type_list = "";
+
+        while (etype->is_container()) {
+          if (type_list.length() > 0) {
+            type_list += ", ";
+          }
+          type_list += etype->is_map() ? "Thrift.copyMap" : "Thrift.copyList";
+          etype = get_contained_type(etype);
+        }
+
+        if (etype->is_struct()) {
+          if (type_list.length() > 0) {
+            type_list += ", ";
+          }
+          type_list += js_type_namespace(etype->get_program()) + etype->get_name();
+        }
+        else {
+          if (type_list.length() > 0) {
+            type_list += ", ";
+          }
+          type_list += "null";
+        }
+
+        out << (" = " + copyFunc + "(args." + (*m_iter)->get_name() +
+                ", [" + type_list + "]);");
+        out << endl;
+      } else {
+        out << " = args." << (*m_iter)->get_name() << ";" << endl;
+      }
+
       if (!(*m_iter)->get_req()) {
         out << indent() << indent() << "} else {" << endl << indent() << indent() << indent()
             << "throw new Thrift.TProtocolException(Thrift.TProtocolExceptionType.UNKNOWN, "

diff --git a/lib/js/Gruntfile.js b/lib/js/Gruntfile.js
@@ -35,7 +35,7 @@ module.exports = function(grunt) {
           'dist/<%= pkg.name %>.min.js': ['<%= concat.dist.dest %>']
         }
       }
-    },  
+    },
     shell: {
       InstallThriftJS: {
         command: 'mkdir test/build; mkdir test/build/js; cp src/thrift.js test/build/js/thrift.js'
@@ -48,6 +48,9 @@ module.exports = function(grunt) {
       },
       ThriftGenJQ: {
         command: 'thrift -gen js:jquery -gen js:node -o test ../../test/ThriftTest.thrift'
+      },
+      ThriftGenDeepConstructor: {
+        command: 'thrift -gen js -o test ../../test/JsDeepConstructorTest.thrift'
       }
     },
     external_daemon: {
@@ -123,6 +126,13 @@ module.exports = function(grunt) {
             'https://localhost:8089/testws.html'
           ]
         }
+      },
+      ThriftDeepConstructor: {
+        options: {
+          urls: [
+            'http://localhost:8088/test-deep-constructor.html'
+          ]
+        }
       }
     },
     jshint: {
@@ -147,13 +157,14 @@ module.exports = function(grunt) {
   grunt.loadNpmTasks('grunt-external-daemon');
   grunt.loadNpmTasks('grunt-shell');
 
-  grunt.registerTask('test', ['jshint', 'shell:InstallThriftJS', 'shell:InstallThriftNodeJSDep', 'shell:ThriftGen', 
-                              'external_daemon:ThriftTestServer', 'external_daemon:ThriftTestServer_TLS', 
+  grunt.registerTask('test', ['jshint', 'shell:InstallThriftJS', 'shell:InstallThriftNodeJSDep', 'shell:ThriftGen',
+                              'external_daemon:ThriftTestServer', 'external_daemon:ThriftTestServer_TLS',
+                              'shell:ThriftGenDeepConstructor', 'qunit:ThriftDeepConstructor',
                               'qunit:ThriftJS', 'qunit:ThriftJS_TLS',
                               'shell:ThriftGenJQ', 'qunit:ThriftJSJQ', 'qunit:ThriftJSJQ_TLS'
                              ]);
-  grunt.registerTask('default', ['jshint', 'shell:InstallThriftJS', 'shell:InstallThriftNodeJSDep', 'shell:ThriftGen', 
-                                 'external_daemon:ThriftTestServer', 'external_daemon:ThriftTestServer_TLS', 
+  grunt.registerTask('default', ['jshint', 'shell:InstallThriftJS', 'shell:InstallThriftNodeJSDep', 'shell:ThriftGen',
+                                 'external_daemon:ThriftTestServer', 'external_daemon:ThriftTestServer_TLS',
                                  'qunit:ThriftJS', 'qunit:ThriftJS_TLS',
                                  'shell:ThriftGenJQ', 'qunit:ThriftJSJQ', 'qunit:ThriftJSJQ_TLS',
                                  'concat', 'uglify', 'jsdoc'

diff --git a/lib/js/src/thrift.js b/lib/js/src/thrift.js
@@ -1203,7 +1203,7 @@ Thrift.Protocol.prototype = {
         r.size = list.shift();
 
         this.rpos.push(this.rstack.length);
-        this.rstack.push(list);
+        this.rstack.push(list.shift());
 
         return r;
     },
@@ -1439,3 +1439,69 @@ Thrift.Multiplexer.prototype.createClient = function (serviceName, SCl, transpor
 
 
 
+var copyList, copyMap;
+
+copyList = function(lst, types) {
+
+  if (!lst) {return lst; }
+
+  var type;
+
+  if (types.shift === undefined) {
+    type = types;
+  }
+  else {
+    type = types[0];
+  }
+  var Type = type;
+
+  var len = lst.length, result = [], i, val;
+  for (i = 0; i < len; i++) {
+    val = lst[i];
+    if (type === null) {
+      result.push(val);
+    }
+    else if (type === copyMap || type === copyList) {
+      result.push(type(val, types.slice(1)));
+    }
+    else {
+      result.push(new Type(val));
+    }
+  }
+  return result;
+};
+
+copyMap = function(obj, types){
+
+  if (!obj) {return obj; }
+
+  var type;
+
+  if (types.shift === undefined) {
+    type = types;
+  }
+  else {
+    type = types[0];
+  }
+  var Type = type;
+
+  var result = {}, val;
+  for(var prop in obj) {
+    if(obj.hasOwnProperty(prop)) {
+      val = obj[prop];
+      if (type === null) {
+        result[prop] = val;
+      }
+      else if (type === copyMap || type === copyList) {
+        result[prop] = type(val, types.slice(1));
+      }
+      else {
+        result[prop] = new Type(val);
+      }
+    }
+  }
+  return result;
+};
+
+Thrift.copyMap = copyMap;
+Thrift.copyList = copyList;