THRIFT-2832 c_glib: Handle string lists correctly

Client: c_glib
Patch: Simon South <ssouth@simonsouth.com>

The compiler now correctly generates code for string lists (i.e. variables of type list<string>) that are
- Passed as a parameter to a service method,
- Returned from a service method or
- Assigned a default value.

Added a unit test that covers containers (initially only string lists) used as parameters to and return values from
service methods, and as members with default values inside structs.

3 files changed, 445 insertions, 0 deletions

diff --git a/lib/c_glib/test/ContainerTest.thrift b/lib/c_glib/test/ContainerTest.thrift
new file mode 100644
index 000000000..c19bae45f
--- /dev/null
+++ b/lib/c_glib/test/ContainerTest.thrift
@@ -0,0 +1,29 @@
+/*
+ * 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.
+ */
+
+namespace c_glib TTest
+
+struct ContainersWithDefaultValues {
+  1: list<string> StringList = [ "Apache", "Thrift" ];
+}
+
+service ContainerService {
+  void receiveStringList(1: list<string> stringList);
+  list<string> returnStringList();
+}
diff --git a/lib/c_glib/test/Makefile.am b/lib/c_glib/test/Makefile.am
index 0de7d2d92..372be2e52 100755
--- a/lib/c_glib/test/Makefile.am
+++ b/lib/c_glib/test/Makefile.am
@@ -21,6 +21,8 @@ AUTOMAKE_OPTIONS = subdir-objects serial-tests
 SUBDIRS =
 
 BUILT_SOURCES = \
+        gen-c_glib/t_test_container_test_types.c \
+        gen-c_glib/t_test_container_test_types.h \
         gen-c_glib/t_test_debug_proto_test_types.h \
         gen-c_glib/t_test_empty_service.h \
         gen-c_glib/t_test_inherited.h \
@@ -28,6 +30,8 @@ BUILT_SOURCES = \
         gen-c_glib/t_test_reverse_order_service.h \
         gen-c_glib/t_test_second_service.h \
         gen-c_glib/t_test_service_for_exception_with_a_map.h \
+        gen-c_glib/t_test_container_service.c \
+        gen-c_glib/t_test_container_service.h \
         gen-c_glib/t_test_srv.h \
         gen-c_glib/t_test_thrift_test.h \
         gen-c_glib/t_test_thrift_test_types.h
@@ -40,6 +44,7 @@ AM_LDFLAGS = $(GLIB_LIBS) $(GOBJECT_LIBS) @GCOV_LDFLAGS@
 
 check_PROGRAMS = \
   testapplicationexception \
+  testcontainertest \
   testtransportsocket \
   testbinaryprotocol \
   testbufferedtransport \
@@ -63,6 +68,22 @@ testapplicationexception_LDADD = \
     $(top_builddir)/lib/c_glib/src/thrift/c_glib/libthrift_c_glib_la-thrift_struct.o \
     $(top_builddir)/lib/c_glib/src/thrift/c_glib/transport/libthrift_c_glib_la-thrift_transport.o
 
+testcontainertest_SOURCES = testcontainertest.c
+testcontainertest_LDADD = \
+    $(top_builddir)/lib/c_glib/src/thrift/c_glib/libthrift_c_glib_la-thrift_struct.o \
+    $(top_builddir)/lib/c_glib/src/thrift/c_glib/protocol/libthrift_c_glib_la-thrift_protocol.o \
+    $(top_builddir)/lib/c_glib/src/thrift/c_glib/transport/libthrift_c_glib_la-thrift_transport.o \
+    $(top_builddir)/lib/c_glib/src/thrift/c_glib/transport/libthrift_c_glib_la-thrift_transport_factory.o \
+    $(top_builddir)/lib/c_glib/src/thrift/c_glib/processor/libthrift_c_glib_la-thrift_processor.o \
+    $(top_builddir)/lib/c_glib/src/thrift/c_glib/protocol/libthrift_c_glib_la-thrift_protocol_factory.o \
+    $(top_builddir)/lib/c_glib/src/thrift/c_glib/protocol/libthrift_c_glib_la-thrift_binary_protocol.o \
+    $(top_builddir)/lib/c_glib/src/thrift/c_glib/protocol/libthrift_c_glib_la-thrift_binary_protocol_factory.o \
+    $(top_builddir)/lib/c_glib/src/thrift/c_glib/transport/libthrift_c_glib_la-thrift_socket.o \
+    $(top_builddir)/lib/c_glib/src/thrift/c_glib/transport/libthrift_c_glib_la-thrift_server_transport.o \
+    $(top_builddir)/lib/c_glib/src/thrift/c_glib/transport/libthrift_c_glib_la-thrift_server_socket.o \
+    $(top_builddir)/lib/c_glib/src/thrift/c_glib/server/libthrift_c_glib_la-thrift_server.o \
+	  libtestgenc.la
+
 testtransportsocket_SOURCES = testtransportsocket.c
 testtransportsocket_LDADD = \
     $(top_builddir)/lib/c_glib/src/thrift/c_glib/transport/libthrift_c_glib_la-thrift_transport.o \
@@ -139,6 +160,7 @@ if WITH_CPP
 endif
 
 nodist_libtestgenc_la_SOURCES = \
+        gen-c_glib/t_test_container_test_types.c \
         gen-c_glib/t_test_debug_proto_test_types.c \
         gen-c_glib/t_test_empty_service.c \
         gen-c_glib/t_test_inherited.c \
@@ -147,8 +169,10 @@ nodist_libtestgenc_la_SOURCES = \
         gen-c_glib/t_test_second_service.c \
         gen-c_glib/t_test_service_for_exception_with_a_map.c \
         gen-c_glib/t_test_srv.c \
+        gen-c_glib/t_test_container_service.c \
         gen-c_glib/t_test_thrift_test.c \
         gen-c_glib/t_test_thrift_test_types.c \
+        gen-c_glib/t_test_container_test_types.h \
         gen-c_glib/t_test_debug_proto_test_types.h \
         gen-c_glib/t_test_empty_service.h \
         gen-c_glib/t_test_inherited.h \
@@ -157,6 +181,7 @@ nodist_libtestgenc_la_SOURCES = \
         gen-c_glib/t_test_second_service.h \
         gen-c_glib/t_test_service_for_exception_with_a_map.h \
         gen-c_glib/t_test_srv.h \
+        gen-c_glib/t_test_container_service.h \
         gen-c_glib/t_test_thrift_test.h \
         gen-c_glib/t_test_thrift_test_types.h
 libtestgenc_la_LIBADD = $(top_builddir)/lib/c_glib/libthrift_c_glib.la
@@ -173,6 +198,9 @@ libtestgencpp_la_CPPFLAGS = -I../../cpp/src $(BOOST_CPPFLAGS) -I./gen-cpp
 
 THRIFT = $(top_builddir)/compiler/cpp/thrift
 
+gen-c_glib/t_test_container_test_types.c gen-c_glib/t_test_container_test_types.h gen-c_glib/t_test_container_service.c gen-c_glib/t_test_container_service.h: ContainerTest.thrift
+	$(THRIFT) --gen c_glib $<
+
 gen-c_glib/t_test_debug_proto_test_types.c gen-c_glib/t_test_debug_proto_test_types.h gen-c_glib/t_test_empty_service.c gen-c_glib/t_test_empty_service.h gen-c_glib/t_test_inherited.c gen-c_glib/t_test_inherited.h gen-c_glib/t_test_reverse_order_service.c gen-c_glib/t_test_reverse_order_service.h gen-c_glib/t_test_service_for_exception_with_a_map.c gen-c_glib/t_test_service_for_exception_with_a_map.h gen-c_glib/t_test_srv.c gen-c_glib/t_test_srv.h: ../../../test/DebugProtoTest.thrift
 	$(THRIFT) --gen c_glib $<
 
