Subject: RM3079 fix for wrong sql datetime/time related datatypes

From: Harshal Dhumal <[email-masked]>
Date: Tue, 27 Feb 2018 20:06:56 +0530

Hi,

Please find patch to fix wrong sql issue for time related type.

Steps to reproduce:

Alter any time/datetime array related data type, it generates sql with
addition array bracket

-- 
*Harshal Dhumal*
*Sr. Software Engineer*

EnterpriseDB India: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/type/macros/get_full_type_sql_format.macros b/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/type/macros/get_full_type_sql_format.macros
index 1520232..a2ad322 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/type/macros/get_full_type_sql_format.macros
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/type/macros/get_full_type_sql_format.macros
@@ -58,6 +58,6 @@ time({{ data.attlen }}) with time zone {% endif %}{% if o_data.hasSqrBracket %}[
 {% if data.attlen and data.attlen != 'None' %}
 ({{ data.attlen }}{% elif o_data.attlen and o_data.attlen != 'None' %}({{ o_data.attlen }}{% endif %}{% if data.attprecision and data.attprecision != 'None' %}
 , {{ data.attprecision }}){% elif o_data.attprecision and o_data.attprecision != 'None' %}, {{ o_data.attprecision }}){% else %}){% endif %}
-{% endif %}
 {% endif %}{% if o_data.hasSqrBracket %}[]{% endif %}
+{% endif %}
 {% endmacro %}

---

From: Dave Page <[email-masked]>
Date: Tue, 27 Feb 2018 15:24:31 +0000

Hi

> Alter any time/datetime array related data type, it generates sql with

This seems to be missing the test case that you noted should be included in
the original bug report!

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Harshal Dhumal <[email-masked]>
Date: Wed, 28 Feb 2018 16:54:54 +0530

Hi,

On Tue, Feb 27, 2018 at 8:54 PM, Dave Page <[email-masked]> wrote:

> This seems to be missing the test case that you noted should be included
> in the original bug report!
>

Please find updated patch with test cases.

Thanks,

diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_add.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_add.py
index 54215d4..a2a8f17 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_add.py
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_add.py
@@ -24,7 +24,7 @@ from regression.python_test_utils import test_utils as utils
 class ColumnAddTestCase(BaseTestGenerator):
     """This class will add new column under table node."""
     scenarios = [
-        ('Add table Node URL', dict(url='/browser/column/obj/'))
+        ('Add column Node URL', dict(url='/browser/column/obj/'))
     ]
 
     def setUp(self):
diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_delete.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_delete.py
index 28fdc7a..1496ea8 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_delete.py
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_delete.py
@@ -24,7 +24,7 @@ from . import utils as columns_utils
 class ColumnDeleteTestCase(BaseTestGenerator):
     """This class will delete column under table node."""
     scenarios = [
-        ('Delete table Node URL', dict(url='/browser/column/obj/'))
+        ('Delete column Node URL', dict(url='/browser/column/obj/'))
     ]
 
     def setUp(self):
diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_msql.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_msql.py
new file mode 100644
index 0000000..3cdc292
--- /dev/null
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_msql.py
@@ -0,0 +1,183 @@
+##########################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2018, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
+
+import json
+import uuid
+
+from pgadmin.browser.server_groups.servers.databases.schemas.tables.tests \
+    import utils as tables_utils
+from pgadmin.browser.server_groups.servers.databases.schemas.tests import \
+    utils as schema_utils
+from pgadmin.browser.server_groups.servers.databases.tests import utils as \
+    database_utils
+from pgadmin.utils.route import BaseTestGenerator
+from regression import parent_node_dict
+from regression.python_test_utils import test_utils as utils
+from . import utils as columns_utils
+
+try:
+    from urllib.parse import urlencode
+except ImportError as e:
+    from urllib import urlencode
+
+
+class ColumnMsqlTestCase(BaseTestGenerator):
+    """This class will test msql route of column with various combinations."""
+    scenarios = [
+        ('msql column change timestamp array length',
+         dict(
+             url='/browser/column/msql/',
+             data_type='timestamp(3) with time zone[]',
+             new_len=6,
+             expected_res='ALTER TABLE {schema}.{table}\n    ALTER COLUMN '
+                          '{column} TYPE timestamp({len}) with time zone [];'
+         )),
+        ('msql column change timestamp length',
+         dict(
+             url='/browser/column/msql/',
+             data_type='timestamp(4) with time zone',
+             new_len=7,
+             expected_res='ALTER TABLE {schema}.{table}\n    ALTER COLUMN '
+                          '{column} TYPE timestamp({len}) with time zone ;'
+         )),
+        ('msql column change numeric array precision',
+         dict(
+             url='/browser/column/msql/',
+             data_type='numeric(5,2)[]',
+             old_len=5,
+             new_precision=4,
+             expected_res='ALTER TABLE {schema}.{table}\n    ALTER COLUMN '
+                          '{column} TYPE numeric ({len}, {precision})[];'
+         )),
+        ('msql column change numeric precision',
+         dict(
+             url='/browser/column/msql/',
+             data_type='numeric(6,3)',
+             old_len=6,
+             new_precision=5,
+             expected_res='ALTER TABLE {schema}.{table}\n    ALTER COLUMN '
+                          '{column} TYPE numeric ({len}, {precision});'
+         )),
+        ('msql column change numeric array length',
+         dict(
+             url='/browser/column/msql/',
+             data_type='numeric(6,3)[]',
+             new_len=8,
+             old_precision=3,
+             expected_res='ALTER TABLE {schema}.{table}\n    ALTER COLUMN '
+                          '{column} TYPE numeric ({len}, {precision})[];'
+         )),
+        ('msql column change numeric length',
+         dict(
+             url='/browser/column/msql/',
+             data_type='numeric(6,4)',
+             new_len=8,
+             old_precision=4,
+             expected_res='ALTER TABLE {schema}.{table}\n    ALTER COLUMN '
+                          '{column} TYPE numeric ({len}, {precision});'
+         )),
+        ('msql column change numeric array len and precision',
+         dict(
+             url='/browser/column/msql/',
+             data_type='numeric(10,5)[]',
+             new_len=15,
+             new_precision=8,
+             expected_res='ALTER TABLE {schema}.{table}\n    ALTER COLUMN '
+                          '{column} TYPE numeric ({len}, {precision})[];'
+         )),
+        ('msql column change numeric len and precision',
+         dict(
+             url='/browser/column/msql/',
+             data_type='numeric(12,6)',
+             new_len=14,
+             new_precision=9,
+             expected_res='ALTER TABLE {schema}.{table}\n    ALTER COLUMN '
+                          '{column} TYPE numeric ({len}, {precision});'
+         ))
+    ]
+
+    def setUp(self):
+        self.db_name = parent_node_dict["database"][-1]["db_name"]
+        schema_info = parent_node_dict["schema"][-1]
+        self.server_id = schema_info["server_id"]
+        self.db_id = schema_info["db_id"]
+        db_con = database_utils.connect_database(self, utils.SERVER_GROUP,
+                                                 self.server_id, self.db_id)
+        if not db_con['data']["connected"]:
+            raise Exception("Could not connect to database to add a table.")
+        self.schema_id = schema_info["schema_id"]
+        self.schema_name = schema_info["schema_name"]
+        schema_response = schema_utils.verify_schemas(self.server,
+                                                      self.db_name,
+                                                      self.schema_name)
+        if not schema_response:
+            raise Exception("Could not find the schema to add a table.")
+        self.table_name = "table_column_%s" % (str(uuid.uuid4())[1:8])
+        self.table_id = tables_utils.create_table(self.server, self.db_name,
+                                                  self.schema_name,
+                                                  self.table_name)
+        self.column_name = "test_column_msql_%s" % (str(uuid.uuid4())[1:8])
+        self.column_id = columns_utils.create_column(self.server,
+                                                     self.db_name,
+                                                     self.schema_name,
+                                                     self.table_name,
+                                                     self.column_name,
+                                                     self.data_type)
+
+    def runTest(self):
+        col_response = columns_utils.verify_column(self.server, self.db_name,
+                                                   self.column_name)
+        if not col_response:
+            raise Exception("Could not find the column to update.")
+
+        data = {"attnum": self.column_id}
+
+        expected_len = None
+        expected_precision = None
+
+        if hasattr(self, 'new_len'):
+            data["attlen"] = self.new_len
+            expected_len = self.new_len
+        if hasattr(self, 'new_precision'):
+            data["attprecision"] = self.new_precision
+            expected_precision = self.new_precision
+
+        response = self.tester.get(
+            self.url + str(utils.SERVER_GROUP) + '/' +
+            str(self.server_id) + '/' +
+            str(self.db_id) + '/' +
+            str(self.schema_id) + '/' +
+            str(self.table_id) + '/' +
+            str(self.column_id) + '?' +
+            urlencode(data),
+            follow_redirects=True)
+        self.assertEquals(response.status_code, 200)
+
+        response_data = json.loads(response.data.decode('utf-8'))
+
+        if not expected_len and hasattr(self, 'old_len'):
+            expected_len = self.old_len
+
+        if not expected_precision and hasattr(self, 'old_precision'):
+            expected_precision = self.old_precision
+
+        self.assertEquals(response_data['data'],
+                          self.expected_res.format(
+                              **dict([('schema', self.schema_name),
+                                      ('table', self.table_name),
+                                      ('column', self.column_name),
+                                      ('len', expected_len),
+                                      ('precision', expected_precision)
+                                      ]
+                                     )
+                          ))
+
+    def tearDown(self):
+        # Disconnect the database
+        database_utils.disconnect_database(self, self.server_id, self.db_id)
diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_put.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_put.py
index aa10d24..922ebb2 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_put.py
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/test_column_put.py
@@ -25,7 +25,7 @@ from . import utils as columns_utils
 class ColumnPutTestCase(BaseTestGenerator):
     """This class will update the column under table node."""
     scenarios = [
-        ('Put table Node URL', dict(url='/browser/column/obj/'))
+        ('Put column Node URL', dict(url='/browser/column/obj/'))
     ]
 
     def setUp(self):
diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/utils.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/utils.py
index 78c052d..4703bfb 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/utils.py
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/column/tests/utils.py
@@ -15,7 +15,8 @@ import traceback
 from regression.python_test_utils import test_utils as utils

-def create_column(server, db_name, schema_name, table_name, col_name):
+def create_column(server, db_name, schema_name, table_name, col_name,
+                  col_data_type='char'):
     """
     This function creates a column under provided table.
     :param server: server details
@@ -28,6 +29,8 @@ def create_column(server, db_name, schema_name, table_name, col_name):
     :type table_name: str
     :param col_name: column name
     :type col_name: str
+    :param col_data_type: column data type
+    :type col_data_type: str
     :return table_id: table id
     :rtype: int
     """
@@ -41,8 +44,8 @@ def create_column(server, db_name, schema_name, table_name, col_name):
         old_isolation_level = connection.isolation_level
         connection.set_isolation_level(0)
         pg_cursor = connection.cursor()
-        query = "ALTER TABLE %s.%s ADD COLUMN %s char" % \
-                (schema_name, table_name, col_name)
+        query = "ALTER TABLE %s.%s ADD COLUMN %s %s" % \
+                (schema_name, table_name, col_name, col_data_type)
         pg_cursor.execute(query)
         connection.set_isolation_level(old_isolation_level)
         connection.commit()
diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/type/macros/get_full_type_sql_format.macros b/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/type/macros/get_full_type_sql_format.macros
index 1520232..a2ad322 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/type/macros/get_full_type_sql_format.macros
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/types/templates/type/macros/get_full_type_sql_format.macros
@@ -58,6 +58,6 @@ time({{ data.attlen }}) with time zone {% endif %}{% if o_data.hasSqrBracket %}[
 {% if data.attlen and data.attlen != 'None' %}
 ({{ data.attlen }}{% elif o_data.attlen and o_data.attlen != 'None' %}({{ o_data.attlen }}{% endif %}{% if data.attprecision and data.attprecision != 'None' %}
 , {{ data.attprecision }}){% elif o_data.attprecision and o_data.attprecision != 'None' %}, {{ o_data.attprecision }}){% else %}){% endif %}
-{% endif %}
 {% endif %}{% if o_data.hasSqrBracket %}[]{% endif %}
+{% endif %}
 {% endmacro %}

---

From: Joao De Almeida Pereira <[email-masked]>
Date: Wed, 28 Feb 2018 14:45:41 +0000

Hello Harshal,

I tried the example you showed and it works. Also passed the patch through
our CI Pipeline and everything is good.

Thanks
Joao

---

From: Harshal Dhumal <[email-masked]>
Date: Wed, 28 Feb 2018 20:30:14 +0530

Great, thanks!

---

From: Dave Page <[email-masked]>
Date: Fri, 2 Mar 2018 11:52:02 +0000

Thanks, applied.

On Wed, Feb 28, 2018 at 3:00 PM, Harshal Dhumal <
[email-masked]> wrote:

>> I tried the example you showed and it works. Also passed the patch
>> through our CI Pipeline and everything is good.
>>
>> Thanks
>> Joao
>>
>>>> This seems to be missing the test case that you noted should be
>>>> included in the original bug report!
>>>>
>>>