Catd graphql play #2100
Catd graphql play #2100
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2100 +/- ##
===========================================
- Coverage 73.54% 52.53% -21.02%
===========================================
Files 78 86 +8
Lines 7240 8890 +1650
===========================================
- Hits 5325 4670 -655
- Misses 1564 3686 +2122
- Partials 351 534 +183
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
closing this as stale - please reopen if needed |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: grokspawn <jordan@nimblewidget.com>
grokspawn
force-pushed
the
catd-graphql-play
branch
from
c7edbd2 to
559b18f
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the catalogd storage layer to introduce GraphQL support for querying catalog data. The changes extract HTTP handler logic into a separate server package, introduce a service layer for GraphQL schema management with caching, and replace direct struct initialization with a constructor pattern for LocalDirV1.
- Introduces a new GraphQL endpoint at
/api/v1/graphqlwith dynamic schema generation - Refactors HTTP handlers into a dedicated
serverpackage with cleaner separation of concerns - Adds a GraphQL service layer with schema caching to improve performance
- Updates
LocalDirV1to use a constructor pattern (NewLocalDirV1) for proper initialization
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/catalogd/storage/localdir.go | Refactored to use constructor pattern, added GraphQL service integration, moved HTTP handlers to server package |
| internal/catalogd/storage/localdir_test.go | Updated all test instantiations to use new NewLocalDirV1 constructor |
| internal/catalogd/storage/http_preconditions_check.go | Removed (moved to server package with simplified implementation) |
| internal/catalogd/server/handlers.go | New file implementing HTTP handlers extracted from storage layer |
| internal/catalogd/server/http_helpers.go | New file with simplified HTTP precondition checking |
| internal/catalogd/service/graphql_service.go | New GraphQL service with caching for schema generation |
| internal/catalogd/graphql/graphql.go | New dynamic GraphQL schema generation implementation |
| internal/catalogd/graphql/graphql_test.go | Tests for GraphQL schema discovery |
| internal/catalogd/graphql/discovery_test.go | Additional comprehensive tests for schema discovery edge cases |
| internal/catalogd/graphql/sample-queries.txt | Documentation of sample GraphQL queries |
| internal/catalogd/graphql/README.md | Documentation for GraphQL integration |
| cmd/catalogd/main.go | Updated to use NewLocalDirV1 constructor |
| go.mod, go.sum | Added graphql-go/graphql dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check If-Modified-Since | ||
| if r.Method == http.MethodGet || r.Method == http.MethodHead { | ||
| if t, err := time.Parse(http.TimeFormat, r.Header.Get("If-Modified-Since")); err == nil { | ||
| // The Date-Modified header truncates sub-second precision, so |
There was a problem hiding this comment.
Corrected spelling of 'Date-Modified' to 'Last-Modified'. The HTTP header name is 'Last-Modified', not 'Date-Modified'.
| // Check If-Unmodified-Since | ||
| if r.Method != http.MethodGet && r.Method != http.MethodHead { | ||
| if t, err := time.Parse(http.TimeFormat, r.Header.Get("If-Unmodified-Since")); err == nil { | ||
| // The Date-Modified header truncates sub-second precision, so |
There was a problem hiding this comment.
Corrected spelling of 'Date-Modified' to 'Last-Modified'. The HTTP header name is 'Last-Modified', not 'Date-Modified'.
| // Allow POST requests only for GraphQL endpoint | ||
| if r.URL.Path != "" && len(r.URL.Path) >= 7 && r.URL.Path[len(r.URL.Path)-7:] != "graphql" && r.Method == http.MethodPost { | ||
| http.Error(w, http.StatusText(http.StatusMethodNotAllowed), http.StatusMethodNotAllowed) | ||
| return | ||
| } |
There was a problem hiding this comment.
The string suffix check using r.URL.Path[len(r.URL.Path)-7:] is fragile and uses a magic number (7). This logic can fail if the path is shorter than 7 characters or doesn't end exactly with 'graphql'. Consider using strings.HasSuffix(r.URL.Path, \"graphql\") or checking against the actual GraphQL path pattern for more robust route matching.
| code = http.StatusInternalServerError | ||
| } | ||
| // Log the actual error for debugging | ||
| fmt.Printf("HTTP Error %d: %v\n", code, err) |
There was a problem hiding this comment.
Using fmt.Printf for error logging bypasses structured logging and may not be captured in production logging systems. Consider using a proper logger (e.g., from a logging library or context) to ensure errors are properly tracked and monitored.
| fmt.Printf("HTTP Error %d: %v\n", code, err) | |
| log.Printf("HTTP Error %d: %v\n", code, err) |
| m sync.RWMutex | ||
| // this singleflight Group is used in `getIndex()`` to handle concurrent HTTP requests | ||
| // optimally. With the use of this slightflight group, the index is loaded from disk | ||
| // this singleflight Group is used in `getIndex()` to handle concurrent HTTP requests |
There was a problem hiding this comment.
The comment refers to getIndex() but the method has been renamed to GetIndex(). Update the comment to reference the correct method name.
| // this singleflight Group is used in `getIndex()` to handle concurrent HTTP requests | |
| // this singleflight Group is used in `GetIndex()` to handle concurrent HTTP requests |
| // appendUnique adds a value to slice if not already present | ||
| func appendUnique(slice []interface{}, value interface{}) []interface{} { | ||
| for _, existing := range slice { | ||
| if reflect.DeepEqual(existing, value) { | ||
| return slice | ||
| } | ||
| } |
There was a problem hiding this comment.
The appendUnique function uses reflect.DeepEqual which can be expensive, especially when called repeatedly during schema discovery. Consider using a simpler equality check or a map-based approach if the performance impact is significant for large catalogs.
| // appendUnique adds a value to slice if not already present | |
| func appendUnique(slice []interface{}, value interface{}) []interface{} { | |
| for _, existing := range slice { | |
| if reflect.DeepEqual(existing, value) { | |
| return slice | |
| } | |
| } | |
| // appendUnique adds a value to slice if not already present, using JSON string as key for uniqueness | |
| func appendUnique(slice []interface{}, value interface{}) []interface{} { | |
| seen := make(map[string]struct{}, len(slice)) | |
| for _, existing := range slice { | |
| key, err := json.Marshal(existing) | |
| if err != nil { | |
| continue // skip values that can't be marshaled | |
| } | |
| seen[string(key)] = struct{}{} | |
| } | |
| valueKey, err := json.Marshal(value) | |
| if err != nil { | |
| return slice // skip value if it can't be marshaled | |
| } | |
| if _, exists := seen[string(valueKey)]; exists { | |
| return slice | |
| } |
2 participants
Description
Reviewer Checklist