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

[ff137]

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.

:question: 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?

[sonarqubecloud[bot]]

Quality Gate failed

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

See analysis details on SonarCloud

[jamshale]

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

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

[swcurran]

Any status update on this PR so we can raise it in the ACA-Pug meeting tomorrow -- 2024.11.16 @ 8:00 Pacific / 17:00 Central Europe?

[ff137]

I think it's ready for review / good to merge. We're using it on our fork. Essentially deprecates the string count and start params, and allows passing new limit and offset integer types. Backward compatible

Apologies for not joining the regular ACA-Pug sessions - I have routine plans for that time on Tuesdays

[swcurran]

Looks like there is a test failing because start is showing up unexpectedly. Can you please take a look @ff137 ? From the log:

=========================== short test summary info ============================
FAILED acapy_agent/protocols/present_proof/v2_0/tests/test_manager_anoncreds.py::TestV20PresManagerAnonCreds::test_no_matching_creds_indy_handler - TypeError: AnonCredsHolder.get_credentials_for_presentation_request_by_referent() got an unexpected keyword argument 'start'
1 failed, 4963 passed, 51 skipped, 6 xfailed in 155.61s (0:02:35)
Error: Process completed with exit code 1.

[sonarqubecloud[bot]]

Quality Gate failed

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

See analysis details on SonarQube Cloud

[ff137]

I'll take a swing at reducing the duplication

[ff137]

Oh shucks, I remember why I didn't deduplicate by inheriting from the PaginatedQuerySchema earlier. It's so that there's still backward compatibility, so that limit/offset is not loaded with default values, and it'll stick to existing defaults instead ... It's really much of a muchness to preserve backward compatibility, but I'm going to revert my changes, since it didn't even improve the quality check.

Rememberdc this thanks to my note: TODO: Remove start/count and swap to PaginatedQuerySchema and get_limit_offset

[ff137]

I'm hoping this one can be merged sometime soon.

[sonarqubecloud[bot]]

Quality Gate failed

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

See analysis details on SonarQube Cloud