Conversation
WalkthroughAdds authenticated GET Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API (v2)
participant Service as OpenGraphSchemaService
participant Repo as OpenGraphSchemaRepository
participant DB as Database
Client->>API: GET /api/v2/extensions (Auth)
API->>API: RequireAuth() middleware (validate token)
API->>Service: ListExtensions(ctx)
Service->>Repo: GetGraphSchemaExtensions(ctx, filters, sort, 0, 0)
Repo->>DB: Query extensions (ORDER BY display_name)
DB-->>Repo: rows
Repo-->>Service: model.GraphSchemaExtensions
Service-->>API: []v2.ExtensionInfo
API-->>Client: 200 OK { "extensions": [...] }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In `@cmd/api/src/api/v2/opengraphschema.go`:
- Around line 95-97: The error string passed to api.WriteErrorResponse in the
block handling s.OpenGraphSchemaService.ListExtensions uses "error listing graph
schema extensions" which mismatches the test expectation; update the error
message passed (the fmt.Sprintf call) to "error getting graph schema extensions"
so the handler (in the code path calling s.OpenGraphSchemaService.ListExtensions
and api.WriteErrorResponse) matches opengraphschema_test.go.
In `@cmd/api/src/services/opengraphschema/extension.go`:
- Around line 74-82: The slice apiExtensions is allocated using count which may
differ from the actual length of extensions and can cause zero-values or panics;
change the allocation to use len(extensions) instead of count so apiExtensions
:= make([]v2.ExtensionInfo, len(extensions)) and keep the existing loop that
assigns into apiExtensions by index (refer to apiExtensions, extensions, count,
and v2.ExtensionInfo in extension.go).
🧹 Nitpick comments (5)
cmd/api/src/services/opengraphschema/extension_test.go (1)
410-416: Swapassert.Equalargument order for clarity.The
assert.Equalfunction in testify expects(t, expected, actual)but the calls here have(t, actual, expected). While testify's comparison still works correctly, the failure messages will show swapped values which can be confusing during debugging.♻️ Suggested fix
if tt.expected.err != nil { assert.EqualError(t, err, tt.expected.err.Error()) - assert.Equal(t, res, tt.expected.extensions) + assert.Equal(t, tt.expected.extensions, res) } else { assert.NoError(t, err) - assert.Equal(t, res, tt.expected.extensions) + assert.Equal(t, tt.expected.extensions, res) }cmd/api/src/api/v2/opengraphschema_test.go (2)
279-279: Minor: Usehttp.MethodGetconstant for consistency.Line 168 uses
http.MethodPut, but this line uses the string literal"GET". Consider usinghttp.MethodGetfor consistency.Suggested fix
- router.HandleFunc("/api/v2/extensions", resources.ListExtensions).Methods("GET") + router.HandleFunc("/api/v2/extensions", resources.ListExtensions).Methods(http.MethodGet)
202-259: Consider adding a test case for empty extensions list.The tests cover error and success (with data) paths, but an edge case for successfully returning an empty list would increase coverage and verify the response is
{"extensions":[]}rather than{"extensions":null}.cmd/api/src/api/v2/opengraphschema.go (2)
84-88: Consider usingIDinstead ofIdper Go naming conventions.Go convention recommends keeping acronyms in all caps for exported identifiers (e.g.,
ID,URL,HTTP). The JSON tag ensures the output remains"id".Suggested fix
type ExtensionInfo struct { - Id string `json:"id"` + ID string `json:"id"` Name string `json:"name"` Version string `json:"version"` }Note: This change would require updates to the test file and any code that references
ExtensionInfo.Id.
90-101: Handler implementation is functional, consider early-return pattern for consistency.The
if-elsewhere the success path is inelsecould use an early return pattern for better readability, though this is a minor stylistic preference.Alternative with early return pattern
func (s Resources) ListExtensions(response http.ResponseWriter, request *http.Request) { - var ( - ctx = request.Context() - ) + ctx := request.Context() - if extensions, err := s.OpenGraphSchemaService.ListExtensions(ctx); err != nil { + extensions, err := s.OpenGraphSchemaService.ListExtensions(ctx) + if err != nil { api.WriteErrorResponse(ctx, api.BuildErrorResponse(http.StatusInternalServerError, fmt.Sprintf("error getting graph schema extensions: %v", err), request), response) return - } else { - api.WriteJSONResponse(ctx, ExtensionsResponse{Extensions: extensions}, http.StatusOK, response) } + api.WriteJSONResponse(ctx, ExtensionsResponse{Extensions: extensions}, http.StatusOK, response) }
2 participants
Description
Get Extension Info
Motivation and Context
Resolves BED-6897
Ability to make HTTP call to get extension info
How Has This Been Tested?
Unit tests & http call
curl --request GET \ --url http://bhe.localhost/api/v2/extensions \ --header 'authorization: Bearer {{TOKEN}}Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.