meraki/dashboard-api-python

Incorrect kwarg is not logged or rejected

mpenning opened this issue · 4 comments

mpenning commented

I spent a while diagnosing why my calls to getOrganizationNetworks() did not return ~20% of the networks that I expected to see.

My call was:

  • getOrganizationNetworks(organizationId="000000000000000000", direction= "prev", total_pages= "all", per_page=100000)

I got back about 8500 networks; I expected about 10750.

At first, I thought it might be a concurrency problem (I use the meraki api with mpire), but the concurrency investigation was a dead-end.

Short story, one of my parameter names is incorrect. per_page should be perPage. It doesn't help that some getOrganizationNetworks() API keywords are snake case, and others are camel case. After I fixed the problem, I got all the networks that I expected. However, I don't think this should happen. If I call the Meraki API an unsupported kwarg, you should throw a warning or error.

TKIPisalegacycipher commented

Thanks for reporting this @mpenning !

Adding additional error conditions to the library methods could be seen as a breaking change; scripts otherwise working would stop working and this could be a big problem for folks who are not actively maintaining their scripts.

I don't think this is a security concern so it's harder to justify in that sense.

Did you have a suggested approach for handling this? Especially if you have a PR, that'd be helpful.

mpenning commented

Adding additional error conditions to the library methods could be seen as a breaking change; scripts otherwise working would stop working and this could be a big problem

Today, errors pass silently... and the result is VERY bad... the API returns partial results. That broke my script, but only because I knew exactly how many networks to expect. Imagine if I didn't catch the problem and it broke my production network.

Your claim is essentially "rejecting an invalid keyword could break a running script". The Zen of Python is very clear about this... "Errors should never pass silently... Unless explicitly silenced."

Logging a red error message to stdout seems rather unlikely to be a breaking change. Perhaps we can agree on this point.

TKIPisalegacycipher commented

Adding an entry to the logger with that severity on such a scenario sounds like a good enhancement. Would you like to send a PR making that change?

mpenning commented

send a PR making that change?

Sounds good... it may take a while, I'm not familiar with the inner working of things