fix: Clean up nolint directives marked as TODO - Part 6#910
Open
cbumb wants to merge 7 commits intoNVIDIA:mainfrom
Open
fix: Clean up nolint directives marked as TODO - Part 6#910cbumb wants to merge 7 commits intoNVIDIA:mainfrom
cbumb wants to merge 7 commits intoNVIDIA:mainfrom
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds internal helper functions and refactors across janitor-provider and store-client: GCP instance client setup, Kind container deletion, Postgres connection building, event processing, changelog parsing, env loading, Mongo ping validation, PostgreSQL query utilities, and struct-unmarshaller pointer handling; expands one unmarshaller signature. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
store-client/pkg/datastore/providers/mongodb/watcher/watch_store.go (1)
196-210: Good extraction—now reuse this helper inGetCollectionClientto remove remaining duplication.The same three ping checks are still duplicated later, which can drift from this canonical validator.
♻️ Proposed follow-up refactor
func GetCollectionClient( ctx context.Context, mongoConfig MongoDBConfig, ) (*mongo.Collection, error) { + if err := validatePingConfig(mongoConfig); err != nil { + return nil, err + } + clientOpts, err := constructMongoClientOptions(mongoConfig) if err != nil { return nil, fmt.Errorf("error creating mongoDB clientOpts: %w", err) } @@ - if mongoConfig.TotalPingTimeoutSeconds <= 0 { - return nil, fmt.Errorf("invalid ping timeout value, value must be a positive integer") - } - - if mongoConfig.TotalPingIntervalSeconds <= 0 { - return nil, fmt.Errorf("invalid ping interval value, value must be a positive integer") - } - - if mongoConfig.TotalPingIntervalSeconds >= mongoConfig.TotalPingTimeoutSeconds { - return nil, fmt.Errorf("invalid ping interval value, value must be less than ping timeout") - } - totalTimeout := time.Duration(mongoConfig.TotalPingTimeoutSeconds) * time.Second interval := time.Duration(mongoConfig.TotalPingIntervalSeconds) * time.Second🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/datastore/providers/mongodb/watcher/watch_store.go` around lines 196 - 210, The three ping-parameter checks duplicated in GetCollectionClient should be replaced by a single call to the extracted helper validatePingConfig(MongoDBConfig) and its error handled; locate the duplicated logic inside GetCollectionClient and remove the individual if-checks for TotalPingTimeoutSeconds and TotalPingIntervalSeconds, call validatePingConfig(config) instead, and return or propagate the returned error (preserving the existing error flow) so validation is centralized in the validatePingConfig function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@janitor-provider/pkg/csp/gcp/gcp.go`:
- Around line 209-226: The three error returns in prepareInstanceOp flow
currently propagate raw errors from getAuthenticatedHTTPClient,
compute.NewInstancesRESTClient, and getNodeFields; update each return to wrap
the original error with context using fmt.Errorf, e.g. replace returns like
"return nil, nil, err" with wrapped errors such as fmt.Errorf("failed to create
authenticated HTTP client: %w", err), fmt.Errorf("failed to create instances
client: %w", err), and fmt.Errorf("failed to get node fields for node %q: %w",
node.Name, err); ensure you still close instancesClient on getNodeFields errors
(keep instancesClient.Close and its logged cerr handling) and update all three
call sites in prepareInstanceOp accordingly.
In `@janitor-provider/pkg/csp/kind/kind.go`:
- Around line 110-114: Replace the substring check using strings.Contains on
output with an exact-name match against containerName: instead of checking if
output contains containerName, run a line-oriented exact-match (or regex
anchored with start/end) against the output so only the exact container name
matches (consistent with the existing pattern used elsewhere like name=^%s$).
Update the block that currently examines output and returns
model.TerminateNodeRequestRef("") to use this exact-match logic (referencing the
variables output and containerName) so false positives like "node-10" matching
"node-1" are avoided.
In `@store-client/pkg/client/event_processor.go`:
- Around line 223-231: markProcessed currently calls
changeStreamWatcher.MarkProcessed with an empty token, which drops the resume
token; change markProcessed to accept the event (or its resume token) and pass
the real token to changeStreamWatcher.MarkProcessed instead of []byte{} (e.g.,
func (p *DefaultEventProcessor) markProcessed(ctx context.Context, evt *Event)
error or markProcessed(ctx context.Context, token []byte) error), and update
call sites in handleSingleEvent and handleProcessingError to pass the event (or
event.ResumeToken) through so the watcher receives the correct resume token.
In `@store-client/pkg/client/postgresql_changestream.go`:
- Around line 401-403: The current check skips $match when entry.newValues is
nil (e.g., delete events), letting events match incorrectly; update the
conditional around w.matchesFilters so that when entry.newValues is nil you
still evaluate matchMap against the available row data (e.g., entry.oldValues)
or otherwise call w.matchesFilters with a non-nil map, and ensure matchesFilters
handles nil safely; specifically change the block that references
entry.newValues and w.matchesFilters(matchMap, entry.newValues) to fall back to
entry.oldValues (or a merged values map) so non-operationType predicates are
still evaluated.
- Around line 459-466: extractValue currently returns nil, which breaks
field-based pipeline matching in matchesFilters; implement dot-notation
traversal in extractValue to walk nested maps and slices using path segments
(split on '.'), interpreting numeric segments as array indices when the current
value is a []interface{}, using type assertions on map[string]interface{} for
object keys, returning nil if any segment is missing or index out of range, and
returning the final resolved value (or the original value if path is empty);
also remove/replace the TODO with a proper issue reference. Use the function
name extractValue and callers like matchesFilters to locate where this behavior
is required.
In `@store-client/pkg/config/env_loader.go`:
- Around line 193-201: The code returns raw errors from getRequiredEnv (e.g.,
when obtaining connectionURI and databaseName and at the other occurrences
flagged) which loses caller context; change each "return ..., err" that
propagates getRequiredEnv errors to wrap the error with fmt.Errorf including a
short caller context (for example: fmt.Errorf("load env <ENV_NAME>: %w", err)),
and ensure fmt is imported if not already; update all instances referenced (the
getRequiredEnv calls for connectionURI, databaseName and the other occurrences
in env_loader.go) to use this wrapping pattern.
In `@store-client/pkg/datastore/providers/mongodb/watcher/watch_store.go`:
- Around line 117-118: The code validates the Mongo config after calling
mongo.Connect which can leak a connected client on validation failure; move the
validatePingConfig(mongoConfig) call to before creating/connecting the client
(the mongo.Connect/mongo.NewClient sequence) or, if keeping connect-first,
ensure any early return after validatePingConfig calls client.Disconnect(ctx) to
clean up; specifically update the function that constructs the client (the code
invoking mongo.Connect and validatePingConfig) so validation occurs prior to
mongo.Connect or add a defer/explicit client.Disconnect(ctx) on validation error
to avoid leaking the Mongo client.
In `@store-client/pkg/datastore/providers/postgresql/database_client.go`:
- Around line 941-953: The code currently treats a nil filterMap (resulting from
convertFilterToMap when encountering unsupported filter types) as an explicit
"no filter" and returns TRUE; change this so only an explicit nil filter value
yields trueString, while a nil produced by convertFilterToMap for unsupported
filter types returns a descriptive error instead. Also, after calling
filterMapToConditions, wrap any returned error with context (e.g., using
fmt.Errorf("filterMapToConditions for table %s: %w", c.tableName, err)) so the
caller gets actionable information; ensure this logic lives alongside the
maintenanceEventTableName handling and transformFilterMapForMaintenanceEvents
call.
In `@store-client/pkg/storewatcher/unmarshaller.go`:
- Around line 97-109: CopyStructFields currently assumes pointer targets are
struct pointers and that same Kind implies assignability, which causes reflect
panics; update the logic in CopyStructFields to (1) when handling
dstField.Kind() == reflect.Ptr only treat it as a struct pointer if
dstField.Elem().Kind() == reflect.Struct and srcField.Kind()==reflect.Ptr and
srcField.Elem().Kind()==reflect.Struct before calling CopyStructFields on
Elem(); (2) for non-struct pointers (e.g., *string, *int) handle by allocating a
new element with reflect.New(dstField.Type().Elem()), and copy/convert the src
value into dstField.Elem() only if types are assignable or convertible (use
Type().AssignableTo/ConvertibleTo checks); (3) when dstField.Kind() ==
reflect.Struct ensure srcField.Kind() == reflect.Struct before recursing; and
(4) before dstField.Set(srcField) verify
srcField.Type().AssignableTo(dstField.Type()) (or perform a safe conversion) and
that dstField.CanSet()/srcField.IsValid() to avoid Set panics.
---
Nitpick comments:
In `@store-client/pkg/datastore/providers/mongodb/watcher/watch_store.go`:
- Around line 196-210: The three ping-parameter checks duplicated in
GetCollectionClient should be replaced by a single call to the extracted helper
validatePingConfig(MongoDBConfig) and its error handled; locate the duplicated
logic inside GetCollectionClient and remove the individual if-checks for
TotalPingTimeoutSeconds and TotalPingIntervalSeconds, call
validatePingConfig(config) instead, and return or propagate the returned error
(preserving the existing error flow) so validation is centralized in the
validatePingConfig function.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
janitor-provider/pkg/csp/gcp/gcp.gojanitor-provider/pkg/csp/kind/kind.gostore-client/pkg/adapter/legacy_adapter.gostore-client/pkg/client/event_processor.gostore-client/pkg/client/postgresql_changestream.gostore-client/pkg/config/env_loader.gostore-client/pkg/datastore/providers/mongodb/watcher/watch_store.gostore-client/pkg/datastore/providers/postgresql/database_client.gostore-client/pkg/storewatcher/unmarshaller.go
store-client/pkg/datastore/providers/mongodb/watcher/watch_store.go
Outdated
Show resolved
Hide resolved
Contributor
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 (5)
janitor-provider/pkg/csp/gcp/gcp.go (1)
128-131:⚠️ Potential issue | 🟡 MinorUnwrapped errors from
ResetandDeleteviolate the error-wrapping guideline.Both
instancesClient.Reset(line 130) andinstancesClient.Delete(line 200) return raw errors, making it hard for callers to know which operation produced the failure. While these lines are pre-existing, they are part of the same error-propagation chain that was just refactored.♻️ Proposed fix
op, err := instancesClient.Reset(ctx, resetReq) if err != nil { - return "", err + return "", fmt.Errorf("failed to reset instance %q: %w", nodeFields.instance, err) }op, err := instancesClient.Delete(ctx, deleteReq) if err != nil { - return "", err + return "", fmt.Errorf("failed to delete instance %q: %w", nodeFields.instance, err) }As per coding guidelines: "Wrap errors with context using
fmt.Errorf("context: %w", err)in Go code".Also applies to: 198-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@janitor-provider/pkg/csp/gcp/gcp.go` around lines 128 - 131, Wrap the raw errors returned from instancesClient.Reset and instancesClient.Delete with contextual messages using fmt.Errorf so callers can identify which GCP operation failed; for the Reset call (instancesClient.Reset(ctx, resetReq)) change the return to wrap err (e.g. fmt.Errorf("reset instance %s: %w", instanceID, err)) and for the Delete call (instancesClient.Delete(ctx, deleteReq)) similarly wrap the error (e.g. fmt.Errorf("delete instance %s: %w", instanceID, err)), ensuring you import fmt if not already present and preserve the original error via %w.store-client/pkg/client/postgresql_changestream.go (1)
422-431:⚠️ Potential issue | 🟠 Major
$infilter only evaluates the first element — multi-operation filters silently broken.
mapOperationTypereturns the mapping ofarr[0]only. For a pipeline filter like{"$in": ["insert", "update"]},expectedOpbecomes"INSERT", and anyUPDATEevent is rejected withentry.operation != expectedOp. Since the PR claims no behaviour changes, this is a regression introduced by extractingmapOperationType.🐛 Proposed fix
case map[string]interface{}: // Handle {"$in": ["insert", "update"]} style filters if inArray, ok := v["$in"]; ok { if arr, ok := inArray.([]interface{}); ok && len(arr) > 0 { - // For simplicity, just check the first operation - if op, ok := arr[0].(string); ok { - return w.mapOperationType(op) + // Return empty string; callers must check each element. + // Signal match by returning a sentinel or refactor caller. } } }A cleaner approach is to change
mapOperationTypeto return[]stringand updatematchesPipelineto checkentry.operationagainst all mapped values:// mapOperationType maps one or more MongoDB operationType values to PostgreSQL. func (w *PostgreSQLChangeStreamWatcher) mapOperationType(opType interface{}) []string { switch v := opType.(type) { case string: mapped := w.mapSingleOpType(v) if mapped != "" { return []string{mapped} } case map[string]interface{}: if inArray, ok := v["$in"]; ok { if arr, ok := inArray.([]interface{}); ok { var ops []string for _, elem := range arr { if s, ok := elem.(string); ok { if mapped := w.mapSingleOpType(s); mapped != "" { ops = append(ops, mapped) } } } return ops } } } return nil }Then in
matchesPipeline:- expectedOp := w.mapOperationType(opType) - if expectedOp != "" && entry.operation != expectedOp { - return false - } + expectedOps := w.mapOperationType(opType) + if len(expectedOps) > 0 && !slices.Contains(expectedOps, entry.operation) { + return false + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/client/postgresql_changestream.go` around lines 422 - 431, The mapOperationType currently maps only the first element of an "$in" array, causing multi-operation filters to drop events; change PostgreSQLChangeStreamWatcher.mapOperationType to return a slice of strings ([]string) by handling string inputs (map to single-item slice via mapSingleOpType) and by iterating over map["$in"] arrays to map each element (skipping non-strings or unmapped values), then update matchesPipeline to treat the returned value as a list and accept the entry when entry.operation equals any of the mapped strings (instead of comparing only to a single expectedOp).store-client/pkg/client/event_processor.go (1)
152-179:⚠️ Potential issue | 🟡 MinorUnmarshal/document-ID error paths ignore
MarkProcessedOnError, always marking as processed.Both error branches (unmarshal at Line 163, document-ID at Line 174) call
markProcessedunconditionally, bypassingconfig.MarkProcessedOnError. Only handler errors go throughhandleProcessingErrorwhich respects that flag. A caller who configuresMarkProcessedOnError: falseto "preserve events for retry on next restart" would expect that contract to hold for parse failures too — instead, malformed events are silently skipped even with the default config.If the intent is that parse failures are unrecoverable and should always be skipped, that decision should be documented in the code and in
EventProcessorConfig.MarkProcessedOnError's godoc.📝 Suggested doc clarification
- // MarkProcessedOnError determines whether to mark events as processed even if handler returns error - // Set to false (default) to preserve event for retry on next restart - // Set to true to skip failed events and continue (use with caution - may lose events) + // MarkProcessedOnError determines whether to mark events as processed when the event + // HANDLER returns an error. Set to false (default) to preserve the event for retry on + // next restart. Set to true to skip failed events and continue processing + // (use with caution - may lose events). + // Note: parse-level failures (unmarshal errors, missing document ID) always mark + // the event as processed regardless of this flag, as they are considered unrecoverable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/client/event_processor.go` around lines 152 - 179, The unmarshal and GetDocumentID error paths in DefaultEventProcessor.handleSingleEvent currently always call markProcessed, ignoring EventProcessorConfig.MarkProcessedOnError; update those branches to respect the config by only calling p.markProcessed(ctx, token) when p.config.MarkProcessedOnError is true, otherwise route the error through p.handleProcessingError(ctx, token, err, eventID?) (or an appropriate wrapper error) so the error-handling logic that respects MarkProcessedOnError is used; alternatively, if parse failures are intended to be always skipped, add a clear comment/godoc on EventProcessorConfig.MarkProcessedOnError and on handleSingleEvent stating parse errors are unrecoverable. Ensure references to p.markProcessed, p.handleProcessingError, DefaultEventProcessor.handleSingleEvent and EventProcessorConfig.MarkProcessedOnError are used to locate and change the code.store-client/pkg/datastore/providers/postgresql/database_client.go (2)
840-892:⚠️ Potential issue | 🟠 Major
FindOneskipstransformFilterMapForMaintenanceEvents, creating asymmetry withFind.
Finddelegates toresolveFilterMapForFind(Line 910), which appliestransformFilterMapForMaintenanceEventsfor themaintenance_eventstable — converting camelCase filter keys (e.g.scheduledStartTime) to the indexed snake_case columns (e.g.scheduled_start_time).FindOnestill uses the inline path (Lines 850–865) and never applies that transformation.For the
maintenance_eventstable this meansFindOnequeries miss indexed columns and may return incorrect results if the filter key doesn't exist in the JSONB document under the camelCase name.🐛 Proposed fix — align `FindOne` with `Find`
func (c *PostgreSQLDatabaseClient) FindOne( ctx context.Context, filter interface{}, options *client.FindOneOptions, ) (client.SingleResult, error) { var whereClause string var args []interface{} if builder, ok := filter.(*query.Builder); ok { whereClause, args = builder.ToSQL() - } else if filterMap != nil { - conditions, err := c.filterMapToConditions(filterMap) - if err != nil { - return nil, err - } - whereClause, args = conditionsToWhereClause(conditions) - } else { - slog.Error("Unsupported filter type", "filterType", fmt.Sprintf("%T", filter)) - return nil, fmt.Errorf("unsupported filter type") + } else { + var err error + whereClause, args, err = c.resolveFilterMapForFind(filter) + if err != nil { + return nil, err + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/datastore/providers/postgresql/database_client.go` around lines 840 - 892, FindOne currently converts the filter inline (via convertFilterToMap) and builds conditions directly, skipping the same maintenance-events key transformation used by Find; update FindOne to reuse resolveFilterMapForFind (the same helper used by Find) to obtain the transformed filter map (which in turn calls transformFilterMapForMaintenanceEvents for the maintenance_events table) before calling filterMapToConditions/conditionsToWhereClause, so camelCase keys like scheduledStartTime are converted to the indexed snake_case columns; ensure the code path still handles query.Builder inputs the same way and preserves existing error handling and SQL construction.
472-524:⚠️ Potential issue | 🟡 MinorWrap errors from
convertUpdateToSetClauseandconvertFilterToWhereClausewith context.Lines 479 and 487 return errors unwrapped, violating the
fmt.Errorf("context: %w", err)guideline and making stack-level attribution harder.📝 Proposed fix
- setClause, updateArgs, err := c.convertUpdateToSetClause(update) - if err != nil { - return nil, err - } + setClause, updateArgs, err := c.convertUpdateToSetClause(update) + if err != nil { + return nil, fmt.Errorf("updateDocuments set clause: %w", err) + } - whereClause, filterArgs, err := c.convertFilterToWhereClause(filter, paramOffset) - if err != nil { - return nil, err - } + whereClause, filterArgs, err := c.convertFilterToWhereClause(filter, paramOffset) + if err != nil { + return nil, fmt.Errorf("updateDocuments where clause: %w", err) + }As per coding guidelines: "Wrap errors with context using
fmt.Errorf("context: %w", err)in Go code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/datastore/providers/postgresql/database_client.go` around lines 472 - 524, In updateDocuments, wrap errors returned from convertUpdateToSetClause and convertFilterToWhereClause with contextual fmt.Errorf calls instead of returning them raw; update the error handling after calling convertUpdateToSetClause to return fmt.Errorf("convert update to SET clause: %w", err) and after convertFilterToWhereClause to return fmt.Errorf("convert filter to WHERE clause: %w", err) so callers see contextual error messages (refer to updateDocuments, convertUpdateToSetClause, convertFilterToWhereClause).
♻️ Duplicate comments (4)
store-client/pkg/client/postgresql_changestream.go (1)
402-404:$matchfield filters still bypassed for delete events (nilnewValues).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/client/postgresql_changestream.go` around lines 402 - 404, The $match filters are skipped for delete events because the code only checks entry.newValues; update the logic in the change-stream evaluation (where entry.newValues and w.matchesFilters(matchMap, ... ) are used) to also run matchesFilters against entry.oldValues when entry.newValues is nil (i.e., for delete events), or otherwise construct the document to match against oldValues for deletes; ensure you reference entry.newValues, entry.oldValues and the matchesFilters(matchMap, ...) call so delete events are filtered by the same $match criteria as inserts/updates.store-client/pkg/datastore/providers/postgresql/database_client.go (2)
934-959: LGTM —resolveFilterMapForFindcorrectly handles nil filters and wraps errors with context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/datastore/providers/postgresql/database_client.go` around lines 934 - 959, No change required: the resolveFilterMapForFind method already handles nil filters, converts and validates the filter map, applies maintenance_events field transformation via transformFilterMapForMaintenanceEvents when c.tableName == maintenanceEventTableName, wraps errors from filterMapToConditions with table context, and returns the whereClause and args produced by conditionsToWhereClause; leave PostgreSQLDatabaseClient.resolveFilterMapForFind as-is.
422-470: LGTM —convertUpdateToSetClausenow correctly errors on unsupported update types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/datastore/providers/postgresql/database_client.go` around lines 422 - 470, The review comment contains duplicated review metadata/tags (e.g. both "[approve_code_changes]" and "[duplicate_comment]")—clean up the PR by removing the redundant tag so only the intended approval marker remains; no code changes in convertUpdateToSetClause, builder.SetDocumentField, builder.Set, or related logic are required, just edit the review/comment to remove the duplicate marker.store-client/pkg/storewatcher/unmarshaller.go (1)
93-108: LGTM —CanSet()guard and type-safety checks address the previous reflect panic risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/storewatcher/unmarshaller.go` around lines 93 - 108, The reflect panic risk has been addressed by guarding with dstField.CanSet() and explicit kind/type checks in the switch; no code changes required—keep the dstField.CanSet() guard and the current cases that call copyPtrField(dstField, srcField), CopyStructFields(dstField, srcField), and use srcField.Type().AssignableTo / ConvertibleTo before calling dstField.Set or dstField.Set(srcField.Convert(...)).
🧹 Nitpick comments (4)
janitor-provider/pkg/csp/gcp/gcp.go (1)
206-229: Error wrapping fully addressed; consider makingprepareInstanceOpa package-level function.The error-context wrapping at lines 211, 216, and 225 is correct (past review comment resolved ✅), and the
instancesClient.Close()guard on thegetNodeFieldsfailure path correctly prevents a resource leak.One optional consistency note:
prepareInstanceOpis defined as a method on*Client, but it never accesses the receiver — matching the pattern of the two sibling helpers (getAuthenticatedHTTPClient,getNodeFields) that are package-level functions. SinceClientis an empty struct, there is no future state it could plausibly need.♻️ Optional: convert to package-level function
-func (c *Client) prepareInstanceOp( - ctx context.Context, node corev1.Node, -) (*compute.InstancesClient, *gcpNodeFields, error) { +func prepareInstanceOp( + ctx context.Context, node corev1.Node, +) (*compute.InstancesClient, *gcpNodeFields, error) {Then update the two call sites:
- instancesClient, nodeFields, err := c.prepareInstanceOp(ctx, node) + instancesClient, nodeFields, err := prepareInstanceOp(ctx, node)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@janitor-provider/pkg/csp/gcp/gcp.go` around lines 206 - 229, prepareInstanceOp is a method on *Client but does not use the receiver; make it a package-level function by removing the receiver from func (c *Client) prepareInstanceOp -> func prepareInstanceOp and update all call sites that currently invoke c.prepareInstanceOp(...) to call prepareInstanceOp(...); keep the existing logic (calls to getAuthenticatedHTTPClient, compute.NewInstancesRESTClient, getNodeFields and the instancesClient.Close() on error) unchanged.store-client/pkg/client/postgresql_changestream.go (1)
342-376:scanChangelogEntrynever uses its receiver — consider making it a package-level function.The method body only operates on
rows *sql.Rowsand never reads or writes any field ofw. A standalone functionscanChangelogEntry(rows *sql.Rows) (*postgresqlEvent, error)would be cleaner and avoid falsely implying watcher state is involved.♻️ Proposed refactor
-func (w *PostgreSQLChangeStreamWatcher) scanChangelogEntry(rows *sql.Rows) (*postgresqlEvent, error) { +func scanChangelogEntry(rows *sql.Rows) (*postgresqlEvent, error) {Then update the call site at line 320:
- entry, err := w.scanChangelogEntry(rows) + entry, err := scanChangelogEntry(rows)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/client/postgresql_changestream.go` around lines 342 - 376, The method scanChangelogEntry has an unused receiver (PostgreSQLChangeStreamWatcher) so convert it to a package-level function scanChangelogEntry(rows *sql.Rows) (*postgresqlEvent, error) that performs the same scanning and JSON unmarshalling into postgresqlEvent, then update every call site that currently invokes w.scanChangelogEntry(...) (e.g. in the change stream processing loop) to call scanChangelogEntry(...) instead; ensure imports and visibility remain correct and run tests to validate behavior.janitor-provider/pkg/csp/kind/kind.go (1)
126-127: Remove the unusedctxparameter fromdeleteAndVerifyContainer.The
ctxparameter is never referenced in the helper function—onlydockerCtxis used. Removing it simplifies the signature and reduces cognitive overhead.Proposed cleanup
- if err := c.deleteAndVerifyContainer(ctx, dockerCtx, containerName); err != nil { + if err := c.deleteAndVerifyContainer(dockerCtx, containerName); err != nil { return "", err } @@ func (c *Client) deleteAndVerifyContainer( - ctx, dockerCtx context.Context, containerName string, + dockerCtx context.Context, containerName string, ) error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@janitor-provider/pkg/csp/kind/kind.go` around lines 126 - 127, The helper method deleteAndVerifyContainer has an unused ctx parameter; update its signature to remove the unused ctx and adjust all call sites (e.g., where c.deleteAndVerifyContainer(ctx, dockerCtx) is invoked) to pass only dockerCtx; modify the method definition for deleteAndVerifyContainer accordingly and ensure any imports/uses inside that function reference dockerCtx (and not ctx) so compilation and behavior remain unchanged.store-client/pkg/storewatcher/unmarshaller.go (1)
110-125: Consider adefaultdebug log incopyPtrFieldfor unmatched pointer combinations.When
srcFieldis non-nil, the pointed-to types are not both structs, the pointer types are not directly assignable, and the elements are not convertible, the function silently leavesdstFieldunchanged. This is safe today, but will silently drop fields if struct shapes diverge in the future.🔍 Suggested default case
case srcField.Type().Elem().ConvertibleTo(dstField.Type().Elem()): newPtr := reflect.New(dstField.Type().Elem()) newPtr.Elem().Set(srcField.Elem().Convert(dstField.Type().Elem())) dstField.Set(newPtr) + default: + slog.Debug("copyPtrField: skipping incompatible pointer field", + "srcType", srcField.Type().String(), + "dstType", dstField.Type().String()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/storewatcher/unmarshaller.go` around lines 110 - 125, In copyPtrField: when none of the existing switch branches match (srcField non-nil, not both struct pointers, not assignable, and not convertible) the function currently returns silently and may drop fields; add a default branch that emits a debug/log message (using the package's logger or a new debug logger) that includes the dstField.Type(), srcField.Type(), and a note that the pointer combination was skipped, referencing copyPtrField and CopyStructFields so the log pinpoints the code path; do not change the current behavior (do not set dstField) — only add the diagnostic log for future debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@janitor-provider/pkg/csp/gcp/gcp.go`:
- Around line 128-131: Wrap the raw errors returned from instancesClient.Reset
and instancesClient.Delete with contextual messages using fmt.Errorf so callers
can identify which GCP operation failed; for the Reset call
(instancesClient.Reset(ctx, resetReq)) change the return to wrap err (e.g.
fmt.Errorf("reset instance %s: %w", instanceID, err)) and for the Delete call
(instancesClient.Delete(ctx, deleteReq)) similarly wrap the error (e.g.
fmt.Errorf("delete instance %s: %w", instanceID, err)), ensuring you import fmt
if not already present and preserve the original error via %w.
In `@store-client/pkg/client/event_processor.go`:
- Around line 152-179: The unmarshal and GetDocumentID error paths in
DefaultEventProcessor.handleSingleEvent currently always call markProcessed,
ignoring EventProcessorConfig.MarkProcessedOnError; update those branches to
respect the config by only calling p.markProcessed(ctx, token) when
p.config.MarkProcessedOnError is true, otherwise route the error through
p.handleProcessingError(ctx, token, err, eventID?) (or an appropriate wrapper
error) so the error-handling logic that respects MarkProcessedOnError is used;
alternatively, if parse failures are intended to be always skipped, add a clear
comment/godoc on EventProcessorConfig.MarkProcessedOnError and on
handleSingleEvent stating parse errors are unrecoverable. Ensure references to
p.markProcessed, p.handleProcessingError,
DefaultEventProcessor.handleSingleEvent and
EventProcessorConfig.MarkProcessedOnError are used to locate and change the
code.
In `@store-client/pkg/client/postgresql_changestream.go`:
- Around line 422-431: The mapOperationType currently maps only the first
element of an "$in" array, causing multi-operation filters to drop events;
change PostgreSQLChangeStreamWatcher.mapOperationType to return a slice of
strings ([]string) by handling string inputs (map to single-item slice via
mapSingleOpType) and by iterating over map["$in"] arrays to map each element
(skipping non-strings or unmapped values), then update matchesPipeline to treat
the returned value as a list and accept the entry when entry.operation equals
any of the mapped strings (instead of comparing only to a single expectedOp).
In `@store-client/pkg/datastore/providers/postgresql/database_client.go`:
- Around line 840-892: FindOne currently converts the filter inline (via
convertFilterToMap) and builds conditions directly, skipping the same
maintenance-events key transformation used by Find; update FindOne to reuse
resolveFilterMapForFind (the same helper used by Find) to obtain the transformed
filter map (which in turn calls transformFilterMapForMaintenanceEvents for the
maintenance_events table) before calling
filterMapToConditions/conditionsToWhereClause, so camelCase keys like
scheduledStartTime are converted to the indexed snake_case columns; ensure the
code path still handles query.Builder inputs the same way and preserves existing
error handling and SQL construction.
- Around line 472-524: In updateDocuments, wrap errors returned from
convertUpdateToSetClause and convertFilterToWhereClause with contextual
fmt.Errorf calls instead of returning them raw; update the error handling after
calling convertUpdateToSetClause to return fmt.Errorf("convert update to SET
clause: %w", err) and after convertFilterToWhereClause to return
fmt.Errorf("convert filter to WHERE clause: %w", err) so callers see contextual
error messages (refer to updateDocuments, convertUpdateToSetClause,
convertFilterToWhereClause).
---
Duplicate comments:
In `@store-client/pkg/client/postgresql_changestream.go`:
- Around line 402-404: The $match filters are skipped for delete events because
the code only checks entry.newValues; update the logic in the change-stream
evaluation (where entry.newValues and w.matchesFilters(matchMap, ... ) are used)
to also run matchesFilters against entry.oldValues when entry.newValues is nil
(i.e., for delete events), or otherwise construct the document to match against
oldValues for deletes; ensure you reference entry.newValues, entry.oldValues and
the matchesFilters(matchMap, ...) call so delete events are filtered by the same
$match criteria as inserts/updates.
In `@store-client/pkg/datastore/providers/postgresql/database_client.go`:
- Around line 934-959: No change required: the resolveFilterMapForFind method
already handles nil filters, converts and validates the filter map, applies
maintenance_events field transformation via
transformFilterMapForMaintenanceEvents when c.tableName ==
maintenanceEventTableName, wraps errors from filterMapToConditions with table
context, and returns the whereClause and args produced by
conditionsToWhereClause; leave PostgreSQLDatabaseClient.resolveFilterMapForFind
as-is.
- Around line 422-470: The review comment contains duplicated review
metadata/tags (e.g. both "[approve_code_changes]" and
"[duplicate_comment]")—clean up the PR by removing the redundant tag so only the
intended approval marker remains; no code changes in convertUpdateToSetClause,
builder.SetDocumentField, builder.Set, or related logic are required, just edit
the review/comment to remove the duplicate marker.
In `@store-client/pkg/storewatcher/unmarshaller.go`:
- Around line 93-108: The reflect panic risk has been addressed by guarding with
dstField.CanSet() and explicit kind/type checks in the switch; no code changes
required—keep the dstField.CanSet() guard and the current cases that call
copyPtrField(dstField, srcField), CopyStructFields(dstField, srcField), and use
srcField.Type().AssignableTo / ConvertibleTo before calling dstField.Set or
dstField.Set(srcField.Convert(...)).
---
Nitpick comments:
In `@janitor-provider/pkg/csp/gcp/gcp.go`:
- Around line 206-229: prepareInstanceOp is a method on *Client but does not use
the receiver; make it a package-level function by removing the receiver from
func (c *Client) prepareInstanceOp -> func prepareInstanceOp and update all call
sites that currently invoke c.prepareInstanceOp(...) to call
prepareInstanceOp(...); keep the existing logic (calls to
getAuthenticatedHTTPClient, compute.NewInstancesRESTClient, getNodeFields and
the instancesClient.Close() on error) unchanged.
In `@janitor-provider/pkg/csp/kind/kind.go`:
- Around line 126-127: The helper method deleteAndVerifyContainer has an unused
ctx parameter; update its signature to remove the unused ctx and adjust all call
sites (e.g., where c.deleteAndVerifyContainer(ctx, dockerCtx) is invoked) to
pass only dockerCtx; modify the method definition for deleteAndVerifyContainer
accordingly and ensure any imports/uses inside that function reference dockerCtx
(and not ctx) so compilation and behavior remain unchanged.
In `@store-client/pkg/client/postgresql_changestream.go`:
- Around line 342-376: The method scanChangelogEntry has an unused receiver
(PostgreSQLChangeStreamWatcher) so convert it to a package-level function
scanChangelogEntry(rows *sql.Rows) (*postgresqlEvent, error) that performs the
same scanning and JSON unmarshalling into postgresqlEvent, then update every
call site that currently invokes w.scanChangelogEntry(...) (e.g. in the change
stream processing loop) to call scanChangelogEntry(...) instead; ensure imports
and visibility remain correct and run tests to validate behavior.
In `@store-client/pkg/storewatcher/unmarshaller.go`:
- Around line 110-125: In copyPtrField: when none of the existing switch
branches match (srcField non-nil, not both struct pointers, not assignable, and
not convertible) the function currently returns silently and may drop fields;
add a default branch that emits a debug/log message (using the package's logger
or a new debug logger) that includes the dstField.Type(), srcField.Type(), and a
note that the pointer combination was skipped, referencing copyPtrField and
CopyStructFields so the log pinpoints the code path; do not change the current
behavior (do not set dstField) — only add the diagnostic log for future
debugging.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
janitor-provider/pkg/csp/gcp/gcp.gojanitor-provider/pkg/csp/kind/kind.gostore-client/pkg/client/event_processor.gostore-client/pkg/client/postgresql_changestream.gostore-client/pkg/datastore/providers/mongodb/watcher/watch_store.gostore-client/pkg/datastore/providers/postgresql/database_client.gostore-client/pkg/storewatcher/unmarshaller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- store-client/pkg/datastore/providers/mongodb/watcher/watch_store.go
cbumb
force-pushed
the
728_nolint-cleanup-6
branch
from
c089785 to
9fde532
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
store-client/pkg/client/postgresql_changestream.go (1)
403-405:⚠️ Potential issue | 🟠 Major
$matchfield predicates are still bypassed whennewValuesis nil.On Line [403], delete events can skip non-
operationTypefiltering entirely, causing false-positive matches. Evaluate againstoldValueswhennewValuesis absent.✅ Proposed patch
- if entry.newValues != nil && !w.matchesFilters(matchMap, entry.newValues) { - return false - } + payload := entry.newValues + if payload == nil { + payload = entry.oldValues + } + + if payload == nil { + // If field predicates exist but no payload is available, fail closed. + if len(matchMap) > 1 { + return false + } + } else if !w.matchesFilters(matchMap, payload) { + return false + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/client/postgresql_changestream.go` around lines 403 - 405, The current check uses entry.newValues only, letting delete events (where newValues is nil) bypass $match predicates; change the logic in the block around the conditional that calls w.matchesFilters(matchMap, ...) so that when entry.newValues is nil it evaluates matches against entry.oldValues instead (i.e., call w.matchesFilters(matchMap, entry.newValues ?? entry.oldValues)), and ensure you treat the case where both newValues and oldValues are nil as a non-match; update the conditional near the call to w.matchesFilters to reflect this fallback behavior.
🧹 Nitpick comments (3)
janitor-provider/pkg/csp/gcp/gcp.go (2)
109-112: Wrap theprepareInstanceOperror with caller context.Both
SendRebootSignalandSendTerminateSignalreturn the error fromprepareInstanceOpunwrapped. Per coding guidelines, every propagation site should add context so the call stack is self-evident in logs.♻️ Proposed fix
- instancesClient, nodeFields, err := prepareInstanceOp(ctx, node) - if err != nil { - return "", err - } + instancesClient, nodeFields, err := prepareInstanceOp(ctx, node) + if err != nil { + return "", fmt.Errorf("send reboot signal: %w", err) + }- instancesClient, nodeFields, err := prepareInstanceOp(ctx, node) - if err != nil { - return "", err - } + instancesClient, nodeFields, err := prepareInstanceOp(ctx, node) + if err != nil { + return "", fmt.Errorf("send terminate signal: %w", err) + }As per coding guidelines: "Wrap errors with context using
fmt.Errorf("context: %w", err)in Go code".Also applies to: 179-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@janitor-provider/pkg/csp/gcp/gcp.go` around lines 109 - 112, In SendRebootSignal and SendTerminateSignal, wrap the error returned from prepareInstanceOp with caller context using fmt.Errorf, e.g. return "", fmt.Errorf("SendRebootSignal: prepareInstanceOp failed: %w", err) (and similarly for SendTerminateSignal) so the propagated error includes the function context; update both call sites that currently do `if err != nil { return "", err }` to use the wrapped error form referencing prepareInstanceOp.
130-130: Use%qfor instance name in error strings for consistency.Lines 130 and 200 use
%sfornodeFields.instance, while line 225 uses%qfornode.Name. Quoted identifiers are easier to distinguish in log output.♻️ Proposed fix
- return "", fmt.Errorf("reset instance %s: %w", nodeFields.instance, err) + return "", fmt.Errorf("reset instance %q: %w", nodeFields.instance, err)- return "", fmt.Errorf("delete instance %s: %w", nodeFields.instance, err) + return "", fmt.Errorf("delete instance %q: %w", nodeFields.instance, err)Also applies to: 200-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@janitor-provider/pkg/csp/gcp/gcp.go` at line 130, Replace the unquoted instance identifiers in the fmt.Errorf calls with quoted format verbs for consistency: change the fmt.Errorf call that formats nodeFields.instance (the "reset instance %s: %w" invocation) to use %q instead of %s, and do the same for the other fmt.Errorf that currently uses nodeFields.instance at the second occurrence; this will match the existing use of %q for node.Name and produce quoted identifiers in logs.store-client/pkg/client/postgresql_changestream.go (1)
321-341: Handle row-iteration errors after therows.Next()loop.
rows.Next()loop exits silently on driver/stream errors unlessrows.Err()is checked. Add a post-loop check so polling failures are observable.💡 Proposed patch
for rows.Next() { entry, err := scanChangelogEntry(rows) if err != nil { slog.Error("Failed to process changelog entry", "error", err) continue } @@ } } + + if err := rows.Err(); err != nil { + slog.Error("Failed while iterating changelog rows", "error", err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/client/postgresql_changestream.go` around lines 321 - 341, The rows.Next() loop in the worker that calls scanChangelogEntry (which updates w.lastSeenID, checks w.matchesPipeline and sends entries on w.eventChan) doesn't check rows.Err() after the loop, so driver/stream errors can be ignored; after the loop finishes add a rows.Err() check and handle any error (log via slog.Error or return it/upstream) and/or close/stop the worker (use ctx.Done()/w.stopChan) so polling failures are observable and not silently ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@store-client/pkg/client/postgresql_changestream.go`:
- Around line 396-400: The code in the change filter incorrectly treats an
operationType that maps to zero PostgreSQL ops as permissive; inside the block
that reads matchMap["operationType"] and calls mapOperationTypes(opType)
(producing expectedOps), change the logic so that if len(expectedOps) == 0 you
immediately return false (reject the event) instead of allowing it to pass; keep
the existing check using slices.Contains(entry.operation) for non-empty
expectedOps to preserve normal behavior.
In `@store-client/pkg/datastore/providers/postgresql/database_client.go`:
- Around line 429-441: When updateMap contains a "$set" key, currently sibling
operator keys are ignored; modify the logic in the function handling updates
(the block that reads updateMap and calls c.convertFilterToMap) to detect any
other keys that start with "$" besides "$set" and return an error instead of
proceeding. Specifically, after detecting hasSet, iterate updateMap's keys and
if any key != "$set" and has the "$" prefix, return a descriptive fmt.Errorf
indicating mixed/unsupported Mongo update operators; keep the existing
convertFilterToMap usage for validating the "$set" value and preserve the
existing debug logs when valid.
- Around line 851-853: In FindOne and Find, when calling
resolveFilterMapForFind, don't return the raw error — wrap it with caller
context using fmt.Errorf to preserve operation context; e.g., replace returns
like "return nil, err" after resolveFilterMapForFind with "return nil,
fmt.Errorf(\"FindOne: resolveFilterMapForFind failed: %w\", err)" (use an
appropriate function name in the message for each site), ensuring you import fmt
if missing and apply the same wrapping pattern for the other call site (the one
referenced near the second occurrence).
- Around line 648-655: The isDescending function currently handles only int,
int64, and float64 causing int32 (and other small numeric types from BSON) to
default to ascending; update isDescending to include cases for int8, int16,
int32 and float32 (and any other numeric types you expect) in the type switch so
those values returning < 0 are treated as descending; modify the switch in
isDescending to add case branches for int8, int16, int32, float32 (and mirror
behavior for other signed numeric types if needed) so negative values across
these types return true.
---
Duplicate comments:
In `@store-client/pkg/client/postgresql_changestream.go`:
- Around line 403-405: The current check uses entry.newValues only, letting
delete events (where newValues is nil) bypass $match predicates; change the
logic in the block around the conditional that calls w.matchesFilters(matchMap,
...) so that when entry.newValues is nil it evaluates matches against
entry.oldValues instead (i.e., call w.matchesFilters(matchMap, entry.newValues
?? entry.oldValues)), and ensure you treat the case where both newValues and
oldValues are nil as a non-match; update the conditional near the call to
w.matchesFilters to reflect this fallback behavior.
---
Nitpick comments:
In `@janitor-provider/pkg/csp/gcp/gcp.go`:
- Around line 109-112: In SendRebootSignal and SendTerminateSignal, wrap the
error returned from prepareInstanceOp with caller context using fmt.Errorf, e.g.
return "", fmt.Errorf("SendRebootSignal: prepareInstanceOp failed: %w", err)
(and similarly for SendTerminateSignal) so the propagated error includes the
function context; update both call sites that currently do `if err != nil {
return "", err }` to use the wrapped error form referencing prepareInstanceOp.
- Line 130: Replace the unquoted instance identifiers in the fmt.Errorf calls
with quoted format verbs for consistency: change the fmt.Errorf call that
formats nodeFields.instance (the "reset instance %s: %w" invocation) to use %q
instead of %s, and do the same for the other fmt.Errorf that currently uses
nodeFields.instance at the second occurrence; this will match the existing use
of %q for node.Name and produce quoted identifiers in logs.
In `@store-client/pkg/client/postgresql_changestream.go`:
- Around line 321-341: The rows.Next() loop in the worker that calls
scanChangelogEntry (which updates w.lastSeenID, checks w.matchesPipeline and
sends entries on w.eventChan) doesn't check rows.Err() after the loop, so
driver/stream errors can be ignored; after the loop finishes add a rows.Err()
check and handle any error (log via slog.Error or return it/upstream) and/or
close/stop the worker (use ctx.Done()/w.stopChan) so polling failures are
observable and not silently ignored.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
janitor-provider/pkg/csp/gcp/gcp.gojanitor-provider/pkg/csp/kind/kind.gostore-client/pkg/client/postgresql_changestream.gostore-client/pkg/datastore/providers/postgresql/database_client.go
🚧 Files skipped from review as they are similar to previous changes (1)
- janitor-provider/pkg/csp/kind/kind.go
store-client/pkg/datastore/providers/postgresql/database_client.go
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
store-client/pkg/datastore/providers/postgresql/database_client.go (3)
847-853:resolveFilterMapForFinderrors still returned unwrapped inFindOneandFind.Lines 852 and 901 return raw errors without caller context. This was flagged in a prior review.
As per coding guidelines: "Wrap errors with context using
fmt.Errorf("context: %w", err)in Go code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/datastore/providers/postgresql/database_client.go` around lines 847 - 853, In FindOne and Find (in database_client.go) you return errors from resolveFilterMapForFind directly; update both call sites to wrap those errors with context using fmt.Errorf, e.g. fmt.Errorf("resolving filter for FindOne: %w", err) (or similar for Find), so the returned error includes caller context and references resolveFilterMapForFind in the message.
429-441: Sibling$operators alongside$setare still silently ignored.When
$setis present, other operator keys (e.g.,$unset) are not validated or rejected. This was flagged in a prior review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/datastore/providers/postgresql/database_client.go` around lines 429 - 441, When parsing updateMap in the update path (the block that checks if setOp, hasSet := updateMap["$set"]), ensure we validate and reject any sibling operator keys that start with '$' other than "$set"; after converting setFields with convertFilterToMap, iterate the keys of updateMap and if any key has prefix "$" and is not "$set" return a clear error (e.g., "$set cannot be mixed with other update operators: <op>"). Update the logic around updateMap, setOp, setFields and convertFilterToMap to perform this check and return an error instead of silently ignoring operators like "$unset".
648-659:isDescendingstill missingint32(and other small numeric types from BSON).This was flagged in a prior review. Sort direction values from BSON unmarshaling commonly arrive as
int32, which would silently default to ascending here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/datastore/providers/postgresql/database_client.go` around lines 648 - 659, The isDescending function currently ignores BSON int32 (and other small numeric types) and should treat any signed integer or float value < 0 as descending; update isDescending (in database_client.go) to either add explicit cases for int32, int8, int16 and float32 or (preferably) use reflect to detect signed integer kinds and float kinds and compare their numeric value < 0 so all BSON numeric types are handled consistently.store-client/pkg/config/env_loader.go (1)
193-201: Raw errors fromgetRequiredEnvreturned without caller context.Lines 195 and 200 return the error directly without wrapping. The same pattern appears at lines 246, 251, 256, 439, and 444. This was flagged in a prior review.
As per coding guidelines: "Wrap errors with context using
fmt.Errorf("context: %w", err)in Go code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/config/env_loader.go` around lines 193 - 201, Calls to getRequiredEnv are returning raw errors without context; update each return to wrap the error with fmt.Errorf including which env var failed. For example, replace "connectionURI, err := getRequiredEnv(EnvMongoDBURI); if err != nil { return nil, err }" with wrapping: if err != nil { return nil, fmt.Errorf("failed to load %s: %w", EnvMongoDBURI, err) } and do the same for EnvMongoDBDatabaseName and every other getRequiredEnv call in this file (the occurrences noted around the other flagged lines), importing fmt if necessary. Ensure each wrapped message names the env constant to provide caller context.store-client/pkg/client/postgresql_changestream.go (1)
396-405: Two prior issues remain unaddressed inmatchesPipeline.
Line 398: When
mapOperationTypesreturns an empty slice for an unsupported operation type,len(expectedOps) > 0is false, so the event passes the filter instead of being rejected. This was flagged in a prior review.Lines 403-405: When
entry.newValuesis nil (e.g., delete events),matchesFiltersis skipped entirely, so non-operationTypepredicates are not evaluated. This broadens matching for delete events. This was also flagged in a prior review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/client/postgresql_changestream.go` around lines 396 - 405, mapOperationTypes returning an empty slice should reject the event: in matchesPipeline, if "operationType" exists in matchMap, call mapOperationTypes(opType) and if the returned slice is empty, immediately return false (do not treat empty as a wildcard). Also ensure non-operationType predicates are always evaluated for delete/update: when checking matchesFilters, don't skip it just because entry.newValues is nil — instead call matchesFilters with entry.newValues if present, otherwise with entry.oldValues (and if both are nil, treat as non-match and return false) so delete events still run through the remaining predicates; update the logic around matchMap, matchesFilters, entry.newValues, and entry.oldValues accordingly.
🧹 Nitpick comments (1)
janitor-provider/pkg/csp/kind/kind.go (1)
172-174: Inconsistent container-name matching:strings.Containshere vs exact line matching at lines 112-117.The initial container lookup was changed to exact line-by-line matching to prevent false positives (e.g., "node-10" matching "node-1"), but the post-delete verification still uses
strings.Contains. For consistency and to avoid the same class of bug, apply the same pattern.Proposed fix
- if strings.Contains(string(output), containerName) { + for _, line := range strings.Split(strings.TrimSpace(string(output)), "\n") { + if line == containerName { + return fmt.Errorf("container %s still exists after deletion attempt", containerName) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@janitor-provider/pkg/csp/kind/kind.go` around lines 172 - 174, The post-delete verification currently uses strings.Contains(output, containerName) which can yield false positives; update the check to use the same exact line-by-line matching used earlier (the initial lookup at lines where the code splits output into lines and compares the container name field exactly) so that you only treat the container as present when a line's container identifier exactly equals containerName; apply this precise matching to the verification that follows the delete attempt (the block referencing containerName and output) instead of strings.Contains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@janitor-provider/pkg/csp/kind/kind.go`:
- Around line 136-138: The deleteAndVerifyContainer function currently accepts
an unused ctx parameter; either remove the unused ctx from the signature of
deleteAndVerifyContainer or actually use it by propagating it to the docker exec
calls and checking for parent cancellation (e.g., verify ctx.Err() before/after
execs and pass ctx into exec.CommandContext alongside dockerCtx where
appropriate); update all call sites of deleteAndVerifyContainer to match the new
signature if you remove the parameter or ensure callers pass the intended
context if you elect to use it.
---
Duplicate comments:
In `@store-client/pkg/client/postgresql_changestream.go`:
- Around line 396-405: mapOperationTypes returning an empty slice should reject
the event: in matchesPipeline, if "operationType" exists in matchMap, call
mapOperationTypes(opType) and if the returned slice is empty, immediately return
false (do not treat empty as a wildcard). Also ensure non-operationType
predicates are always evaluated for delete/update: when checking matchesFilters,
don't skip it just because entry.newValues is nil — instead call matchesFilters
with entry.newValues if present, otherwise with entry.oldValues (and if both are
nil, treat as non-match and return false) so delete events still run through the
remaining predicates; update the logic around matchMap, matchesFilters,
entry.newValues, and entry.oldValues accordingly.
In `@store-client/pkg/config/env_loader.go`:
- Around line 193-201: Calls to getRequiredEnv are returning raw errors without
context; update each return to wrap the error with fmt.Errorf including which
env var failed. For example, replace "connectionURI, err :=
getRequiredEnv(EnvMongoDBURI); if err != nil { return nil, err }" with wrapping:
if err != nil { return nil, fmt.Errorf("failed to load %s: %w", EnvMongoDBURI,
err) } and do the same for EnvMongoDBDatabaseName and every other getRequiredEnv
call in this file (the occurrences noted around the other flagged lines),
importing fmt if necessary. Ensure each wrapped message names the env constant
to provide caller context.
In `@store-client/pkg/datastore/providers/postgresql/database_client.go`:
- Around line 847-853: In FindOne and Find (in database_client.go) you return
errors from resolveFilterMapForFind directly; update both call sites to wrap
those errors with context using fmt.Errorf, e.g. fmt.Errorf("resolving filter
for FindOne: %w", err) (or similar for Find), so the returned error includes
caller context and references resolveFilterMapForFind in the message.
- Around line 429-441: When parsing updateMap in the update path (the block that
checks if setOp, hasSet := updateMap["$set"]), ensure we validate and reject any
sibling operator keys that start with '$' other than "$set"; after converting
setFields with convertFilterToMap, iterate the keys of updateMap and if any key
has prefix "$" and is not "$set" return a clear error (e.g., "$set cannot be
mixed with other update operators: <op>"). Update the logic around updateMap,
setOp, setFields and convertFilterToMap to perform this check and return an
error instead of silently ignoring operators like "$unset".
- Around line 648-659: The isDescending function currently ignores BSON int32
(and other small numeric types) and should treat any signed integer or float
value < 0 as descending; update isDescending (in database_client.go) to either
add explicit cases for int32, int8, int16 and float32 or (preferably) use
reflect to detect signed integer kinds and float kinds and compare their numeric
value < 0 so all BSON numeric types are handled consistently.
---
Nitpick comments:
In `@janitor-provider/pkg/csp/kind/kind.go`:
- Around line 172-174: The post-delete verification currently uses
strings.Contains(output, containerName) which can yield false positives; update
the check to use the same exact line-by-line matching used earlier (the initial
lookup at lines where the code splits output into lines and compares the
container name field exactly) so that you only treat the container as present
when a line's container identifier exactly equals containerName; apply this
precise matching to the verification that follows the delete attempt (the block
referencing containerName and output) instead of strings.Contains.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
janitor-provider/pkg/csp/gcp/gcp.gojanitor-provider/pkg/csp/kind/kind.gostore-client/pkg/adapter/legacy_adapter.gostore-client/pkg/client/event_processor.gostore-client/pkg/client/postgresql_changestream.gostore-client/pkg/config/env_loader.gostore-client/pkg/datastore/providers/mongodb/watcher/watch_store.gostore-client/pkg/datastore/providers/postgresql/database_client.gostore-client/pkg/storewatcher/unmarshaller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- store-client/pkg/datastore/providers/mongodb/watcher/watch_store.go
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
cbumb
force-pushed
the
728_nolint-cleanup-6
branch
from
546bd49 to
bb681ac
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
store-client/pkg/client/postgresql_changestream.go (1)
403-405:⚠️ Potential issue | 🟠 Major
$matchfield predicates are still skipped whennewValuesis nil.At Line 403, filtering only against
entry.newValueslets delete events bypass non-operationTypepredicates. Fall back toentry.oldValues(or reject if field predicates exist and no payload is available).✅ Proposed fix
- if entry.newValues != nil && !w.matchesFilters(matchMap, entry.newValues) { - return false - } + payload := entry.newValues + if payload == nil { + payload = entry.oldValues + } + + if payload == nil { + // If field predicates exist but no payload is available, reject. + if len(matchMap) > 1 { + return false + } + } else if !w.matchesFilters(matchMap, payload) { + return false + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/client/postgresql_changestream.go` around lines 403 - 405, The current filter short-circuits by only calling w.matchesFilters(matchMap, entry.newValues) which lets delete events (where entry.newValues == nil) bypass field predicates; change the logic in the block around entry.newValues to: if entry.newValues != nil use w.matchesFilters(matchMap, entry.newValues), else if entry.oldValues != nil use w.matchesFilters(matchMap, entry.oldValues), else if matchMap contains any non-operationType predicates return false (reject because no payload to evaluate). Update the check that currently references entry.newValues and w.matchesFilters to try oldValues as a fallback and enforce rejection when neither payload exists but field predicates are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@store-client/pkg/client/postgresql_changestream.go`:
- Around line 403-405: The current filter short-circuits by only calling
w.matchesFilters(matchMap, entry.newValues) which lets delete events (where
entry.newValues == nil) bypass field predicates; change the logic in the block
around entry.newValues to: if entry.newValues != nil use
w.matchesFilters(matchMap, entry.newValues), else if entry.oldValues != nil use
w.matchesFilters(matchMap, entry.oldValues), else if matchMap contains any
non-operationType predicates return false (reject because no payload to
evaluate). Update the check that currently references entry.newValues and
w.matchesFilters to try oldValues as a fallback and enforce rejection when
neither payload exists but field predicates are present.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
store-client/pkg/client/postgresql_changestream.gostore-client/pkg/datastore/providers/postgresql/database_client.go
🚧 Files skipped from review as they are similar to previous changes (1)
- store-client/pkg/datastore/providers/postgresql/database_client.go
cbumb
force-pushed
the
728_nolint-cleanup-6
branch
from
296cb3a to
f7eb403
Compare
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
Merging this branch will increase overall coverage
Coverage by fileChanged unit test files
|
🛡️ CodeQL Analysis🚨 Found 1 security alert(s) |
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
🛡️ CodeQL Analysis🚨 Found 1 security alert(s) |
Merging this branch changes the coverage (2 decrease, 1 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
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
1 participant
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
Refactors nolint directives by lowering complexity and improving structure. No intended behavior change.
Modules updated in this PR:
store-client
janitor-provider
Type of Change
Component(s) Affected
Testing
Checklist
Summary by CodeRabbit
Refactor
Bug Fixes
Chores