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.

Sign up for GitHub

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

Jump to bottom

Do not require all extras for SalesforceHook #19530

Merged
merged 5 commits into from
Nov 12, 2021
Merged

Do not require all extras for SalesforceHook #19530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0083fc3
Do not require all extras for SalesforceHook and allow extras without…
alexbegg Nov 10, 2021
2eec5d8
Using cached_property to cache the Salesforce instance
alexbegg Nov 11, 2021
5e565b5
Removing ability to allow extras without prefix for SalesforceHook
alexbegg Nov 11, 2021
bfc723a
Replacing parameterized tests with one test for default to None
alexbegg Nov 11, 2021
96c2227
Not including airflow.compat.* in a provider
alexbegg Nov 11, 2021
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
57 changes: 35 additions & 22 deletions airflow/providers/salesforce/hooks/salesforce.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@
import time
from typing import Any, Dict, Iterable, List, Optional

try:
from functools import cached_property
except ImportError:
from cached_property import cached_property

alexbegg marked this conversation as resolved.
Show resolved Hide resolved
import pandas as pd
from requests import Session
from simple_salesforce import Salesforce, api
Expand Down Expand Up @@ -75,7 +80,6 @@ def __init__(
) -> None:
super().__init__()
self.conn_id = salesforce_conn_id
self.conn = None
self.session_id = session_id
self.session = session

Expand Down Expand Up @@ -126,28 +130,37 @@ def get_ui_field_behaviour() -> Dict:
},
}

@cached_property
def conn(self) -> api.Salesforce:
"""Returns a Salesforce instance. (cached)"""
connection = self.get_connection(self.conn_id)
extras = connection.extra_dejson
# all extras below (besides the version one) are explicitly defaulted to None
# because simple-salesforce has a built-in authentication-choosing method that
# relies on which arguments are None and without "or None" setting this connection
# in the UI will result in the blank extras being empty strings instead of None,
# which would break the connection if "get" was used on its own.
conn = Salesforce(
username=connection.login,
password=connection.password,
security_token=extras.get('extra__salesforce__security_token') or None,
domain=extras.get('extra__salesforce__domain') or None,
session_id=self.session_id,
instance=extras.get('extra__salesforce__instance') or None,
instance_url=extras.get('extra__salesforce__instance_url') or None,
organizationId=extras.get('extra__salesforce__organization_id') or None,
version=extras.get('extra__salesforce__version') or api.DEFAULT_API_VERSION,
proxies=extras.get('extra__salesforce__proxies') or None,
session=self.session,
client_id=extras.get('extra__salesforce__client_id') or None,
consumer_key=extras.get('extra__salesforce__consumer_key') or None,
privatekey_file=extras.get('extra__salesforce__private_key_file_path') or None,
privatekey=extras.get('extra__salesforce__private_key') or None,
)
return conn

def get_conn(self) -> api.Salesforce:
"""Sign into Salesforce, only if we are not already signed in."""
if not self.conn:
connection = self.get_connection(self.conn_id)
extras = connection.extra_dejson
self.conn = Salesforce(
username=connection.login,
password=connection.password,
security_token=extras["extra__salesforce__security_token"] or None,
domain=extras["extra__salesforce__domain"] or None,
session_id=self.session_id,
instance=extras["extra__salesforce__instance"] or None,
instance_url=extras["extra__salesforce__instance_url"] or None,
organizationId=extras["extra__salesforce__organization_id"] or None,
version=extras["extra__salesforce__version"] or api.DEFAULT_API_VERSION,
proxies=extras["extra__salesforce__proxies"] or None,
session=self.session,
client_id=extras["extra__salesforce__client_id"] or None,
consumer_key=extras["extra__salesforce__consumer_key"] or None,
privatekey_file=extras["extra__salesforce__private_key_file_path"] or None,
privatekey=extras["extra__salesforce__private_key"] or None,
)
"""Returns a Salesforce instance. (cached)"""
return self.conn

