Skip to content

Commit 42c22cd

Browse files
committed
fix(notifications): address review feedback and lint
Address all review comments from PR #2528, add missing test coverage, and fix lint/formatting issues: - SDK integration: use SDK CleanProject (CleanAllNotificationSubscriptionsRequest) for test cleanup instead of manual deletion - Add E2E test documenting push behavior with unsupported custom filter (API returns HTTP 400 and local files remain unchanged) - Address review comments: field naming, error handling, code structure - Fix push operation: add persist support, fix stale key removal, preserve folder paths - Fix DeleteObject: clean up manifest entry and clear remote state on delete - Fix manifest: move Notifications field after Rows in ConfigManifestWithRows - Fix E2E tests: correct test expectations for push/pull/filter scenarios - Fix encrypt E2E test manifest: correct naming fields - Lint: gofumpt, go mod tidy, golangci-lint, import ordering
1 parent e9bceea commit 42c22cd

77 files changed

Lines changed: 637 additions & 546 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

docs/CLI_OBJECT_LIFECYCLE.md

Lines changed: 0 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -350,39 +350,6 @@ u.changes.AddCreated(notificationState)
350350
// IDs in manifest.json are updated for new notifications via manifest.Save()
351351
```
352352

353-
### Auto-Filter Behavior
354-
355-
Users write only `event` + `recipient` (and optionally custom `filters`) in `config.json`. During push, three standard filters are **auto-populated** by `createNotificationRequest()`:
356-
357-
- `branch.id` — from the notification's `BranchID`
358-
- `job.component.id` — from the notification's `ComponentID`
359-
- `job.configuration.id` — from the notification's `ConfigID`
360-
361-
These filters are required by the API to scope the notification to a specific config. They are appended before any user-defined filters.
362-
363-
The API returns all filters (auto + user) in the response, so after a `pull`, `config.json` contains all three auto-filters plus any user-defined ones.
364-
365-
**Consequence:** On a repeated `push` without `pull`:
366-
- Local `config.json` has no auto-filters (they were never written locally)
367-
- Remote notification has all three auto-filters
368-
- Diff engine shows `changed: filters`
369-
- This is **expected behavior**, not a bug
370-
371-
### Delete-Then-Create for Updates
372-
373-
The notifications API has no update endpoint. Changes are implemented as:
374-
1. Delete the old notification subscription
375-
2. Create a new one with the changed values
376-
377-
This is implemented via `WithOnSuccess` callback chaining in `UnitOfWork.SaveObject`:
378-
```go
379-
// Pseudocode in remote/manager.go
380-
deleteReq.WithOnSuccess(func(...) {
381-
// On successful delete, create the new one
382-
return createReq.Send(ctx)
383-
})
384-
```
385-
386353
---
387354

388355
## Common Pitfalls

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ require (
4343
github.com/json-iterator/go v1.1.12
4444
github.com/keboola/go-cloud-encrypt v0.0.0-20250422071622-41a5d5547c43
4545
github.com/keboola/go-utils v1.4.0
46-
github.com/keboola/keboola-sdk-go/v2 v2.12.1-0.20260223100110-432345f77343
46+
github.com/keboola/keboola-sdk-go/v2 v2.12.1-0.20260224141819-0b0f6e9b37d3
4747
github.com/klauspost/compress v1.18.4
4848
github.com/klauspost/pgzip v1.2.6
4949
github.com/kylelemons/godebug v1.1.0

go.sum

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -589,8 +589,8 @@ github.com/keboola/go-oauth2-proxy/v7 v7.13.1-0.20251120082210-251fbcb18c16 h1:m
589589
github.com/keboola/go-oauth2-proxy/v7 v7.13.1-0.20251120082210-251fbcb18c16/go.mod h1:2KeAM0/QPbyUAoky+PXVgQDt/5m0qNcn30z9jh4ig8A=
590590
github.com/keboola/go-utils v1.4.0 h1:WTyj95yrr8O8HxtC8TSTyUcElZiRGDeEdVvDpFo6HUo=
591591
github.com/keboola/go-utils v1.4.0/go.mod h1:IopwJzFz2gh0Yj3fUbIe2eamRoDKzbXvjqFjQyw3ZdQ=
592-
github.com/keboola/keboola-sdk-go/v2 v2.12.1-0.20260223100110-432345f77343 h1:xcHtXh3n6F2f3nBqGf+/AwdEKV06zAXXYlKZwVcOgI8=
593-
github.com/keboola/keboola-sdk-go/v2 v2.12.1-0.20260223100110-432345f77343/go.mod h1:N/PkJnEHcyHMbVjHPjTdQwj5b9Iajl7PEaFUVtywHKU=
592+
github.com/keboola/keboola-sdk-go/v2 v2.12.1-0.20260224141819-0b0f6e9b37d3 h1:cM+iINgvPYtdeFtbaVRLMZBxpzu9gAp2VPuY2nFg740=
593+
github.com/keboola/keboola-sdk-go/v2 v2.12.1-0.20260224141819-0b0f6e9b37d3/go.mod h1:N/PkJnEHcyHMbVjHPjTdQwj5b9Iajl7PEaFUVtywHKU=
594594
github.com/keybase/go-keychain v0.0.1 h1:way+bWYa6lDppZoZcgMbYsvC7GxljxrskdNInRtuthU=
595595
github.com/keybase/go-keychain v0.0.1/go.mod h1:PdEILRW3i9D8JcdM+FmY6RwkHGnhHxXwkPPMeUgOK1k=
596596
github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8=

internal/pkg/encoding/json/json.go

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,18 +10,22 @@ import (
1010
type RawMessage = json.RawMessage
1111

1212
func Encode(v any, pretty bool) ([]byte, error) {
13-
var data []byte
14-
var err error
13+
var buf bytes.Buffer
14+
enc := json.NewEncoder(&buf)
15+
enc.SetEscapeHTML(false)
1516
if pretty {
16-
data, err = json.MarshalIndent(v, "", " ")
17-
data = append(data, '\n')
18-
} else {
19-
data, err = json.Marshal(v)
17+
enc.SetIndent("", " ")
2018
}
21-
if err != nil {
19+
if err := enc.Encode(v); err != nil {
2220
return nil, processJSONEncodeError(err)
2321
}
24-
return data, nil
22+
data := buf.Bytes()
23+
if pretty {
24+
// json.Encoder always appends a newline, which matches the previous behavior for pretty output.
25+
return data, nil
26+
}
27+
// For non-pretty output, strip the trailing newline appended by json.Encoder.
28+
return bytes.TrimRight(data, "\n"), nil
2529
}
2630

2731
func MustEncode(v any, pretty bool) []byte {

internal/pkg/model/manifest.go

Lines changed: 4 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
11
package model
22

33
import (
4-
"encoding/json"
54
"fmt"
65

76
"github.com/keboola/go-utils/pkg/orderedmap"
8-
"github.com/keboola/keboola-sdk-go/v2/pkg/keboola"
97

108
"github.com/keboola/keboola-as-code/internal/pkg/filesystem"
119
"github.com/keboola/keboola-as-code/internal/pkg/utils/errors"
@@ -79,9 +77,8 @@ type ConfigManifest struct {
7977
RecordState `json:"-"`
8078
ConfigKey
8179
Paths
82-
Relations Relations `json:"relations,omitempty" validate:"dive"` // relations with other objects, for example variables definition
83-
Metadata *orderedmap.OrderedMap `json:"metadata,omitempty"`
84-
Notifications []*NotificationManifest `json:"notifications,omitempty"`
80+
Relations Relations `json:"relations,omitempty" validate:"dive"` // relations with other objects, for example variables definition
81+
Metadata *orderedmap.OrderedMap `json:"metadata,omitempty"`
8582
}
8683

8784
type ConfigRowManifest struct {
@@ -91,42 +88,10 @@ type ConfigRowManifest struct {
9188
Relations Relations `json:"relations,omitempty" validate:"dive"` // relations with other objects, for example variables values definition
9289
}
9390

94-
type NotificationManifestRef struct {
95-
ID keboola.NotificationSubscriptionID `json:"id"`
96-
Path string `json:"path,omitempty"`
97-
}
98-
9991
type ConfigManifestWithRows struct {
10092
ConfigManifest
101-
Rows []*ConfigRowManifest `json:"rows"`
102-
}
103-
104-
// MarshalJSON implements custom JSON marshaling to simplify notifications to just IDs.
105-
func (c *ConfigManifestWithRows) MarshalJSON() ([]byte, error) {
106-
// Convert notifications to simplified refs
107-
notificationRefs := make([]*NotificationManifestRef, 0, len(c.Notifications))
108-
for _, n := range c.Notifications {
109-
if n != nil {
110-
notificationRefs = append(notificationRefs, &NotificationManifestRef{ID: n.ID, Path: n.GetRelativePath()})
111-
}
112-
}
113-
114-
// Create a copy with simplified notifications
115-
return json.Marshal(&struct {
116-
ConfigKey
117-
Paths
118-
Relations Relations `json:"relations,omitempty"`
119-
Metadata *orderedmap.OrderedMap `json:"metadata,omitempty"`
120-
Rows []*ConfigRowManifest `json:"rows"`
121-
Notifications []*NotificationManifestRef `json:"notifications,omitempty"`
122-
}{
123-
ConfigKey: c.ConfigKey,
124-
Paths: c.Paths,
125-
Relations: c.Relations,
126-
Metadata: c.Metadata,
127-
Rows: c.Rows,
128-
Notifications: notificationRefs,
129-
})
93+
Rows []*ConfigRowManifest `json:"rows"`
94+
Notifications []*NotificationManifest `json:"notifications,omitempty"`
13095
}
13196

13297
func (p *Paths) ClearRelatedPaths() {

internal/pkg/model/notification_validation.go

Lines changed: 38 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -9,31 +9,33 @@ import (
99
"github.com/keboola/keboola-as-code/internal/pkg/utils/errors"
1010
)
1111

12-
// Valid notification filter field names as per the API specification.
13-
var validFilterFields = []string{
14-
"branch.id",
15-
"job.id",
16-
"job.component.id",
17-
"job.configuration.id",
18-
"job.token.id",
19-
"project.id",
20-
"eventType",
12+
func validNotificationFilterFields() []string {
13+
return []string{
14+
"branch.id",
15+
"job.id",
16+
"job.component.id",
17+
"job.configuration.id",
18+
"job.token.id",
19+
"project.id",
20+
"eventType",
21+
}
2122
}
2223

23-
// Map of deprecated field names to their correct equivalents.
24-
var deprecatedFieldNames = map[string]string{
25-
"configId": "job.configuration.id",
26-
"componentId": "job.component.id",
27-
"branchId": "branch.id",
28-
"jobId": "job.id",
29-
"tokenId": "job.token.id",
30-
"projectId": "project.id",
31-
"configuration": "job.configuration.id",
32-
"component": "job.component.id",
33-
"branch": "branch.id",
34-
"job": "job.id",
35-
"token": "job.token.id",
36-
"project": "project.id",
24+
func deprecatedNotificationFilterFieldNames() map[string]string {
25+
return map[string]string{
26+
"configId": "job.configuration.id",
27+
"componentId": "job.component.id",
28+
"branchId": "branch.id",
29+
"jobId": "job.id",
30+
"tokenId": "job.token.id",
31+
"projectId": "project.id",
32+
"configuration": "job.configuration.id",
33+
"component": "job.component.id",
34+
"branch": "branch.id",
35+
"job": "job.id",
36+
"token": "job.token.id",
37+
"project": "project.id",
38+
}
3739
}
3840

3941
// ValidateNotificationFilters validates that all filter field names are valid.
@@ -48,13 +50,16 @@ func ValidateNotificationFilters(filters []keboola.NotificationFilter) error {
4850
}
4951

5052
func validateFilterField(fieldName string, index int) error {
53+
validFields := validNotificationFilterFields()
54+
deprecatedFields := deprecatedNotificationFilterFieldNames()
55+
5156
// Check if field is valid
52-
if slices.Contains(validFilterFields, fieldName) {
57+
if slices.Contains(validFields, fieldName) {
5358
return nil
5459
}
5560

5661
// Check if it's a deprecated field name
57-
if correctName, ok := deprecatedFieldNames[fieldName]; ok {
62+
if correctName, ok := deprecatedFields[fieldName]; ok {
5863
return errors.Errorf(
5964
`filter[%d] uses deprecated field name "%s". Use "%s" instead`,
6065
index,
@@ -78,16 +83,19 @@ func validateFilterField(fieldName string, index int) error {
7883
`filter[%d] has invalid field name "%s". Valid field names are: %s`,
7984
index,
8085
fieldName,
81-
strings.Join(validFilterFields, ", "),
86+
strings.Join(validFields, ", "),
8287
)
8388
}
8489

8590
func findSimilarFieldNames(input string) []string {
91+
validFields := validNotificationFilterFields()
92+
deprecatedFields := deprecatedNotificationFilterFieldNames()
93+
8694
var suggestions []string
8795
lowerInput := strings.ToLower(input)
8896

8997
// Check if input contains any part of valid field names
90-
for _, validField := range validFilterFields {
98+
for _, validField := range validFields {
9199
lowerValid := strings.ToLower(validField)
92100
// If input contains part of valid field, or valid field contains part of input
93101
if strings.Contains(lowerValid, lowerInput) || strings.Contains(lowerInput, lowerValid) {
@@ -96,11 +104,9 @@ func findSimilarFieldNames(input string) []string {
96104
}
97105

98106
// Also check deprecated names that contain the input
99-
for deprecatedName, correctName := range deprecatedFieldNames {
100-
if strings.Contains(strings.ToLower(deprecatedName), lowerInput) {
101-
if !slices.Contains(suggestions, correctName) {
102-
suggestions = append(suggestions, correctName)
103-
}
107+
for deprecatedName, correctName := range deprecatedFields {
108+
if strings.Contains(strings.ToLower(deprecatedName), lowerInput) && !slices.Contains(suggestions, correctName) {
109+
suggestions = append(suggestions, correctName)
104110
}
105111
}
106112

internal/pkg/model/notification_validation_test.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55

66
"github.com/keboola/keboola-sdk-go/v2/pkg/keboola"
77
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
89
)
910

1011
func TestValidateNotificationFilters_ValidFields(t *testing.T) {
@@ -49,7 +50,7 @@ func TestValidateNotificationFilters_DeprecatedFields(t *testing.T) {
4950
}
5051

5152
err := ValidateNotificationFilters(filters)
52-
assert.Error(t, err, "deprecated field should return error")
53+
require.Error(t, err, "deprecated field should return error")
5354
assert.Contains(t, err.Error(), "deprecated field name")
5455
assert.Contains(t, err.Error(), tt.field)
5556
assert.Contains(t, err.Error(), tt.correctField)
@@ -77,7 +78,7 @@ func TestValidateNotificationFilters_InvalidFields(t *testing.T) {
7778
}
7879

7980
err := ValidateNotificationFilters(filters)
80-
assert.Error(t, err, "invalid field should return error")
81+
require.Error(t, err, "invalid field should return error")
8182
assert.Contains(t, err.Error(), "invalid field name")
8283
assert.Contains(t, err.Error(), tt.field)
8384
})
@@ -101,7 +102,7 @@ func TestValidateNotificationFilters_MultipleFilters(t *testing.T) {
101102
}
102103

103104
err := ValidateNotificationFilters(filters)
104-
assert.Error(t, err, "should fail on first invalid filter")
105+
require.Error(t, err, "should fail on first invalid filter")
105106
assert.Contains(t, err.Error(), "filter[1]")
106107
assert.Contains(t, err.Error(), "configId")
107108
}

internal/pkg/model/notificationmanifest.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
package model
22

33
import (
4+
"encoding/json"
45
"fmt"
6+
7+
"github.com/keboola/keboola-sdk-go/v2/pkg/keboola"
58
)
69

710
// NotificationManifest represents a notification manifest record.
@@ -12,6 +15,18 @@ type NotificationManifest struct {
1215
Relations Relations `json:"relations,omitempty" validate:"dive"`
1316
}
1417

18+
// MarshalJSON serializes only the ID and path, omitting the embedded NotificationKey fields
19+
// (branchId, componentId, configId) which would otherwise appear in the manifest JSON.
20+
func (n *NotificationManifest) MarshalJSON() ([]byte, error) {
21+
return json.Marshal(&struct {
22+
ID keboola.NotificationSubscriptionID `json:"id"`
23+
Path string `json:"path,omitempty"`
24+
}{
25+
ID: n.ID,
26+
Path: n.GetRelativePath(),
27+
})
28+
}
29+
1530
// NewEmptyObject creates an empty Notification object with the key from this manifest.
1631
func (n NotificationManifest) NewEmptyObject() Object {
1732
return &Notification{NotificationKey: n.NotificationKey}

internal/pkg/naming/matcher.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package naming
22

33
import (
4+
"strings"
5+
46
"github.com/keboola/keboola-sdk-go/v2/pkg/keboola"
57

68
"github.com/keboola/keboola-as-code/internal/pkg/model"
@@ -56,6 +58,11 @@ func (m PathMatcher) MatchConfigPath(parentKey model.Key, path model.AbsPath) (c
5658
return "", nil
5759
}
5860

61+
// MatchNotificationPath returns true if the path is a notification directory.
62+
func (m PathMatcher) MatchNotificationPath(path model.AbsPath) bool {
63+
return strings.HasPrefix(path.GetRelativePath(), "notifications/")
64+
}
65+
5966
func (m PathMatcher) MatchConfigRowPath(component *keboola.Component, path model.AbsPath) bool {
6067
// Shared code
6168
if component.IsSharedCode() {

internal/pkg/plan/persist/executor.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55

66
"github.com/keboola/keboola-sdk-go/v2/pkg/keboola"
77

8+
"github.com/keboola/keboola-as-code/internal/pkg/idgenerator"
89
"github.com/keboola/keboola-as-code/internal/pkg/log"
910
"github.com/keboola/keboola-as-code/internal/pkg/model"
1011
"github.com/keboola/keboola-as-code/internal/pkg/state"
@@ -68,6 +69,11 @@ func (e *executor) persistNewObject(action *newObjectAction) {
6869
case model.ConfigRowKey:
6970
k.ID = keboola.RowID(newID)
7071
key = k
72+
case model.NotificationKey:
73+
// Assign a short random local ID to ensure unique manifest key.
74+
// The real ID is assigned by the API on push (see buildNotificationCreateRequest success callback).
75+
k.ID = keboola.NotificationSubscriptionID(idgenerator.Random(6))
76+
key = k
7177
default:
7278
panic(errors.Errorf(`unexpected type "%s" of the persisted object "%s"`, key.Kind(), key.Desc()))
7379
}

0 commit comments

Comments
 (0)