openleadr-python

openleadr-python copied to clipboard

Hi Stan,

I discovered a bug in Openleadr's XML-to-object behaviour. The error path is approximately this:

• My VEN client had only a single report (created with my_OpenADRClient.register_reports(report=[...]))
• This gets correctly turned into XML and sent to the VTN.
• On the VTN, xmltodict.parse turns that single report entry (in the <oadr:oadrPendingReports> tag) into a dict with a bare string in it -- if there were more entries, it would produce a dist with a list of strings. (Arguably, this is the source of the error ...)
• openleadr.util.normalize_dict fails to normalize that structure, because it only expects a dict with a list of strings. (... but this is the easiest place to fix it.)
• Further down the line, the VTN's openleadr.service.report_service.ReportService.on_created_report blows up and we get a 500 error.

For now, our workaround is to register a second, dummy, report on our VEN.

About this PR:

• First commit in this PR adds the failing test.
• Second commit in this PR contains one possible fix -- using glom, a library that specializes in extracting data from nested data structures.

Motivation:

• The error is essentially a schema error: xml2dict.parse's result has a different schema depending on if there are 0, 1, or more pending reports, and then openleadr.util.normalize_dict only handles one of those schemas.
• Python is really bad at checking a nested data structures: it forces you to write error-prone code like if isinstance(d[key], dict) and 'report_request_id' in d[key] and isinstance(d[key]['report_request_id'], list). (Footnote: Python 3.10's structural pattern matching is perfect for extracting data from nested structures, and handling multiple cases. But I figure you want to keep supporting Python<=3.9 for a while longer.)
• So a library that makes it easy to check and use schemas will make openleadr.util.normalize_dict easier to maintain. For this WIP proposal, I chose glom. (Another option might be schema might also be an option?)

Choices from here, depending on how you feel about Glom:

1. "Not feeling it": Rewrite the pending_reports schema checks using plain python x in y and isinstance(y[x], mytype) and ...
2. "Looks nice enough": Accept this PR, including Glom, as is; leave the rest of normalize_dict, we can use glom if more bugs crop up
3. "Let's try a few more cases": First (someone, probably me) rewrites the other ~16 transformations in normalize_dict to also use Glom where it makes sense. That's not much work.

• "Let's get radical": Fix this by solving #17 .

Any thoughts?

Cheers, Sietse

Hi Sietse,

Thanks so much for the detailed analysis, useful suggestions, and a working test and fix.

As you noted by referencing #17, I was already not very fond of the xml-to-object functionality and the normalization. It started out as a good idea to make the unwieldy OpenADR / EiInterop schemas more user friendly (which I think it does), but it is a little error-prone and inefficient.

For the moment, I added another manual check that report_request_id is in fact a list, and then turn it into a list if it is not, like I did for some of the other proprties that undergo the normalization. Mostly because I don't (yet) know Glom or Schema, but it looks like an elegant and more expressive way to get the data in the correct shape.

(In the process of thinking about this problem, I found another possible failure in this code (which your fix does not yet address, I think): if the report_request_id is strictly numeric (like "123"), it will get turned into an int instead of a str. That should now be fixed as well.)

I will be looking into some of the alternatives that you mentioned, and I'd love to hear your suggestions on #17 if you have any (and on other areas of improvemen for OpenLEADR).

Thanks again, hopefully we'll speak again.