Validate Kwargs before and possibly after passing them to Shopify

[samLozier]

Overview

Validate kwargs before passing them to shopify and either raise an exception or warn if the kwarg is likely to produce no effect (the current behavior). I'd propose implementing this as a session-level flag to change behavior which would likely be implemented in the __encoded_params_for_signature method of the Session class.

something like:

with shopify.Session.temp(shop_url, api_version, token, validate_kwargs='fail'):
   orders = shopify.Order.find(ids="1234")

where:
default - current behavior
warn - current behavior, with warning that kwargs will not work as expected
fail - raise exception before generating query string.

With this new behavior:

shopify.Order.find(ids="1234")
##################
InvalidKwargError: The ids kwarg should be a comma-separated list of ids.

...

Type

• New feature
• Changes to existing features

Motivation

What inspired this feature request? What problems were you facing?

I recently had the experience of trying to fetch two orders while testing some code:

shopify.Order.find(ids="1234,5678")
Next I wanted to check for a single order so I deleted the second order:
shopify.Order.find(ids="1234")

Those with a keen eye could spot the bug: ID's needs a comma-separated list of order IDs. according to the docs. Without the comma, the kwarg is basically disregarded and all (non-archived) orders are returned. This can lead to unexpectedly long running queries that is hard to debug when this code is generated dynamically or buried under several layers of other code.

Here are a few examples of the proposed validation:

• IDS : if "ids" in kwargs, validate that there is at least one "," and that the values in the "ids" string are integers. The response could also be validated as we'd expect to receive <= number of objects in the response. eg: if the "ids" filter has two elements, we can expect <=2 objects in the response.
• status: check that the status kwarg has one of the valid values ['open', 'closed', 'cancelled', 'any']
• created_at_min: check that the min-date is a valid iso-formatted date

With the warn/fail arguments I proposed above, if the kwarg didn't pass the test, the code would loudly warn or fail and make debugging much easier.

An alternative, and possibly easier to implement solution, would be to add configurable logging. The ability to easily see the full query string as well as the raw response body could make the same debugging a lot easier.

I'm happy to open a PR for either proposed solution, but would appreciate some guidance from the maintainers before doing so.
...

Area

• Add any relevant Area: <area> labels to this issue

---

Checklist

• I have described this feature request in a way that is actionable (if possible)

[mllemango]

Hey @samLozier, thanks for submitting such a thought out issue. I would suggest that you try out the configurable logging first (that's been on my to-do list for a long time now, and would help with not only kwargs issues 😅)
I much prefer the logging option to this sort of validation - our code doesn’t really know what attributes are allowed for each individual resource, and any validation we do here will be based solely on assumptions (or more code that will need to be maintained), and it will ultimately only help in identifying requests made using the wrong arguments.
Being able to toggle on a debugging flag that prints out the requests made by the library seems like a much more reliable feature to me, I would personally go for that solution. Happy to help out with any PRs drafted!

[github-actions]

This issue is stale because it has been open for 60 days with no activity. It will be closed if no further action occurs in 14 days.

[github-actions]

We are closing this issue because it has been inactive for a few months.
This probably means that it is not reproducible or it has been fixed in a newer version.
If it’s an enhancement and hasn’t been taken on since it was submitted, then it seems other issues have taken priority.

If you still encounter this issue with the latest stable version, please reopen using the issue template. You can also contribute directly by submitting a pull request– see the CONTRIBUTING.md file for guidelines

Thank you!