[IO-1626][external] Client fails when data structure is unexpected #655
Conversation
IO-1626 BUG: bug with adding different datatypes in client.py, specifically create_dataset(), list_remote_datasets(), & get_remote_dataset()
BUG submission from: John Wilkie The use of dataset.get("{dict_key}", 0) means that sometimes None is returned, throwing a TypeError because we try to add NoneType to integers. It would be better to replace these gets with: This is having a minor impact on one client at the moment. We have not been able to reproduce this yet, even on the client'd affected dataset. However, introducing better tolerance for this will prevent any further cases Impact Expected Behaviour Team & Dataset Link Customer Name Renewal timeline ARR Tier Assigned CSM |
| else: | ||
| self.log = log | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
This is the biggest part of the change - most other changes are just code cleanup
| import logging | ||
| import os | ||
| import time | ||
| import zlib |
There was a problem hiding this comment.
There are many changes in here which are just formatting or typing
Any->UnknownTypeare equivalent, but one doesn't cause issues with linter.- Reformatting of lines has occurred in several unrelated places.
Only the implementation of the _get_item_count and its' use and testing are relevant to the fix
| assert_dataset(remote_datasets[0], expected_dataset_1) | ||
| assert_dataset(remote_datasets[1], expected_dataset_2) | ||
|
|
||
| @responses.activate |
There was a problem hiding this comment.
This is a socialised test, testing that the client is using the _get_item_count function correctly.
There is a functional unit test as well, testing the function in a pure functional way.
simedw
left a comment
simedw
left a comment
There was a problem hiding this comment.
overall LGTM
normally I don't try to test _ methods directly but I think it's fine.
Also _get_item_count doesn't really belong as a class method on Client, it's more of a utility function thing or could even live inside the RemoteDataset class?
Problem
On occasion, the API returns a nullish response instead of 0 for a zero count item, image or video count, which causes a crash
Solution
Have written a single function that calculates the item count for each dataset, based on appropriate elimination logic
Changelog
New function to calculate item counts, applied to all creations of
RemoteDatasetV1andRemoteDatasetV2items.Tests for new function
Tests to ensure that client and new function behave correctly together
Some cleanup of old typing and linting