database: fix notification design and add vulnerability history

api/v1/models.go

@@ -162,7 +162,7 @@ type Notification struct {
 	Page     string                   `json:"Page,omitempty"`
 	NextPage string                   `json:"NextPage,omitempty"`
 	Old      *VulnerabilityWithLayers `json:"Old,omitempty"`
-	New      VulnerabilityWithLayers  `json:"New,omitempty"`
+	New      *VulnerabilityWithLayers `json:"New,omitempty"`
 	Changed  []string                 `json:"Changed,omitempty"`
 }
 
@@ -172,6 +172,11 @@ func NotificationFromDatabaseModel(dbNotification database.VulnerabilityNotifica
 		*oldVuln = VulnerabilityWithLayersFromDatabaseModel(*dbNotification.OldVulnerability)
 	}
 
+	var newVuln *VulnerabilityWithLayers
+	if dbNotification.NewVulnerability != nil {
+		*newVuln = VulnerabilityWithLayersFromDatabaseModel(*dbNotification.NewVulnerability)
+	}
+
 	var nextPageStr string
 	if nextPage != database.NoVulnerabilityNotificationPage {
 		nextPageStr = DBPageNumberToString(nextPage)
@@ -187,7 +192,7 @@ func NotificationFromDatabaseModel(dbNotification database.VulnerabilityNotifica
 		Page:     DBPageNumberToString(page),
 		NextPage: nextPageStr,
 		Old:      oldVuln,
-		New:      VulnerabilityWithLayersFromDatabaseModel(dbNotification.NewVulnerability),
+		New:      newVuln,
 	}
 }
 

database/models.go

@@ -61,8 +61,9 @@ type FeatureVersion struct {
 type Vulnerability struct {
 	Model
 
-	Name        string
-	Namespace   Namespace
+	Name      string
+	Namespace Namespace
+
 	Description string
 	Link        string
 	Severity    types.Priority
@@ -102,7 +103,7 @@ type VulnerabilityNotification struct {
 	Deleted  time.Time
 
 	OldVulnerability *Vulnerability
-	NewVulnerability Vulnerability
+	NewVulnerability *Vulnerability
 }
 
 type VulnerabilityNotificationPageNumber struct {

database/pgsql/complex_test.go

@@ -155,6 +155,4 @@ func TestRaceAffects(t *testing.T) {
 		assert.Len(t, utils.CompareStringLists(expectedAffectedNames, actualAffectedNames), 0)
 		assert.Len(t, utils.CompareStringLists(actualAffectedNames, expectedAffectedNames), 0)
 	}
-
-	// TODO(Quentin-M): May be worth having a test for updates as well.
 }

database/pgsql/migrations/20151222113213_Initial.sql

@@ -89,8 +89,8 @@ CREATE TABLE IF NOT EXISTS Vulnerability (
   link VARCHAR(128) NULL,
   severity severity NOT NULL,
   metadata TEXT NULL,
-
-  UNIQUE (namespace_id, name));
+  created_at TIMESTAMP WITH TIME ZONE,
+  deleted_at TIMESTAMP WITH TIME ZONE NULL);
 
 
 -- -----------------------------------------------------
@@ -152,8 +152,8 @@ CREATE TABLE IF NOT EXISTS Vulnerability_Notification (
   created_at TIMESTAMP WITH TIME ZONE,
   notified_at TIMESTAMP WITH TIME ZONE NULL,
   deleted_at TIMESTAMP WITH TIME ZONE NULL,
-  old_vulnerability TEXT NULL,
-  new_vulnerability TEXT);
+  old_vulnerability_id INT NULL REFERENCES Vulnerability ON DELETE CASCADE,
+  new_vulnerability_id INT NULL REFERENCES Vulnerability ON DELETE CASCADE);
 
 CREATE INDEX ON Vulnerability_Notification (notified_at);
 

database/pgsql/notification.go

