ConsumerDataStandardsAustralia / standards-maintenance

This repository houses the interactions, consultations and work management to support the maintenance of baselined components of the Consumer Data Right API Standards and Information Security profile.
41 stars 9 forks source link

Add isQueryParamUnsupported to MetaPaginated for schema validation #469

Closed CDR-API-Stream closed 11 months ago

CDR-API-Stream commented 2 years ago

Description

A participant raised an issue on ZenDesk in that the isQueryParamUnsupported is not explicitly included in the MetaPaginated swagger which causes some implementations to fail schema validation.

Area Affected

MetaPaginated

Change Proposed

Update the MetaPaginated schema to include the conditional isQueryParamUnsupported so that schema validation works.

perlboy commented 2 years ago

isQueryParamUnsupported was never documented nor agreed in any decision proposal and this issue was highlighted in February 2020. Modifying the definition of MetaPaginated to include an endpoint specific attribute is (and always was) an anti-pattern for which there is now a more prescriptive and informative solution within Enhanced Error Handling.

I oppose modifying MetaPaginated and instead propose the following: 1) The introduction of this attribute be reversed and it be removed from the specification 2) An additional Enhanced Error (probably 422) is introduced of something like urn:au-cds:error:cds-banking: Resource/TransactionFilterTextNotSupportecd. This would be aligned with the existing behaviour of other 422 errors.

This approach will provide the ability for ADRs to appropriately behave based on the more explicit pattern defined while also enabling other potential use cases for optionally supported parameters in the future.

ghost commented 2 years ago

We are also an ADR affected by this issue when trying to validate responses against the schema. Could we please have an update as to the intended direction on this one? As it is, we are needing to maintain a work-around that ignores the isQueryParamUnsupported property.

perlboy commented 2 years ago

I still maintain this addition was never consulted on. Other than actually consulting on it the other option is to add a MetaPaginatedTransaction model so at least it passes the "pub test" in conformity. I know that's what we've done in our internal codegen setups.

biza-io commented 1 year ago

Altering MetaPaginated will cascade into all paginated endpoints. In order to align this more succinctly we suggest introducing MetaPaginatedTransaction so that at least the Standards are technically aligned with their own nomenclature with relation to Pagination.

JamesMBligh commented 1 year ago

I still maintain this addition was never consulted on. Other than actually consulting on it the other option is to add a MetaPaginatedTransaction model so at least it passes the "pub test" in conformity. I know that's what we've done in our internal codegen setups.

This change did arise via consultation and was consulted on, even if there was no specific DP referencing it.

The history here is that a number of banks made a specific request of the Chair to review a number of issues. One of these issues was to remove the transaction search query parameter entirely. The decision of the chair was to compromise and make this param optional. This occurred during a workshop and the change was incorporated into v1.0.0 of the standards as part of Decision #87.

The introduction of isQueryParamUnsupported came about from a comment made by Westpac in #79 that some form of error response should be documented if the query parameter is used but not supported. An error would mean an ADR would receive no result and would have no way to determine if the bank supported the parameter before calling so a successful response with a meta field was introduced in #87 as the expected behaviour.

As this is only required for one API and is not a pattern we will ever reuse it seems reasonable to introduce another model, or an inline allOf, for this specific case.

perlboy commented 1 year ago

Honestly, it's all ancient history now but one participant passing a comment buried in a holistic thread while suggesting an error is pretty far away from any reasonable interpretation of consultation when it comes to diverging from previous decisions defined by the Chair (eg. DP22 which defined pagination meta in the first place). I guess both of us are driven by a desire to be "right", it just seems we are misaligned on the threshold expected for feedback being "reasonably considered" 🤷. Regardless, I look back at the resource consumed on this and wonder what value could have been created if energy had been more focused on things of greater value.

As this is only required for one API and is not a pattern we will ever reuse it seems reasonable to introduce another model, or an inline allOf, for this specific case.

Sounds good. At least then, after 3 years, the Standards won't be in violation of it's own High Level Standards.

nils-work commented 1 year ago

This issue was discussed in the Maintenance Iteration (MI) call on 9 Aug 2023. The intention is to resolve this issue as part of MI16 by defining the expected schema details.

It was suggested that the original intent may have been for the 'text' parameter name and 'isQueryParamUnsupported' field name to be aligned, creating a 1:1 mapping. The current difference in naming may have led to issue #176 - Update Query Parameter Transaction which questioned the lack of specificity in the 'Query' field name and discussed alternative approaches to conveying querying support where multiple parameters are defined.

It was also suggested on the MI call that filtering capability could be a candidate for a separate feature support/negotiation solution.

nils-work commented 1 year ago

A fix for this issue has been staged for review: https://github.com/ConsumerDataStandardsAustralia/standards-staging/commit/e52f16e78d8db5911e09b53f0ece4a123169c984

Updated with default as true (the expected state when this property is present), added x-cds-type for Boolean and added the version delta and release notes - https://github.com/ConsumerDataStandardsAustralia/standards-staging/commit/ffa6f4553c07c0db54613784ba9f460e1bd0b60f

nils-work commented 11 months ago

This change has been incorporated into version 1.27.0