Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect kwarg is not logged or rejected #212

Open
mpenning opened this issue May 15, 2023 · 4 comments
Open

Incorrect kwarg is not logged or rejected #212

mpenning opened this issue May 15, 2023 · 4 comments

Comments

@mpenning
Copy link

mpenning commented May 15, 2023

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

mpenning commented May 15, 2023

send a PR making that change?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants