Repository: tableau/server-client-python

Issue:
Downloads with invalid characters in the workbook name fail
When downloading workbooks with a name like `Option A or B?` it fails with the error `OSError(22, 'Invalid argument')` on windows because question marks aren't allowed in file names. (Note: when downloading through the browser it converts characters like this to safe ones.. i.e. `>` becomes `_`)

Looking at the [code](https://github.com/tableau/server-client-python/blob/8e6c9078c0f5894e034393f7fcbc9f14f10c5838/tableauserverclient/server/endpoint/workbooks_endpoint.py#L91) I don't see an easy way to overwrite the downloaded workbook name.

Two Requests: 
  1) adding an optional string parameter to set the downloaded workbook name.
  2) adding an optional boolean parameter to download the workbook name the way the browser / server handles it.

(One of these options should take precedence over the other as they are mutually exclusive but both options would be useful).

Discussion:

Patch:
diff --git a/samples/initialize_server.py b/samples/initialize_server.py
index 848159a..a3e312c 100644
--- a/samples/initialize_server.py
+++ b/samples/initialize_server.py
@@ -11,7 +11,6 @@ import logging
 import tableauserverclient as TSC
 
 
-
 def main():
     parser = argparse.ArgumentParser(description='Initialize a server with content.')
     parser.add_argument('--server', '-s', required=True, help='server address')
@@ -68,7 +67,8 @@ def main():
         ################################################################################
         # Step 4: Create the project we need only if it doesn't exist
         ################################################################################
-        import time; time.sleep(2)  # sad panda...something about eventually consistent model
+        import time
+        time.sleep(2)  # sad panda...something about eventually consistent model
         all_projects = TSC.Pager(server_upload.projects)
         project = next((p for p in all_projects if p.name.lower() == args.project.lower()), None)
 
diff --git a/tableauserverclient/filesys_helpers.py b/tableauserverclient/filesys_helpers.py
new file mode 100644
index 0000000..0cf304b
--- /dev/null
+++ b/tableauserverclient/filesys_helpers.py
@@ -0,0 +1,6 @@
+ALLOWED_SPECIAL = (' ', '.', '_', '-')
+
+
+def to_filename(string_to_sanitize):
+    sanitized = (c for c in string_to_sanitize if c.isalnum() or c in ALLOWED_SPECIAL)
+    return "".join(sanitized)
diff --git a/tableauserverclient/server/endpoint/datasources_endpoint.py b/tableauserverclient/server/endpoint/datasources_endpoint.py
index 6ab275d..5491736 100644
--- a/tableauserverclient/server/endpoint/datasources_endpoint.py
+++ b/tableauserverclient/server/endpoint/datasources_endpoint.py
@@ -2,6 +2,7 @@ from .endpoint import Endpoint, api, parameter_added_in
 from .exceptions import MissingRequiredFieldError
 from .fileuploads_endpoint import Fileuploads
 from .. import RequestFactory, DatasourceItem, PaginationItem, ConnectionItem
+from ...filesys_helpers import to_filename
 import os
 import logging
 import copy
@@ -77,7 +78,7 @@ class Datasources(Endpoint):
 
         with closing(self.get_request(url, parameters={'stream': True})) as server_response:
             _, params = cgi.parse_header(server_response.headers['Content-Disposition'])
-            filename = os.path.basename(params['filename'])
+            filename = to_filename(os.path.basename(params['filename']))
             if filepath is None:
                 filepath = filename
             elif os.path.isdir(filepath):
diff --git a/tableauserverclient/server/endpoint/groups_endpoint.py b/tableauserverclient/server/endpoint/groups_endpoint.py
index a1485a4..243aa54 100644
--- a/tableauserverclient/server/endpoint/groups_endpoint.py
+++ b/tableauserverclient/server/endpoint/groups_endpoint.py
@@ -58,10 +58,10 @@ class Groups(Endpoint):
     def remove_user(self, group_item, user_id):
         self._remove_user(group_item, user_id)
         try:
-            user_set = group_item.users
-            for user in user_set:
+            users = group_item.users
+            for user in users:
                 if user.id == user_id:
-                    user_set.remove(user)
+                    users.remove(user)
                     break
         except UnpopulatedPropertyError:
             # If we aren't populated, do nothing to the user list
@@ -73,9 +73,9 @@ class Groups(Endpoint):
     def add_user(self, group_item, user_id):
         new_user = self._add_user(group_item, user_id)
         try:
