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

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented Aug 29, 2024

Affected endpoints:

  • v1 present_proof_credentials_list (GET /present-proof/records/{pres_ex_id}/credentials)
  • v2 present_proof_credentials_list (GET /present-proof-2.0/records/{pres_ex_id}/credentials)
  • holder credentials_list (GET /credentials)

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.

❓ Questions:

  • For consistency, count/start should probably be renamed to limit/offset? As with other paginated endpoints.
  • Note: w3c_creds_list (GET /credentials/w3c) uses the same query schema as GET /credentials, but it does not make use of count or start ... Should that be implemented?

Copy link

sonarcloud bot commented Aug 29, 2024

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

@jamshale
Copy link
Contributor

  • Yes, The count/start endpoints should be renamed limit/offset.
  • 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.

@ff137
Copy link
Contributor Author

ff137 commented Aug 30, 2024

  • 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.

@ff137 ff137 marked this pull request as draft August 30, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants