bugfix: failed send could bury error message coming from ODS

[johncmerfeld]

Fixes an edge case that results in Lightbeam logging a Python error message instead of the ODS error message

Let's say you send an invalid payload and get a 400 response. I'm not sure exactly what changed (or when) on the ODS side, – it's possible this is only a problem in the local ODS I'm running – but the do_post function makes assumptions about the structure of the error response payload that don't always match what is returned.

Currently:

• Code expects "message" to be a field in the response body, and passes its value to linearize: util.linearize(json.loads(body).get("message")
• If "message" is not present in the body, linearize throws a TypeError when it tries to perform a string operation on None
• This TypeError gets caught by do_post's general exception catcher: except Exception and gets logged as an ordinary error
• The real error reported by the ODS is dropped

This change:

• Supports both "message" and "errors" syntax coming from the ODS - "errors" comes as a list of strings
• Allow an empty log message in the base case
• Changes the generic Exception to a ValueError to reduce the likelihood that a Python error will get caught and reported

As I say, I don't know whether or when we'll see "errors" instead of "message" in the wild, but it is at least possible under some of the packages Ed-Fi has published and it's otherwise harmless. In any case, not catching the generic Exception is the deeper fix

[tomreitz]

This is not a bug. Starting in ODS API version 7.2, Ed-Fi changed how API errors are reported. We do eventually need to account for this change in lightbeam, however:

• 7.2 is not yet being widely used (at EA, at least), so there's no need to rush a solution here.
• Careful thought is needed around how to handle the new error structure. Among other differences, the new structure can result in multiple errors being reported for a single API request, which would require deeper changes for lightbeam to report properly.

lightbeam doesn't yet have a solid pattern for handling breaking changes from Ed-Fi. The one example where it does is with the Swagger / OpenAPI spec, which changed versions (from OAS 2.0 to 3.0) with ODS API version 7.1. This required lightbeam to look for schema definitions in different places depending on the OAS version.

I would like to move slowly here with a strong philosophy for supporting future Ed-Fi changes that balances minimizing the branching factor while also keeping lightbeam's code interpretable when it must branch.

[johncmerfeld]

Setting aside fully supporting the Ed-Fi 7.2 error structure, there is still a bug in the existing code. Currently we are printing a Python TypeError as if it is a message from the ODS. Whether we branch or not, that is an incorrect behavior that could confuse users. Should we not address that?