@@ -2,7 +2,6 @@ package pgsql
 
 import (
 	"database/sql"
-	"encoding/json"
 	"time"
 
 	"github.com/coreos/clair/database"
@@ -13,29 +12,13 @@ import (
 
 // do it in tx so we won't insert/update a vuln without notification and vice-versa.
 // name and created doesn't matter.
-// Vuln ID must be filled in.
-func (pgSQL *pgSQL) insertNotification(tx *sql.Tx, notification database.VulnerabilityNotification) error {
-	defer observeQueryTime("insertNotification", "all", time.Now())
-
-	// Marshal old and new Vulnerabilities.
-	var oldVulnerability sql.NullString
-	if notification.OldVulnerability != nil {
-		oldVulnerabilityJSON, err := json.Marshal(notification.OldVulnerability)
-		if err != nil {
-			tx.Rollback()
-			return cerrors.NewBadRequestError("could not marshal old Vulnerability in insertNotification")
-		}
-		oldVulnerability = sql.NullString{String: string(oldVulnerabilityJSON), Valid: true}
-	}
-
-	newVulnerability, err := json.Marshal(notification.NewVulnerability)
-	if err != nil {
-		tx.Rollback()
-		return cerrors.NewBadRequestError("could not marshal new Vulnerability in insertNotification")
-	}
+func createNotification(tx *sql.Tx, oldVulnerabilityID, newVulnerabilityID int) error {
+	defer observeQueryTime("createNotification", "all", time.Now())
 
 	// Insert Notification.
-	_, err = tx.Exec(getQuery("i_notification"), uuid.New(), oldVulnerability, newVulnerability)
+	oldVulnerabilityNullableID := sql.NullInt64{Int64: int64(oldVulnerabilityID), Valid: oldVulnerabilityID != 0}
+	newVulnerabilityNullableID := sql.NullInt64{Int64: int64(newVulnerabilityID), Valid: newVulnerabilityID != 0}
+	_, err := tx.Exec(getQuery("i_notification"), uuid.New(), oldVulnerabilityNullableID, newVulnerabilityNullableID)
 	if err != nil {
 		tx.Rollback()
 		return handleError("i_notification", err)
@@ -51,7 +34,7 @@ func (pgSQL *pgSQL) GetAvailableNotification(renotifyInterval time.Duration) (da
 
 	before := time.Now().Add(-renotifyInterval)
 	row := pgSQL.QueryRow(getQuery("s_notification_available"), before)
-	notification, err := scanNotification(row, false)
+	notification, err := pgSQL.scanNotification(row, false)
 
 	return notification, handleError("s_notification_available", err)
 }
@@ -60,63 +43,91 @@ func (pgSQL *pgSQL) GetNotification(name string, limit int, page database.Vulner
 	defer observeQueryTime("GetNotification", "all", time.Now())
 
 	// Get Notification.
-	notification, err := scanNotification(pgSQL.QueryRow(getQuery("s_notification"), name), true)
+	notification, err := pgSQL.scanNotification(pgSQL.QueryRow(getQuery("s_notification"), name), true)
 	if err != nil {
 		return notification, page, handleError("s_notification", err)
 	}
 
 	// Load vulnerabilities' LayersIntroducingVulnerability.
 	page.OldVulnerability, err = pgSQL.loadLayerIntroducingVulnerability(
-		notification.OldVulnerability, limit, page.OldVulnerability)
+		notification.OldVulnerability,
+		limit,
+		page.OldVulnerability,
+	)
+
 	if err != nil {
 		return notification, page, err
 	}
 
 	page.NewVulnerability, err = pgSQL.loadLayerIntroducingVulnerability(
-		&notification.NewVulnerability, limit, page.NewVulnerability)
+		notification.NewVulnerability,
+		limit,
+		page.NewVulnerability,
+	)
+
 	if err != nil {
 		return notification, page, err
 	}
 
 	return notification, page, nil
 }
 
-func scanNotification(row *sql.Row, hasVulns bool) (notification database.VulnerabilityNotification, err error) {
+func (pgSQL *pgSQL) scanNotification(row *sql.Row, hasVulns bool) (database.VulnerabilityNotification, error) {
+	var notification database.VulnerabilityNotification
 	var created zero.Time
 	var notified zero.Time
 	var deleted zero.Time
-	var oldVulnerability []byte
-	var newVulnerability []byte
+	var oldVulnerabilityNullableID sql.NullInt64
+	var newVulnerabilityNullableID sql.NullInt64
 
-	// Query notification.
+	// Scan notification.
 	if hasVulns {
-		err = row.Scan(&notification.ID, &notification.Name, &created, &notified, &deleted,
-			&oldVulnerability, &newVulnerability)
+		err := row.Scan(
+			&notification.ID,
+			&notification.Name,
+			&created,
+			&notified,
+			&deleted,
+			&oldVulnerabilityNullableID,
+			&newVulnerabilityNullableID,
+		)
+
+		if err != nil {
+			return notification, err
+		}
 	} else {
-		err = row.Scan(&notification.ID, &notification.Name, &created, &notified, &deleted)
-	}
-	if err != nil {
-		return
+		err := row.Scan(&notification.ID, &notification.Name, &created, &notified, &deleted)
+
+		if err != nil {
+			return notification, err
+		}
 	}
 
 	notification.Created = created.Time
 	notification.Notified = notified.Time
 	notification.Deleted = deleted.Time
 
 	if hasVulns {
-		// Unmarshal old and new Vulnerabilities.
-		err = json.Unmarshal(oldVulnerability, notification.OldVulnerability)
-		if err != nil {
-			err = cerrors.NewBadRequestError("could not unmarshal old Vulnerability in GetNotification")
+		if oldVulnerabilityNullableID.Valid {
+			vulnerability, err := pgSQL.findVulnerabilityByIDWithDeleted(int(oldVulnerabilityNullableID.Int64))
+			if err != nil {
+				return notification, err
+			}
+
+			notification.OldVulnerability = &vulnerability
 		}
 
-		err = json.Unmarshal(newVulnerability, &notification.NewVulnerability)
-		if err != nil {
-			err = cerrors.NewBadRequestError("could not unmarshal new Vulnerability in GetNotification")
+		if newVulnerabilityNullableID.Valid {
+			vulnerability, err := pgSQL.findVulnerabilityByIDWithDeleted(int(newVulnerabilityNullableID.Int64))
+			if err != nil {
+				return notification, err
+			}
+
+			notification.NewVulnerability = &vulnerability
 		}
 	}
 
-	return
+	return notification, nil
 }
 
 // Fills Vulnerability.LayersIntroducingVulnerability.

database/pgsql/notification_test.go

@@ -4,8 +4,6 @@ import (
 	"testing"
 	"time"
 
-	"fmt"
-
 	"github.com/coreos/clair/database"
 	cerrors "github.com/coreos/clair/utils/errors"
 	"github.com/coreos/clair/utils/types"
@@ -60,64 +58,68 @@ func TestNotification(t *testing.T) {
 		},
 	}
 
-	if assert.Nil(t, datastore.InsertLayer(l1)) && assert.Nil(t, datastore.InsertLayer(l2)) &&
-		assert.Nil(t, datastore.InsertLayer(l3)) {
-
-		// Insert a new vulnerability that is introduced by three layers.
-		v1 := database.Vulnerability{
-			Name:        "TestNotificationVulnerability1",
-			Namespace:   f1.Namespace,
-			Description: "TestNotificationDescription1",
-			Link:        "TestNotificationLink1",
-			Severity:    "Unknown",
-			FixedIn: []database.FeatureVersion{
-				database.FeatureVersion{
-					Feature: f1,
-					Version: types.NewVersionUnsafe("1.0"),
-				},
+	if !assert.Nil(t, datastore.InsertLayer(l1)) || !assert.Nil(t, datastore.InsertLayer(l2)) || !assert.Nil(t, datastore.InsertLayer(l3)) {
+		return
+	}
+
+	// Insert a new vulnerability that is introduced by three layers.
+	v1 := database.Vulnerability{
+		Name:        "TestNotificationVulnerability1",
+		Namespace:   f1.Namespace,
+		Description: "TestNotificationDescription1",
+		Link:        "TestNotificationLink1",
+		Severity:    "Unknown",
+		FixedIn: []database.FeatureVersion{
+			database.FeatureVersion{
+				Feature: f1,
+				Version: types.NewVersionUnsafe("1.0"),
 			},
-		}
-		assert.Nil(t, datastore.insertVulnerability(v1))
+		},
+	}
+	assert.Nil(t, datastore.insertVulnerability(v1, false))
 
-		// Get the notification associated to the previously inserted vulnerability.
-		notification, err := datastore.GetAvailableNotification(time.Second)
-		assert.Nil(t, err)
-		assert.NotEmpty(t, notification.Name)
+	// Get the notification associated to the previously inserted vulnerability.
+	notification, err := datastore.GetAvailableNotification(time.Second)
+	assert.Nil(t, err)
+	assert.NotEmpty(t, notification.Name)
 
-		// Verify the renotify behaviour.
-		if assert.Nil(t, datastore.SetNotificationNotified(notification.Name)) {
-			_, err := datastore.GetAvailableNotification(time.Second)
-			assert.Equal(t, cerrors.ErrNotFound, err)
+	// Verify the renotify behaviour.
+	if assert.Nil(t, datastore.SetNotificationNotified(notification.Name)) {
+		_, err := datastore.GetAvailableNotification(time.Second)
+		assert.Equal(t, cerrors.ErrNotFound, err)
 
-			time.Sleep(50 * time.Millisecond)
-			notificationB, err := datastore.GetAvailableNotification(20 * time.Millisecond)
-			assert.Nil(t, err)
-			assert.Equal(t, notification.Name, notificationB.Name)
+		time.Sleep(50 * time.Millisecond)
+		notificationB, err := datastore.GetAvailableNotification(20 * time.Millisecond)
+		assert.Nil(t, err)
+		assert.Equal(t, notification.Name, notificationB.Name)
 
-			datastore.SetNotificationNotified(notification.Name)
-		}
+		datastore.SetNotificationNotified(notification.Name)
+	}
 
-		// Get notification.
-		filledNotification, nextPage, err := datastore.GetNotification(notification.Name, 2, database.VulnerabilityNotificationFirstPage)
-		assert.Nil(t, err)
-		assert.NotEqual(t, database.NoVulnerabilityNotificationPage, nextPage)
-		assert.Nil(t, filledNotification.OldVulnerability)
-		assert.Equal(t, v1.Name, filledNotification.NewVulnerability.Name)
-		assert.Len(t, filledNotification.NewVulnerability.LayersIntroducingVulnerability, 2)
+	// Get notification.
+	filledNotification, nextPage, err := datastore.GetNotification(notification.Name, 2, database.VulnerabilityNotificationFirstPage)
+	assert.Nil(t, err)
+	assert.NotEqual(t, database.NoVulnerabilityNotificationPage, nextPage)
+	assert.Nil(t, filledNotification.OldVulnerability)
+	assert.Equal(t, v1.Name, filledNotification.NewVulnerability.Name)
+	assert.Len(t, filledNotification.NewVulnerability.LayersIntroducingVulnerability, 2)
+
+	// Get second page.
+	filledNotification, nextPage, err = datastore.GetNotification(notification.Name, 2, nextPage)
+	assert.Nil(t, err)
+	assert.Equal(t, database.NoVulnerabilityNotificationPage, nextPage)
+	assert.Nil(t, filledNotification.OldVulnerability)
+	assert.Equal(t, v1.Name, filledNotification.NewVulnerability.Name)
+	assert.Len(t, filledNotification.NewVulnerability.LayersIntroducingVulnerability, 1)
+
+	// Delete notification.
+	assert.Nil(t, datastore.DeleteNotification(notification.Name))
+
+	_, err = datastore.GetAvailableNotification(time.Millisecond)
+	assert.Equal(t, cerrors.ErrNotFound, err)
 
-		// Get second page.
-		filledNotification, nextPage, err = datastore.GetNotification(notification.Name, 2, nextPage)
-		assert.Nil(t, err)
-		assert.Equal(t, database.NoVulnerabilityNotificationPage, nextPage)
-		assert.Nil(t, filledNotification.OldVulnerability)
-		assert.Equal(t, v1.Name, filledNotification.NewVulnerability.Name)
-		assert.Len(t, filledNotification.NewVulnerability.LayersIntroducingVulnerability, 1)
+	// Update a vulnerability and ensure that the old/new vulnerabilities are correct.
 
-		// Delete notification.
-		assert.Nil(t, datastore.DeleteNotification(notification.Name))
+	// Delete a vulnerability and verify the notification.
 
-		n, err := datastore.GetAvailableNotification(time.Millisecond)
-		assert.Equal(t, cerrors.ErrNotFound, err)
-		fmt.Println(n)
-	}
 }