[CIVIS-9192] ENH clean up database credential IDs at civis.APIClient (

#502)

* DEP get_database_credential_id

* MAINT update changelog

* EMH add default_database_credential_id and deprecate default_credential

* MAINT update changelog

CHANGELOG.md

@@ -18,12 +18,20 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - `Response` objects are now fully typed through the attribute syntax. (#501)
 - Both `Response` and `PaginatedResponse` are now directly available under the `civis` namespace. (#501)
 - Added support for Python 3.13. (#501)
+- Added the new property `default_database_credential_id` at `civis.APIClient`,
+  which is going to replace the existing `default_credential`. (#502)
 
 ### Changed
 - When a `PaginatedResponse` object is returned from an API call,
   a user-specified `limit` kwarg is now honored to facilitate speeding up the pagination. (#501)
 
 ### Deprecated
+- The method `get_database_credential_id` at `civis.APIClient` has been deprecated
+  and will be removed at civis-python v3.0.0. There's no replacement for this method. (#502)
+- The property `default_credential` at `civis.APIClient` has been deprecated
+  and will be removed at civis-python v3.0.0,
+  in favor of the new property `default_database_credential_id`. (#502)
+
 ### Removed
 - Dropped support for Python 3.9. (#499)
 

docs/source/user_guide.rst

@@ -146,7 +146,7 @@ the current user.
 .. code:: python
 
    >>> db_id = client.get_database_id('cluster-name')
-   >>> cred_id = client.default_credential
+   >>> cred_id = client.default_database_credential_id
 
 In order to export a table, we need to write some SQL that will generate the
 data to export. Then we create the export job and run it.

src/civis/client.py

@@ -3,6 +3,7 @@
 from functools import lru_cache
 import logging
 import textwrap
+import warnings
 from typing import TYPE_CHECKING
 
 import civis
@@ -168,6 +169,18 @@ def get_database_credential_id(self, username, database_name):
         >>> client.get_database_credential_id(1111, 'redshift-general')
         1111
         """
+        warnings.warn(
+            "The method `get_database_credential_id` is deprecated and will be removed "
+            "at civis-python v3.0.0. Its continued usage is strongly discouraged. "
+            "Given the way Civis Platform has evolved over the years, "
+            "there's currently no reliable way to get a database credential ID "
+            "from a username and database name. No replacement for this method is "
+            "being planned. If you need to programmatically access a database "
+            "credential ID that is or may likely be the default credential, "
+            "consider the property `default_database_credential_id`.",
+            FutureWarning,
+            stacklevel=2,  # Point to the user code that calls this method.
+        )
         if isinstance(username, int):
             return username
         else:
@@ -339,10 +352,25 @@ def get_storage_host_id(self, storage_host):
     @lru_cache(maxsize=128)
     def default_credential(self):
         """The current user's default credential."""
-        # NOTE: this should be optional to endpoints...so this could go away
+        warnings.warn(
+            "The property `default_credential` is deprecated and will be removed "
+            "at civis-python v3.0.0. "
+            "Please use `default_database_credential_id` instead.",
+            FutureWarning,
+            stacklevel=2,  # Point to the user code that calls this method.
+        )
         creds = self.credentials.list(default=True)
         return creds[0]["id"] if len(creds) > 0 else None
 
+    @property
+    @lru_cache(maxsize=128)
+    def default_database_credential_id(self):
+        """The current user's default database credential ID."""
+        creds = self.credentials.list(
+            default=True, type="Database", remote_host_id=None
+        )
+        return creds[0]["id"] if len(creds) > 0 else None
+
     @property
     @lru_cache(maxsize=128)
     def username(self):

src/civis/futures.py

@@ -51,7 +51,7 @@ class CivisFuture(PollableResult):
     >>> import civis
     >>> client = civis.APIClient()
     >>> database_id = client.get_database_id("my_database")
-    >>> cred_id = client.default_credential
+    >>> cred_id = client.default_database_credential_id
     >>> sql = "SELECT 1"
     >>> preview_rows = 10
     >>> response = client.queries.post(database_id, sql, preview_rows,

src/civis/io/_databases.py

@@ -56,7 +56,7 @@ def query_civis(
     if client is None:
         client = APIClient()
     database_id = client.get_database_id(database)
-    cred_id = credential_id or client.default_credential
+    cred_id = credential_id or client.default_database_credential_id
     resp = client.queries.post(
         database_id, sql, preview_rows, credential=cred_id, hidden=hidden
     )
@@ -127,8 +127,8 @@ def transfer_table(
     """
     if client is None:
         client = APIClient()
-    source_cred_id = source_credential_id or client.default_credential
-    dest_cred_id = dest_credential_id or client.default_credential
+    source_cred_id = source_credential_id or client.default_database_credential_id
+    dest_cred_id = dest_credential_id or client.default_database_credential_id
     job_name = maybe_get_random_name(job_name)
     source = {
         "remote_host_id": client.get_database_id(source_db),

src/civis/io/_tables.py

@@ -346,7 +346,7 @@ def read_civis_sql(
         raise ImportError("use_pandas is True but pandas is not installed.")
 
     db_id = client.get_database_id(database)
-    credential_id = credential_id or client.default_credential
+    credential_id = credential_id or client.default_database_credential_id
 
     script_id, run_id = _sql_script(
         client,
@@ -485,7 +485,7 @@ def civis_to_csv(
         client = APIClient()
 
     db_id = client.get_database_id(database)
-    credential_id = credential_id or client.default_credential
+    credential_id = credential_id or client.default_database_credential_id
 
     # don't fix bug that would cause breaking change for now
     # when gzip compression is requested, a gzip file is not actually returned
@@ -1150,7 +1150,7 @@ def civis_file_to_table(
     if schema is None:
         raise ValueError("Provide a schema as part of the `table` input.")
     db_id = client.get_database_id(database)
-    cred_id = credential_id or client.default_credential
+    cred_id = credential_id or client.default_database_credential_id
     if delimiter is not None:  # i.e. it was provided as an argument
         delimiter = DELIMITERS.get(delimiter)
         if not delimiter:
@@ -1271,7 +1271,7 @@ def _sql_script(
 ):
     job_name = maybe_get_random_name(job_name)
     db_id = client.get_database_id(database)
-    credential_id = credential_id or client.default_credential
+    credential_id = credential_id or client.default_database_credential_id
     csv_settings = csv_settings or {}
     sql_params_arguments = sql_params_arguments or {}
 

src/civis/polling.py

@@ -71,7 +71,7 @@ class PollableResult(CivisAsyncResultBase):
     --------
     >>> client = civis.APIClient()
     >>> database_id = client.get_database_id("my_database")
-    >>> cred_id = client.default_credential
+    >>> cred_id = client.default_database_credential_id
     >>> sql = "SELECT 1"
     >>> preview_rows = 10
     >>> response = client.queries.post(database_id, sql, preview_rows,

src/civis/resources/_client_pyi.py

@@ -130,6 +130,7 @@ def generate_client_pyi(client_pyi_path, api_spec_path):
 # before we can define APIClient to use them.
 class APIClient:
     default_credential: int | None
+    default_database_credential_id: int | None
     username: str
     feature_flags: tuple[str]
     last_response: Any

src/civis/tests/mocks.py

@@ -1,5 +1,6 @@
 """Mock client creation and tooling"""
 
+import warnings
 from functools import lru_cache
 from unittest import mock
 
@@ -30,7 +31,9 @@ def create_client_mock(cache=None):
     real_client = _real_client(cache)
 
     # Prevent the client from trying to talk to the real API when autospeccing
-    with mock.patch("requests.Session", mock.MagicMock):
+    with mock.patch("requests.Session", mock.MagicMock), warnings.catch_warnings():
+        # Ignore deprecation warning from `client.default_credential`.
+        warnings.simplefilter("ignore", FutureWarning)
         mock_client = mock.create_autospec(real_client, spec_set=True)
 
     return mock_client

tests/test_io.py

@@ -199,7 +199,7 @@ def test_civis_file_to_table_table_exists(m_run_cleaning, m_process_cleaning_res
 
     mock_civis.imports.post_files_csv.return_value.id = mock_import_id
     mock_civis.get_database_id.return_value = 42
-    mock_civis.default_credential = 713
+    mock_civis.default_database_credential_id = 713
 
     mock_civis.databases.get_schemas_tables.return_value = Response({"name": "table1"})
     m_process_cleaning_results.return_value = (
@@ -288,7 +288,7 @@ def test_civis_file_to_table_table_doesnt_exist(
 
     mock_civis.imports.post_files_csv.return_value.id = mock_import_id
     mock_civis.get_database_id.return_value = 42
-    mock_civis.default_credential = 713
+    mock_civis.default_database_credential_id = 713
 
     mock_civis.databases.get_schemas_tables.side_effect = MockAPIError(404)
     mock_columns = [{"name": "foo", "sql_type": "INTEGER"}]
@@ -378,7 +378,7 @@ def test_civis_file_to_table_table_doesnt_exist_all_sql_types_missing(
 
     mock_civis.imports.post_files_csv.return_value.id = mock_import_id
     mock_civis.get_database_id.return_value = 42
-    mock_civis.default_credential = 713
+    mock_civis.default_database_credential_id = 713
     mock_civis.databases.get_schemas_tables.side_effect = MockAPIError(404)
     table_columns = [{"name": "a", "sql_type": ""}, {"name": "b", "sql_type": ""}]
     detected_columns = [
@@ -471,7 +471,7 @@ def test_civis_file_to_table_table_does_not_exist_some_sql_types_missing(
 
     mock_civis.imports.post_files_csv.return_value.id = mock_import_id
     mock_civis.get_database_id.return_value = 42
-    mock_civis.default_credential = 713
+    mock_civis.default_database_credential_id = 713
     mock_civis.databases.get_schemas_tables.side_effect = MockAPIError(404)
     table_columns = [{"name": "a", "sql_type": "INT"}, {"name": "b", "sql_type": ""}]
 
@@ -503,7 +503,7 @@ def test_civis_file_to_table_table_columns_keys_misspelled(
 
     mock_civis.imports.post_files_csv.return_value.id = mock_import_id
     mock_civis.get_database_id.return_value = 42
-    mock_civis.default_credential = 713
+    mock_civis.default_database_credential_id = 713
     mock_civis.databases.get_schemas_tables.side_effect = MockAPIError(404)
     table_columns = [{"name": "a", "sqlType": "INT"}, {"name": "b", "bad_type": ""}]
 
@@ -541,7 +541,7 @@ def run_subtest(mock_file_ids):
 
         mock_civis.imports.post_files_csv.return_value.id = mock_import_id
         mock_civis.get_database_id.return_value = 42
-        mock_civis.default_credential = 713
+        mock_civis.default_database_credential_id = 713
         mock_civis.databases.get_schemas_tables.side_effect = MockAPIError(404)
         table_columns = [
             {"name": "foo", "sql_type": "INTEGER"},
@@ -645,7 +645,7 @@ def test_civis_file_to_table_multi_file(m_run_cleaning, m_process_cleaning_resul
 
     mock_civis.imports.post_files_csv.return_value.id = mock_import_id
     mock_civis.get_database_id.return_value = 42
-    mock_civis.default_credential = 713
+    mock_civis.default_database_credential_id = 713
 
     mock_civis.databases.get_schemas_tables.side_effect = MockAPIError(404)
     mock_columns = [{"name": "foo", "sql_type": "INTEGER"}]
@@ -1449,7 +1449,7 @@ def test_sql_script():
     mock_client = create_client_mock()
     mock_client.scripts.post_sql.return_value = response
     mock_client.get_database_id.return_value = database_id
-    mock_client.default_credential = credential_id
+    mock_client.default_database_credential_id = credential_id
 
     civis.io._tables._sql_script(
         client=mock_client,