Tdl 24072 fix typecasting bug max api limit #84
Conversation
tap_marketo/client.py
Outdated
| # Raises TypeError for None / Null Value | ||
| # Raises ValueError for "" / bool value |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
i have added a few more test scenarios and handled the negative / zero limit condition
There was a problem hiding this comment.
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
tests/test_client_max_api_limit.py
Outdated
| Verify that max_daily_calls is default if no value is passed | ||
| """ | ||
| # Initialize Client object | ||
| client = Client(**{'endpoint': "123-ABC-789",'client_id':'ABC-123','client_secret':'123-QRT'}) |
tests/test_client_max_api_limit.py
Outdated
| def test_maxdailycalls_default_bool_val(self): | ||
| """ | ||
| Verify that max_daily_calls 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) |
There was a problem hiding this comment.
True bool value should also be tested.
try:
self.max_daily_calls = int(max_daily_calls or MAX_DAILY_CALLS)
if self.max_daily_calls <= 1:
raise ValueError("Limit Cannot be Negative or Zero")
except (ValueError, TypeError) as err:
singer.log_critical(f"Invalid Value passed for max_daily_calls: {max_daily_calls}")
raise errThis is the only issue i see with the current implementation👇🏼 If the value is passed as a string "0" it throws an exception since the value is negative/zero, but if the same value is passed as 0 it sets it to default. eg: >>> int("0" or 40000)
0
>>> int(0 or 40000)
40000 |
somethingmorerelevant
deleted the
TDL-24072-fix-typecasting-bug-max-api-limit
branch
Description of change
int() argument must be a string, a bytes-like object or a number, not 'NoneType'issue with apI_max_limit typecastingManual QA steps
Risks
Rollback steps