Skip to content

Commit 70aad84

Browse files
Matovidloclaude
andcommitted
fix(notifications): address new bot review feedback
- notification.go: use composite "branchID/configID" key in configsByKey map to prevent collisions when two branches share the same config ID; also collect branch.id filter value alongside job.configuration.id before doing the lookup, require both for a correct match - push.go: add nil guard for v.Remote in parentExists() before dereferencing the NotificationState's Remote pointer - generator.go: remove spurious trailing quote from NotificationPath panic message - mapper.go: correct LocalSaveMapper comment — parent-child manifest relationships (ConfigManifest.Notifications) are populated by setRecords() during manifest load, not by MapBeforeLocalSave - CLI_CONTEXT.md: fix two inaccuracies — notifications are stored in config.json (not meta.json), and filter operators are ==, !=, >, <, >=, <= (not eq, ne, gt, lt, ge, le) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent a0b3ad7 commit 70aad84

5 files changed

Lines changed: 35 additions & 18 deletions

File tree

internal/pkg/mapper/mapper.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,9 @@ import (
1212
// LocalSaveMapper is intended to modify how the object will be saved in the filesystem.
1313
// If you need a list of all saved objects, when they are already saved, use the AfterLocalOperationListener instead.
1414
//
15-
// IMPORTANT for parent-child relationships (e.g., Config → Notifications):
16-
// MapBeforeLocalSave is where you should populate parent manifest relationships like ConfigManifest.Notifications.
17-
// Do NOT populate these in remote load - let the mapper control how objects are grouped and saved.
15+
// Note: parent-child manifest relationships (e.g., ConfigManifest.Notifications) are populated
16+
// automatically by setRecords() during manifest loading, not by this mapper.
17+
// MapBeforeLocalSave is used to write object files (config.json, meta.json, etc.) to the filesystem.
1818
// See docs/CLI_OBJECT_LIFECYCLE.md for the complete flow.
1919
type LocalSaveMapper interface {
2020
MapBeforeLocalSave(ctx context.Context, recipe *model.LocalSaveRecipe) error

internal/pkg/naming/generator.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ func (g Generator) ConfigRowPath(parentPath string, component *keboola.Component
188188

189189
func (g Generator) NotificationPath(parentPath string, notification *model.Notification) model.AbsPath {
190190
if len(parentPath) == 0 {
191-
panic(errors.Errorf(`notification "%s" parent path cannot be empty"`, notification))
191+
panic(errors.Errorf(`notification "%s" parent path cannot be empty`, notification))
192192
}
193193

194194
p := model.AbsPath{}

internal/pkg/plan/push/push.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,9 @@ func parentExists(objectState model.ObjectState, objects model.ObjectStates) boo
5151
branch, branchFound := objects.Get(row.BranchKey())
5252
return configFound && config.HasLocalState() && branchFound && branch.HasLocalState()
5353
case *model.NotificationState:
54+
if v.Remote == nil {
55+
return false
56+
}
5457
notification := v.Remote
5558
config, configFound := objects.Get(notification.ConfigKey())
5659
branch, branchFound := objects.Get(notification.ConfigKey().BranchKey())

internal/pkg/service/cli/CLI_CONTEXT.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,13 @@ This document provides architectural context for the Keboola CLI service (`kbc`)
44

55
## Notification Subscriptions
66

7-
The CLI supports managing notification subscriptions for configurations. Notifications are stored in `{config}/notifications/{subscription-id}/meta.json` files and tracked in the manifest.
7+
The CLI supports managing notification subscriptions for configurations. Notifications are stored in `{config}/notifications/{subscription-id}/config.json` files and tracked in the manifest.
88

99
**Key Features:**
1010
- Config-level notifications (branch-level not yet supported)
1111
- Events: job-failed, job-succeeded, job-warning, job-processing-long
1212
- Channels: email or webhook
13-
- Filters: Fine-grained control with operators (eq, ne, gt, lt, ge, le)
13+
- Filters: Fine-grained control with operators (==, !=, >, <, >=, <=)
1414
- Full pull/push/sync support
1515

1616
**Operations:**

internal/pkg/state/remote/notification.go

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package remote
22

33
import (
44
"context"
5+
"fmt"
56
"time"
67

78
"github.com/keboola/keboola-sdk-go/v2/pkg/keboola"
@@ -22,29 +23,42 @@ func (u *UnitOfWork) loadNotifications(ctx context.Context) error {
2223
return err
2324
}
2425

25-
// Build map of config ID -> config manifest for quick lookup
26-
configsByID := make(map[keboola.ConfigID]*model.ConfigManifest)
26+
// Build map of "branchID/configID" -> config manifest for collision-safe lookup
27+
configsByKey := make(map[string]*model.ConfigManifest)
2728
for _, record := range u.Manifest().All() {
2829
if configManifest, ok := record.(*model.ConfigManifest); ok {
29-
configsByID[configManifest.ID] = configManifest
30+
key := fmt.Sprintf("%d/%s", configManifest.BranchID, configManifest.ID)
31+
configsByKey[key] = configManifest
3032
}
3133
}
3234

3335
// Load each notification from API
3436
for _, apiSub := range *subscriptions {
35-
// Find which config this notification belongs to by checking filters
36-
var parentConfig *model.ConfigManifest
37+
// Collect branch.id and job.configuration.id from equality filters
38+
var branchIDStr, configIDStr string
3739
for _, filter := range apiSub.Filters {
38-
if filter.Field == "job.configuration.id" && filter.Operator == keboola.NotificationFilterOperatorEquals {
39-
// Found the config ID equality filter - look up the config
40-
configID := keboola.ConfigID(filter.Value)
41-
if cfg, found := configsByID[configID]; found {
42-
parentConfig = cfg
43-
break
44-
}
40+
if filter.Operator != keboola.NotificationFilterOperatorEquals {
41+
continue
42+
}
43+
switch filter.Field {
44+
case "branch.id":
45+
branchIDStr = filter.Value
46+
case "job.configuration.id":
47+
configIDStr = filter.Value
4548
}
4649
}
4750

51+
// Skip if no config ID filter present
52+
if configIDStr == "" {
53+
continue
54+
}
55+
56+
// Look up config using composite key to avoid branch collisions
57+
var parentConfig *model.ConfigManifest
58+
if branchIDStr != "" {
59+
parentConfig = configsByKey[branchIDStr+"/"+configIDStr]
60+
}
61+
4862
// Skip if we don't have this config locally
4963
if parentConfig == nil {
5064
continue

0 commit comments

Comments
 (0)