Repository navigation
feat(cpp): expose get and delete personal access tokens - #4628
Mitshah2406 wants to merge 6 commits into
Conversation
|
Note for maintainers: @Mitshah2406 also has #4617 open. New contributors are asked to keep one open PR at a time. See CONTRIBUTING.md. |
|
@Mitshah2406 please edit your pr description to follow the repository template. Also, is this ready for review, or do you need to time? |
cdd8986 to
8ba4dac
Compare
|
/ready |
|
/skill team-review-slim |
|
No skill named |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## master #4628 +/- ##
=============================================
- Coverage 90.85% 80.59% -10.26%
- Complexity 3104 3107 +3
=============================================
Files 1479 1479
Lines 361309 361379 +70
Branches 305179 305200 +21
=============================================
- Hits 328277 291267 -37010
- Misses 26124 63171 +37047
- Partials 6908 6941 +33
🚀 New features to boost your workflow:
|
|
/skill team-review |
There was a problem hiding this comment.
Summary: The C++ binding adds personal access token listing and deletion in the existing bridge pattern, but the new delete call reaches an HTTP SDK path that writes the raw token name into the URL, so a name such as ../users/42 retargets the DELETE to another endpoint. The README error list and the delete method's outcome note are also inconsistent with the rest of the API.
Counts: critical 1, warning 0, nit 2, simplification 0
This review was generated by Claude Code 2.1.284 on deepseek-flash[1m]. Review the output before you act on it.
|
/ready |
hubcio
left a comment
There was a problem hiding this comment.
main gap is tests - nothing lists or deletes a real token, and nothing covers the new HTTP path encoding. a few things outside the diff:
- the PR description only covers the C++ side - please mention the rust SDK fix in
core/sdk/src/http/personal_access_tokens.rs, since it changes the delete path for everyone on the rust HTTP client. foreign/cpp/README.md:103still leavesDeletePersonalAccessTokenout of the list of methods that fillCode()andKind().- please open the issue you offered for C# and java - both HTTP clients still put the raw token name in the delete path (
HttpMessageStream.cs:906,PersonalAccessTokensHttpClient.java:56). - follow-up, not this PR:
core/sdk/src/http/users.rs:90(also lines 37, 101, 114, 131) has the same raw-path bug, soDeleteUser("../streams/1")over HTTP sendsDELETE /streams/1. - follow-up, not this PR: a token named
logincan't be deleted over HTTP - the POST-only/personal-access-tokens/loginroute incore/server/src/http.rs:409wins over{name}and answers 405.
| }); | ||
| } | ||
|
|
||
| std::vector<PersonalAccessTokenInfo> IggyBlockingClient::GetPersonalAccessTokens() { |
There was a problem hiding this comment.
simplification: this list-and-convert loop is the seventh copy of the same shape in this file. not for this PR, but one private template helper could replace all seven.
There was a problem hiding this comment.
Agreed, a shared helper would touch the other six loops that aren't part of this change, so I am keeping it out of this PR. Happy to do it as a follow up if you want.
|
@Mitshah2406 could you rebase? create_personal_access_token's pr was merged. please use that function in the tests for the two functions you've added as well |
Sure. Will do |
…t and delete test
092a0db to
913af66
Compare
|
/ready |
Which issue does this PR address?
Closes #4612
Rationale
A C++ app could not list or delete its personal access tokens, although the Rust SDK already supports both.
What changed?
GetPersonalAccessTokens()andDeletePersonalAccessToken(name). A failed delete fillsCode(),Kind()andOperationId(), likeDeleteUser.core/sdk/src/http/personal_access_tokens.rs), which affects every Rust HTTP user. The token name went into the URL raw, soabc?xdeletedabcand../users/42hit/users/42. The name is now percent-encoded, and.and..are rejected withInvalidPersonalAccessTokenName, because the URL parser resolves them even when encoded.Local Execution
AI Usage