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

Update Query Parameter Transaction #176

Closed
commbankoss opened this issue Apr 2, 2020 · 10 comments
Closed

Update Query Parameter Transaction #176

commbankoss opened this issue Apr 2, 2020 · 10 comments

Comments

@commbankoss
Copy link

Description

For the transactions endpoint, specifically Get Transactions for an Account, a data recipient who chooses not to support the “text” search field as a query parameter, are required to return a flag “isQueryParamUnsupported” in the response meta data object to indicate that the field is not supported.

  • The field name is not indicative of which field is not supported (i.e.) no reference to the text search field.

  • It’s not clear what’s is the purpose of returning this field with every response. It should suffice to include it in the API documentation.

Area Affected

Handling optionality of the “text” field in the Get Transactions for an Account query parameter if not supported by the data holder.

Change Proposed

In case the Data Holder choose not to support the “text” search query parameter for the transactions summary endpoint (Get Transactions for an Account), the data holder must:

  • Document the fact that the query parameter is not supported in the API documentation published on the Data Holder’s dev portal.

  • If the text field is provided by the data recipient whilst not supported by the data holder, the data holder will return an error indicating that the “text” parameter is not supported. This is in alignment with providing any other unsupported query parameter.

@perlboy
Copy link

perlboy commented Apr 2, 2020

Related and unanswered since February 22: #141

@commbankoss
Copy link
Author

Commonwealth Bank posted this change request 02/04/2020, and related issue #141 was posted 22/02/2020. No responses were provided to either issue.

Commonwealth Bank requested that this issue be included in maintenance iteration 3, but it was not included. We request this issue be labelled urgent and addressed for July.

@CDR-API-Stream
Copy link
Collaborator

Hi @commbankoss currently the standards require text filtering to be managed through the use of the isQueryParamUnsupported. This issue won't be treated as urgent because there is already clear guidance for July implementations. The optionality of text filtering was originally introduced at the request of CBA.

If [the text filter] is not implemented then a response should be provided as normal without text filtering applied and an additional boolean field named isQueryParamUnsupported should be included in the meta object and set to true (whether the text parameter is supplied or not)

Whilst a Data Holder may not support a given feature such as text filtering, it was intentional to avoid a hard error. This allows:

  • the ADR to get a response which can be filtered on the ADR side,
  • reduces load on the Data Holder servicing one call rather than two,
  • benefits Data Holders because they can see the volume of requests for text filtering, and the nature of the text filtering to optimise their data management and improve their service to ADRs.

Continuing to return a 200 OK in situations like this but having a more expressive structure to identify which fields are unsupported may be practical. The DSB would welcome any other participants to provide their views.

@CDR-API-Stream CDR-API-Stream moved this from Full Backlog to Iteration Candidates in Data Standards Maintenance Jul 9, 2020
@CDR-API-Stream CDR-API-Stream added this to Iteration Candidates in Maintenance Iteration 4 Jul 9, 2020
@CDR-API-Stream CDR-API-Stream moved this from Iteration Candidates to In Progress in Maintenance Iteration 4 Jul 17, 2020
@CDR-API-Stream
Copy link
Collaborator

CDR-API-Stream commented Jul 17, 2020

Given previous questions have been clarified why an error is not desired, it is still reasonable to expect that isQueryParamUnsupported may apply to other optional fields in future. Having Data Holders publish their support on their own Developer Portals is useful but cannot be the basis for ADRs to determine whether a given result set does not apply a certain filter.

The following proposed changes to be made are:

  1. Extend the MetaPaginated response to define the conditional support of isQueryParamUnsupported. Refer to List Transactions text filter isQueryParamUnsupported #141.
  2. Describe the isQueryParamUnsupported parameter and applicable endpoints in a new "Filtering" section under the "Standards" section of ACDS. Refer to List Transactions text filter isQueryParamUnsupported #141.
  3. Extend the MetaPaginated response to describe which parameters are unsupported. Options seeking input are presented below.

3. Options for handling unsupported query parameters

The options proposed for feedback are:

OptionDescriptionConsiderations
Option A - Do nothingCurrently the standards only define the conditional text filter so there is a 1:1 mapping between text filter support and isQueryParamUnsupported. Address in future if any further fields are introduced.
  • The standards remain slightly ambiguous and inference is required when using isQueryParamUnsupported
  • No build change to current standards
  • Introducing additional unsupported parameters in future will be a breaking change
Option B - Array of parameters

Provide an array of parameters that are not supported in the provided response. An empty array would be supplied if isQueryParamUnsupported == false

Example:


GET https://data.holder.com.au/cds-au/v1/
banking/accounts/{accountId}/transactions HTTP/1.1
Host: data.holder.com.au
# Response
{
  "data": {
    "transactions": [
      { ... }
    ]
  },
  "meta": {
    "isQueryParamUnsupported": true,
    "unsupportedParameters": ["text", "..."]
  }
}
  
  • isQueryParamUnsupported continues to be used so ADR implementations can keep working
  • Ensures future compatibility so isQueryParamUnsupported isn't ambiguously overloaded
  • Transaction Detail APIs are not currently live so it is not considered a breaking change yet
  • It may be considered without a future dated obligation, or Feb 2021
Option C - Structured response

Provide an array of parameters that are not supported in the provided response. Unsupported parameters are described using a JSONAPI like structure. An empty array would be supplied if isQueryParamUnsupported == false

Example:


GET https://data.holder.com.au/cds-au/v1/
banking/accounts/{accountId}/transactions HTTP/1.1
Host: data.holder.com.au
# Response
{
  "data": {
    "transactions": [
      { ... }
    ]
  },
  "meta": {
    "isQueryParamUnsupported": true,
    "unsupportedParameters": [
      {
        "source": {
          "parameter": "text"
        },
        "title": "Unsupported Filter: text",
        "detail": "Text filtering is unsupported. 
                   The list of transactions has not been filtered by 'text'."
      },
      {
        "source": {
          "pointer": "/pointer/to/parameter"
        },
        "title": "Unsupported Filter: XYZ",
        "detail": "XYZ filtering is unsupported. 
                   The list of transactions has not been filtered by 'XYZ'."
      }
    ]
  }
}
  
  • isQueryParamUnsupported continues to be used so ADR implementations can keep working
  • Ensures future compatibility so isQueryParamUnsupported isn't ambiguously overloaded
  • Benefit is that query and body parameters can be described using the JSONAPI source format.
  • Transaction Detail APIs are not currently live so it is not considered a breaking change yet
  • It may be considered without a future dated obligation, or Feb 2021
  • This may be more complex than required. Useful if Title/Description fields are considered important

@perlboy
Copy link

perlboy commented Jul 17, 2020

As per #141 this addition was never documented nor has a decision proposal associated with it. This was raised ~5 months ago with no response from the DSB after more than 2 months and closed because I gave up waiting.

On this basis @CDR-API-Stream Option D is to remove it as an undocumented change, accept that text filtering may or may not be supported (essentially consider it non-functional for 1.0.0) and formalise a proposal that considers things effectively. There are now live implementations in play so what support for text filtering is implemented by Big 4? If zero or all have implemented text filtering than Option D is the lowest impact. In addition, with only two Data Recipients, are either currently using text filtering? @pcurtisrab @brett-frollo?

In addition, Option E would be to establish an ErrorResponse specifically for "unsupported text filtering" and allow recipients to detect this and make a decision. This would also remove the need for the parameter to exist (ie. Option E implies Option D). Electronic Thumb already assumes this behaviour because Babelfish does not supported this undocumented parameter.

As for your proposed options:

Option A: The parameter is poorly named and this approach precludes any ability to effectively use it to signal lack of support for other parameters in the future. This proposal only seems appropriate if the parameter name itself is changed to isTextFilteringUnsupported but is a stop gap to an undocumented hack injected without approval by representatives of the government.

Option B: This is an option but is boolean in nature. There are foreseeable situations where certain parts of a query parameter (for instance, some of an enum but not others) are unsupported but the parameter itself is supported in some cases.

Option C: This blows up the complexity of meta across all objects which use a shared MetaPaginated object, should probably be it's own decision proposal and, with reference to the proposed structure, appears to require programmatic introspection of request data by way of specifying location.

This issue was highlighted months ago with no response. The easiest solution is to drop it entirely, start fresh as to what the goals are and make a decision in the interim behaviour. ErrorResponse seems the most natural without altering the structure if it's absolutely necessary to support existing use cases.

@commbankoss
Copy link
Author

Thank you for your responses @perlboy and @CDR-API-Stream. We support perlboy's proposed Option E, as it aligns closely to our original proposal. The main changes we see as necessary are:

  1. Remove the isQueryParamUnsupported field
  2. Require Data Holders to respond with an error when the text field query is not supported, aligning to behaviour where other query parameters are unsupported
  3. Raise a formal decision proposal to facilitate feedback from DHs and ADRs on filtering and filtering support

We invite other ADRs to comment on their use of the text and the isQueryParamUnsupported fields.

@arthuroz
Copy link

Agreed with @perlboy and @commbankoss, HttpStatus code 422 and ErrorResponse(s) in body would works well for this case and similar scenarios in future.

@perlboy
Copy link

perlboy commented Jan 11, 2021

We invite other ADRs to comment on their use of the text and the isQueryParamUnsupported fields.

Based on what we've observed in market we do not believe any ADR is currently using any filter parameters in transactions, primarily because DH implementations are inconsistent for min/max-amount and start/end-date implementations. This makes this proposed change non-breaking.

Given the cycle time on maintenance this change looks likely to take until February before approval resulting a resolution time in excess of one year from the original issue being raised.

@nils-work
Copy link
Member

Hi @commbankoss

As this change resolved an issue with missing schema detail in Get Transactions For Account, do you consider this issue can now be closed, or further consultation is required?

I believe some of the original concerns may have been addressed by comments here and here.

@nils-work
Copy link
Member

If there are no further comments on this issue it will be closed on 28 June 2024.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Banking Banking domain APIs
Projects
Status: Done
Development

No branches or pull requests

5 participants