fix: address code review findings
- Add is_ignored=0 filter to GetUnnotifiedReleases query per spec section 4.4 (notification lifecycle must exclude ignored releases) - Add FK constraint on notifications_sent.rgid referencing external_releases(rgid) per spec schema definition - Wrap migration application + recording in transactions for atomicity - Add config.yaml to .gitignore to prevent accidental secret commits - Pin Dockerfile base image to alpine:3.21 and add non-root appuser - Add test TestGetUnnotifiedReleases_IgnoredExcluded Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
1
.gitignore
vendored
1
.gitignore
vendored
@@ -3,5 +3,6 @@ naviwatcher
|
|||||||
naviwatcher-linux
|
naviwatcher-linux
|
||||||
naviwatcher-mac
|
naviwatcher-mac
|
||||||
naviwatcher.exe
|
naviwatcher.exe
|
||||||
|
config.yaml
|
||||||
data/
|
data/
|
||||||
coverage.out
|
coverage.out
|
||||||
|
|||||||
@@ -13,17 +13,22 @@ COPY . .
|
|||||||
RUN CGO_ENABLED=1 GOOS=linux go build -o naviwatcher ./cmd/naviwatcher
|
RUN CGO_ENABLED=1 GOOS=linux go build -o naviwatcher ./cmd/naviwatcher
|
||||||
|
|
||||||
# Runtime stage
|
# Runtime stage
|
||||||
FROM alpine:latest
|
FROM alpine:3.21
|
||||||
|
|
||||||
RUN apk add --no-cache ca-certificates sqlite-libs
|
RUN apk add --no-cache ca-certificates sqlite-libs && \
|
||||||
|
adduser -D -g '' appuser
|
||||||
|
|
||||||
WORKDIR /app
|
WORKDIR /app
|
||||||
|
|
||||||
COPY --from=builder /app/naviwatcher .
|
COPY --from=builder /app/naviwatcher .
|
||||||
|
|
||||||
|
RUN chown appuser:appuser /app
|
||||||
|
|
||||||
EXPOSE 8080
|
EXPOSE 8080
|
||||||
|
|
||||||
VOLUME ["/app/data"]
|
VOLUME ["/app/data"]
|
||||||
|
|
||||||
|
USER appuser
|
||||||
|
|
||||||
ENTRYPOINT ["./naviwatcher"]
|
ENTRYPOINT ["./naviwatcher"]
|
||||||
CMD ["-config=/app/data/config.yaml"]
|
CMD ["-config=/app/data/config.yaml"]
|
||||||
|
|||||||
@@ -96,7 +96,7 @@ func (db *DB) migrate() error {
|
|||||||
{
|
{
|
||||||
name: "003_create_notifications_sent",
|
name: "003_create_notifications_sent",
|
||||||
sql: `CREATE TABLE IF NOT EXISTS notifications_sent (
|
sql: `CREATE TABLE IF NOT EXISTS notifications_sent (
|
||||||
rgid TEXT NOT NULL,
|
rgid TEXT NOT NULL REFERENCES external_releases(rgid),
|
||||||
sent_at DATETIME DEFAULT CURRENT_TIMESTAMP,
|
sent_at DATETIME DEFAULT CURRENT_TIMESTAMP,
|
||||||
PRIMARY KEY (rgid, sent_at)
|
PRIMARY KEY (rgid, sent_at)
|
||||||
);`,
|
);`,
|
||||||
@@ -112,13 +112,24 @@ func (db *DB) migrate() error {
|
|||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
if _, err := db.conn.Exec(m.sql); err != nil {
|
tx, err := db.conn.Begin()
|
||||||
|
if err != nil {
|
||||||
|
return fmt.Errorf("begin transaction for migration %s: %w", m.name, err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if _, err := tx.Exec(m.sql); err != nil {
|
||||||
|
tx.Rollback()
|
||||||
return fmt.Errorf("apply migration %s: %w", m.name, err)
|
return fmt.Errorf("apply migration %s: %w", m.name, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
if err := db.markMigrationApplied(m.name); err != nil {
|
if _, err := tx.Exec("INSERT INTO _migrations (name) VALUES (?)", m.name); err != nil {
|
||||||
|
tx.Rollback()
|
||||||
return fmt.Errorf("record migration %s: %w", m.name, err)
|
return fmt.Errorf("record migration %s: %w", m.name, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if err := tx.Commit(); err != nil {
|
||||||
|
return fmt.Errorf("commit migration %s: %w", m.name, err)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
return nil
|
return nil
|
||||||
@@ -134,11 +145,6 @@ func (db *DB) isMigrationApplied(name string) (bool, error) {
|
|||||||
return count > 0, nil
|
return count > 0, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// markMigrationApplied records a migration as applied.
|
|
||||||
func (db *DB) markMigrationApplied(name string) error {
|
|
||||||
_, err := db.conn.Exec("INSERT INTO _migrations (name) VALUES (?)", name)
|
|
||||||
return err
|
|
||||||
}
|
|
||||||
|
|
||||||
// ArtistSettings represents a row in the artist_settings table.
|
// ArtistSettings represents a row in the artist_settings table.
|
||||||
type ArtistSettings struct {
|
type ArtistSettings struct {
|
||||||
|
|||||||
@@ -34,7 +34,7 @@ func GetUnnotifiedReleases(db *DB) ([]ExternalRelease, error) {
|
|||||||
SELECT e.rgid, e.artist_id, e.title, e.type, e.release_date, e.is_ignored
|
SELECT e.rgid, e.artist_id, e.title, e.type, e.release_date, e.is_ignored
|
||||||
FROM external_releases e
|
FROM external_releases e
|
||||||
LEFT JOIN notifications_sent n ON e.rgid = n.rgid
|
LEFT JOIN notifications_sent n ON e.rgid = n.rgid
|
||||||
WHERE n.rgid IS NULL
|
WHERE n.rgid IS NULL AND e.is_ignored = 0
|
||||||
`)
|
`)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("query unnotified releases: %w", err)
|
return nil, fmt.Errorf("query unnotified releases: %w", err)
|
||||||
|
|||||||
@@ -364,3 +364,44 @@ func TestMarkNotificationSent_MultipleReleases(t *testing.T) {
|
|||||||
t.Errorf("expected 3 notification rows, got %d", count)
|
t.Errorf("expected 3 notification rows, got %d", count)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestGetUnnotifiedReleases_IgnoredExcluded verifies that releases marked as ignored
|
||||||
|
// are not returned by GetUnnotifiedReleases, per the notification lifecycle spec (section 4.4).
|
||||||
|
func TestGetUnnotifiedReleases_IgnoredExcluded(t *testing.T) {
|
||||||
|
db, err := New(":memory:")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("New() error: %v", err)
|
||||||
|
}
|
||||||
|
defer db.Close()
|
||||||
|
|
||||||
|
if err := insertTestArtist(db, "artist-1"); err != nil {
|
||||||
|
t.Fatalf("insertTestArtist: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
releases := []ExternalRelease{
|
||||||
|
{RGID: "rg1", ArtistID: "artist-1", Title: "Normal Album", Type: "album", ReleaseDate: "2024-01-01", IsIgnored: false},
|
||||||
|
{RGID: "rg2", ArtistID: "artist-1", Title: "Ignored Album", Type: "album", ReleaseDate: "2024-06-01", IsIgnored: true},
|
||||||
|
{RGID: "rg3", ArtistID: "artist-1", Title: "Another Normal", Type: "single", ReleaseDate: "2024-03-01", IsIgnored: false},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, r := range releases {
|
||||||
|
if err := SaveExternalRelease(db, &r); err != nil {
|
||||||
|
t.Fatalf("SaveExternalRelease(%s) error: %v", r.RGID, err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
results, err := GetUnnotifiedReleases(db)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("GetUnnotifiedReleases() error: %v", err)
|
||||||
|
}
|
||||||
|
if len(results) != 2 {
|
||||||
|
t.Fatalf("expected 2 unnotified releases (ignored excluded), got %d", len(results))
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify the ignored release is not in the results.
|
||||||
|
for _, r := range results {
|
||||||
|
if r.RGID == "rg2" {
|
||||||
|
t.Error("ignored release rg2 should not appear in unnotified releases")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user