Fix bulk low-level get request with non-existent MBID #223
Conversation
GET request for bulk low-level data was failing when any single one of the MBIDs were not present in the database. These entries are now put into an error field appended to the fetched data for existing recordings.
webserver/views/api/v1/core.py
Outdated
| try: | ||
| recording_details.setdefault(recording_id, {})[offset] = db.data.load_low_level(recording_id, offset) | ||
| except NoDataFoundException: | ||
| errors = recording_details.setdefault("errors", {}) |
There was a problem hiding this comment.
I've been thinking about this errors key. I don't think it's necessary. My opinion now is that if a recording doesn't exist, we just skip it.
| calls = [mock.call("c5f4909e-1d7b-4f15-a6f6-1af376bc01c9", 0), | ||
| mock.call("7f27d7a9-27f0-4663-9d20-2c9c40200e6d", 3), | ||
| mock.call("405a5ff4-7ee2-436b-95c1-90ce8a83b359", 2)] | ||
| load_low_level.assert_has_calls(calls) |
|
This is great, thanks. One thing I missed in the first review is that we need to add to the documentation to say that if data for a recording id doesn't exist, it is silently ignored and will not appear in the returned data. |
webserver/views/api/v1/core.py
Outdated
| errors = recording_details.setdefault("errors", {}) | ||
| mbid_entry = errors.setdefault(recording_id, {}) | ||
| mbid_entry[offset] = {"message": "recording does not exist"} | ||
| pass |
There was a problem hiding this comment.
A small thing on this commit - because you're doing two distinct things, I would have made it as two commits, to make it easier to follow what changes are being made.
There was a problem hiding this comment.
RIght! I'll seperate the test from this change.
There was a problem hiding this comment.
You didn't have to make the change again - I was just mentioning it for future work. Thanks for going back and doing it 👍
Add test for low-level bulk GET requests with mbids not present in database
Remove 'errors' key from result of GET requests.
|
🎉 |
GET request for bulk low-level data was failing when any single
one of the MBIDs were not present in the database. These entries
are now put into an error field appended to the fetched data for
existing recordings.
Relevant Jira ticket: https://tickets.metabrainz.org/browse/AB-296