Fix/organization connections empty properties#55
Fix/organization connections empty properties#55BrandtKruger wants to merge 9 commits intokinde-oss:mainfrom
Conversation
- Implemented GetAuthURLWithInvitation method to include invitation_code and is_invitation parameters in the authentication URL. - Updated UseKindeAuth function to utilize the new method for redirecting users with invitation codes. - Added comprehensive tests for invitation code handling in authorization code flow. This enhancement allows for better user onboarding through invitation codes.
- Changed Connections field and related methods in GetConnectionsResponse to use ConnectionConnection instead of Connection. - Updated the Decode method to reflect the new type for better consistency and clarity in the API response structure.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds OAuth2 invitation-code support, extensive Management API surface changes (new optional types, billing and APIScopes fields, renamed/deleted generated types, validation updates), many generated client/server renames for DeleteAPIApplicationScope, updated handlers/metrics, and new tests plus example programs exercising GetOrganizationConnections. (38 words) Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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
🤖 Fix all issues with AI agents
In `@kinde/management_api/get_organization_connections_integration_test.go`:
- Around line 226-227: The for-loop starting with "for _, tt := range tests {"
is indented one extra tab; remove the extra tab so the line aligns with
surrounding code (same indentation level as the preceding test setup) to fix
formatting inconsistency around the loop that encloses t.Run(tt.name, func(t
*testing.T) { ... }) in the test function.
🧹 Nitpick comments (3)
examples/test_organization_connections/main.go (1)
122-190: Consider extracting duplicate error response handling into a helper function.The error handling blocks for 400, 403, and 429 responses follow an identical pattern: cast to
ErrorResponse, iterate errors, print guidance. This code is duplicated across this file andtest_get_organization_connections.go.♻️ Suggested refactor
func printErrorDetails(errResp *management_api.ErrorResponse) { errors := errResp.GetErrors() if len(errors) > 0 { for i, err := range errors { if code, ok := err.Code.Get(); ok { fmt.Printf("Error %d Code: %s\n", i+1, code) } if message, ok := err.Message.Get(); ok { fmt.Printf("Error %d Message: %s\n", i+1, message) } } } }test_get_organization_connections.go (1)
1-268: Significant duplication withexamples/test_organization_connections/main.go.This file is nearly identical to the example file. Both serve the same purpose of testing the GetOrganizationConnections fix. Consider:
- Keeping only one: The
examples/version has better documentation and follows the project's example structure.- If both are needed: Extract shared logic into a reusable package to avoid maintenance burden.
The only notable differences are minor wording variations in error messages and comments.
kinde/management_api/get_connections_response_test.go (1)
214-235: Assert “unset” for omitted fields to guard the empty-properties bug.In
TestConnectionConnection_Decode, whenwant*is empty the test skips validation, which could allowDecodeto incorrectly mark fields as set (the bug this PR addresses). Consider assertingIsSet() == falsefor omitted fields.🧪 Proposed test tightening
- if tt.wantID != "" { - if id, ok := conn.ID.Get(); !ok || id != tt.wantID { - t.Errorf("ID = %v (ok=%v), want %v", id, ok, tt.wantID) - } - } + if tt.wantID != "" { + if id, ok := conn.ID.Get(); !ok || id != tt.wantID { + t.Errorf("ID = %v (ok=%v), want %v", id, ok, tt.wantID) + } + } else if conn.ID.IsSet() { + t.Errorf("ID should be unset when not provided") + } - if tt.wantName != "" { - if name, ok := conn.Name.Get(); !ok || name != tt.wantName { - t.Errorf("Name = %v (ok=%v), want %v", name, ok, tt.wantName) - } - } + if tt.wantName != "" { + if name, ok := conn.Name.Get(); !ok || name != tt.wantName { + t.Errorf("Name = %v (ok=%v), want %v", name, ok, tt.wantName) + } + } else if conn.Name.IsSet() { + t.Errorf("Name should be unset when not provided") + } - if tt.wantDisp != "" { - if disp, ok := conn.DisplayName.Get(); !ok || disp != tt.wantDisp { - t.Errorf("DisplayName = %v (ok=%v), want %v", disp, ok, tt.wantDisp) - } - } + if tt.wantDisp != "" { + if disp, ok := conn.DisplayName.Get(); !ok || disp != tt.wantDisp { + t.Errorf("DisplayName = %v (ok=%v), want %v", disp, ok, tt.wantDisp) + } + } else if conn.DisplayName.IsSet() { + t.Errorf("DisplayName should be unset when not provided") + } - if tt.wantStrat != "" { - if strat, ok := conn.Strategy.Get(); !ok || strat != tt.wantStrat { - t.Errorf("Strategy = %v (ok=%v), want %v", strat, ok, tt.wantStrat) - } - } + if tt.wantStrat != "" { + if strat, ok := conn.Strategy.Get(); !ok || strat != tt.wantStrat { + t.Errorf("Strategy = %v (ok=%v), want %v", strat, ok, tt.wantStrat) + } + } else if conn.Strategy.IsSet() { + t.Errorf("Strategy should be unset when not provided") + }
kinde/management_api/get_organization_connections_integration_test.go
Outdated
Show resolved
Hide resolved
- Modified the indentation of the test loop in get_organization_connections_integration_test.go for improved readability and consistency with Go formatting standards.
- Updated WithInvitationCode function to set the prompt parameter to "create" when an invitation code is provided. - Added assertions in tests to verify that the prompt is correctly set in authURLOptions and included in the generated authentication URL. - Improved test coverage for scenarios involving invitation codes, ensuring proper handling of the prompt parameter.
- Updated GetAuthURLWithInvitation to set the prompt parameter to "create" only if no custom prompt is provided. - Added tests to verify that the prompt parameter behaves correctly with and without invitation codes, ensuring custom prompts are preserved. - Improved test coverage for scenarios involving invitation codes and prompt handling.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
oauth2/authorization_code/authorization_code.go (1)
41-48: Update in-repo mock to implement the complete interface.Adding
GetAuthURLWithInvitationtoIAuthorizationCodeFlowis a breaking change for external implementations. TheMockAuthorizationCodeFlowinoauth2/authorization_code/middleware_test.gois incomplete—it only implementsMiddleware()but lacks the required interface methods includingGetAuthURL(),GetAuthURLWithInvitation(), and others. Either implement all interface methods on the mock or remove it if it's not intended to satisfy the interface contract. Additionally, ensure external consumers are aware of this change.
🧹 Nitpick comments (1)
oauth2/authorization_code/authorization_code_test.go (1)
98-103: Assert constructor errors to avoid silent test failures.Right now the error return is discarded. Please assert
err == nil(and optionallykindeAuthFlow != nil) to fail fast if setup changes. This pattern applies to the other new tests too.♻️ Proposed update
- kindeAuthFlow, _ := NewAuthorizationCodeFlow( + kindeAuthFlow, err := NewAuthorizationCodeFlow( testKindeServerURL, "b9da18c441b44d81bab3e8232de2e18d", "client_secret", callbackURL, WithSessionHooks(newTestSessionHooks()), WithCustomStateGenerator(func(*AuthorizationCodeFlow) string { return "test_state" }), ) + assert.NoError(err, "failed to create auth flow") + assert.NotNil(kindeAuthFlow, "auth flow should not be nil")
…a spec Add local OpenAPI spec and fix get_connections_response so connections array decodes correctly. API returns connection objects directly; spec had items as connection (wrapper). Use connection_connection schema for get_connections_response.connections items and connection.connection. - Add spec/kinde-management-api-spec.yaml (from api-spec.kinde.com) - Add connection_connection schema; get_connections_response.connections items and connection.connection reference it - Update generate.go to use local spec - Regenerate management_api (ogen) so GetConnectionsResponse.Connections is []ConnectionConnection; no manual oas_* patches Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kinde/management_api/oas_parameters_gen.go (1)
1487-1659:⚠️ Potential issue | 🟠 MajorPublic type rename is breaking for downstream users.
DeleteAPIAppliationScopeParams→DeleteAPIApplicationScopeParamswill break existing imports; consider a compatibility alias in a hand-written file (and release notes/semver) to smooth upgrades.✅ Compatibility alias (non-generated file)
+// file: kinde/management_api/compat_types.go +package management_api + +// Deprecated: use DeleteAPIApplicationScopeParams. +type DeleteAPIAppliationScopeParams = DeleteAPIApplicationScopeParams
🧹 Nitpick comments (1)
kinde/management_api/oas_json_gen.go (1)
23673-23691: Minor: value assigned before error check in generated decoder.Lines 23678–23680 assign
elem = string(v)before checkingerr. While this is safe here (the error triggers an early return beforeelemis appended), it's an ordering inversion that could mask issues if the pattern were ever hand-edited. Since this is ogen-generated code, no action needed—just noting for awareness.v, err := d.Str() elem = string(v) // assigned before err check if err != nil { return err }
1 participant
Explain your changes
Fix bug for organization connections empty properties
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.