openwallet-foundation / acapy

ACA-Py is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://aca-py.org
Apache License 2.0
419 stars 512 forks source link

:art: Deprecate count/start query params and implement limit/offset #3208

Open ff137 opened 2 months ago

ff137 commented 2 months 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:


Edit: now keeps count/start behavior as previous, but marks it as deprecated. limit/offset is implemented alongside the old query params, and will be used instead, if provided.

So - no breaking changes, but count/start should be dropped in a future release.

sonarcloud[bot] commented 2 months ago

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

jamshale commented 2 months 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 2 months 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.

sonarcloud[bot] commented 1 week ago

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarQube Cloud