Repository: CartoDB/cartoframes
Pull request #1726 (2021-03-12 15:11:02)

Avoid renaming geometry if geometry name is already GEOM_COLUMN_NAME
### Context

In this small PR from Support we aim to fix an undesired behavior when using the `rename_geometry` method from the `to_carto` function. 

Since the latest additions to GeoPandas, if the column name to be used as input for the referred function is already present in the GeoDataFrame columns, an error would be raised, which seems to break data uploads when the geometry column is named `the_geom`.

Further details can be found in [this CH story](https://app.clubhouse.io/cartoteam/story/141147/cartoframes-geopandas-rename-geometry-raises-error-in-to-carto-if-geometry-name-is-the-geom).

### PR changes
io/carto.py: Avoid using the `rename_geometry` method if the GeoDataFrame's geometry column is named as *GEOM_COLUMN_NAME* (the_geom)

### Result with PR changes

```python
>>> import cartoframes
>>> cartoframes.auth.set_default_credentials(username='mmoncada', api_key='master_api_key')
>>> gdf = cartoframes.read_carto('cod')
>>> gdf.head()
   cartodb_id                                           the_geom  fid  cod_postal
0          18  MULTIPOLYGON (((-2.21981 43.03500, -2.21978 43...    5     20213.0
1          23  MULTIPOLYGON (((-3.05027 37.78892, -3.04790 37...    7     23487.0
2          32  MULTIPOLYGON (((-3.50303 40.52600, -3.50304 40...   10     28864.0
3          39  MULTIPOLYGON (((-2.54156 38.20036, -2.52919 38...   12     23297.0
4          43  MULTIPOLYGON (((-5.95369 37.99535, -5.95369 37...   13     41250.0
>>> cartoframes.to_carto(gdf, 'cod_test_2')
Success! Data uploaded to table "cod_test_2" correctly
'cod_test_2'
```



Patch:
diff --git a/cartoframes/data/observatory/catalog/repository/dataset_repo.py b/cartoframes/data/observatory/catalog/repository/dataset_repo.py
index cc9f906a..4bd26ade 100644
--- a/cartoframes/data/observatory/catalog/repository/dataset_repo.py
+++ b/cartoframes/data/observatory/catalog/repository/dataset_repo.py
@@ -25,7 +25,6 @@ class DatasetRepository(EntityRepository):
 
         # Using user credentials to fetch entities
         self.client.set_user_credentials(credentials)
-        filters = self.client.get_entities_filters(filters)
         entities = self._get_filtered_entities(filters)
         self.client.reset_user_credentials()
         return entities
diff --git a/cartoframes/data/observatory/catalog/repository/geography_repo.py b/cartoframes/data/observatory/catalog/repository/geography_repo.py
index 19d8f652..d23fc922 100644
--- a/cartoframes/data/observatory/catalog/repository/geography_repo.py
+++ b/cartoframes/data/observatory/catalog/repository/geography_repo.py
@@ -28,7 +28,6 @@ class GeographyRepository(EntityRepository):
 
         # Using user credentials to fetch entities
         self.client.set_user_credentials(credentials)
-        filters = self.client.get_entities_filters(filters)
         entities = self._get_filtered_entities(filters)
         self.client.reset_user_credentials()
         return entities
diff --git a/cartoframes/data/observatory/catalog/repository/repo_client.py b/cartoframes/data/observatory/catalog/repository/repo_client.py
index 6ef42d99..b828b4c9 100644
--- a/cartoframes/data/observatory/catalog/repository/repo_client.py
+++ b/cartoframes/data/observatory/catalog/repository/repo_client.py
@@ -73,13 +73,6 @@ class RepoClient:
             entity = 'datasets/{0}/variables_groups'.format(filters.pop('dataset'))
             return self._fetch_entity(entity, filters)
 
-    def get_entities_filters(self, filters=None):
-        self.set_external_credentials()
-        if filters is not None:
-            filters['filter_catalog'] = False
-        entities = self._fetch_entity('entities', filters)
-        return {'id': [result['id'] for result in entities['results']]}
-
     def _get_filter_id(self, filters, use_slug=False):
         if isinstance(filters, dict):
             filter_id = filters.get('id')
diff --git a/cartoframes/io/carto.py b/cartoframes/io/carto.py
index 51e51092..6a42101c 100644
--- a/cartoframes/io/carto.py
+++ b/cartoframes/io/carto.py
@@ -156,7 +156,8 @@ def to_carto(dataframe, table_name, credentials=None, if_exists='fail', geom_col
             gdf.drop(columns=[GEOM_COLUMN_NAME], inplace=True)
 
         # Prepare geometry column for the upload
-        gdf.rename_geometry(GEOM_COLUMN_NAME, inplace=True)
+        if dataframe.geometry.name != GEOM_COLUMN_NAME:
+            gdf.rename_geometry(GEOM_COLUMN_NAME, inplace=True)
 
     elif isinstance(dataframe, GeoDataFrame):
         log.warning('Geometry column not found in the GeoDataFrame.')


Test patch:
diff --git a/tests/unit/data/observatory/catalog/repository/test_dataset_repo.py b/tests/unit/data/observatory/catalog/repository/test_dataset_repo.py
index cc92d742..20471a21 100644
--- a/tests/unit/data/observatory/catalog/repository/test_dataset_repo.py
+++ b/tests/unit/data/observatory/catalog/repository/test_dataset_repo.py
@@ -18,11 +18,9 @@ from ..examples import test_dataset1, test_datasets, db_dataset1, db_dataset2
 class TestDatasetRepo(object):
 
     @patch.object(RepoClient, 'get_datasets')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_all(self, mocked_filters, mocked_repo):
+    def test_get_all(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_dataset1, db_dataset2]
-        mocked_filters.side_effect = lambda f: f
         repo = DatasetRepository()
 
         # When
@@ -34,12 +32,10 @@ class TestDatasetRepo(object):
         assert datasets == test_datasets
 
     @patch.object(RepoClient, 'get_datasets')
-    @patch.object(RepoClient, 'get_entities_filters')
     @patch('cartoframes.data.observatory.catalog.repository.entity_repo.get_subscription_ids')
-    def test_get_all_credentials(self, mocked_get_subscription_ids, mocked_filters, mocked_get_datasets):
+    def test_get_all_credentials(self, mocked_get_subscription_ids, mocked_get_datasets):
         # Given
         mocked_get_subscription_ids.return_value = [db_dataset1['id'], db_dataset2['id']]
-        mocked_filters.side_effect = lambda f: f
         mocked_get_datasets.return_value = [db_dataset1, db_dataset2]
         credentials = Credentials('user', '1234')
         repo = DatasetRepository()
@@ -53,11 +49,9 @@ class TestDatasetRepo(object):
         assert datasets == test_datasets
 
     @patch.object(RepoClient, 'get_datasets')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_all_when_empty(self, mocked_filters, mocked_repo):
+    def test_get_all_when_empty(self, mocked_repo):
         # Given
         mocked_repo.return_value = []
-        mocked_filters.side_effect = lambda f: f
         repo = DatasetRepository()
 
         # When
@@ -68,11 +62,9 @@ class TestDatasetRepo(object):
         assert datasets == []
 
     @patch.object(RepoClient, 'get_datasets')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_all_only_uses_allowed_filters(self, mocked_filters, mocked_repo):
+    def test_get_all_only_uses_allowed_filters(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_dataset1, db_dataset2]
-        mocked_filters.side_effect = lambda f: f
         repo = DatasetRepository()
         filters = {
             COUNTRY_FILTER: 'usa',
@@ -98,11 +90,9 @@ class TestDatasetRepo(object):
         assert datasets == test_datasets
 
     @patch.object(RepoClient, 'get_datasets')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_by_id(self, mocked_filters, mocked_repo):
+    def test_get_by_id(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_dataset1]
-        mocked_filters.side_effect = lambda f: f
         requested_id = db_dataset1['id']
         repo = DatasetRepository()
 
@@ -114,11 +104,9 @@ class TestDatasetRepo(object):
         assert dataset == test_dataset1
 
     @patch.object(RepoClient, 'get_datasets')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_by_id_unknown_fails(self, mocked_filters, mocked_repo):
+    def test_get_by_id_unknown_fails(self, mocked_repo):
         # Given
         mocked_repo.return_value = []
-        mocked_filters.side_effect = lambda f: f
         requested_id = 'unknown_id'
         repo = DatasetRepository()
 
@@ -127,11 +115,9 @@ class TestDatasetRepo(object):
             repo.get_by_id(requested_id)
 
     @patch.object(RepoClient, 'get_datasets')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_by_slug(self, mocked_filters, mocked_repo):
+    def test_get_by_slug(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_dataset1]
-        mocked_filters.side_effect = lambda f: f
         requested_slug = db_dataset1['slug']
         repo = DatasetRepository()
 
@@ -143,11 +129,9 @@ class TestDatasetRepo(object):
         assert dataset == test_dataset1
 
     @patch.object(RepoClient, 'get_datasets')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_by_id_list(self, mocked_filters, mocked_repo):
+    def test_get_by_id_list(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_dataset1, db_dataset2]
-        mocked_filters.side_effect = lambda f: f
         repo = DatasetRepository()
 
         # When
@@ -159,11 +143,9 @@ class TestDatasetRepo(object):
         assert datasets == test_datasets
 
     @patch.object(RepoClient, 'get_datasets')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_by_slug_list(self, mocked_filters, mocked_repo):
+    def test_get_by_slug_list(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_dataset1, db_dataset2]
-        mocked_filters.side_effect = lambda f: f
         repo = DatasetRepository()
 
         # When
@@ -175,11 +157,9 @@ class TestDatasetRepo(object):
         assert datasets == test_datasets
 
     @patch.object(RepoClient, 'get_datasets')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_by_slug_and_id_list(self, mocked_filters, mocked_repo):
+    def test_get_by_slug_and_id_list(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_dataset1, db_dataset2]
-        mocked_filters.side_effect = lambda f: f
         repo = DatasetRepository()
 
         # When
@@ -191,11 +171,9 @@ class TestDatasetRepo(object):
         assert datasets == test_datasets
 
     @patch.object(RepoClient, 'get_datasets')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_missing_fields_are_mapped_as_None(self, mocked_filters, mocked_repo):
+    def test_missing_fields_are_mapped_as_None(self, mocked_repo):
         # Given
         mocked_repo.return_value = [{'id': 'dataset1'}]
-        mocked_filters.side_effect = lambda f: f
         repo = DatasetRepository()
 
         expected_datasets = CatalogList([Dataset({
diff --git a/tests/unit/data/observatory/catalog/repository/test_geography_repo.py b/tests/unit/data/observatory/catalog/repository/test_geography_repo.py
index 2a4efc61..5a1ed001 100644
--- a/tests/unit/data/observatory/catalog/repository/test_geography_repo.py
+++ b/tests/unit/data/observatory/catalog/repository/test_geography_repo.py
@@ -18,11 +18,9 @@ from ..examples import test_geography1, test_geographies, db_geography1, db_geog
 class TestGeographyRepo(object):
 
     @patch.object(RepoClient, 'get_geographies')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_all(self, mocked_filters, mocked_repo):
+    def test_get_all(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_geography1, db_geography2]
-        mocked_filters.side_effect = lambda f: f
         repo = GeographyRepository()
 
         # When
@@ -34,12 +32,10 @@ class TestGeographyRepo(object):
         assert geographies == test_geographies
 
     @patch.object(RepoClient, 'get_geographies')
-    @patch.object(RepoClient, 'get_entities_filters')
     @patch('cartoframes.data.observatory.catalog.repository.entity_repo.get_subscription_ids')
-    def test_get_all_credentials(self, mocked_get_subscription_ids, mocked_filters, mocked_get_geographies):
+    def test_get_all_credentials(self, mocked_get_subscription_ids, mocked_get_geographies):
         # Given
         mocked_get_subscription_ids.return_value = [db_geography1['id'], db_geography2['id']]
-        mocked_filters.side_effect = lambda f: f
         mocked_get_geographies.return_value = [db_geography1, db_geography2]
         credentials = Credentials('user', '1234')
         repo = GeographyRepository()
@@ -53,11 +49,9 @@ class TestGeographyRepo(object):
         assert geographies == test_geographies
 
     @patch.object(RepoClient, 'get_geographies')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_all_when_empty(self, mocked_filters, mocked_repo):
+    def test_get_all_when_empty(self, mocked_repo):
         # Given
         mocked_repo.return_value = []
-        mocked_filters.side_effect = lambda f: f
         repo = GeographyRepository()
 
         # When
@@ -68,11 +62,9 @@ class TestGeographyRepo(object):
         assert geographies == []
 
     @patch.object(RepoClient, 'get_geographies')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_all_only_uses_allowed_filters(self, mocked_filters, mocked_repo):
+    def test_get_all_only_uses_allowed_filters(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_geography1, db_geography2]
-        mocked_filters.side_effect = lambda f: f
         repo = GeographyRepository()
         filters = {
             COUNTRY_FILTER: 'usa',
@@ -97,11 +89,9 @@ class TestGeographyRepo(object):
         assert geographies == test_geographies
 
     @patch.object(RepoClient, 'get_geographies')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_by_id(self, mocked_filters, mocked_repo):
+    def test_get_by_id(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_geography1]
-        mocked_filters.side_effect = lambda f: f
         requested_id = db_geography1['id']
         repo = GeographyRepository()
 
@@ -114,11 +104,9 @@ class TestGeographyRepo(object):
         assert geography == test_geography1
 
     @patch.object(RepoClient, 'get_geographies')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_by_id_unknown_fails(self, mocked_filters, mocked_repo):
+    def test_get_by_id_unknown_fails(self, mocked_repo):
         # Given
         mocked_repo.return_value = []
-        mocked_filters.side_effect = lambda f: f
         requested_id = 'unknown_id'
         repo = GeographyRepository()
 
@@ -127,11 +115,9 @@ class TestGeographyRepo(object):
             repo.get_by_id(requested_id)
 
     @patch.object(RepoClient, 'get_geographies')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_by_slug(self, mocked_filters, mocked_repo):
+    def test_get_by_slug(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_geography1]
-        mocked_filters.side_effect = lambda f: f
         requested_slug = db_geography1['slug']
         repo = GeographyRepository()
 
@@ -143,11 +129,9 @@ class TestGeographyRepo(object):
         assert geography == test_geography1
 
     @patch.object(RepoClient, 'get_geographies')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_by_id_list(self, mocked_filters, mocked_repo):
+    def test_get_by_id_list(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_geography1, db_geography2]
-        mocked_filters.side_effect = lambda f: f
         repo = GeographyRepository()
 
         # When
@@ -159,11 +143,9 @@ class TestGeographyRepo(object):
         assert geographies == test_geographies
 
     @patch.object(RepoClient, 'get_geographies')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_by_slug_list(self, mocked_filters, mocked_repo):
+    def test_get_by_slug_list(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_geography1, db_geography2]
-        mocked_filters.side_effect = lambda f: f
         repo = GeographyRepository()
 
         # When
@@ -175,11 +157,9 @@ class TestGeographyRepo(object):
         assert geographies == test_geographies
 
     @patch.object(RepoClient, 'get_geographies')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_by_slug_and_id_list(self, mocked_filters, mocked_repo):
+    def test_get_by_slug_and_id_list(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_geography1, db_geography2]
-        mocked_filters.side_effect = lambda f: f
         repo = GeographyRepository()
 
         # When
@@ -191,11 +171,9 @@ class TestGeographyRepo(object):
         assert geographies == test_geographies
 
     @patch.object(RepoClient, 'get_geographies')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_get_all_with_join_filters(self, mocked_filters, mocked_repo):
+    def test_get_all_with_join_filters(self, mocked_repo):
         # Given
         mocked_repo.return_value = [db_geography1, db_geography2]
-        mocked_filters.side_effect = lambda f: f
         repo = GeographyRepository()
 
         # When
@@ -207,11 +185,9 @@ class TestGeographyRepo(object):
         assert geographies == test_geographies
 
     @patch.object(RepoClient, 'get_geographies')
-    @patch.object(RepoClient, 'get_entities_filters')
-    def test_missing_fields_are_mapped_as_None(self, mocked_filters, mocked_repo):
+    def test_missing_fields_are_mapped_as_None(self, mocked_repo):
         # Given
         mocked_repo.return_value = [{'id': 'geography1'}]
-        mocked_filters.side_effect = lambda f: f
         repo = GeographyRepository()
 
         expected_geographies = CatalogList([Geography({