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

Tdl 24072 fix typecasting bug max api limit #84

Merged
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Changelog

## 2.5.1
* Fixed Validation Error for `max_daily_calls` [#84](https://github.com/singer-io/tap-marketo/pull/84)

## 2.5.0
* Add campaignId field to activities streams [#82](https://github.com/singer-io/tap-marketo/pull/82)

Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from setuptools import setup

setup(name='tap-marketo',
version='2.5.0',
version='2.5.1',
description='Singer.io tap for extracting data from the Marketo API',
author='Stitch',
url='http://singer.io',
Expand Down
12 changes: 9 additions & 3 deletions tap_marketo/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
# Marketo limits REST requests to 50000 per day with a rate limit of 100
# calls per 20 seconds.
# http://developers.marketo.com/rest-api/
MAX_DAILY_CALLS = int(50000 * 0.8)
MAX_DAILY_CALLS = 50000 * 0.8
RATE_LIMIT_CALLS = 100
RATE_LIMIT_SECONDS = 20

Expand Down Expand Up @@ -80,7 +80,7 @@ def raise_for_rate_limit(data):
class Client:
# pylint: disable=unused-argument
def __init__(self, endpoint, client_id, client_secret,
max_daily_calls=MAX_DAILY_CALLS,
max_daily_calls=None,
user_agent=DEFAULT_USER_AGENT,
job_timeout=JOB_TIMEOUT,
poll_interval=POLL_INTERVAL,
Expand All @@ -89,7 +89,13 @@ def __init__(self, endpoint, client_id, client_secret,
self.domain = extract_domain(endpoint)
self.client_id = client_id
self.client_secret = client_secret
self.max_daily_calls = int(max_daily_calls)
try:
self.max_daily_calls = int(max_daily_calls or MAX_DAILY_CALLS)
except (ValueError, TypeError):
# Raises TypeError for None / Null Value
# Raises ValueError for "" / bool value

Choose a reason for hiding this comment

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

can we make this a logger error like:

LOGGER.warning("Invalid max_daily_calls was passed: \'{}\', using default value of 40,000.".format("max_daily_calls"))

That way it's a little clearer what might have happened to folks reading the logs

Choose a reason for hiding this comment

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

int(max_daily_calls or MAX_DAILY_CALLS) expression will fallback to default value for None, True/False and "".

IMO any other exception should be raised instead of falling back to default value.

>>> int("" or 100)
100
>>> int(True or 100)
1
>>> int(False or 100)
100
>>> int('1000' or 100)
1000
>>> int('True' or 100)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: invalid literal for int() with base 10: 'True'

Copy link
Member Author

@somethingmorerelevant somethingmorerelevant Sep 13, 2023

Choose a reason for hiding this comment

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

I don't like the idea of failing the code for a non-mandatory value but suppressing it will not handle all scenarios, this will only result in the user observing unexpected behaviour.

  • Number with "," eg: 1,000,000 (internationalisation issues)
  • Negative Number
  • Decimal input
  • Zero as a string i.e "0" (which will result in the limit being set to Zero).

Hence i find it best to raise an error incase of any such values instead of using a fallback mechanism.

Copy link
Member Author

@somethingmorerelevant somethingmorerelevant Sep 13, 2023

Choose a reason for hiding this comment

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

the helper text in UI for this tap also displays a value with a comma for the maximum daily api calls limit i.e 50,000 which will also be a invalid input

image

Copy link
Member Author

Choose a reason for hiding this comment

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

i have added a few more test scenarios and handled the negative / zero limit condition

Choose a reason for hiding this comment

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

We have precedence in other taps of logging if we're going to fall back to a default value and allowing the tap to try to proceed with the default, but not really for an API quota scenario, so I'm good either way

self.max_daily_calls = int(MAX_DAILY_CALLS)

self.user_agent = user_agent
self.job_timeout = job_timeout
self.poll_interval = poll_interval
Expand Down
57 changes: 57 additions & 0 deletions tests/test_client_max_api_limit.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import unittest
from datetime import datetime,timedelta
from tap_marketo.client import Client, MAX_DAILY_CALLS

class TestDateWindowConfig(unittest.TestCase):

def test_datewindow_disabled_no_val(self):
dsprayberry marked this conversation as resolved.
Show resolved Hide resolved
"""
Verify that daily_calls_limit is default if no value is passed
"""
# Initialize Client object
client = Client(**{'endpoint': "123-ABC-789",'client_id':'ABC-123','client_secret':'123-QRT'})

Choose a reason for hiding this comment

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

Be consistent with the ' and "


self.assertEqual(client.max_daily_calls, MAX_DAILY_CALLS)

def test_datewindow_disabled_empty_str(self):
"""
Verify that daily_calls_limit is default if empty string value is passed
Verify no Exception is raised for typecasting error between str to num
"""
# Initialize Client object
client = Client(**{'endpoint': "123-ABC-789",'client_id':'ABC-123','client_secret':'123-QRT',"max_daily_calls":""})

self.assertEqual(client.max_daily_calls, MAX_DAILY_CALLS)

def test_datewindow_disabled_bool_val(self):
"""
Verify that daily_calls_limit is default if bool value is passed
"""
# Initialize Client object
client = Client(**{'endpoint': "123-ABC-789",'client_id':'ABC-123','client_secret':'123-QRT','max_daily_calls':False})
self.assertEqual(client.max_daily_calls ,MAX_DAILY_CALLS)

def test_datewindow_disabled_num_val(self):
"""
Verify that api_limit is 0 if 0 value is passed
"""
# Initialize Client object
client = Client(**{'endpoint': "123-ABC-789",'client_id':'ABC-123','client_secret':'123-QRT',"max_daily_calls":0})
self.assertEqual(client.max_daily_calls, MAX_DAILY_CALLS)

def test_datewindow_disabled_none_val(self):
"""
Verify that api_limit is default if None value is passed
"""
# Initialize Client object
client = Client(**{'endpoint': "123-ABC-789",'client_id':'ABC-123','client_secret':'123-QRT',"max_daily_calls":None})
self.assertEqual(client.max_daily_calls, MAX_DAILY_CALLS)

def test_datewindow_enabled_num_val(self):
"""
Verify that api_limit is set appropriately if num value is passed
"""
# Initialize Client object
client = Client(**{'endpoint': "123-ABC-789",'client_id':'ABC-123','client_secret':'123-QRT',"max_daily_calls":3})

self.assertEqual(client.max_daily_calls, 3)