Cleanup interfaces of http requests and /token#202
Conversation
avalerio-tkd
commented
Dec 18, 2025
avalerio-tkd
commented
- Cleanups and small refactors for /token endpoint and client http requests.
argmarco-tkd
left a comment
argmarco-tkd
left a comment
There was a problem hiding this comment.
Thanks for this. Overall LGTM. Left a few relatively minor comments. No need for a new PR unless you need me to.
| struct TokenRequest { | ||
| std::string client_id; | ||
| std::string api_key; | ||
| std::optional<std::string> error_message; |
There was a problem hiding this comment.
wait, error_message in the Request object?
There was a problem hiding this comment.
You're right, that's an overlook. It's an attempt to compile JsonRequest / Token / Response. Will rework it a bit and ping back.
|
|
||
| // GenerateJWT implementation | ||
| std::optional<std::string> ClientCredentialStore::GenerateJWT(const std::string& client_id, const std::string& api_key) const { | ||
| std::optional<std::pair<std::string, std::int64_t>> ClientCredentialStore::GenerateJWT( |
There was a problem hiding this comment.
can wait for a later iteration, but should 'Token' be a more complex type which also contains the 'expiration time' ? (rather than keeping the concepts together via a pair)
There was a problem hiding this comment.
Yeah, was attempting to not add too many structs around. Will rework a bit these classes. Thanks for the comments.
| auto response2 = store.ProcessTokenRequest(valid_json2); | ||
| EXPECT_TRUE(response2.token.has_value()); | ||
| EXPECT_FALSE(response2.token.value().empty()); | ||
| EXPECT_TRUE(response2.expires_at.has_value()); |
There was a problem hiding this comment.
where are we testing that expires_at has, well, a 'proper' value? Is that in an upcoming PR?
There was a problem hiding this comment.
Added it, then removed it. Will add it back again.
- Updated logic to populate TokenResponse with error message and status code. - Added TokenWithExpiration struct to auth_utils.h and auth_utils.cpp.
|
Ok, finally cleaned up lots of nits. Merging.. |
2 participants
