Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Airtable connector #631

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pandasai/connectors/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from .snowflake import SnowFlakeConnector
from .databricks import DatabricksConnector
from .yahoo_finance import YahooFinanceConnector
from .airtable import AirtableConnector

__all__ = [
"BaseConnector",
Expand All @@ -18,4 +19,5 @@
"YahooFinanceConnector",
"SnowFlakeConnector",
"DatabricksConnector",
"AirtableConnector"
]
74 changes: 74 additions & 0 deletions pandasai/connectors/airtable.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
"""
Airtable connectors are used to connect airtable records.
"""

from .base import AirtableConnectorConfig , BaseConnector
from typing import Union ,Optional
import os
import requests
import pandas as pd

class AirtableConnector(BaseConnector):
"""
Airtable connector to retrieving record data.
"""
def __init__(
self,
baseid : Optional[str] = None,
bearer_token : Optional[str] = None,
table_name : Optional[str] = None,
config : Union[AirtableConnectorConfig,dict] = None
):
Comment on lines +15 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor of AirtableConnector accepts both individual parameters and a config object. This could lead to confusion and potential bugs if both are provided but with conflicting values. Consider simplifying the constructor to accept only a config object. This would make the function signature simpler and more maintainable.

if not bearer_token and not config:
raise ValueError(
"You must specify bearer token for authentication or a config object."
)
if not baseid and not config.baseID:
raise ValueError(
"You must specify baseId or a proper config object."
)
Comment on lines +22 to +29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error messages here are not very informative. It would be better to specify which parameter is missing in the error message. This would make debugging easier.

if not isinstance(config,AirtableConnectorConfig):
if not config :
config = {}
if table_name :
config['table'] = table_name

airtable_config = AirtableConnectorConfig(**config)
else :
airtable_config = config
Comment on lines +30 to +38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handling of the config parameter is a bit convoluted. It would be clearer to handle the config parameter separately depending on its type. This would make the code easier to read and maintain.


self._session = requests.Session()

self._session.headers = {
"Authorization" : f"Bearer {airtable_config['token']}"
}

self._response : str = None

super().__init__(airtable_config)

def _connect_load(self,config:AirtableConnectorConfig):
"""
Authenticate and connect to the instance

Args :
config (AirtableConnectorConfig) :Configuration to load table data.
"""
_response = self._session.get(f"https://api.airtable.com/v0/{config['baseID']}/{config['table']}")
if _response.status_code == 200 :
self._response = pd.read_json(_response.json())
else :
raise ValueError(
"Please verify authentication details , baseid or table_name are correct ."
)
Comment on lines +57 to +63
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling here could be improved. If the status code is not 200, it would be helpful to include the status code and the response message in the error. This would make debugging easier.



def head(self):
"""
Return the head of the table that the connector is connected to.

Returns :
DatFrameType: The head of the data source that the conector is connected to .
"""
return self._response.head()

Comment on lines +66 to +74
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The head method assumes that _response is not None and that it is a DataFrame. It would be safer to check that _response is not None and that it is a DataFrame before calling head on it. This would prevent runtime errors.

Here are the suggested changes:

    def __init__(
            self,
-            baseid : Optional[str] = None,
-            bearer_token : Optional[str] = None,
-            table_name : Optional[str] = None,
            config : Union[AirtableConnectorConfig,dict] = None
    ):
-        if not bearer_token and not config:
-            raise ValueError(
-                "You must specify bearer token for authentication or a config object."
-            )
-        if not baseid and not config.baseID:
-            raise ValueError(
-                "You must specify baseId or a proper config object."
-            )
-        if not isinstance(config,AirtableConnectorConfig):
-            if not config :
-                config = {}
-            if table_name :
-                config['table'] = table_name
-            
-            airtable_config = AirtableConnectorConfig(**config)
-        else :
-            airtable_config = config
+        if isinstance(config, dict):
+            airtable_config = AirtableConnectorConfig(**config)
+        elif isinstance(config, AirtableConnectorConfig):
+            airtable_config = config
+        else:
+            raise ValueError("Invalid config parameter. Expected a dict or an AirtableConnectorConfig instance.")

        _response = self._session.get(f"https://api.airtable.com/v0/{config['baseID']}/{config['table']}")
        if _response.status_code == 200 :
            self._response = pd.read_json(_response.json())
        else :
-            raise ValueError(
-                "Please verify authentication details , baseid or table_name are correct ."
-            )
+            raise ValueError(f"Failed to connect to Airtable. Status code: {_response.status_code}, message: {_response.text}")

    def head(self):
        """
        Return the head of the table that the connector is connected to.

        Returns :
            DatFrameType: The head of the data source that the conector is connected to . 
        """
-        return self._response.head()
+        if self._response is not None and isinstance(self._response, pd.DataFrame):
+            return self._response.head()
+        else:
+            raise ValueError("No data loaded. Please call _connect_load first.")

6 changes: 6 additions & 0 deletions pandasai/connectors/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ class DatabricksConnectorConfig(SQLBaseConnectorConfig):
token: str
httpPath: str

class AirtableConnectorConfig(BaseConnectorConfig):
"""
Connector configuration for Airtable.
"""
token : str
baseID : str

class BaseConnector(ABC):
"""
Expand Down
42 changes: 42 additions & 0 deletions tests/connectors/test_airtable.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import unittest
import pandas
from unittest.mock import Mock, patch
from pandasai.connectors.base import AirtableConnectorConfig
from pandasai.connectors import AirtableConnector
import requests

class TestAirTableConnector(unittest.TestCase):
def setUp(self):

# Define your ConnectorConfig instance here
self.config = AirtableConnectorConfig(
token="your_token",
baseID="your_baseid",
table="your_table_name"
).dict()

#Create an instance of Connector
self.connector = AirtableConnector(config=self.config)

@patch("requests.Session")
def test_constructor_and_properties(self,mock_request_session):
# Test constructor and properties
self.assertEqual(self.connector._config,self.config)
self.assertEqual(self.connector._session,mock_request_session)
self.assertEqual(self.connector._session.headers, {
"Authorization" : f"Bearer {self.config['token']}"
})

@patch("pandasai.connectors.AirtableConnector._connect_load")
@patch("pandas.DataFrame")
def test_connect_load(self,_connect_load,mock_dataframe):
response = _connect_load(self.config)
self.assertEqual(type(self.connector._response),mock_dataframe())

Comment on lines +30 to +35
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case checks the _connect_load method of the AirtableConnector class. It verifies that the _response attribute is an instance of pandas.DataFrame. However, the _connect_load method is patched but not used in the test. Instead, the self.config is passed directly to _connect_load which is incorrect. The _connect_load method should be called on the self.connector instance. Here is the corrected code:

-        response = _connect_load(self.config)
+        response = self.connector._connect_load()

@patch("pandasai.connectors.AirtableConnector.head")
@patch("pandas.DataFrame")
def test_head(self,mock_head,mock_dataframe):
response = mock_head()
self.assertEqual(type(response),mock_dataframe())
Comment on lines +36 to +40
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case checks the head method of the AirtableConnector class. It verifies that the returned response is an instance of pandas.DataFrame. However, similar to the previous test case, the mock_head method is patched but not used in the test. Instead, the head method should be called on the self.connector instance. Here is the corrected code:

-        response = mock_head()
+        response = self.connector.head()



Loading