Conversation
This migration replaces the gRPC + grpc-gateway setup with Connect RPC,
following the patterns established by raystack/frontier.
Key changes:
- Single port (8080) instead of dual ports (HTTP + gRPC)
- Native HTTP support via Connect instead of grpc-gateway reverse proxy
- New interceptors in pkg/server/interceptor/ replacing pkg/grpc_interceptor/
- Buf v2 proto generation with connectrpc/go plugin
Server changes:
- Use h2c.NewHandler for HTTP/2 cleartext support
- Single http.ServeMux with Connect service handlers
- gRPC reflection support via connectrpc/grpcreflect
Handler changes:
- Signatures use connect.Request[T] and connect.Response[T] wrappers
- Access request fields via req.Msg.GetXxx()
- Return responses via connect.NewResponse(&pb.Response{})
- Errors via connect.NewError(connect.CodeXxx, err)
Client changes:
- Connect client with HTTP transport
- Requests wrapped with connect.NewRequest()
- Responses accessed via resp.Msg
Test changes:
- Updated to use connect.NewRequest() for handler calls
- Fixed error checking: err == nil for success (not connect.CodeOf)
- connect.CodeOf(nil) returns CodeUnknown, not 0
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request migrates the Compass service from a gRPC-based architecture to Connect RPC (a modern RPC framework supporting HTTP as a first-class protocol). Changes include: updating protobuf generation configuration to use Connect plugins, replacing the gRPC server with a Connect HTTP server, replacing the gRPC client with a Connect HTTP client, migrating all handler method signatures from gRPC request/response patterns to Connect request/response wrappers, converting error handling from gRPC status codes to Connect error codes, establishing new Connect-compatible interceptor implementations, updating dependencies in go.mod, and adjusting configuration files to reflect the new HTTP-based server architecture (port change from 8081 to 8080). Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI Client
participant Client as Connect Client<br/>(HTTP)
participant Interceptor as Interceptors<br/>(namespace/auth)
participant Server as Connect Server<br/>(HTTP)
participant Handler as RPC Handler
participant Store as Data Store
User->>CLI: Run command (e.g., create asset)
CLI->>Client: client.Create(config)
Client->>Client: Initialize HTTP client
Client-->>CLI: *Client
CLI->>Client: NewRequest(config, namespaceID, proto)
Client->>Client: Set headers (namespace, user UUID)
Client-->>CLI: *connect.Request[T]
CLI->>Server: clnt.Method(ctx, req)
Server->>Interceptor: Chain interceptors
Interceptor->>Interceptor: Extract namespace<br/>from headers
Interceptor->>Interceptor: Build context with<br/>namespace
Interceptor->>Handler: next(ctx, req)
Handler->>Handler: req.Msg.Validate()
Handler->>Handler: Extract fields<br/>from req.Msg
Handler->>Store: Execute query
Store-->>Handler: Data
Handler->>Handler: Build response
Handler->>Handler: connect.NewResponse(resp)
Handler-->>Server: *connect.Response[T]
Server-->>CLI: Response with data
CLI->>CLI: Process res.Msg
CLI-->>User: Display output
sequenceDiagram
participant Old as gRPC Flow<br/>(Previous)
participant New as Connect Flow<br/>(New)
rect rgba(200, 100, 100, 0.5)
Note over Old: Old Architecture
Old->>Old: grpc.NewClient()
Old->>Old: metadata.NewOutgoingContext()
Old->>Old: client.SetMetadata(ctx, config)
Old->>Old: clnt.Method(ctx, &proto.Request)
end
rect rgba(100, 150, 200, 0.5)
Note over New: New Architecture
New->>New: http.Client with timeout
New->>New: client.NewRequest(config,<br/>namespaceID, proto)
New->>New: Set request headers directly
New->>New: clnt.Method(ctx, req)
end
rect rgba(100, 200, 100, 0.5)
Note over Old: Old Response Handling
Old->>Old: res.GetData()
Old->>Old: res.Id (direct field)
end
rect rgba(100, 200, 100, 0.5)
Note over New: New Response Handling
New->>New: res.Msg.GetData()
New->>New: res.Msg.Id (wrapped)
end
🚥 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. 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 |
2 tasks
Update PROTON_COMMIT to 7121b18 which removes grpc-gateway annotations from the compass proto definitions. This results in cleaner generated code with 1770 fewer lines as the OpenAPI and HTTP annotations are no longer included. Related: raystack/proton#459
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pkg/server/interceptor/namespace.go (1)
34-50:⚠️ Potential issue | 🔴 CriticalFix unsafe JWT claim handling—
ParseInsecureskips validation but claims control namespace and user identity.
jwt.ParseInsecure(line 37) bypasses signature and expiry validation. The unverified claims at lines 40–50 then control namespace selection and user UUID override—security-critical operations on the Connect surface. The direct type assertion at line 41 lacks validation and will panic on malformed claims.Replace
ParseInsecurewith verified token parsing, validate claim types before use, and establish trust boundaries for tenant/user identification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/interceptor/namespace.go` around lines 34 - 50, Replace the insecure, unverified claim handling around jwt.ParseInsecure: stop using jwt.ParseInsecure and parse/validate tokens with a verified parser (validate signature and expiry using the appropriate keyfunc/verification method) before trusting claims; only read namespaceClaimKey via token.Get after verification and confirm its type (e.g., assert string safely) before passing into getNamespaceByNameOrID, handle token.Get errors and malformed types without panicking, and only call req.Header().Set(userUUIDHeaderKey, token.Subject()) when the token is verified and Subject() is non-empty and userUUIDHeaderKey is configured.internal/store/postgres/discussion_repository_test.go (1)
42-55:⚠️ Potential issue | 🟠 MajorContext is overwritten, breaking namespace injection.
Line 42 correctly sets the namespace context using
interceptor.BuildContextWithNamespace, but Line 55 immediately overwritesr.ctxwithcontext.TODO(), discarding the namespace context. This means subsequent repository operations won't have the expected namespace for RLS enforcement.🐛 Proposed fix
- r.ctx = interceptor.BuildContextWithNamespace(context.Background(), r.ns) - - logger := log.NewLogrus() - r.client, r.pool, r.resource, err = newTestClient(logger) - if err != nil { - r.T().Fatal(err) - } - - r.userRepo, err = postgres.NewUserRepository(r.client) - if err != nil { - r.T().Fatal(err) - } - - r.ctx = context.TODO() + logger := log.NewLogrus() + r.client, r.pool, r.resource, err = newTestClient(logger) + if err != nil { + r.T().Fatal(err) + } + + r.userRepo, err = postgres.NewUserRepository(r.client) + if err != nil { + r.T().Fatal(err) + } + + r.ctx = interceptor.BuildContextWithNamespace(context.Background(), r.ns)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/store/postgres/discussion_repository_test.go` around lines 42 - 55, The test overwrites the namespace-injected context by setting r.ctx = context.TODO() after calling interceptor.BuildContextWithNamespace; remove or replace that final assignment so r.ctx retains the namespace context used for RLS enforcement (locate the setup in discussion_repository_test.go where interceptor.BuildContextWithNamespace is called and delete or adjust the following r.ctx = context.TODO() line to preserve the context for subsequent calls to the repository).cli/assets.go (1)
387-393:⚠️ Potential issue | 🟡 MinorTypo in command example.
The example shows
$ compass unasset star <id>but should be$ compass asset unstar <id>.📝 Proposed fix
Example: heredoc.Doc(` - $ compass unasset star <id> + $ compass asset unstar <id> `),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/assets.go` around lines 387 - 393, In unstarAssetCommand update the Example heredoc to correct the typo: replace the current example string "$ compass unasset star <id>" with the correct invocation "$ compass asset unstar <id>" so the example matches the command defined by unstarAssetCommand.cli/discussions.go (1)
99-119:⚠️ Potential issue | 🟡 MinorMissing argument validation for
viewDiscussionByIDCommand.The command expects exactly one argument (
args[0]) but doesn't specifyArgs: cobra.ExactArgs(1)on the command definition, unlike other commands in this file. If no argument is provided, this will panic at line 108.🛡️ Proposed fix to add argument validation
cmd := &cobra.Command{ Use: "view <id>", Short: "view discussion for the given ID", Example: heredoc.Doc(` $ compass discussion view <id> `), + Args: cobra.ExactArgs(1), Annotations: map[string]string{ "action:core": "true", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/discussions.go` around lines 99 - 119, The viewDiscussionByIDCommand is missing argument validation and will panic when args[0] is accessed; add Args: cobra.ExactArgs(1) (or cobra.MinimumNArgs(1)) to the command definition that contains the RunE closure so the CLI validates exactly one argument before entering viewDiscussionByIDCommand's RunE, preventing out-of-range access to args[0].internal/server/server.go (1)
24-35:⚠️ Potential issue | 🟡 MinorConfig structure change may orphan existing YAML configuration.
The
GRPCconfig field was removed, but existingconfig.yamlfiles may still have:grpc: port: 8081 max_send_msg_size: 33554432 max_recv_msg_size: 33554432The
max_recv_msg_sizeandmax_send_msg_sizehave been moved to the top level of theConfigstruct, but the nestedgrpc:section will be silently ignored. Users migrating from the old configuration may not realize their settings aren't being applied.Consider documenting this breaking change or adding a deprecation warning when the old config structure is detected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/server.go` around lines 24 - 35, The Config struct now moves max_recv_msg_size/max_send_msg_size to top-level and removed the GRPC field, which will silently ignore legacy "grpc:" YAML; update the config load path to detect the old nested "grpc" key (e.g., after unmarshalling into Config or during Viper/Decode) and emit a clear deprecation/warning message and fallback behavior that copies grpc.max_recv_msg_size -> Config.MaxRecvMsgSize and grpc.max_send_msg_size -> Config.MaxSendMsgSize when present; reference the Config type and its MaxRecvMsgSize/MaxSendMsgSize fields and add the warning where the config is decoded/loaded so existing configs are not silently ignored.
🧹 Nitpick comments (15)
internal/server/v1beta1/lineage_test.go (1)
17-22: Minor: Remove extraneous blank lines in imports.Lines 18-19 contain trailing whitespace/blank lines within the import block.
♻️ Clean up imports
compassv1beta1 "github.com/raystack/compass/proto/raystack/compass/v1beta1" log "github.com/raystack/salt/observability/logger" - - "google.golang.org/protobuf/testing/protocmp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/v1beta1/lineage_test.go` around lines 17 - 22, The import block in lineage_test.go contains extraneous blank lines and trailing whitespace around the import of log ("github.com/raystack/salt/observability/logger") and the following imports ("google.golang.org/protobuf/testing/protocmp", "google.golang.org/protobuf/types/known/timestamppb"); open the import section and remove the empty lines and any trailing spaces so the imports are contiguous and formatted consistently.internal/server/v1beta1/namespace_test.go (1)
17-20: Minor: Remove extraneous blank lines in imports.Lines 17-19 contain trailing whitespace/blank lines within the import block.
♻️ Clean up imports
"github.com/stretchr/testify/mock" - - "google.golang.org/protobuf/types/known/structpb"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/v1beta1/namespace_test.go` around lines 17 - 20, Remove the extra blank lines inside the import block in namespace_test.go so imports are contiguous; specifically, tidy the import declaration that currently contains stray empty lines around "google.golang.org/protobuf/types/known/structpb" so there are no blank lines within the parentheses of the import statement.cli/search.go (1)
59-76: Remove unusednamespaceIDparameter frommakeSearchAssetRequest.The
namespaceIDparameter is accepted but never used in the function body. The namespace is already handled separately viaclient.NewRequeston line 39.♻️ Proposed fix
-func makeSearchAssetRequest(namespaceID, text, filter, query, rankby string, size uint32) *compassv1beta1.SearchAssetsRequest { +func makeSearchAssetRequest(text, filter, query, rankby string, size uint32) *compassv1beta1.SearchAssetsRequest {And update the call site on line 38:
- searchReq := makeSearchAssetRequest(namespaceID, args[0], filter, query, rankBy, size) + searchReq := makeSearchAssetRequest(args[0], filter, query, rankBy, size)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/search.go` around lines 59 - 76, The function makeSearchAssetRequest currently accepts an unused namespaceID parameter; remove namespaceID from the function signature and all call sites (e.g., the call inside client.NewRequest) so the function becomes makeSearchAssetRequest(text, filter, query, rankby string, size uint32) and callers pass only those arguments; update any code that invoked makeSearchAssetRequest(namespaceID, ...) to call it without namespaceID and run a build to fix any remaining call mismatches.internal/server/v1beta1/type_test.go (1)
18-22: Minor: Remove extraneous blank lines in imports.Lines 19-20 contain trailing whitespace/blank lines within the import block.
♻️ Clean up imports
log "github.com/raystack/salt/observability/logger" - - "google.golang.org/protobuf/testing/protocmp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/v1beta1/type_test.go` around lines 18 - 22, The import block in internal/server/v1beta1/type_test.go contains extraneous blank lines (between log "github.com/raystack/salt/observability/logger" and "google.golang.org/protobuf/testing/protocmp"); remove those blank/trailing lines so imports are contiguous and properly grouped, then run gofmt/goimports to reformat and fix ordering if needed.pkg/server/interceptor/user.go (1)
16-22: Simplify variable declarations.The variable initialization to empty strings (lines 16-19) is redundant since
Header().Get()returns an empty string when the header is missing. Use short variable declarations instead.♻️ Simplified variable declarations
- return func(ctx context.Context, req connect.AnyRequest) (connect.AnyResponse, error) { - var ( - userUUID = "" - userEmail = "" - ) - - userUUID = req.Header().Get(identityHeaderKeyUUID) - userEmail = req.Header().Get(identityHeaderKeyEmail) + return func(ctx context.Context, req connect.AnyRequest) (connect.AnyResponse, error) { + userUUID := req.Header().Get(identityHeaderKeyUUID) + userEmail := req.Header().Get(identityHeaderKeyEmail)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/interceptor/user.go` around lines 16 - 22, Remove the redundant pre-declaration of userUUID and userEmail and use short declarations when reading headers; replace the two-step pattern with direct assignments like using req.Header().Get(identityHeaderKeyUUID) and req.Header().Get(identityHeaderKeyEmail) (refer to variables identityHeaderKeyUUID, identityHeaderKeyEmail and the req.Header().Get calls in the interceptor code) so the variables are declared and initialized in one line each.cli/namespace.go (1)
68-73: Unusedindexvariable.The
indexvariable is declared and incremented but never used. Consider removing it.♻️ Proposed fix
var report [][]string report = append(report, []string{"ID", "NAME", "STATE"}) - index := 1 for _, i := range res.Msg.GetNamespaces() { report = append(report, []string{i.GetId(), i.GetName(), i.GetState()}) - index++ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/namespace.go` around lines 68 - 73, Remove the unused loop counter by deleting the declaration and increments of index in namespace.go: the variable index (initialized to 1) and the index++ inside the for loop that iterates over res.Msg.GetNamespaces() are not used anywhere, so remove those lines and leave the report append logic (report = append(report, []string{i.GetId(), i.GetName(), i.GetState()})) untouched.internal/server/v1beta1/tag_template_test.go (1)
21-22: Minor: Empty import lines.Lines 21-22 contain empty import entries. Consider cleaning these up for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/v1beta1/tag_template_test.go` around lines 21 - 22, Remove the empty import entries in the import block of tag_template_test.go to clean up the imports; edit the import declaration in that test file (the import block around the top of tag_template_test.go) and delete any blank lines or empty strings so only required packages remain.internal/server/v1beta1/search_test.go (1)
19-20: Minor: Empty import lines.Lines 19-20 contain empty import entries that should be cleaned up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/v1beta1/search_test.go` around lines 19 - 20, Remove the stray empty import entries from the imports block in internal/server/v1beta1/search_test.go so there are no blank lines in the import list; edit the import section used by the test (the package's imports in search_test.go) to only include actual packages and delete any empty import lines to clean up the file.internal/server/v1beta1/tag_test.go (1)
21-22: Minor: Empty import lines.Lines 21-22 contain empty import entries that appear to be formatting artifacts from the migration. Consider cleaning these up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/v1beta1/tag_test.go` around lines 21 - 22, Remove the empty import entries in the import block of tag_test.go (the stray blank lines in the import list), leaving only the actual package imports; update the import block in internal/server/v1beta1/tag_test.go so it contains no empty lines or placeholders and then run gofmt/goimports to ensure imports are properly formatted.internal/server/v1beta1/asset_test.go (1)
25-26: Minor: Empty import lines.Lines 25-26 contain empty import entries. Consider removing these formatting artifacts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/v1beta1/asset_test.go` around lines 25 - 26, Remove the stray empty import entries in the import block of internal/server/v1beta1/asset_test.go (the import declaration in that test file currently has blank lines 25-26); delete those empty lines so the import list only contains actual imports, then run gofmt/goimports to ensure formatting is clean.internal/client/client.go (1)
34-35: Consider: HTTP-only base URL.The base URL is hardcoded to
http://. For production deployments requiring TLS, the client would need modification. If this is intentional for internal/dev usage, consider documenting this limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/client.go` around lines 34 - 35, The base URL is built with a hardcoded "http://" scheme; update the construction to use a configurable scheme instead of forcing HTTP: read a scheme field from the config (e.g., cfg.Scheme) or default to "http" and then build baseURL using scheme + "://" + cfg.Host (refer to the baseURL variable and cfg.Host), and ensure callers that construct cfg populate cfg.Scheme or the default is clearly documented; if TLS should be auto-detected, add a flag like cfg.UseTLS that toggles "https" vs "http" when creating baseURL.internal/server/server.go (1)
108-112: Health endpoint returns 200 but no Content-Type header.The
/pingendpoint writes "pong" but doesn't setContent-Type. While browsers/clients typically handle this gracefully, it's good practice to set it explicitly.🔧 Suggested improvement
mux.HandleFunc("/ping", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/plain; charset=utf-8") w.WriteHeader(http.StatusOK) w.Write([]byte("pong")) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/server.go` around lines 108 - 112, The /ping handler registered via mux.HandleFunc("/ping", func(w http.ResponseWriter, r *http.Request) { ... }) currently writes "pong" but does not set a Content-Type; update the handler to call w.Header().Set("Content-Type", "text/plain; charset=utf-8") (or another appropriate MIME type) before writing the body so responses include an explicit Content-Type header.internal/server/v1beta1/comment_test.go (1)
21-25: Minor: Remove extra blank lines in imports.Same cleanup opportunity as in
discussion_test.go- empty lines (22-23) within the import block.🧹 Suggested cleanup
"github.com/raystack/compass/pkg/server/interceptor" compassv1beta1 "github.com/raystack/compass/proto/raystack/compass/v1beta1" log "github.com/raystack/salt/observability/logger" "github.com/stretchr/testify/mock" - - "google.golang.org/protobuf/testing/protocmp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/v1beta1/comment_test.go` around lines 21 - 25, Remove the extra blank lines in the import block of comment_test.go so the imports are contiguous; specifically collapse the gap between "github.com/stretchr/testify/mock" and "google.golang.org/protobuf/testing/protocmp" (and the following "google.golang.org/protobuf/types/known/timestamppb") so there are no empty lines inside the import block.internal/server/v1beta1/discussion_test.go (1)
22-26: Minor: Remove extra blank lines in imports.There are empty lines (23-24) within the import block that appear to be artifacts from the migration.
🧹 Suggested cleanup
"github.com/raystack/compass/pkg/server/interceptor" compassv1beta1 "github.com/raystack/compass/proto/raystack/compass/v1beta1" log "github.com/raystack/salt/observability/logger" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" - - "google.golang.org/protobuf/testing/protocmp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/v1beta1/discussion_test.go` around lines 22 - 26, Remove the two empty lines inside the import block (the gap between "github.com/stretchr/testify/mock" and "google.golang.org/protobuf/testing/protocmp") so all imports are contiguous; tidy the import block (preserve ordering/grouping) and run gofmt/goimports to ensure formatting is clean.internal/server/v1beta1/comment.go (1)
121-127: Remove the duplicated request validation.
ValidateAll()is invoked twice back-to-back with identical handling. Keeping one call is enough.🛠️ Suggested fix
if err := req.Msg.ValidateAll(); err != nil { return nil, connect.NewError(connect.CodeInvalidArgument, errors.New(bodyParserErrorMsg(err))) } - - if err := req.Msg.ValidateAll(); err != nil { - return nil, connect.NewError(connect.CodeInvalidArgument, errors.New(bodyParserErrorMsg(err))) - } if err := server.validateIDInteger(req.Msg.DiscussionId); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/v1beta1/comment.go` around lines 121 - 127, There is a duplicated validation call—remove the redundant second call to req.Msg.ValidateAll() and its identical error handling; keep a single validation block that returns connect.NewError(connect.CodeInvalidArgument, errors.New(bodyParserErrorMsg(err))) when req.Msg.ValidateAll() fails (refer to req.Msg.ValidateAll, connect.NewError, connect.CodeInvalidArgument, and bodyParserErrorMsg to locate the duplicated logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@buf.gen.yaml`:
- Around line 1-16: Update the protocolbuffers/go plugin version string from
"buf.build/protocolbuffers/go:v1.31.0" to
"buf.build/protocolbuffers/go:v1.36.11" in the buf.gen.yaml plugins list
(preserve the existing out and opt fields); locate the entry that begins with
remote: buf.build/protocolbuffers/go:v1.31.0 and replace the version suffix only
so it aligns with the newer protobuf-go release.
In `@internal/server/v1beta1/asset.go`:
- Around line 208-209: The handler is returning a NotFound error for syntax
validation failures from asset.ParseVersion(req.Msg.GetVersion()); change the
status to InvalidArgument instead: where asset.ParseVersion(...) currently
causes return nil, connect.NewError(connect.CodeNotFound, err), replace the
error code with connect.CodeInvalidArgument so malformed versions map to an
InvalidArgument response while preserving the original error message.
- Around line 371-375: The handler currently returns the raw backend error for
AddProbe failures; update the error branch after server.assetService.AddProbe so
it calls the project-wide internalServerError helper instead of returning
connect.NewError(connect.CodeInternal, err). Specifically, locate the AddProbe
call in the handler and replace the connect.CodeInternal return with a call to
internalServerError(ctx, "add probe", err) (or the existing internalServerError
signature used elsewhere) so the error is logged via the centralized reference
path and internal details are not leaked to clients.
In `@internal/server/v1beta1/comment.go`:
- Around line 103-106: The 404 branch is returning errMissingUserInfo instead of
the actual comment-not-found error from server.discussionService.GetComment;
update the error handling around GetComment (the errors.As check for
discussion.NotFoundError) to return/connect.NewError(connect.CodeNotFound, err)
or otherwise propagate the original discussion.NotFoundError (instead of
errMissingUserInfo) so callers receive the real lookup error from GetComment.
In `@internal/server/v1beta1/namespace.go`:
- Around line 41-49: Reject empty urns early in the read/update handlers by
validating req.Msg.GetUrn() (and the analogous fields in the update handler)
before attempting uuid.Parse or calling namespaceService methods; if the urn is
empty return connect.NewError(connect.CodeInvalidArgument, errors.New("urn must
be provided")) so callers get CodeInvalidArgument instead of falling through to
GetByName("") or performing updates with empty ID/Name (update the GetNamespace
function and the UpdateNamespace handler referenced in the 90-113 range to
perform this check).
In `@internal/server/v1beta1/server.go`:
- Line 63: Replace the direct connect.NewError return in the ValidateUser
failure path with the existing internalServerError sanitization pattern: when
ValidateUser returns a non-nil err use internalServerError(ctx, "ValidateUser",
err) (or the same helper invocation used on line 71) instead of returning
connect.NewError(connect.CodeInternal, err), so the client receives a generic
500 ref string while the real error is logged internally.
In `@pkg/server/interceptor/error_response.go`:
- Around line 39-40: The fallback return currently exposes raw error text via
connect.NewError(connect.CodeInternal, err); instead, follow the existing
sanitization pattern used by internalServerError() in
internal/server/v1beta1/server.go: log the original err internally (using the
same logger/context) and return a sanitized
connect.NewError(connect.CodeInternal, sanitizedErr) where sanitizedErr contains
only a generic message and a reference ID (do not pass the original err to the
client). Locate the use of connect.NewError in error_response.go and replace it
with the logging + sanitized error construction mirroring internalServerError().
In `@pkg/server/interceptor/namespace.go`:
- Around line 81-86: The code is mapping every error from the namespace lookup
to connect.CodeNotFound; instead, check whether the returned error from
service.GetByName or service.GetByID is an actual "not found" sentinel (e.g.,
errors.Is(err, store.ErrNotFound) or a service.IsNotFound(err) helper) and only
wrap it as connect.NewError(connect.CodeNotFound, err) when it truly is a
not-found error; for all other errors return or wrap the original error
(preserving its type/message) so failures like timeouts or store outages are not
misclassified (update the branches around GetByName/GetByID accordingly).
In `@pkg/server/interceptor/recovery.go`:
- Around line 15-21: The current defer recovery handler captures the panic and
creates a Connect error containing the panic value and full stack trace (the
defer func in recovery.go that assigns err = connect.NewError(...)), which
exposes sensitive details to clients; change it to log the panic and stack
server-side (e.g., using your server logger or log.Printf) and instead set err =
connect.NewError(connect.CodeInternal, fmt.Errorf("internal server error")) or a
similarly generic message so the client receives no stack or panic details while
full details remain in server logs. Ensure the logging records both the
recovered value and debug.Stack() before creating the generic connect.NewError.
---
Outside diff comments:
In `@cli/assets.go`:
- Around line 387-393: In unstarAssetCommand update the Example heredoc to
correct the typo: replace the current example string "$ compass unasset star
<id>" with the correct invocation "$ compass asset unstar <id>" so the example
matches the command defined by unstarAssetCommand.
In `@cli/discussions.go`:
- Around line 99-119: The viewDiscussionByIDCommand is missing argument
validation and will panic when args[0] is accessed; add Args: cobra.ExactArgs(1)
(or cobra.MinimumNArgs(1)) to the command definition that contains the RunE
closure so the CLI validates exactly one argument before entering
viewDiscussionByIDCommand's RunE, preventing out-of-range access to args[0].
In `@internal/server/server.go`:
- Around line 24-35: The Config struct now moves
max_recv_msg_size/max_send_msg_size to top-level and removed the GRPC field,
which will silently ignore legacy "grpc:" YAML; update the config load path to
detect the old nested "grpc" key (e.g., after unmarshalling into Config or
during Viper/Decode) and emit a clear deprecation/warning message and fallback
behavior that copies grpc.max_recv_msg_size -> Config.MaxRecvMsgSize and
grpc.max_send_msg_size -> Config.MaxSendMsgSize when present; reference the
Config type and its MaxRecvMsgSize/MaxSendMsgSize fields and add the warning
where the config is decoded/loaded so existing configs are not silently ignored.
In `@internal/store/postgres/discussion_repository_test.go`:
- Around line 42-55: The test overwrites the namespace-injected context by
setting r.ctx = context.TODO() after calling
interceptor.BuildContextWithNamespace; remove or replace that final assignment
so r.ctx retains the namespace context used for RLS enforcement (locate the
setup in discussion_repository_test.go where
interceptor.BuildContextWithNamespace is called and delete or adjust the
following r.ctx = context.TODO() line to preserve the context for subsequent
calls to the repository).
In `@pkg/server/interceptor/namespace.go`:
- Around line 34-50: Replace the insecure, unverified claim handling around
jwt.ParseInsecure: stop using jwt.ParseInsecure and parse/validate tokens with a
verified parser (validate signature and expiry using the appropriate
keyfunc/verification method) before trusting claims; only read namespaceClaimKey
via token.Get after verification and confirm its type (e.g., assert string
safely) before passing into getNamespaceByNameOrID, handle token.Get errors and
malformed types without panicking, and only call
req.Header().Set(userUUIDHeaderKey, token.Subject()) when the token is verified
and Subject() is non-empty and userUUIDHeaderKey is configured.
---
Nitpick comments:
In `@cli/namespace.go`:
- Around line 68-73: Remove the unused loop counter by deleting the declaration
and increments of index in namespace.go: the variable index (initialized to 1)
and the index++ inside the for loop that iterates over res.Msg.GetNamespaces()
are not used anywhere, so remove those lines and leave the report append logic
(report = append(report, []string{i.GetId(), i.GetName(), i.GetState()}))
untouched.
In `@cli/search.go`:
- Around line 59-76: The function makeSearchAssetRequest currently accepts an
unused namespaceID parameter; remove namespaceID from the function signature and
all call sites (e.g., the call inside client.NewRequest) so the function becomes
makeSearchAssetRequest(text, filter, query, rankby string, size uint32) and
callers pass only those arguments; update any code that invoked
makeSearchAssetRequest(namespaceID, ...) to call it without namespaceID and run
a build to fix any remaining call mismatches.
In `@internal/client/client.go`:
- Around line 34-35: The base URL is built with a hardcoded "http://" scheme;
update the construction to use a configurable scheme instead of forcing HTTP:
read a scheme field from the config (e.g., cfg.Scheme) or default to "http" and
then build baseURL using scheme + "://" + cfg.Host (refer to the baseURL
variable and cfg.Host), and ensure callers that construct cfg populate
cfg.Scheme or the default is clearly documented; if TLS should be auto-detected,
add a flag like cfg.UseTLS that toggles "https" vs "http" when creating baseURL.
In `@internal/server/server.go`:
- Around line 108-112: The /ping handler registered via mux.HandleFunc("/ping",
func(w http.ResponseWriter, r *http.Request) { ... }) currently writes "pong"
but does not set a Content-Type; update the handler to call
w.Header().Set("Content-Type", "text/plain; charset=utf-8") (or another
appropriate MIME type) before writing the body so responses include an explicit
Content-Type header.
In `@internal/server/v1beta1/asset_test.go`:
- Around line 25-26: Remove the stray empty import entries in the import block
of internal/server/v1beta1/asset_test.go (the import declaration in that test
file currently has blank lines 25-26); delete those empty lines so the import
list only contains actual imports, then run gofmt/goimports to ensure formatting
is clean.
In `@internal/server/v1beta1/comment_test.go`:
- Around line 21-25: Remove the extra blank lines in the import block of
comment_test.go so the imports are contiguous; specifically collapse the gap
between "github.com/stretchr/testify/mock" and
"google.golang.org/protobuf/testing/protocmp" (and the following
"google.golang.org/protobuf/types/known/timestamppb") so there are no empty
lines inside the import block.
In `@internal/server/v1beta1/comment.go`:
- Around line 121-127: There is a duplicated validation call—remove the
redundant second call to req.Msg.ValidateAll() and its identical error handling;
keep a single validation block that returns
connect.NewError(connect.CodeInvalidArgument,
errors.New(bodyParserErrorMsg(err))) when req.Msg.ValidateAll() fails (refer to
req.Msg.ValidateAll, connect.NewError, connect.CodeInvalidArgument, and
bodyParserErrorMsg to locate the duplicated logic).
In `@internal/server/v1beta1/discussion_test.go`:
- Around line 22-26: Remove the two empty lines inside the import block (the gap
between "github.com/stretchr/testify/mock" and
"google.golang.org/protobuf/testing/protocmp") so all imports are contiguous;
tidy the import block (preserve ordering/grouping) and run gofmt/goimports to
ensure formatting is clean.
In `@internal/server/v1beta1/lineage_test.go`:
- Around line 17-22: The import block in lineage_test.go contains extraneous
blank lines and trailing whitespace around the import of log
("github.com/raystack/salt/observability/logger") and the following imports
("google.golang.org/protobuf/testing/protocmp",
"google.golang.org/protobuf/types/known/timestamppb"); open the import section
and remove the empty lines and any trailing spaces so the imports are contiguous
and formatted consistently.
In `@internal/server/v1beta1/namespace_test.go`:
- Around line 17-20: Remove the extra blank lines inside the import block in
namespace_test.go so imports are contiguous; specifically, tidy the import
declaration that currently contains stray empty lines around
"google.golang.org/protobuf/types/known/structpb" so there are no blank lines
within the parentheses of the import statement.
In `@internal/server/v1beta1/search_test.go`:
- Around line 19-20: Remove the stray empty import entries from the imports
block in internal/server/v1beta1/search_test.go so there are no blank lines in
the import list; edit the import section used by the test (the package's imports
in search_test.go) to only include actual packages and delete any empty import
lines to clean up the file.
In `@internal/server/v1beta1/tag_template_test.go`:
- Around line 21-22: Remove the empty import entries in the import block of
tag_template_test.go to clean up the imports; edit the import declaration in
that test file (the import block around the top of tag_template_test.go) and
delete any blank lines or empty strings so only required packages remain.
In `@internal/server/v1beta1/tag_test.go`:
- Around line 21-22: Remove the empty import entries in the import block of
tag_test.go (the stray blank lines in the import list), leaving only the actual
package imports; update the import block in internal/server/v1beta1/tag_test.go
so it contains no empty lines or placeholders and then run gofmt/goimports to
ensure imports are properly formatted.
In `@internal/server/v1beta1/type_test.go`:
- Around line 18-22: The import block in internal/server/v1beta1/type_test.go
contains extraneous blank lines (between log
"github.com/raystack/salt/observability/logger" and
"google.golang.org/protobuf/testing/protocmp"); remove those blank/trailing
lines so imports are contiguous and properly grouped, then run gofmt/goimports
to reformat and fix ordering if needed.
In `@pkg/server/interceptor/user.go`:
- Around line 16-22: Remove the redundant pre-declaration of userUUID and
userEmail and use short declarations when reading headers; replace the two-step
pattern with direct assignments like using
req.Header().Get(identityHeaderKeyUUID) and
req.Header().Get(identityHeaderKeyEmail) (refer to variables
identityHeaderKeyUUID, identityHeaderKeyEmail and the req.Header().Get calls in
the interceptor code) so the variables are declared and initialized in one line
each.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf398d67-edba-4caf-b283-bc1d1f28d5d1
⛔ Files ignored due to path filters (9)
go.sumis excluded by!**/*.sumproto/google/api/annotations.pb.gois excluded by!**/*.pb.goproto/google/api/http.pb.gois excluded by!**/*.pb.goproto/protoc-gen-openapiv2/options/annotations.pb.gois excluded by!**/*.pb.goproto/protoc-gen-openapiv2/options/openapiv2.pb.gois excluded by!**/*.pb.goproto/raystack/compass/v1beta1/service.pb.gois excluded by!**/*.pb.goproto/raystack/compass/v1beta1/service.pb.gw.gois excluded by!**/*.pb.gw.goproto/raystack/compass/v1beta1/service_grpc.pb.gois excluded by!**/*.pb.goproto/validate/validate.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (55)
buf.gen.yamlcli/assets.gocli/discussions.gocli/lineage.gocli/namespace.gocli/search.gogo.modinternal/client/client.gointernal/server/server.gointernal/server/v1beta1/asset.gointernal/server/v1beta1/asset_test.gointernal/server/v1beta1/comment.gointernal/server/v1beta1/comment_test.gointernal/server/v1beta1/discussion.gointernal/server/v1beta1/discussion_test.gointernal/server/v1beta1/lineage.gointernal/server/v1beta1/lineage_test.gointernal/server/v1beta1/namespace.gointernal/server/v1beta1/namespace_test.gointernal/server/v1beta1/search.gointernal/server/v1beta1/search_test.gointernal/server/v1beta1/server.gointernal/server/v1beta1/tag.gointernal/server/v1beta1/tag_template.gointernal/server/v1beta1/tag_template_test.gointernal/server/v1beta1/tag_test.gointernal/server/v1beta1/type.gointernal/server/v1beta1/type_test.gointernal/server/v1beta1/user.gointernal/server/v1beta1/user_test.gointernal/store/postgres/asset_repository_test.gointernal/store/postgres/discussion_repository_test.gointernal/store/postgres/lineage_repository_test.gointernal/store/postgres/postgres.gointernal/store/postgres/star_repository_test.gointernal/store/postgres/tag_repository_test.gointernal/store/postgres/tag_template_repository_test.gointernal/store/postgres/user_repository_test.gopkg/grpc_interceptor/interceptor_test.gopkg/grpc_interceptor/mocks/namespace_service.gopkg/grpc_interceptor/namespace_test.gopkg/grpc_interceptor/user.gopkg/grpc_interceptor/user_test.gopkg/server/interceptor/error_response.gopkg/server/interceptor/interceptor.gopkg/server/interceptor/logger.gopkg/server/interceptor/namespace.gopkg/server/interceptor/recovery.gopkg/server/interceptor/user.goproto/google/api/annotations.pb.validate.goproto/google/api/http.pb.validate.goproto/protoc-gen-openapiv2/options/annotations.pb.validate.goproto/protoc-gen-openapiv2/options/openapiv2.pb.validate.goproto/raystack/compass/v1beta1/compassv1beta1connect/service.connect.goproto/validate/validate.pb.validate.go
💤 Files with no reviewable changes (5)
- pkg/grpc_interceptor/interceptor_test.go
- pkg/grpc_interceptor/user.go
- pkg/grpc_interceptor/user_test.go
- pkg/grpc_interceptor/namespace_test.go
- pkg/grpc_interceptor/mocks/namespace_service.go
Remove stale generated dependency stubs (google/api, validate, openapiv2, swagger) that are no longer needed. Flatten proto output from proto/raystack/compass/v1beta1/ to proto/compassv1beta1/ by switching buf.gen.yaml from paths=source_relative to module mode with an explicit go_package override.
- Remove unused gRPC health check (internal/server/health/) - Replace gRPC status error handling with Connect errors in main.go - Simplify error_response interceptor to remove gRPC dependency - Switch client from gRPC wire format to Connect ProtoJSON - Remove stale gRPC config from config.yaml and docs - Remove gRPC ClientConn reference from golangci config - Update README and docs to reflect Connect RPC
Fix golangci-lint errors (errcheck, ineffassign, staticcheck) and address CodeRabbit review findings: correct error codes, prevent internal error leakage, add input validation, and sanitize panic recovery output.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/server/v1beta1/namespace.go (1)
41-49:⚠️ Potential issue | 🟠 MajorReject empty
urnvalues before dispatch.
GetNamespacestill falls through toGetByName(""), andUpdateNamespacecan forward an all-zero identifier/name to the service. Both handlers should returnCodeInvalidArgumentimmediately instead of delegating an empty target.Suggested fix
import ( "context" + "errors" "connectrpc.com/connect" "github.com/google/uuid" "github.com/raystack/compass/core/namespace" compassv1beta1 "github.com/raystack/compass/proto/compassv1beta1" "google.golang.org/protobuf/types/known/structpb" ) @@ func (server *APIServer) GetNamespace(ctx context.Context, req *connect.Request[compassv1beta1.GetNamespaceRequest]) (*connect.Response[compassv1beta1.GetNamespaceResponse], error) { + if req.Msg.GetUrn() == "" { + return nil, connect.NewError(connect.CodeInvalidArgument, errors.New("urn is required")) + } + var ns *namespace.Namespace if nsID, err := uuid.Parse(req.Msg.GetUrn()); err == nil { @@ func (server *APIServer) UpdateNamespace(ctx context.Context, req *connect.Request[compassv1beta1.UpdateNamespaceRequest]) (*connect.Response[compassv1beta1.UpdateNamespaceResponse], error) { + if req.Msg.GetUrn() == "" { + return nil, connect.NewError(connect.CodeInvalidArgument, errors.New("urn is required")) + } + var nsID uuid.UUID var nsName stringAlso applies to: 90-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/v1beta1/namespace.go` around lines 41 - 49, In GetNamespace and UpdateNamespace handlers, validate and reject empty urn/identifier/name values before calling the service: check req.Msg.GetUrn() (and for UpdateNamespace the incoming name/ID fields) and immediately return connect.NewError(connect.CodeInvalidArgument, errors.New("empty urn/identifier")) instead of calling namespaceService.GetByName/GetByID or forwarding an all-zero identifier; update the APIServer.GetNamespace and APIServer.UpdateNamespace entry points to perform this simple non-empty check to prevent delegating empty targets to the service.
🧹 Nitpick comments (2)
internal/client/client.go (1)
34-35: HTTPS configuration currently not needed for this local development client.The hardcoded
http://scheme is appropriate for the CLI client's intended use case (local development with defaultlocalhost:8080). Production deployments typically handle TLS at the infrastructure level (ingress/proxy) rather than in the application client.If future requirements include remote server support or client-side HTTPS enforcement, the suggested configuration approach would be a reasonable enhancement. For now, this is not a critical concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/client.go` around lines 34 - 35, Replace the hardcoded "http://" when building baseURL with a configurable scheme: add a scheme field to the config (e.g., cfg.Scheme defaulting to "http"), then build baseURL as cfg.Scheme + "://" + cfg.Host; update any config initialization to set cfg.Scheme to "http" for local dev so current behavior is unchanged but future HTTPS can be enabled by setting cfg.Scheme to "https". Ensure you update usages of baseURL in client construction to use the new scheme-aware baseURL and validate cfg.Scheme is non-empty.internal/server/v1beta1/user.go (1)
27-28: Consider usingerrors.Newfor consistency with other handlers.This file uses
fmt.Errorf("%s", bodyParserErrorMsg(err))whilecomment.gouseserrors.New(bodyParserErrorMsg(err)). The latter is more idiomatic for wrapping a string as an error.♻️ Example fix (apply to all similar occurrences in this file)
- return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("%s", bodyParserErrorMsg(err))) + return nil, connect.NewError(connect.CodeInvalidArgument, errors.New(bodyParserErrorMsg(err)))This would also allow removing the
"fmt"import if no other usages remain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/v1beta1/user.go` around lines 27 - 28, Replace the use of fmt.Errorf("%s", bodyParserErrorMsg(err)) with errors.New(bodyParserErrorMsg(err)) wherever request validation fails (e.g., in the block around req.Msg.ValidateAll()), and update the connect.NewError call to pass the errors.New result; also remove the "fmt" import if it becomes unused after this change to match the style used in comment.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/v1beta1/comment.go`:
- Around line 121-127: The code calls req.Msg.ValidateAll() twice in succession
in the request handling path; remove the duplicate block so validation runs only
once and the error returned via connect.NewError(connect.CodeInvalidArgument,
errors.New(bodyParserErrorMsg(err))) is produced from the single call. Locate
the consecutive duplicate calls to req.Msg.ValidateAll() in comment.go and
delete the redundant if-block (leave the first occurrence that constructs the
connect.NewError with bodyParserErrorMsg).
In `@internal/server/v1beta1/namespace.go`:
- Around line 44-49: The code currently maps all errors from
server.namespaceService.GetByID and GetByName to connect.CodeNotFound; change
this to only return connect.CodeNotFound when the error is actually a “not
found” sentinel (use errors.Is(err, yourNotFoundError) or type-assert via
errors.As for the service’s NotFound error), and for all other errors return
connect.CodeInternal (preserving the original err in the NewError call). Update
the branches around namespaceService.GetByID and namespaceService.GetByName to
perform this conditional check and map errors accordingly.
In `@internal/server/v1beta1/tag.go`:
- Around line 93-95: The current nil-check on req.Msg.GetTagValues() in tag.go
doesn't catch an explicit empty slice; update the validation around the calls to
req.Msg.GetTagValues() (the checks at the shown occurrence and the similar one
later) to verify both non-nil and non-empty (e.g., if vals :=
req.Msg.GetTagValues(); len(vals) == 0) and return the same
connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("empty tag values"))
when empty so an explicit [] is rejected at the handler level.
---
Duplicate comments:
In `@internal/server/v1beta1/namespace.go`:
- Around line 41-49: In GetNamespace and UpdateNamespace handlers, validate and
reject empty urn/identifier/name values before calling the service: check
req.Msg.GetUrn() (and for UpdateNamespace the incoming name/ID fields) and
immediately return connect.NewError(connect.CodeInvalidArgument,
errors.New("empty urn/identifier")) instead of calling
namespaceService.GetByName/GetByID or forwarding an all-zero identifier; update
the APIServer.GetNamespace and APIServer.UpdateNamespace entry points to perform
this simple non-empty check to prevent delegating empty targets to the service.
---
Nitpick comments:
In `@internal/client/client.go`:
- Around line 34-35: Replace the hardcoded "http://" when building baseURL with
a configurable scheme: add a scheme field to the config (e.g., cfg.Scheme
defaulting to "http"), then build baseURL as cfg.Scheme + "://" + cfg.Host;
update any config initialization to set cfg.Scheme to "http" for local dev so
current behavior is unchanged but future HTTPS can be enabled by setting
cfg.Scheme to "https". Ensure you update usages of baseURL in client
construction to use the new scheme-aware baseURL and validate cfg.Scheme is
non-empty.
In `@internal/server/v1beta1/user.go`:
- Around line 27-28: Replace the use of fmt.Errorf("%s",
bodyParserErrorMsg(err)) with errors.New(bodyParserErrorMsg(err)) wherever
request validation fails (e.g., in the block around req.Msg.ValidateAll()), and
update the connect.NewError call to pass the errors.New result; also remove the
"fmt" import if it becomes unused after this change to match the style used in
comment.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5d80f9c-6a99-4e4b-9bcf-185fece8d045
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumproto/compassv1beta1/service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (42)
.golangci.ymlMakefileREADME.mdbuf.gen.yamlcli/assets.gocli/discussions.gocli/lineage.gocli/namespace.gocli/search.goconfig/config.yamldocs/docs/concepts/internals.mddocs/docs/configuration.mdgo.modinternal/client/client.gointernal/server/health/health.gointernal/server/health/health_test.gointernal/server/server.gointernal/server/v1beta1/asset.gointernal/server/v1beta1/asset_test.gointernal/server/v1beta1/comment.gointernal/server/v1beta1/comment_test.gointernal/server/v1beta1/discussion.gointernal/server/v1beta1/discussion_test.gointernal/server/v1beta1/lineage.gointernal/server/v1beta1/lineage_test.gointernal/server/v1beta1/namespace.gointernal/server/v1beta1/namespace_test.gointernal/server/v1beta1/search.gointernal/server/v1beta1/search_test.gointernal/server/v1beta1/tag.gointernal/server/v1beta1/tag_template.gointernal/server/v1beta1/tag_template_test.gointernal/server/v1beta1/tag_test.gointernal/server/v1beta1/type.gointernal/server/v1beta1/type_test.gointernal/server/v1beta1/user.gointernal/server/v1beta1/user_test.gomain.gopkg/server/interceptor/error_response.goproto/compass.swagger.yamlproto/compassv1beta1/compassv1beta1connect/service.connect.goproto/compassv1beta1/service.pb.validate.go
💤 Files with no reviewable changes (4)
- .golangci.yml
- proto/compass.swagger.yaml
- internal/server/health/health_test.go
- internal/server/health/health.go
✅ Files skipped from review due to trivial changes (3)
- docs/docs/concepts/internals.md
- internal/server/v1beta1/namespace_test.go
- internal/server/v1beta1/asset.go
🚧 Files skipped from review as they are similar to previous changes (15)
- internal/server/v1beta1/lineage_test.go
- internal/server/v1beta1/type_test.go
- pkg/server/interceptor/error_response.go
- internal/server/v1beta1/discussion_test.go
- cli/lineage.go
- cli/search.go
- internal/server/v1beta1/search_test.go
- internal/server/v1beta1/asset_test.go
- internal/server/v1beta1/user_test.go
- internal/server/v1beta1/lineage.go
- internal/server/v1beta1/tag_test.go
- cli/namespace.go
- internal/server/v1beta1/discussion.go
- internal/server/server.go
- internal/server/v1beta1/tag_template.go
- Update protocolbuffers/go buf plugin to v1.36.11 to match go.mod - Sanitize ErrorResponse interceptor to prevent leaking internal errors by accepting a logger and returning generic error with ref ID - Add namespace.ErrNotFound sentinel to core/namespace and use it in the namespace interceptor to distinguish not-found from other errors - Update postgres namespace repo and cli/server.go to use the new domain-level sentinel
- Remove duplicate ValidateAll() call in UpdateComment handler - Differentiate not-found from other errors in GetNamespace handler using namespace.ErrNotFound sentinel - Use len() check instead of nil for tag_values validation to catch explicit empty slices
ravisuhag
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR migrates Compass from gRPC + grpc-gateway to Connect RPC, following the patterns established by raystack/frontier v0.91.0.
Key Changes
pkg/grpc_interceptor/pkg/server/interceptor/Benefits
Files Changed
New:
pkg/server/interceptor/- Connect interceptors (logger, recovery, error_response, namespace, user)proto/raystack/compass/v1beta1/compassv1beta1connect/- Connect service definitionsModified:
buf.gen.yaml- Buf v2 format with Connect plugininternal/server/server.go- Single port with h2c handlerinternal/client/client.go- Connect clientcli/*.go- Updated to use Connect clientinternal/server/v1beta1/*.go- Handler signatures with Connect wrappers*_test.go- Updated for Connect error handlingDeleted:
pkg/grpc_interceptor/- Old gRPC interceptorsproto/raystack/compass/v1beta1/service.pb.gw.go- Gateway generated codeproto/raystack/compass/v1beta1/service_grpc.pb.go- gRPC generated codeTest plan
go build ./...go test ./...(excluding Docker-dependent tests)curl -X POST http://localhost:8080/raystack.compass.v1beta1.CompassService/GetAllAssets -H "Content-Type: application/json" -d '{}'grpcurl -plaintext localhost:8080 list