def make_query(
Expand Down
91 changes: 56 additions & 35 deletions tests/providers/salesforce/hooks/test_salesforce.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ def test_get_conn_password_auth(self, mock_salesforce):
"""
Testing mock password authentication to Salesforce. Users should provide a username, password, and
security token in the Connection. Providing a client ID, Salesforce API version, proxy mapping, and
domain are optional. Connection params set as empty strings should be converted to `None`.
domain are optional. Connection params not provided or set as empty strings should be converted to
`None`.
"""

password_auth_conn = Connection(
Expand All @@ -64,14 +65,7 @@ def test_get_conn_password_auth(self, mock_salesforce):
extra='''
{
"extra__salesforce__client_id": "my_client",
"extra__salesforce__consumer_key": "",
"extra__salesforce__domain": "test",
"extra__salesforce__instance": "",
"extra__salesforce__instance_url": "",
"extra__salesforce__organization_id": "",
"extra__salesforce__private_key": "",
"extra__salesforce__private_key_file_path": "",
"extra__salesforce__proxies": "",
"extra__salesforce__security_token": "token",
"extra__salesforce__version": "42.0"
}
Expand Down Expand Up @@ -107,7 +101,7 @@ def test_get_conn_direct_session_access(self, mock_salesforce):
Testing mock direct session access to Salesforce. Users should provide an instance
(or instance URL) in the Connection and set a `session_id` value when calling `SalesforceHook`.
Providing a client ID, Salesforce API version, proxy mapping, and domain are optional. Connection
params set as empty strings should be converted to `None`.
params not provided or set as empty strings should be converted to `None`.
"""

direct_access_conn = Connection(
Expand All @@ -118,15 +112,8 @@ def test_get_conn_direct_session_access(self, mock_salesforce):
extra='''
{
"extra__salesforce__client_id": "my_client2",
"extra__salesforce__consumer_key": "",
"extra__salesforce__domain": "test",
"extra__salesforce__instance": "",
"extra__salesforce__instance_url": "https://my.salesforce.com",
"extra__salesforce__organization_id": "",
"extra__salesforce__private_key": "",
"extra__salesforce__private_key_file_path": "",
"extra__salesforce__proxies": "",
"extra__salesforce__security_token": "",
"extra__salesforce__version": "29.0"
}
''',
Expand Down Expand Up @@ -164,8 +151,8 @@ def test_get_conn_jwt_auth(self, mock_salesforce):
"""
Testing mock JWT bearer authentication to Salesforce. Users should provide consumer key and private
key (or path to a private key) in the Connection. Providing a client ID, Salesforce API version, proxy
mapping, and domain are optional. Connection params set as empty strings should be converted to
`None`.
mapping, and domain are optional. Connection params not provided or set as empty strings should be
converted to `None`.
"""

jwt_auth_conn = Connection(
Expand All @@ -178,13 +165,7 @@ def test_get_conn_jwt_auth(self, mock_salesforce):
"extra__salesforce__client_id": "my_client3",
"extra__salesforce__consumer_key": "consumer_key",
"extra__salesforce__domain": "login",
"extra__salesforce__instance": "",
"extra__salesforce__instance_url": "",
"extra__salesforce__organization_id": "",
"extra__salesforce__private_key": "private_key",
"extra__salesforce__private_key_file_path": "",
"extra__salesforce__proxies": "",
"extra__salesforce__security_token": "",
"extra__salesforce__version": "34.0"
}
''',
Expand Down Expand Up @@ -218,8 +199,8 @@ def test_get_conn_ip_filtering_auth(self, mock_salesforce):
"""
Testing mock IP filtering (aka allow-listing) authentication to Salesforce. Users should provide
username, password, and organization ID in the Connection. Providing a client ID, Salesforce API
version, proxy mapping, and domain are optional. Connection params set as empty strings should be
converted to `None`.
version, proxy mapping, and domain are optional. Connection params not provided or set as empty
strings should be converted to `None`.
"""

ip_filtering_auth_conn = Connection(
Expand All @@ -228,36 +209,76 @@ def test_get_conn_ip_filtering_auth(self, mock_salesforce):
login="username",
password="password",
extra='''
{
"extra__salesforce__organization_id": "my_organization"
}
''',
)
TestSalesforceHook._insert_conn_db_entry(ip_filtering_auth_conn.conn_id, ip_filtering_auth_conn)

self.salesforce_hook = SalesforceHook(salesforce_conn_id="ip_filtering_auth_conn")
self.salesforce_hook.get_conn()

extras = ip_filtering_auth_conn.extra_dejson
mock_salesforce.assert_called_once_with(
username=ip_filtering_auth_conn.login,
password=ip_filtering_auth_conn.password,
security_token=None,
domain=None,
session_id=None,
instance=None,
instance_url=None,
organizationId=extras["extra__salesforce__organization_id"],
version=api.DEFAULT_API_VERSION,
proxies=None,
session=None,
client_id=None,
consumer_key=None,
privatekey_file=None,
privatekey=None,
)

@patch("airflow.providers.salesforce.hooks.salesforce.Salesforce")
def test_get_conn_default_to_none(self, mock_salesforce):
"""
Testing mock authentication to Salesforce so that every extra connection param set as an empty
string will be converted to `None`.
"""

default_to_none_conn = Connection(
conn_id="default_to_none_conn",
conn_type="salesforce",
login=None,
password=None,
extra='''
{
"extra__salesforce__client_id": "",
"extra__salesforce__consumer_key": "",
"extra__salesforce__domain": "",
"extra__salesforce__instance": "",
"extra__salesforce__instance_url": "",
"extra__salesforce__organization_id": "my_organization",
"extra__salesforce__organization_id": "",
"extra__salesforce__private_key": "",
"extra__salesforce__private_key_file_path": "",
"extra__salesforce__proxies": "",
"extra__salesforce__security_token": "",
"extra__salesforce__version": ""
"extra__salesforce__security_token": ""
}
''',
)
TestSalesforceHook._insert_conn_db_entry(ip_filtering_auth_conn.conn_id, ip_filtering_auth_conn)
TestSalesforceHook._insert_conn_db_entry(default_to_none_conn.conn_id, default_to_none_conn)

self.salesforce_hook = SalesforceHook(salesforce_conn_id="ip_filtering_auth_conn")
self.salesforce_hook = SalesforceHook(salesforce_conn_id="default_to_none_conn")
self.salesforce_hook.get_conn()

extras = ip_filtering_auth_conn.extra_dejson
mock_salesforce.assert_called_once_with(
username=ip_filtering_auth_conn.login,
password=ip_filtering_auth_conn.password,
username=default_to_none_conn.login,
password=default_to_none_conn.password,
security_token=None,
domain=None,
session_id=None,
instance=None,
instance_url=None,
organizationId=extras["extra__salesforce__organization_id"],
organizationId=None,
version=api.DEFAULT_API_VERSION,
proxies=None,
session=None,
Expand Down