-            user_set = group_item.users
-            user_set.add(new_user)
-            group_item._set_users(user_set)
+            users = group_item.users
+            users.append(new_user)
+            group_item._set_users(users)
         except UnpopulatedPropertyError:
             # If we aren't populated, do nothing to the user list
             pass
diff --git a/tableauserverclient/server/endpoint/workbooks_endpoint.py b/tableauserverclient/server/endpoint/workbooks_endpoint.py
index 850df9f..4d72f69 100644
--- a/tableauserverclient/server/endpoint/workbooks_endpoint.py
+++ b/tableauserverclient/server/endpoint/workbooks_endpoint.py
@@ -3,6 +3,7 @@ from .exceptions import MissingRequiredFieldError
 from .fileuploads_endpoint import Fileuploads
 from .. import RequestFactory, WorkbookItem, ConnectionItem, ViewItem, PaginationItem
 from ...models.tag_item import TagItem
+from ...filesys_helpers import to_filename
 import os
 import logging
 import copy
@@ -112,7 +113,7 @@ class Workbooks(Endpoint):
 
         with closing(self.get_request(url, parameters={"stream": True})) as server_response:
             _, params = cgi.parse_header(server_response.headers['Content-Disposition'])
-            filename = os.path.basename(params['filename'])
+            filename = to_filename(os.path.basename(params['filename']))
             if filepath is None:
                 filepath = filename
             elif os.path.isdir(filepath):


Test patch:
diff --git a/test/test_datasource.py b/test/test_datasource.py
index ebf17cf..a2732db 100644
--- a/test/test_datasource.py
+++ b/test/test_datasource.py
@@ -145,6 +145,17 @@ class DatasourceTests(unittest.TestCase):
             self.assertTrue(os.path.exists(file_path))
         os.remove(file_path)
 
+    def test_download_sanitizes_name(self):
+        filename = "Name,With,Commas.tds"
+        disposition = 'name="tableau_workbook"; filename="{}"'.format(filename)
+        with requests_mock.mock() as m:
+            m.get(self.baseurl + '/1f951daf-4061-451a-9df1-69a8062664f2/content',
+                  headers={'Content-Disposition': disposition})
+            file_path = self.server.datasources.download('1f951daf-4061-451a-9df1-69a8062664f2')
+            self.assertEqual(os.path.basename(file_path), "NameWithCommas.tds")
+            self.assertTrue(os.path.exists(file_path))
+        os.remove(file_path)
+
     def test_download_extract_only(self):
         # Pretend we're 2.5 for 'extract_only'
         self.server.version = "2.5"
diff --git a/test/test_group.py b/test/test_group.py
index 2f7f227..20c4545 100644
--- a/test/test_group.py
+++ b/test/test_group.py
@@ -92,7 +92,7 @@ class GroupTests(unittest.TestCase):
             m.post(self.baseurl + '/e7833b48-c6f7-47b5-a2a7-36e7dd232758/users', text=response_xml)
             single_group = TSC.GroupItem('test')
             single_group._id = 'e7833b48-c6f7-47b5-a2a7-36e7dd232758'
-            single_group._users = set()
+            single_group._users = []
             self.server.groups.add_user(single_group, '5de011f8-5aa9-4d5b-b991-f462c8dd6bb7')
 
         self.assertEqual(1, len(single_group.users))
diff --git a/test/test_workbook.py b/test/test_workbook.py
index d276ece..0c5ecca 100644
--- a/test/test_workbook.py
+++ b/test/test_workbook.py
@@ -170,6 +170,17 @@ class WorkbookTests(unittest.TestCase):
             self.assertTrue(os.path.exists(file_path))
         os.remove(file_path)
 
+    def test_download_sanitizes_name(self):
+        filename = "Name,With,Commas.twbx"
+        disposition = 'name="tableau_workbook"; filename="{}"'.format(filename)
+        with requests_mock.mock() as m:
+            m.get(self.baseurl + '/1f951daf-4061-451a-9df1-69a8062664f2/content',
+                  headers={'Content-Disposition': disposition})
+            file_path = self.server.workbooks.download('1f951daf-4061-451a-9df1-69a8062664f2')
+            self.assertEqual(os.path.basename(file_path), "NameWithCommas.twbx")
+            self.assertTrue(os.path.exists(file_path))
+        os.remove(file_path)
+
     def test_download_extract_only(self):
         # Pretend we're 2.5 for 'extract_only'
         self.server.version = "2.5"