mojaloop / central-settlement

Service to expose the Mojaloop Settlements API
Other
5 stars 34 forks source link

feat: better settlement query filters #372

Closed partiallyordered closed 1 year ago

partiallyordered commented 3 years ago

Enable much better queries for settlements, for example:

GET /settlements?accountId=1,2,3&state=PENDING_SETTLEMENT,PS_TRANSFERS_COMMITTED
partiallyordered commented 3 years ago

@elnyry-sam-k @mdebarros I'd appreciate any feedback on this in principle, when you have a moment

elnyry-sam-k commented 3 years ago

hi Matt @partiallyordered , thanks for the PR..

The new feature will be very helpful, I'm sure; the swagger changes are ok.. Do the changes, for example number to string effect current implementations and if so will this be a breaking change?

Furthermore, a more naïve question: the change to the query params from the swagger - does this work with no additional implementation (than what's in the PR?)

elnyry-sam-k commented 3 years ago

Also @mdebarros - please review when you get a chance..

partiallyordered commented 3 years ago

Do the changes, for example number to string effect current implementations

The changes to the actual API should be backward compatible. Because these are URL query parameters rather than object fields, and fundamentally URL query parameters are strings anyway, the only difference should be in what validation is performed. For example, the API should change from, e.g.

GET /settlements?accountId=2

to

GET /settlements?accountId=1,2,3

and if so will this be a breaking change?

I'll address this separately. As far as I can tell, the "multiple parameter" changes should be completely backwards-compatible. But the default values for from/to dates will result in some queries succeeding where they would not previously have done. Specifically the following invalid request:

GET /settlements

which is not currently valid because a request without query parameters is not valid, would become valid, by virtue of the default values. Thinking a bit more about this, it might be better to simply allow queries with no filters and remove these default date values because

  1. the user can query all settlements if they want to, by specifying the earliest possible date as a from date
  2. having these default values means the db query always has additional clauses, which will decrease its efficiency
  3. because the user is able to request all settlements, why make their job difficult if that's what they really want? let's just remove the requirement of having any filters at all (especially as this change to the API should enable them to make more targeted queries and reduce their need to request all settlements)
  4. these particular default date values are actually the smallest and largest valid values representable by the mysql date type; it's probably better not to have this leaking into the API spec; that could have some unintended consequences, for example the possible commitment to supporting this rather odd date range in future

Furthermore, a more naïve question: the change to the query params from the swagger - does this work with no additional implementation (than what's in the PR?)

I actually don't know; I'd be surprised if there isn't more to do, not least some actual meaningful tests. I thought I'd put this out there and get some feedback on the idea of it before putting a lot of effort into it. It's probably worth mentioning that this change would ideally be applied to the /settlementWindows endpoint also; which I'll likely just do in this PR if it doesn't turn out to be too different to this.

elnyry-sam-k commented 3 years ago

Thanks for the response, Matt. please go ahead with this; like your idea about allowing queries without any filters in some cases..

partiallyordered commented 2 years ago

Removed default date filters; did not remove enforcement of minimum number of query parameters.

partiallyordered commented 2 years ago

does this work with no additional implementation (than what's in the PR?)

Turns out: yes; nothing else required.

elnyry-sam-k commented 2 years ago

does this work with no additional implementation (than what's in the PR?)

Turns out: yes; nothing else required.

Excellent !