hyperledger / aries-cloudagent-python

Hyperledger Aries Cloud Agent Python (ACA-Py) is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://wiki.hyperledger.org/display/aries
Apache License 2.0
405 stars 510 forks source link

:art: Modify count/start query params to be Integers, not Strings #3208

Open ff137 opened 4 weeks ago

ff137 commented 4 weeks ago

Affected endpoints:

These endpoints have query params: start and count. They are of string type, and then cast to integers in method logic. count also uses a default value of 10, which was previously just in code, and not in the openapi spec.

So this PR modifies those query params to be of int type, with more logical validation, and clearer default value.

:question: Questions:

sonarcloud[bot] commented 4 weeks ago

Quality Gate Failed Quality Gate failed

Failed conditions
17.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

jamshale commented 4 weeks ago

I guess this would be considered a breaking change. I'm still not sure what the release version strategy for changes like this.

ff137 commented 4 weeks ago
  • Yes, The count/start endpoints should be renamed limit/offset.

Cool, I can implement the same limit/offset pagination query params for these 3 endpoints, as for the other endpoints.

  • GET /credentials/w3c should have limit and offset capabilities. Could be done as a separate task.

👍

I guess this would be considered a breaking change. I'm still not sure what the release version strategy for changes like this.

Indeed. One strategy is just do it! Rip and replace. Alternatively, a better strategy is to announce deprecation before ripping and replacing. So, I can keep the count/start query params in place (just mark as deprecated for the openapi spec), with existing functionality, but also add limit/offset as new query params, which would override count/start if set. People unaware of count/start change won't get any breaking changes. And people aware can start using limit/offset instead. So, announce deprecation in next release, and then remove count/start in next minor release after that. That's in general a good approach to follow, just a bit more work to maintain backward compatibility in the interim.