Error Handling
“I am the cat who codes by himself, and all mornings are alike to me.” Mostly, anyway. Well, similar.1
Notes on my index card say “Smoke Test”, “Private or Not”, “Bulletproof World Execute”, and “Handle errors”. Those last two are closely related. Today, we’ll get started on handling errors, so that the World’s execute_requests method cannot crash the program, and returns possibly meaningful messages to the client.
We can be pretty sure that a judicious sprinkling of try: except: will protect the World code from crashing. And we can build an exception of our own, if that makes sense, or just create “meaningful” strings as the exception messages. But how should those messages be returned to the client?
Currently, when it is given a list of requests, World saves the bot id provided in each request, and finishes up by returning a list of dictionaries, one for each bot, with that bot’s details. I had been thinking that we might add a new detail, an error message that would be set non blank if an error affected that Bot. That’s one way.
Another way might be just to record error messages in a separate list, and to return that as a separate element of the return package. That would allow for messages that are not associated with any particular Bot. There are at least a couple: the request list might not be a list, or a command might not include the requisite entity id that would let us associate it with any bot.
I think that the object returned from a call to execute_requests is a simple list. We may need to change that.
class World:
def execute_requests(self, actions_list):
self.ids_used = set()
self.execute_actions(actions_list)
return [ self.fetch(bot_id) for bot_id in self.ids_used ]
The receivers all suppose that they’re getting a raw list back:
class DirectConnection:
def run_request(self, cohort, rq):
results = self.world.execute_requests(rq)
cohort.update(results)
class Cohort:
def update(self, results):
for result_dict in results:
bot_id = result_dict['eid']
bot = self.by_id(bot_id)
bot.update(result_dict)
I suspect that passing a naked list through JSON isn’t an ideal practice. It’s perhaps better to ensure that each element passed in has an associated name, identifying it. It seems that we will need two lists, so let’s suppose that the new object returned will be a dictionary, with two keys, maybe ‘updates’ and ‘messages’, each pointing to a list, of bot updates on the one hand and message strings on the other.
This change is kind of a big deal, because it requires at least some changes on both server and client sides. It’s a good thing that we have no users.
So, new rule: the object returned from execute_requests will be a dictionary with potential keys ‘messages’ and ‘updates’, each pointing to a list.
I think what we’ll do is make the change, and break things. If it looks to break too many things, we’ll back away slowly and do something else. I’ll just change the return to be a dictionary:
class World:
def execute_requests(self, actions_list):
self.ids_used = set()
self.execute_actions(actions_list)
updates = [ self.fetch(bot_id) for bot_id in self.ids_used ]
return { 'updates': updates }
I am surprised to see that only five tests fail. Let’s fix DC:
class DirectConnection:
def run_request(self, cohort, rq):
results = self.world.execute_requests(rq)
cohort.update(results['updates'])
That fixes four of the five tests. What remains?
def test_returns_results(self):
WorldEntity.next_id = 100
world = World(10, 10)
bot_1_id = world.add_bot(5, 5)
bot_2_id = world.add_bot(7, 7, Direction.NORTH)
rq = [
{ 'entity': bot_1_id, 'verb': 'turn', 'direction': 'NORTH'},
{ 'entity': bot_1_id, 'verb': 'step'},
{ 'entity': bot_2_id, 'verb': 'step'},
{ 'entity': bot_2_id, 'verb': 'step'},
]
result = world.execute_requests(rq)
assert len(result) == 2
for d in result:
# print(d)
match d['eid']:
case 101:
assert d['location'] == Location(5, 4)
case 102:
assert d['location'] == Location(7, 5)
We just need the same fix here.
result = world.execute_requests(rq)['updates']
Green: Commit: results now returned in dictionary with bot updates key = ‘updates’. Commit: convert returned results to dictionary.
So that went perfectly. Now World can stuff messages into the result as it wishes, and no one will be harmed, but anyone who wants to can read them.
What would be a good first error to handle? How about the possibility that the request is not a list?
My first change is just this:
class World:
def execute_requests(self, actions_list):
self.ids_used = set()
self.messages = []
self.execute_actions(actions_list)
updates = [ self.fetch(bot_id) for bot_id in self.ids_used ]
return { 'updates': updates, 'messages': self.messages }
That should put a harmless new key into the result dictionary, which no one will access as yet. However, a test breaks.
def test_add_bot(self):
WorldEntity.next_id = 100
world = World(10, 10)
command = {'entity': 0,
'verb': 'add_bot',
'x': 5,
'y': 6,
'direction': 'EAST'}
rq = [ command ]
result = world.execute_requests(rq)
assert len(result) == 1
This is a uniquely weak test, isn’t it? Let’s improve it. And then let’s check and see if there are other calls to execute_requests that are this weak.
def test_add_bot(self):
WorldEntity.next_id = 100
world = World(10, 10)
command = {'entity': 0,
'verb': 'add_bot',
'x': 5,
'y': 6,
'direction': 'EAST'}
rq = [ command ]
result = world.execute_requests(rq)['updates']
assert len(result) == 1
bot_info = result[0]
assert bot_info['eid'] == WorldEntity.next_id
assert bot_info['location'] == Location(5, 6)
This runs, but I think there’s an issue here. The returned dictionary includes a Location instance. That won’t survive a JSON cycle unless we do something special. I make a note of that.
So where were we? Oh, right, messages. We’re green, Commit: world request response now includes a list under the name ‘messages’.
Thinking how to write that message caused me to come up with “world request response”. What we have at this moment, however is not an object that we own, WorldRequestResponse, but a simple dictionary. We like to encapsulate native collections, so we should probably do this one Real Soon Now. But not just now. It’s only used in this one place in World. We’ll see if we need it.
I think we should have some tests for any actual messages that come out. I’ll make yet another test file.
class TestWorldMessages:
def test_list_exists(self):
world = World(10, 10)
requests = []
result = world.execute_requests(requests)
assert result['messages'] == []
assert result['updates'] == []
For our first check, let’s require the requests to be a list.
def test_requests_must_be_list(self):
world = World(10, 10)
requests = { 'entity': 0, 'verb': 'add_bot'}
messages = world.execute_requests(requests)['messages']
assert 'must be a list' in messages
This fails with an exception, no surprise there.
def execute_requests(self, actions_list):
self.ids_used = set()
self.messages = []
updates = []
try:
assert isinstance(actions_list, list)
except AssertionError:
self.messages.append('requests must be a list of actions')
else:
self.execute_actions(actions_list)
updates = [ self.fetch(bot_id) for bot_id in self.ids_used ]
return { 'updates': updates, 'messages': self.messages }
I hate that already. And I had to adjust the test so that it would pass:
def test_requests_must_be_list(self):
world = World(10, 10)
requests = { 'entity': 0, 'verb': 'add_bot'}
messages = world.execute_requests(requests)['messages']
assert any('must be a list' in message
for message in messages)
We may need to figure out some good way to test for messages that allows us to change them for clarity without breaking tests. Perhaps an Enum kind of thing.
We need to refactor that execute_requests method now, before it’s too late. I think we’ll make the updates a member variable as well, to help with that.
def execute_requests(self, actions_list):
self.ids_used = set()
self.messages = []
self.updates = []
try:
assert isinstance(actions_list, list)
except AssertionError:
self.messages.append('requests must be a list of actions')
else:
self.execute_actions(actions_list)
self.updates = [ self.fetch(bot_id) for bot_id in self.ids_used ]
return { 'updates': self.updates, 'messages': self.messages }
We could commit this, since we’re green. Should we? Probably yes, we have something that works, that we’re about to try to make more nearly right. So this is a decent save point. Commit: error detected, request not a list.
Now we can extract that whole try block:
def execute_requests(self, actions_list):
self.ids_used = set()
self.messages = []
self.updates = []
self.execute_valid_list(actions_list)
return { 'updates': self.updates, 'messages': self.messages }
def execute_valid_list(self, actions_list):
try:
assert isinstance(actions_list, list)
except AssertionError:
self.messages.append('requests must be a list of actions')
else:
self.execute_actions(actions_list)
self.updates = [self.fetch(bot_id) for bot_id in self.ids_used]
I think this is better, in that the upper level is pretty flat and the new method mostly just manages the conditional. You could make the case that the last two lines should be extracted. Let’s do.
def execute_requests(self, actions_list):
self.ids_used = set()
self.messages = []
self.updates = []
self.execute_valid_list(actions_list)
return { 'updates': self.updates, 'messages': self.messages }
def execute_valid_list(self, actions_list):
try:
assert isinstance(actions_list, list)
except AssertionError:
self.messages.append('requests must be a list of actions')
else:
self.get_updates(actions_list)
def get_updates(self, actions_list):
self.execute_actions(actions_list)
self.updates = [self.fetch(bot_id) for bot_id in self.ids_used]
def execute_actions(self, actions_list):
for action in actions_list:
self.execute_with_entity(**action)
I think that’s nearly good. Some folks might prefer to see more of that in one method, but local practice prefers tiny methods. Commit: refactoring.
We could stop now, having put an actual error message into the system and made it more bulletproof. Let’s do, there’s fiction to be read and bacon coming right up.
Summary
One might fear that waiting this long to deal with error messaging would lead to trouble. So far, that has not been the case. I think we’ll find that having lots of separate methods doing small things will allow us to place quite accurate messages right where they need to go. I could be wrong, and if I am you’ll be the second person to know, because I’ll certainly reveal whatever happens.
For now, I’m confident that it is a bit harder to crash the World, and that it returns a sensible message in the case detected. A good morning’s work: see you next time!
- Added in Post
- It’s almost certainly a mistake to have made DirectConnection unwrap the updates from the message. DC doesn’t know anything about what’s going on. We should pass the whole response to the Cohort and deal with it there. Note made.
-
Well, except that on weekdays I can listen to Breakfast with the Beatles live, and there’s generally bacon on Sunday. ↩