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

Changing the approach to filtering params #399

Open
nnc opened this issue Dec 6, 2017 · 1 comment
Open

Changing the approach to filtering params #399

nnc opened this issue Dec 6, 2017 · 1 comment
Labels

Comments

@nnc
Copy link
Contributor

nnc commented Dec 6, 2017

I'm putting this proposal forward just to get feedback from Xeroizer maintainers on how important it is to them to have Xeroizer explicitly be aware of each option/param/etc in Xero API, vs having a more lenient approach that allows greater flexibility and more direct mapping between Xero API docs and Xeroizer param names, but also allows users to shoot themselves in the foot more.

Xero recently added filtering params to the Invoices and Contacts endpoints, which seem to be the "officially preferred" way to do filtering, instead of building large where queries.

While it's currently only for Invoices, and to much lesser extent Contacts, expanding list of params and endpoints covered is already planned, it would seem.

With that in mind, I wonder if approach currently taken by Xeroizer, where each param added by the API needs to also be added to Xeroizer before it can be used, is the best path forward.

While I can see a value of Xeroizer explicitly being aware of some params, where some more specialized data formatting is required, filtering params seem to all be just simple lists of things API needs to filter by, and I think it's reasonable to expect that to be the case for future filtering params as well.

In my opinion, it would be easier on both maintainers and users to have a simple passthrough (perhaps called filter_by) which would take any input from the user, convert it into a comma separated list of things, and tack it on to a URL.
This way any new param supported by the API would instantly be available for users to use, without having to wait on Xeroizer to be patched (like I did in #398) and then updating their apps to use that new version.

So instead of

invoices = xero.Invoice.all(
  ContactIDs: ["uuid1", "uuid2"],
  Statuses: ["SUBMITTED", "AUTHORISED"]
)

it could be

invoices = xero.Invoice.all(
  filter_by: {
    ContactIDs: ["uuid1", "uuid2"],
    Statuses: ["SUBMITTED", "AUTHORISED"],
    CurrencyCodes: ["USD", "CAD"]
  }
)

Notice how the latter has a CurrencyCodes param, as an example of a future filtering param, as anything passed in filter_by will be added to the resulting API endpoint URL.

It's a bit more code to write, but I don't think it's that bad.

What is a problem with this approach is mapping between Xero API docs and Xeroizer what is a filtering param vs a regular param that doesn't go into filter_by, which may be confusing.
Another potential issue is backwards compatibility, with how IDs filtering is already implemented.

@CloCkWeRX
Copy link
Collaborator

I think its a variant of the 'open for extension, closed for modification' principle. If we don't control the source of truth around validations, params, etc we should probably allow people to do whatever they want, to avoid having a million mapping changes a minute.

That said, when it comes to performing model validations locally (ie; only permit these enumerated types); I'm in the opposite camp - the benefit of knowing a request is invalid before you send it is big enough.
A happy medium would be if there was XSD published - https://github.com/XeroAPI/XeroAPI-Schemas - and we used those to power the model validations, plus let users easily update the packaged versions of 'em. Seperate issue, but I think a similar problem for end consumers.

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

No branches or pull requests

2 participants