diff --git a/lib/c_glib/test/testcontainertest.c b/lib/c_glib/test/testcontainertest.c
new file mode 100644
index 000000000..dc61666ba
--- /dev/null
+++ b/lib/c_glib/test/testcontainertest.c
@@ -0,0 +1,388 @@
+/*
+ * 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.
+ */
+
+#include "gen-c_glib/t_test_container_test_types.h"
+#include "gen-c_glib/t_test_container_service.h"
+
+#include <thrift/c_glib/thrift.h>
+#include <thrift/c_glib/protocol/thrift_binary_protocol_factory.h>
+#include <thrift/c_glib/protocol/thrift_binary_protocol.h>
+#include <thrift/c_glib/protocol/thrift_protocol_factory.h>
+#include <thrift/c_glib/server/thrift_server.h>
+#include <thrift/c_glib/server/thrift_simple_server.h>
+#include <thrift/c_glib/transport/thrift_buffered_transport_factory.h>
+#include <thrift/c_glib/transport/thrift_buffered_transport.h>
+#include <thrift/c_glib/transport/thrift_server_socket.h>
+#include <thrift/c_glib/transport/thrift_server_transport.h>
+#include <thrift/c_glib/transport/thrift_socket.h>
+
+#include <glib-object.h>
+#include <glib.h>
+
+#include <unistd.h>
+#include <signal.h>
+#include <string.h>
+#include <sys/wait.h>
+#include <sys/types.h>
+
+#define TEST_SERVER_HOSTNAME "localhost"
+#define TEST_SERVER_PORT     9090
+
+/* --------------------------------------------------------------------------
+   The ContainerService handler we'll use for testing */
+
+G_BEGIN_DECLS
+
+GType test_container_service_handler_get_type (void);
+
+#define TYPE_TEST_CONTAINER_SERVICE_HANDLER \
+  (test_container_service_handler_get_type ())
+
+#define TEST_CONTAINER_SERVICE_HANDLER(obj)                             \
+  (G_TYPE_CHECK_INSTANCE_CAST ((obj),                                   \
+                               TYPE_TEST_CONTAINER_SERVICE_HANDLER,     \
+                               TestContainerServiceHandler))
+#define TEST_CONTAINER_SERVICE_HANDLER_CLASS(c)                         \
+  (G_TYPE_CHECK_CLASS_CAST ((c),                                        \
+                            TYPE_TEST_CONTAINER_SERVICE_HANDLER,        \
+                            TestContainerServiceHandlerClass))
+#define IS_TEST_CONTAINER_SERVICE_HANDLER(obj)                          \
+  (G_TYPE_CHECK_INSTANCE_TYPE ((obj),                                   \
+                               TYPE_TEST_CONTAINER_SERVICE_HANDLER))
+#define IS_TEST_CONTAINER_SERVICE_HANDLER_CLASS(c)                      \
+  (G_TYPE_CHECK_CLASS_TYPE ((c),                                        \
+                            TYPE_TEST_CONTAINER_SERVICE_HANDLER))
+#define TEST_CONTAINER_SERVICE_HANDLER_GET_CLASS(obj)                   \
+  (G_TYPE_INSTANCE_GET_CLASS ((obj),                                    \
+                              TYPE_TEST_CONTAINER_SERVICE_HANDLER,      \
+                              TestContainerServiceHandlerClass))
+
+struct _TestContainerServiceHandler {
+  TTestContainerServiceHandler parent_instance;
+
+  /* private */
+  GPtrArray *string_list;
+};
+typedef struct _TestContainerServiceHandler TestContainerServiceHandler;
+
+struct _TestContainerServiceHandlerClass {
+  TTestContainerServiceHandlerClass parent_class;
+};
+typedef struct _TestContainerServiceHandlerClass
+  TestContainerServiceHandlerClass;
+
+G_END_DECLS
+
+/* -------------------------------------------------------------------------- */
+
+G_DEFINE_TYPE (TestContainerServiceHandler,
+               test_container_service_handler,
+               T_TEST_TYPE_CONTAINER_SERVICE_HANDLER)
+
+/* A helper function used to append copies of strings to a string list */
+static void append_string_to_ptr_array (gpointer element, gpointer ptr_array)
+{
+  g_ptr_array_add ((GPtrArray *)ptr_array, g_strdup ((gchar *)element));
+}
+
+/* Accept a string list from the client and append its contents to our internal
+   list */
+static gboolean
+test_container_service_handler_receive_string_list (TTestContainerServiceIf *iface,
+                                                    const GPtrArray *stringList,
+                                                    GError **error)
+{
+  TestContainerServiceHandler *self = TEST_CONTAINER_SERVICE_HANDLER (iface);
+
+  /* Append the client's strings to our own internal string list */
+  g_ptr_array_foreach ((GPtrArray *)stringList,
+                       append_string_to_ptr_array,
+                       self->string_list);
+
+  g_clear_error (error);
+  return TRUE;
+}
+
+/* Return the contents of our internal string list to the client */
+static gboolean
+test_container_service_handler_return_string_list (TTestContainerServiceIf *iface,
+                                                   GPtrArray **_return,
+                                                   GError **error)
+{
+  TestContainerServiceHandler *self = TEST_CONTAINER_SERVICE_HANDLER (iface);
+
+  /* Return (copies of) the strings contained in our list */
+  g_ptr_array_foreach (self->string_list,
+                       append_string_to_ptr_array,
+                       *_return);
+
+  g_clear_error (error);
+  return TRUE;
+}
+
+static void
+test_container_service_handler_finalize (GObject *object) {
+  TestContainerServiceHandler *self = TEST_CONTAINER_SERVICE_HANDLER (object);
+
+  /* Destroy our internal containers */
+  g_ptr_array_unref (self->string_list);
+  self->string_list = NULL;
+
+  G_OBJECT_CLASS (test_container_service_handler_parent_class)->
+    finalize (object);
+}
+
+static void
+test_container_service_handler_init (TestContainerServiceHandler *self)
+{
+  /* Create our internal containers */
+  self->string_list = g_ptr_array_new_with_free_func (g_free);
+}
+
+static void
+test_container_service_handler_class_init (TestContainerServiceHandlerClass *klass)
+{
+  GObjectClass *gobject_class = G_OBJECT_CLASS (klass);
+  TTestContainerServiceHandlerClass *parent_class =
+    T_TEST_CONTAINER_SERVICE_HANDLER_CLASS (klass);
+
+  gobject_class->finalize = test_container_service_handler_finalize;
+
+  parent_class->receive_string_list =
+    test_container_service_handler_receive_string_list;
+  parent_class->return_string_list =
+    test_container_service_handler_return_string_list;
+}
+
+/* -------------------------------------------------------------------------- */
+
+/* Our test server, declared globally so we can access it within a signal
+   handler */
+ThriftServer *server = NULL;
+
+/* A signal handler used to detect when the child process (the test suite) has
+   exited so we know to shut down the server and terminate ourselves */
+static void
+sigchld_handler (int signal_number)
+{
+  THRIFT_UNUSED_VAR (signal_number);
+
+  /* The child process (the tests) has exited or been terminated; shut down the
+     server gracefully */
+  if (server != NULL)
+    thrift_server_stop (server);
+}
+
+static void
+test_containers_with_default_values (void)
+{
+  TTestContainersWithDefaultValues *default_values;
+  GPtrArray *string_list;
+
+  /* Fetch a new ContainersWithDefaultValues struct and its StringList member */
+  default_values = g_object_new (T_TEST_TYPE_CONTAINERS_WITH_DEFAULT_VALUES,
+                                 NULL);
+  g_object_get (default_values,
+                "StringList", &string_list,
+                NULL);
+
+  /* Make sure the list has been populated with its default values */
+  g_assert_cmpint (string_list->len, ==, 2);
+  g_assert_cmpstr (((gchar **)string_list->pdata)[0], ==, "Apache");
+  g_assert_cmpstr (((gchar **)string_list->pdata)[1], ==, "Thrift");
+
+  g_ptr_array_unref (string_list);
+  g_object_unref (default_values);
+}
+
+static void
+test_container_service_string_list (void)
+{
+  ThriftSocket *socket;
+  ThriftTransport *transport;
+  ThriftProtocol *protocol;
+
+  TTestContainerServiceIf *client;
+
+  GPtrArray *outgoing_string_list;
+  GPtrArray *incoming_string_list;
+
+  gchar *test_data[] = { "one", "two", "three" };
+  guint8 index;
+
+  GError *error = NULL;
+
+  /* Prepare our test data (our string list to send) */
+  outgoing_string_list = g_ptr_array_new ();
+  for (index = 0; index < 3; index += 1)
+    g_ptr_array_add (outgoing_string_list, &test_data[index]);
+
+  /* Create a client with which to access the server */
+  socket    = g_object_new (THRIFT_TYPE_SOCKET,
+                            "hostname", TEST_SERVER_HOSTNAME,
+                            "port",     TEST_SERVER_PORT,
+                            NULL);
+  transport = g_object_new (THRIFT_TYPE_BUFFERED_TRANSPORT,
+                            "transport", socket,
+                            NULL);
+  protocol  = g_object_new (THRIFT_TYPE_BINARY_PROTOCOL,
+                            "transport", transport,
+                            NULL);
+
+  thrift_transport_open (transport, &error);
+  g_assert_no_error (error);
+
+  client = g_object_new (T_TEST_TYPE_CONTAINER_SERVICE_CLIENT,
+                         "input_protocol",  protocol,
+                         "output_protocol", protocol,
+                         NULL);
+
+  /* Send our data to the server and make sure we get the same data back on
+     retrieve */
+  g_assert
+    (t_test_container_service_client_receive_string_list (client,
+                                                          outgoing_string_list,
+                                                          &error) &&
+     error == NULL);
+
+  incoming_string_list = g_ptr_array_new ();
+  g_assert
+    (t_test_container_service_client_return_string_list (client,
+                                                         &incoming_string_list,
+                                                         &error) &&
+     error == NULL);
+
+  /* Make sure the two lists are equivalent */
+  g_assert_cmpint (incoming_string_list->len, ==, outgoing_string_list->len);
+  for (index = 0; index < incoming_string_list->len; index += 1)
+    g_assert_cmpstr (((gchar **)incoming_string_list->pdata)[index],
+                     ==,
+                     ((gchar **)outgoing_string_list->pdata)[index]);
+
+  /* Clean up and exit */
+  thrift_transport_close (transport, NULL);
+
+  g_ptr_array_unref (incoming_string_list);
+  g_ptr_array_unref (outgoing_string_list);
+
+  g_object_unref (client);
+  g_object_unref (protocol);
+  g_object_unref (transport);
+  g_object_unref (socket);
+}
+
+int
+main(int argc, char *argv[])
+{
+  pid_t pid;
+  int status;
+
+#if (!GLIB_CHECK_VERSION (2, 36, 0))
+  g_type_init ();
+#endif
+
+  /* Fork to run our test suite in a child process */
+  pid = fork ();
+  g_assert_cmpint (pid, >=, 0);
+
+  if (pid == 0) {    /* The child process */
+    /* Wait a moment for the server to finish starting */
+    sleep (1);
+
+    g_test_init (&argc, &argv, NULL);
+
+    g_test_add_func
+      ("/testcontainertest/ContainerTest/Structs/ContainersWithDefaultValues",
+       test_containers_with_default_values);
+    g_test_add_func
+      ("/testcontainertest/ContainerTest/Services/ContainerService/StringList",
+       test_container_service_string_list);
+
+    /* Run the tests and make the result available to our parent process */
+    _exit (g_test_run ());
+  }
+  else {
+    TTestContainerServiceHandler *handler;
+    TTestContainerServiceProcessor *processor;
+
+    ThriftServerTransport *server_transport;
+    ThriftTransportFactory *transport_factory;
+    ThriftProtocolFactory *protocol_factory;
+
+    struct sigaction sigchld_action;
+
+    GError *error = NULL;
+    int exit_status = 1;
+
+    /* Trap the event of the child process terminating so we know to stop the
+       server and exit */
+    memset (&sigchld_action, 0, sizeof (sigchld_action));
+    sigchld_action.sa_handler = sigchld_handler;
+    sigchld_action.sa_flags = SA_RESETHAND;
+    sigaction (SIGCHLD, &sigchld_action, NULL);
+
+    /* Create our test server */
+    handler = g_object_new (TYPE_TEST_CONTAINER_SERVICE_HANDLER,
+                            NULL);
+    processor = g_object_new (T_TEST_TYPE_CONTAINER_SERVICE_PROCESSOR,
+                              "handler", handler,
+                              NULL);
+    server_transport = g_object_new (THRIFT_TYPE_SERVER_SOCKET,
+                                     "port", TEST_SERVER_PORT,
+                                     NULL);
+    transport_factory = g_object_new (THRIFT_TYPE_BUFFERED_TRANSPORT_FACTORY,
+                                      NULL);
+    protocol_factory = g_object_new (THRIFT_TYPE_BINARY_PROTOCOL_FACTORY,
+                                     NULL);
+
+    server = g_object_new (THRIFT_TYPE_SIMPLE_SERVER,
+                           "processor",                processor,
+                           "server_transport",         server_transport,
+                           "input_transport_factory",  transport_factory,
+                           "output_transport_factory", transport_factory,
+                           "input_protocol_factory",   protocol_factory,
+                           "output_protocol_factory",  protocol_factory,
+                           NULL);
+
+    /* Start the server */
+    thrift_server_serve (server, &error);
+
+    /* Make sure the server stopped only because it was interrupted (by the
+       child process terminating) */
+    g_assert (g_error_matches (error,
+                               THRIFT_SERVER_SOCKET_ERROR,
+                               THRIFT_SERVER_SOCKET_ERROR_ACCEPT));
+
+    /* Free our resources */
+    g_object_unref (server);
+    g_object_unref (transport_factory);
+    g_object_unref (protocol_factory);
+    g_object_unref (server_transport);
+
+    g_object_unref (processor);
+    g_object_unref (handler);
+
+    /* Wait for the child process to complete and return its exit status */
+    g_assert (wait (&status) == pid);
+    if (WIFEXITED (status))
+      exit_status = WEXITSTATUS (status);
+
+    return exit_status;
+  }
+}
\ No newline at end of file