[client] add auth tokens management methods #36
Conversation
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
WalkthroughAdds CI/workflow upgrades and a disabled benchmark job; replaces Makefile; migrates golangci-lint config; introduces NewClient constructors and BaseURL constants/aliases; adds smsgateway Config builder, token DTOs and token endpoints; expands health/response types and refactors tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev
participant Config as smsgateway.Config
participant Client as smsgateway.Client
participant REST as rest.Client
Note right of Config `#DFF0D8`: Build config (With*/Validate)
Dev->>Config: WithBaseURL(...).WithJWTAuth(...)
Dev->>Config: Validate()
Config-->>Dev: validated Config
Dev->>Client: NewClient(Config)
Client->>Client: apply defaults (http.Client, BaseURL, auth)
Client->>REST: wrap configured rest.Client
alt Generate token flow
Dev->>Client: GenerateToken(ctx, TokenRequest)
Client->>REST: POST /auth/token (with auth header)
REST-->>Client: TokenResponse / error
Client-->>Dev: TokenResponse / error
end
alt Revoke token flow
Dev->>Client: RevokeToken(ctx, jti)
Client->>REST: DELETE /auth/token/{jti}
REST-->>Client: 200 / error
Client-->>Dev: success / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code graph analysis (1)smsgateway/client.go (2)
🔇 Additional comments (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
smsgateway/domain_devices.go (1)
19-25: Consider aligning the LastSeen field tags for consistency.Lines 19-23 have been reformatted with aligned spacing between the
jsonandexampletags, but Line 25 (LastSeen) retains single-space formatting. Aligning all fields uniformly would improve readability.Apply this diff to align the LastSeen field:
- LastSeen time.Time `json:"lastSeen" example:"2020-01-01T00:00:00Z"` // Last seen at (read only) + LastSeen time.Time `json:"lastSeen" example:"2020-01-01T00:00:00Z"` // Last seen at (read only)ca/const.go (1)
3-6: LGTM! Good backward compatibility approach.The introduction of
BaseURLfollowing Go naming conventions while maintainingBASE_URLas an alias ensures backward compatibility. The expanded nolint directive appropriately suppresses linter warnings for the deprecated naming convention.Consider adding a deprecation comment to
BASE_URLto guide users toward the new constant:-//nolint:revive,staticcheck // backward compatibility -const BASE_URL = BaseURL +// Deprecated: Use BaseURL instead. +//nolint:revive,staticcheck // backward compatibility +const BASE_URL = BaseURLsmsgateway/dto_auth.go (1)
5-31: Auth DTOs look good; consider clarifying TTL omitempty semanticsThe
TokenRequest/TokenResponseshapes and tags look appropriate for token management, and the validation onScopesenforces a non-empty set, which is good.One minor nit: with
TTLtagged asjson:"ttl,omitempty", a zero value won’t be sent at all. Since the comment states “A value of 0 will result in a token with the maximum allowed lifetime”, you might slightly rephrase to something like “omitted or 0” (depending on server semantics) to make the client behavior and server contract perfectly aligned.smsgateway/responses_health.go (1)
21-31: Consider omitempty for ObservedValue.The
ObservedValuefield is anintwithout theomitemptyJSON tag, meaning zero values will always be serialized. If0is a meaningful observed value, this is correct. Otherwise, consider addingomitemptyor using a pointer type.If zero should be omitted:
- ObservedValue int `json:"observedValue"` + ObservedValue int `json:"observedValue,omitempty"`Or if you need to distinguish between "not set" and "zero":
- ObservedValue int `json:"observedValue"` + ObservedValue *int `json:"observedValue,omitempty"`smsgateway/config_test.go (2)
41-69: Prefer usingsmsgateway.BaseURLconstant instead of duplicating the literalThe
TestConfig_WithBaseURLdefault case hardcodes"https://api.sms-gate.app/3rdparty/v1". Usingsmsgateway.BaseURLforexpectedwould keep the test aligned with the package’s exported default and avoid divergence if the constant ever changes.
164-234: MakeValidatetests assert on error type rather than full string
TestConfig_Validatecompareserr.Error()to"invalid config: missing auth credentials", which is brittle and tightly couples the test to the exact formatting of the wrapped error. SinceValidatewrapsErrInvalidConfig, consider switching toerrors.Is(err, smsgateway.ErrInvalidConfig)and optionally asserting that the message contains"missing auth credentials"instead of matching the whole string.smsgateway/config.go (1)
55-60: Consider makingValidatepart of the documented client-construction flow
Validatecorrectly enforces “at least one of user/password/token” and wrapsErrInvalidConfig. However, nothing in this package currently calls it;NewClientcan be constructed with an all-zeroConfig, which will then send requests with a dummyBasic Og==header and rely on the server to reject them. It might be worth:
- Documenting that callers should invoke
config.Validate()beforeNewClient, and/or- Adding helper(s) that both validate and construct the client.
Not critical, but it would make misuse harder.
smsgateway/client.go (1)
14-21: New client construction and auth header wiring look correct; consider clarifying zero-auth behavior
BaseURL/BASE_URLplusNewClientcorrectly defaultBaseURLwhen empty and honorTokenover basic credentials when building theAuthorizationheader. Centralizing auth into a singleheadersmap is clean and keeps per-call code simple.One nuance: if callers pass a
Configwith all auth fields empty (and skipValidate),NewClientwill still send aBasic Og==header. Given you’ve introducedConfig.Validate, it may be worth documenting that callers should validate configs before constructing the client, or updating the doc comment onNewClientto mention that behavior.Also applies to: 28-48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.github/workflows/go.yml(3 hunks).gitignore(3 hunks).golangci.yml(12 hunks)Makefile(1 hunks)ca/client.go(1 hunks)ca/client_test.go(2 hunks)ca/config.go(1 hunks)ca/config_test.go(1 hunks)ca/const.go(1 hunks)ca/requests.go(1 hunks)rest/client.go(3 hunks)smsgateway/client.go(11 hunks)smsgateway/config.go(1 hunks)smsgateway/config_test.go(1 hunks)smsgateway/domain_devices.go(1 hunks)smsgateway/domain_logs.go(1 hunks)smsgateway/domain_messages.go(4 hunks)smsgateway/domain_settings_test.go(1 hunks)smsgateway/domain_upstream.go(1 hunks)smsgateway/domain_webhooks.go(2 hunks)smsgateway/dto_auth.go(1 hunks)smsgateway/errors.go(1 hunks)smsgateway/requests.go(1 hunks)smsgateway/requests_mobile.go(1 hunks)smsgateway/responses.go(1 hunks)smsgateway/responses_health.go(2 hunks)smsgateway/responses_mobile.go(1 hunks)smsgateway/webhooks/webhook.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
smsgateway/config_test.go (2)
smsgateway/client.go (2)
Client(22-26)BaseURL(14-14)smsgateway/config.go (1)
Config(8-14)
ca/requests.go (1)
ca/enums.go (1)
CSRType(4-4)
smsgateway/responses_mobile.go (2)
smsgateway/domain_devices.go (1)
Device(18-26)smsgateway/domain_messages.go (1)
Message(72-90)
ca/config.go (2)
smsgateway/client.go (1)
BaseURL(14-14)ca/const.go (1)
BaseURL(3-3)
ca/config_test.go (2)
smsgateway/client.go (1)
BaseURL(14-14)ca/const.go (1)
BaseURL(3-3)
smsgateway/requests_mobile.go (1)
smsgateway/domain_messages.go (2)
ProcessingState(11-11)RecipientState(175-179)
rest/client.go (3)
smsgateway/client.go (2)
NewClient(29-48)Client(22-26)ca/client.go (2)
NewClient(17-29)Client(12-14)ca/config.go (1)
Config(7-10)
ca/client.go (2)
rest/client.go (3)
NewClient(21-27)Client(17-19)Config(12-15)ca/config.go (2)
Option(5-5)Config(7-10)
smsgateway/config.go (2)
smsgateway/client.go (2)
Client(22-26)BaseURL(14-14)smsgateway/errors.go (1)
ErrInvalidConfig(7-7)
smsgateway/requests.go (1)
smsgateway/domain_upstream.go (1)
PushNotification(17-21)
ca/const.go (1)
smsgateway/client.go (1)
BaseURL(14-14)
smsgateway/client.go (3)
rest/client.go (3)
Client(17-19)NewClient(21-27)Config(12-15)smsgateway/config.go (1)
Config(8-14)smsgateway/dto_auth.go (2)
TokenRequest(12-15)TokenResponse(26-31)
🔇 Additional comments (29)
smsgateway/domain_devices.go (1)
5-17: LGTM! Excellent documentation.The block comment comprehensively documents the Device struct and clarifies which fields are read-only, helping API consumers understand the data model.
ca/requests.go (1)
7-9: LGTM! Improved readability.The formatting changes improve code readability by aligning struct tags and inline comments consistently.
ca/config.go (1)
21-21: LGTM! Consistent with naming standardization.The change from
BASE_URLtoBaseURLaligns with the new naming convention. The oldBASE_URLconstant remains available as an alias for backward compatibility.ca/config_test.go (1)
57-57: LGTM! Test updated correctly.The test correctly references the new
BaseURLconstant, maintaining consistency with the naming standardization across the package.ca/client.go (1)
16-29: LGTM! Well-implemented builder pattern.The new
NewClientconstructor follows the functional options pattern idiomatically, allowing flexible configuration while providing sensible defaults throughconfig.Client()andconfig.BaseURL().ca/client_test.go (2)
38-42: LGTM! Formatting improvement.The multi-line format improves readability of the JSON response without changing the test behavior.
103-107: LGTM! Formatting improvement.Consistent with the formatting change above, improving readability of the JSON response.
smsgateway/domain_settings_test.go (1)
93-96: Test helper comment change is fineComment punctuation update around
ptris harmless; helper remains clear and local to tests.smsgateway/requests_mobile.go (1)
5-52: Request DTO documentation looks consistent with existing APIThe added comments for the mobile request types improve clarity and match the current field tags/validation (e.g., required IDs, optional push tokens, password constraints, and patch message structure). No behavioral changes spotted.
smsgateway/responses.go (1)
3-14: ErrorResponse docs are clear and non-breakingThe new documentation for
ErrorResponseand its fields is clear and matches the existing tags and usage; no API or behavioral changes introduced..gitignore (1)
2-136: New ignore rules are sensible for this repoThe updated template header,
Iconfix, and added rules for build artifacts, configs, workspaces, coverage, and benchmarks all look appropriate and low-risk for this project.smsgateway/webhooks/webhook.go (1)
5-26: Webhook alias deprecation messaging is clearThe deprecation comments on
EventType, constants,IsValidEventType, andWebhookclearly direct users to thesmsgatewaypackage while preserving the existing aliases and behavior.smsgateway/responses_mobile.go (1)
5-48: Mobile response docs align with existing structuresThe enhanced comments for mobile response types make the API surface clearer (device info, registration result, one-time codes, and messages) without changing JSON tags or field types. Looks good.
smsgateway/domain_messages.go (1)
1-179: LGTM! Documentation improvements enhance clarity.The comprehensive documentation added throughout this file improves code maintainability and developer experience. All changes are confined to comments and documentation—no functional logic has been altered.
Makefile (3)
6-7: Verify DOCKER_CR calculation logic.The
DOCKER_CRvariable derives frombasename $$(dirname $(PWD)), which assumes the parent directory name is the container registry. This assumption may not hold in all repository layouts and could produce unexpected image names.Consider making this configurable with a clearer default:
-DOCKER_CR ?= $(shell basename $$(dirname $(PWD))) +DOCKER_CR ?= ghcr.io/android-sms-gatewayOr document the expected directory structure if this is intentional.
20-26: LGTM! Excellent test configuration.The test flags are well-chosen:
-raceenables race detection-shuffle=on -count=1ensures test independence-covermode=atomic -coverpkg=./...provides comprehensive coverage
30-36: LGTM! Proper tool availability check.The
airtarget correctly validates tool installation before execution and provides clear installation instructions if missing.smsgateway/domain_upstream.go (1)
1-21: LGTM! Documentation and formatting improvements.The inline comments for constants and enhanced struct documentation improve code clarity. Field validation remains appropriate.
smsgateway/requests.go (1)
8-22: LGTM! Well-designed time range validation.The
MessagesExportRequeststruct correctly validates the time range with bidirectional constraints:
Sincevalidatesltefield=UntilUntilvalidatesgtefield=SinceThis ensures
Since < Untilin both directions, preventing invalid time ranges.smsgateway/domain_webhooks.go (2)
51-79: LGTM! Enhanced documentation and internal consistency.The
Webhookstruct documentation has been improved, and theValidatemethod correctly uses the renamedIsValidWebhookEventfunction.
44-49: All callers have been correctly updated; backward compatibility is maintained.The function rename from
IsValidtoIsValidWebhookEventhas been properly handled. Tests are using the new function name, and a deprecated wrapper functionIsValidEventTypeexists in the webhooks package to maintain backward compatibility for existing users. No remaining calls to the old function name exist in the codebase.smsgateway/errors.go (1)
7-7: LGTM! New error constant follows established patterns.The new
ErrInvalidConfigerror is appropriately defined and follows the existing naming and message conventions..github/workflows/go.yml (3)
21-23: LGTM! Proper concurrency configuration.The concurrency settings will cancel in-progress workflow runs when new commits are pushed to the same ref, preventing wasted CI resources.
75-130: Benchmark job is disabled but well-configured.The new Benchmark job is currently disabled (
if: falseon line 78) but appears properly configured for future use. When enabled, it will:
- Track benchmark performance over time
- Fail on performance regressions (
fail-on-alert: true)- Cache results for historical comparison
Consider documenting why this job is disabled and when it will be enabled.
42-42: No compatibility issues identified—upgrades are safe to proceed.Verification of the workflow configuration against the breaking changes in both actions:
golangci-lint-action v8.0.0: The workflow uses
version: latestand does not employ theworking-directoryoption. Breaking changes (absolute paths with working-directory, golangci-lint >= v2.1.0 requirement) do not apply here.codecov-action v5: The workflow explicitly provides
token: ${{ secrets.CODECOV_TOKEN }}and does not use the deprecatedfileorplugininputs. Breaking changes in input names and token handling do not affect this configuration.smsgateway/config.go (1)
8-53: Config builder API looks consistent and chainable
ConfigplusWithClient,WithBaseURL,WithJWTAuth, andWithBasicAuthform a clear, immutable-style builder. Value receivers and returningConfigmake chaining (Config{}.WithClient(...).WithBaseURL(...)...) straightforward, and the default handling forclient == nilandbaseURL == ""matches the struct comments. No functional issues spotted here.smsgateway/client.go (2)
50-208: Refactor toDowith shared headers is consistent across endpointsAll existing methods (
Send,GetState,ListDevices,DeleteDevice,CheckHealth,ExportInbox,GetLogs,GetSettings,UpdateSettings,ReplaceSettings,ListWebhooks,RegisterWebhook,DeleteWebhook) now usec.Do(..., c.headers, ...)and wrap errors with operation-specific messages. Paths, HTTP verbs, and URL escaping (url.PathEscape) all look consistent, and sharingc.headersensures auth is applied uniformly across calls without duplicating logic.
210-233: Token management methods align with the rest of the client API
GenerateToken(POST/auth/token) andRevokeToken(DELETE/auth/token/{jti}) follow the sameDo+c.headerspattern and wrap errors with clear operation-specific messages. Usingurl.PathEscapeforjtiis appropriate, and the request/response types (TokenRequest/TokenResponse) fit the existing DTO style. No functional issues spotted..golangci.yml (1)
4-205: v2 golangci-lint config structure and enabled linters look coherent for a strict setupThe config uses the v2 schema (
version: "2",issues,formatters,linters.settings,linters.exclusions) correctly and pulls in a broad but consistent set of linters plus formatters (goimports/golines/swaggo). Given the project size, this strict setup should be manageable and helps keep style and error handling uniform. I don’t see anything here that would obviously break this repo; any remaining tuning can be done incrementally as real false positives surface.Also applies to: 216-471
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
smsgateway/client_test.go (5)
474-511: DeleteDevice tests now cover only the success pathThe
TestClient_DeleteDevicetable currently exercises only the 204-success case. That’s fine functionally, but you might want to keep (or reintroduce) at least one non-2xx scenario (e.g., 404) to ensure error propagation is covered, similar toTestClient_DeleteWebhook.
513-578: Remove duplicatedCheckHealthtest loop
TestClient_CheckHealthruns the same table of cases twice via two identicalfor _, tt := range tests { ... }loops. This is redundant and doubles runtime without adding coverage. Keeping just one loop will simplify maintenance and avoid confusion.func TestClient_CheckHealth(t *testing.T) { @@ - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - server := newMockServer(mockServerExpectedInput{ - method: http.MethodGet, - path: "/health", - }, mockServerOutput{ - code: tt.code, - body: tt.body, - }) - defer server.Close() - - client := newClient(server.URL) - resp, err := client.CheckHealth(context.Background()) - if (err != nil) != tt.wantErr { - t.Errorf("CheckHealth error = %v, wantErr %v", err, tt.wantErr) - } - if !tt.wantErr && !reflect.DeepEqual(resp, tt.want) { - t.Errorf("CheckHealth response = %v, want %v", resp, tt.want) - } - }) - } - for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { server := newMockServer(mockServerExpectedInput{ method: http.MethodGet, path: "/health", @@ }) } }
581-626: ExportInbox test is good; body check is effectively only for the success caseThe
TestClient_ExportInboxtable nicely validates content type and JSON body for the happy-path case. Note that in the “Error response” case the emptyMessagesExportRequestwill make the body differ from the hard-coded expected JSON, so the mock will return 400 instead of the configured 500. Since you only assertwantErr, this is still correct, but thecodefield in that test case isn’t actually exercised.
687-736: Ensureptrhelper is defined somewhere in thesmsgateway_testpackageThis file uses
ptr(30)to build*intvalues forLogLifetimeDays, but there’s noptrhelper defined in this file or imported. If there isn’t afunc ptr[T any](v T) *Tin another_test.gofile in the same package, these tests will fail to compile.If you need to add it, something like this in any
_test.goin thesmsgateway_testpackage is enough:func ptr[T any](v T) *T { return &v }
923-961: RevokeToken tests exercise both success and error paths; consider tying mock path tott.jti
TestClient_RevokeTokencorrectly checks both 204 and error status handling forRevokeToken. Right now the mock path is hard-coded to/auth/token/abc123and both cases usejti: "abc123", so you won’t catch path-construction bugs for other values.Not critical, but you could tighten the test by deriving the expected path from
tt.jti:for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - server := newMockServer(mockServerExpectedInput{ - method: http.MethodDelete, - path: "/auth/token/abc123", - }, mockServerOutput{ + server := newMockServer(mockServerExpectedInput{ + method: http.MethodDelete, + path: "/auth/token/" + tt.jti, + }, mockServerOutput{ code: tt.code, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
smsgateway/client_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
smsgateway/client_test.go (5)
smsgateway/responses_health.go (1)
HealthResponse(47-58)smsgateway/requests.go (1)
MessagesExportRequest(15-22)smsgateway/domain_logs.go (1)
LogEntry(15-28)smsgateway/domain_settings.go (2)
DeviceSettings(42-57)SettingsMessages(73-101)smsgateway/dto_auth.go (2)
TokenRequest(12-15)TokenResponse(26-31)
🪛 Gitleaks (8.29.0)
smsgateway/client_test.go
[high] 879-879: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 883-883: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (2)
smsgateway/client_test.go (2)
628-685: Logs tests correctly pin query encoding and response decodingThe
TestClient_GetLogscases look solid: you precomputefrom/to, useurl.QueryEscapeplus RFC3339 formatting to match the client’s expected query string, and verify both the success decode path and an error path. No issues here.
738-864: Settings tests provide good coverage of PATCH vs PUT behavior
TestClient_UpdateSettingsandTestClient_ReplaceSettingsboth validate:
- Request method (PATCH vs PUT),
- JSON body shape/content type for the same payload,
- Happy-path decoding and error-path handling.
That’s a nice, consistent pattern that will catch regressions in settings serialization and response handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
smsgateway/client_test.go (1)
951-989: RevokeToken tests exercise both success and error responses; small optional improvement
TestClient_RevokeTokencleanly covers both the 204 and error cases forRevokeTokenusing the samejtiand endpoint. Behavior‑wise this is fine.If you later add more cases with different JTIs, it might be slightly clearer to derive the mock path from
tt.jti(e.g.path: "/auth/token/" + tt.jti) so the expectation and input stay in lockstep, but it’s not required for correctness here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
smsgateway/client_test.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
smsgateway/client_test.go (1)
smsgateway/client.go (3)
Client(22-26)NewClient(29-48)BaseURL(14-14)
🔇 Additional comments (8)
smsgateway/client_test.go (8)
22-29: Mock server authorization defaults look correct and flexibleDefaulting
input.authorizationtoauthorizationHeaderand then validatingAuthorizationagainstinput.authorizationlets you reuse the same helper for both Basic and Bearer flows without duplicating setup. This keeps tests readable while still asserting headers strictly. No issues here.Also applies to: 36-39, 57-60
88-123: JWT client helper and send test cover the Bearer flow well
newJWTClientcorrectly exercises theTokenbranch ofNewClient, andTestJWTClient_Sendasserts the Bearer header, JSON body, and content type against the mock server. This gives you a clean regression test for the JWT send path without impacting the existing Basic‑auth tests.
563-607: Table‑driven CheckHealth tests are solidThe refactor to a table‑driven
TestClient_CheckHealthcleanly covers success and error responses, reusing the same mock harness and asserting bothwantErrand the parsedHealthResponseon success. This is a nice improvement in structure and coverage.
656-713: GetLogs test correctly validates query encoding and response parsingUsing shared
from/tovariables andurl.QueryEscapeto build the expectedRawQuerymatches how a typical client would encode RFC3339 timestamps. The test also checks both success (includingCreatedAtparsing) and error paths via table cases, which is a good balance of behavior and robustness.
715-764: GetSettings tests nicely cover success and error pathsThe table‑driven
TestClient_GetSettingsasserts both HTTP behavior and the decodedDeviceSettings, including the nestedMessages.LogLifetimeDaysviaptr(30). The error case confirms that failures return an empty struct with an error. This is clear and maintainable.
766-828: UpdateSettings tests enforce JSON payload and round‑trip behavior
TestClient_UpdateSettingsverifies that:
- The client sends the correct JSON body for
DeviceSettings(includinglog_lifetime_days), and- The response is unmarshalled back into the same structure on success, with a separate case for server errors.
This is a good end‑to‑end test of the settings update path using the shared mock helper.
830-892: ReplaceSettings tests mirror UpdateSettings coverage appropriately
TestClient_ReplaceSettingsmirrors theUpdateSettingstests but against the PUT endpoint, asserting the same body shape and response round‑trip. Keeping these tests parallel makes future changes to the settings payload much easier to validate.
894-949: GenerateToken tests look correct and resolve prior Gitleaks concern
TestClient_GenerateTokenvalidates:
- The request body for
TokenRequest(ttlandscopes),- The
/auth/tokenendpoint wiring, and- Proper parsing of
TokenResponse, includingExpiresAt.The switch to clearly non‑secret fixture values (
token_id_example,access_token_example) also avoids the Gitleaks “generic API key” false positives mentioned in the earlier review, without weakening test coverage.
capcom6
force-pushed
the
client/add-tokens-management-methods
branch
from
29a18b8 to
f6ac35f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
smsgateway/client_test.go (1)
609-653: Align ExportInbox error test with configured mock body and statusThe
"Error response"case still uses the same expected request body innewMockServeras the success case:
- For
"Success"the body matches the JSON generated fromMessagesExportRequestand returnstt.code(200).- For
"Error response"tt.reqis the zero value, so the actual body will not equal the fixed string. The handler returns400 Bad Requestfrom the body‑mismatch branch, nottt.code(500).The test only checks
wantErr, so it passes, but it doesn’t exercise the intended 500‑status path.You can make the behavior match the table by driving the expected request body from the test case:
func TestClient_ExportInbox(t *testing.T) { tests := []struct { - name string - req smsgateway.MessagesExportRequest - code int - wantErr bool + name string + req smsgateway.MessagesExportRequest + code int + expectedReqBody string + wantErr bool }{ { name: "Success", req: smsgateway.MessagesExportRequest{ DeviceID: "qTRWxZkF", Since: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC), Until: time.Date(2023, 1, 2, 0, 0, 0, 0, time.UTC), }, - code: http.StatusOK, + code: http.StatusOK, + expectedReqBody: `{"deviceId":"qTRWxZkF","since":"2023-01-01T00:00:00Z","until":"2023-01-02T00:00:00Z"}`, wantErr: false, }, { - name: "Error response", - req: smsgateway.MessagesExportRequest{}, - code: http.StatusInternalServerError, - wantErr: true, + name: "Error response", + req: smsgateway.MessagesExportRequest{}, + code: http.StatusInternalServerError, + // leave expectedReqBody empty so the mock doesn't enforce a specific body + expectedReqBody: "", + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { server := newMockServer(mockServerExpectedInput{ method: http.MethodPost, path: "/inbox/export", contentType: "application/json", - body: `{"deviceId":"qTRWxZkF","since":"2023-01-01T00:00:00Z","until":"2023-01-02T00:00:00Z"}`, + body: tt.expectedReqBody, }, mockServerOutput{ code: tt.code, body: `{}`, })This way, the success case validates the exact JSON body, and the error case actually tests the configured error status code.
🧹 Nitpick comments (6)
smsgateway/config_test.go (3)
41-69: Avoid hardcoding the default BaseURL literal in testsThe
WithBaseURLtest duplicates the default URL string instead of using the exported constant. This will silently drift ifBaseURLever changes.You can make the test more robust by referencing the constant:
func TestConfig_WithBaseURL(t *testing.T) { @@ tests := []struct { name string baseURL string expected string }{ @@ { name: "with empty base URL should use default", baseURL: "", - expected: "https://api.sms-gate.app/3rdparty/v1", + expected: smsgateway.BaseURL, }, }
164-234: LoosenValidateexpectations away from exact error string
TestConfig_Validatecompareserr.Error()to the full string"invalid config: missing auth credentials". This is brittle and will break on minor wording changes, even if the semantic error (e.g.ErrInvalidConfig) remains correct.Consider instead:
- Asserting
err == nil/err != nilas you already do, and- If you expose a sentinel (e.g.
smsgateway.ErrInvalidConfig), usingerrors.Is(err, smsgateway.ErrInvalidConfig), optionally with a substring check on the message.This keeps tests aligned with the public error contract instead of the exact formatting.
10-39: WithClient defaulting behavior is tightly coupled to http.DefaultClientThe
WithClienttests assert that passingnilresults inhttp.DefaultClient. Sincerest.NewClientalready applies this default at construction time, this pushes the defaulting logic into two places (config and REST client).It’s fine if this is intentional, but be aware it couples the config layer directly to
http.DefaultClient. If you later want to inject a different default client inrest, you’ll need to update this test (and possibly the builder) as well.smsgateway/client.go (2)
17-20: Use Go’s standard deprecation format forBASE_URLTo integrate better with
go docand tooling, it’s helpful to start the comment withDeprecated:.You can keep the same meaning and improve discoverability:
-// BASE_URL is deprecated, use BaseURL instead -// -//nolint:revive,staticcheck // backward compatibility +// Deprecated: BASE_URL is kept for backward compatibility. Use BaseURL instead. +//nolint:revive,staticcheck // backward compatibility const BASE_URL = BaseURL
28-48: Guard Basic auth header when no credentials are provided
NewClientalways sets anAuthorizationheader: Bearer whenTokenis set, otherwise Basic built fromUser:Password. If all ofToken,User, andPasswordare empty, you still emit a Basic header for":", which is unlikely to be meaningful and can make misconfiguration harder to spot.Since you already have configuration validation, you could make the auth behavior more intentional:
func NewClient(config Config) *Client { if config.BaseURL == "" { config.BaseURL = BaseURL } - headers := make(map[string]string, 1) - if config.Token != "" { - headers["Authorization"] = "Bearer " + config.Token - } else { - headers["Authorization"] = "Basic " + base64.StdEncoding.EncodeToString([]byte(config.User+":"+config.Password)) - } + headers := make(map[string]string, 1) + switch { + case config.Token != "": + headers["Authorization"] = "Bearer " + config.Token + case config.User != "" || config.Password != "": + headers["Authorization"] = "Basic " + base64.StdEncoding.EncodeToString([]byte(config.User+":"+config.Password)) + // else: no Authorization header; callers can enforce config.Validate() beforehand + }This avoids sending an obviously-invalid header and nudges callers toward running
Config.Validate()before constructing the client.smsgateway/client_test.go (1)
951-989: Make RevokeToken path expectation depend onjtiBoth table rows in
TestClient_RevokeTokenusejti: "abc123", and the mock server path is hardcoded to/auth/token/abc123. This passes, but if the implementation ever mis-escapes or misplaces thejti, the test wouldn’t catch it.You can get slightly stronger coverage by tying the expected path to the table data:
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - server := newMockServer(mockServerExpectedInput{ - method: http.MethodDelete, - path: "/auth/token/abc123", - }, mockServerOutput{ - code: tt.code, - }) + server := newMockServer(mockServerExpectedInput{ + method: http.MethodDelete, + path: "/auth/token/" + tt.jti, + }, mockServerOutput{ + code: tt.code, + })This keeps the behavior the same today but will catch regressions where the
jtiis not correctly interpolated into the URL.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
smsgateway/client.go(11 hunks)smsgateway/client_test.go(7 hunks)smsgateway/config.go(1 hunks)smsgateway/config_test.go(1 hunks)smsgateway/dto_auth.go(1 hunks)smsgateway/errors.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- smsgateway/dto_auth.go
- smsgateway/errors.go
- smsgateway/config.go
🧰 Additional context used
🧬 Code graph analysis (2)
smsgateway/config_test.go (3)
smsgateway/client.go (2)
Client(22-26)BaseURL(14-14)smsgateway/config.go (1)
Config(8-14)ca/config.go (2)
WithClient(26-30)WithBaseURL(32-36)
smsgateway/client.go (3)
rest/client.go (3)
Client(17-19)NewClient(21-27)Config(12-15)smsgateway/config.go (1)
Config(8-14)smsgateway/dto_auth.go (2)
TokenRequest(12-15)TokenResponse(26-31)
🔇 Additional comments (6)
smsgateway/client_test.go (6)
22-78: Mock server enhancements nicely centralize request validationThe extended
mockServerExpectedInput(method, path, query, authorization, contentType, body) plus the defaulting ofinput.authorizationtoauthorizationHeaderinnewMockServergives you a single, consistent place to validate HTTP method, path, query, headers, and body across tests. This makes the per-test setup succinct and keeps assertions uniform.No issues spotted here; the structure looks solid for current and future tests.
88-123: JWT client send path is covered cleanly
newJWTClientandTestJWTClient_Sendcorrectly exercise the Bearer-token auth path:
- Client is built with
Config{BaseURL: ..., Token: password}.- Mock expects
Authorization: "Bearer " + passwordand the JSON-encoded message body.- Test asserts only the absence of error, which is sufficient to prove the JWT header + payload wiring.
This gives good coverage of the new auth mode without over-constraining response details.
563-607: CheckHealth table test refactor looks goodThe new table-driven
TestClient_CheckHealthcleanly exercises:
- A successful
/healthresponse with parsedHealthResponse.- An error status path where only
wantErris asserted.Using the same mock server helper as other tests keeps the pattern consistent. No issues spotted here.
656-713: GetLogs test correctly validates encoded query parameters
TestClient_GetLogsnow:
- Reuses shared
from/totimes.- Builds the expected query with
url.QueryEscapeand RFC3339 formatting.- Verifies both success and error paths, with a full DeepEqual on the parsed
[]LogEntryfor the success case.This matches the client implementation that uses
url.Values.Encode()and gives strong coverage of both query construction and response decoding.
715-892: Settings tests provide solid coverage for success and error flowsThe
GetSettings,UpdateSettings, andReplaceSettingstests:
- Use structured expectations with
DeviceSettingsandSettingsMessages{LogLifetimeDays: ptr(30)}.- Validate the exact JSON bodies sent for write operations.
- Cover both success and error responses with clear
want/wantErrsemantics.This gives good end-to-end confidence that the client hits
/settingswith the right method, body, and decoding behavior.
894-949: GenerateToken tests are thorough and avoid secret-looking fixtures
TestClient_GenerateTokennow:
- Verifies the exact JSON request body for
TokenRequest{Scopes:["messages:read"], TTL:3600}.- Checks both success and error responses, including
ExpiresAtparsing.- Uses clearly non-secret fixture values (
token_id_example,access_token_example), which should avoid Gitleaks-style false positives highlighted in earlier reviews.This is a solid coverage of the new token generation API.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
smsgateway/client.go(11 hunks)smsgateway/config_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
smsgateway/config_test.go (3)
smsgateway/client.go (2)
Client(22-26)BaseURL(14-14)smsgateway/config.go (1)
Config(8-14)ca/config.go (2)
WithClient(26-30)WithBaseURL(32-36)
smsgateway/client.go (3)
rest/client.go (3)
Client(17-19)NewClient(21-27)Config(12-15)smsgateway/config.go (1)
Config(8-14)smsgateway/dto_auth.go (2)
TokenRequest(12-15)TokenResponse(26-31)
🔇 Additional comments (8)
smsgateway/config_test.go (5)
10-39: LGTM!The test correctly validates that
WithClientaccepts custom clients and defaults tohttp.DefaultClientwhen nil.
41-69: LGTM!The test correctly validates that
WithBaseURLaccepts custom URLs and defaults tosmsgateway.BaseURLwhen empty.
71-99: LGTM!The test correctly validates that
WithJWTAuthproperly assigns JWT tokens, including empty strings.
101-162: LGTM!The test correctly validates that
WithBasicAuthaccepts all combinations of credentials. Validation of credential completeness is appropriately deferred to theValidate()method.
236-260: LGTM!The test correctly validates that all builder methods support fluent chaining and set the expected field values.
smsgateway/client.go (3)
14-20: LGTM!The deprecation of
BASE_URLin favor ofBaseURLfollows Go naming conventions, and the backward compatibility alias is properly documented.
50-209: LGTM!The comment updates improve documentation clarity, and the existing method implementations remain correct.
210-233: LGTM!The token management methods follow the established patterns in the codebase. The error wrapping and path construction are correct.
capcom6
force-pushed
the
client/add-tokens-management-methods
branch
from
d40f4c1 to
eb508c9
Compare
Summary by CodeRabbit
New Features
Chores
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.