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

Add isQueryParamUnsupported to MetaPaginated for schema validation #469

Closed
CDR-API-Stream opened this issue Feb 10, 2022 · 9 comments
Closed
Labels
Banking Banking domain APIs Non-breaking change A change that is not expected to result in a new endpoint version. Proposal made The DSB has proposed a specific change to the standards to address the change request Schema Issues related to schema.
Milestone

Comments

@CDR-API-Stream
Copy link
Collaborator

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.

@CDR-API-Stream CDR-API-Stream added Documentation Improvements, additions or queries related to documentation change request Energy Banking Banking domain APIs labels Feb 10, 2022
@perlboy
Copy link

perlboy commented Feb 13, 2022

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
Copy link

ghost commented Sep 15, 2022

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
Copy link

perlboy commented Sep 18, 2022

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.

@nils-work nils-work added the Schema Issues related to schema. label Feb 3, 2023
@nils-work nils-work removed the Energy label Apr 17, 2023
@nils-work nils-work removed the Documentation Improvements, additions or queries related to documentation label Apr 28, 2023
@biza-io
Copy link

biza-io commented Jun 13, 2023

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
Copy link
Contributor

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
Copy link

perlboy commented Jun 13, 2023

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
Copy link
Member

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 nils-work added this to the v1.27.0 milestone Aug 24, 2023
@nils-work
Copy link
Member

nils-work commented Sep 6, 2023

A fix for this issue has been staged for review: ConsumerDataStandardsAustralia/standards-staging@e52f16e

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 -
ConsumerDataStandardsAustralia/standards-staging@ffa6f45

nils-work added a commit to ConsumerDataStandardsAustralia/standards-staging that referenced this issue Sep 13, 2023
@nils-work nils-work added the Proposal made The DSB has proposed a specific change to the standards to address the change request label Sep 20, 2023
JamesMBligh added a commit to ConsumerDataStandardsAustralia/standards that referenced this issue Oct 10, 2023
* Remove 1.25.0 diff statements

* Rebuild

* Add release note skeleton
Update version numbers
Update archive links
Removed obsolete known issue

* Release notes
Add details of DP322

* Fixed typo

* Fix typos

* Fixed broken link

* Updated diff

* Fixed typo

* Fixed typos

* Fixed date formats

* Updated code samples
Rebuild

* Rebuild

* Fixed diff statement
Rebuild

* Fix broken links

* Rebuild

* Rebuild

* Fix publish date

* Fixed issues from review
Rebuild

* Create releasenotes.1.27.0.html.md

* Standards Maintenance Issue 599: Update CDR website link in the Informative References section.

Address comment: ConsumerDataStandardsAustralia/standards-maintenance#599 (comment)

* Updated Version to 1.27.0

* Standards Maintenance Issue 599: Fix typo: registeration

Addresses comment: ConsumerDataStandardsAustralia/standards-maintenance#599 (comment)

* Standards Maintenance Issue 599: Fix v1.25.0 notes

Addresses issue comment: ConsumerDataStandardsAustralia/standards-maintenance#599 (comment)

* Corrected typo in description of software_roles

Addresses comment: ConsumerDataStandardsAustralia/standards-maintenance#599 (comment)

* Corrected typos in EnergyInvoiceGasUsageCharges

Addresses comment: ConsumerDataStandardsAustralia/standards-maintenance#599 (comment)

* Corrected authorisations reqt in Get Metrics v4/v5

Addresses comment: ConsumerDataStandardsAustralia/standards-maintenance#599 (comment)

* Corrections to Bad Request responses

Addresses comment: ConsumerDataStandardsAustralia/standards-maintenance#599 (comment)

* Updated FAPI-RW reference

Addresses comment: ConsumerDataStandardsAustralia/standards-maintenance#599 (comment)

* Removed references to 'Other Versions' in Telco

Addresses comment: ConsumerDataStandardsAustralia/standards-maintenance#599 (comment)

* Clarified deprecated Energy endpoint expectations

Addresses comment: ConsumerDataStandardsAustralia/standards-maintenance#599 (comment)

* Remove g old diff statements, update version number in intro and master swagger files, add entry in changelog table, create blank releasenotes file, add entry for previous version in archives table, remove all diff statements excluding intro section.  Added python script to perform all these tasks in new utils folder

* Updated version utility script with fix to version regex and improved error handling

* Updated example SSAs

Addresses: ConsumerDataStandardsAustralia/standards-staging#197

* Line wrapping and scrollbars

Addresses: ConsumerDataStandardsAustralia/standards-staging#305

* Changed Register endpoint x-v headers to mandatory

Addresses comment: ConsumerDataStandardsAustralia/standards-maintenance#599 (comment)

* Fixed URL mistakes introduced by version script

* isQueryParamUnsupported property

Addresses: ConsumerDataStandardsAustralia/standards-maintenance#469

* Body parameter for JWT POST + PUT

Addresses: ConsumerDataStandardsAustralia/standards-staging#306

* Updated type

* Removing hyphen from 'High-Level Standards'

For consistency with older release notes and current section heading

* Incorrect levelling of largeSecondary object

Addresses: ConsumerDataStandardsAustralia/standards-staging#196

* Metrics: Performance threshold including SDH

Addresses: ConsumerDataStandardsAustralia/standards-maintenance#605

* Clarification of Energy PRD Obligations

Addresses: ConsumerDataStandardsAustralia/standards-maintenance#611

* Incorrect non-normative example

Addresses: ConsumerDataStandardsAustralia/standards-staging#307

* Fix link to FAPI section reference

Addresses: ConsumerDataStandardsAustralia/standards-staging#308

* Property names wrapping to two lines

Addresses: ConsumerDataStandardsAustralia/standards-staging#309

* Links to schemas land below the schema name

Addresses: ConsumerDataStandardsAustralia/standards-staging#311

* Adding Metrics v3 and V4 with bold MUST

Addresses: ConsumerDataStandardsAustralia/standards-maintenance#605

* Non-Bank Lending Draft

Based on Banking and Register APIs with proposed changes to product-category values and new path in Register Data Holder endpoints only.

* Updates to Change Log and Release Notes

* Update _register.md

Updated to remove the reference to a specific technology pattern (ie. 'redirect')

* Update _register.md

Reverted the change because the text was explicit in the decision

* Update cds_banking.json

Default of absent isQueryParamUnsupported should be false

* Rebuild

* Fix issue introduced in merge

* Rebuild

* Minor fixes

* Update release date of 1.27.0
Added release notes and diff statement for NBL draft
Minor changes to NBL draft

* Rebuild

* Update

---------

Co-authored-by: Nils Berge <60594671+nils-work@users.noreply.github.com>
Co-authored-by: kirkycdr <brian.kirkpatrick@consumerdatastandards.gov.au>
Co-authored-by: Hemang Rathod <hemang.rathod@consumerdatastandards.gov.au>
Co-authored-by: Mark Verstege <2514377+markverstege@users.noreply.github.com>
Co-authored-by: James Bligh <james@promisetopay.com.au>
@nils-work
Copy link
Member

This change has been incorporated into version 1.27.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Banking Banking domain APIs Non-breaking change A change that is not expected to result in a new endpoint version. Proposal made The DSB has proposed a specific change to the standards to address the change request Schema Issues related to schema.
Projects
Status: Done
Development

No branches or pull requests

5 participants