Merge pull request #263 from Quentin-M/rhel_unique_fixedin

pgsql: Do not insert entry in Vulnerability_FixedIn_Feature if existing

database/pgsql/feature.go

@@ -129,20 +129,21 @@ func (pgSQL *pgSQL) insertFeatureVersion(featureVersion database.FeatureVersion)
 	}
 
 	// Find or create FeatureVersion.
-	var newOrExisting string
+	var created bool
 
 	t = time.Now()
 	err = tx.QueryRow(soiFeatureVersion, featureID, &featureVersion.Version).
-		Scan(&newOrExisting, &featureVersion.ID)
+		Scan(&created, &featureVersion.ID)
 	observeQueryTime("insertFeatureVersion", "soiFeatureVersion", t)
 
 	if err != nil {
 		tx.Rollback()
 		return 0, handleError("soiFeatureVersion", err)
 	}
 
-	if newOrExisting == "exi" {
-		// That featureVersion already exists, return its id.
+	if !created {
+		// The featureVersion already existed, no need to link it to
+		// vulnerabilities.
 		tx.Commit()
 
 		if pgSQL.cache != nil {

database/pgsql/queries.go

@@ -63,9 +63,9 @@ const (
 			WHERE NOT EXISTS (SELECT id FROM FeatureVersion WHERE feature_id = $1 AND version = $2)
 			RETURNING id
 		)
-		SELECT 'exi', id FROM FeatureVersion WHERE feature_id = $1 AND version = $2
+		SELECT false, id FROM FeatureVersion WHERE feature_id = $1 AND version = $2
 		UNION
-		SELECT 'new', id FROM new_featureversion`
+		SELECT true, id FROM new_featureversion`
 
 	searchVulnerabilityFixedInFeature = `
 		SELECT id, vulnerability_id, version FROM Vulnerability_FixedIn_Feature
@@ -160,10 +160,16 @@ const (
 		VALUES($1, $2, $3, $4, $5, $6, CURRENT_TIMESTAMP)
 		RETURNING id`
 
-	insertVulnerabilityFixedInFeature = `
-		INSERT INTO Vulnerability_FixedIn_Feature(vulnerability_id, feature_id, version)
-		VALUES($1, $2, $3)
-		RETURNING id`
+	soiVulnerabilityFixedInFeature = `
+		WITH new_fixedinfeature AS (
+			INSERT INTO Vulnerability_FixedIn_Feature(vulnerability_id, feature_id, version)
+			SELECT CAST($1 AS INTEGER), CAST($2 AS INTEGER), CAST($3 AS VARCHAR)
+			WHERE NOT EXISTS (SELECT id FROM Vulnerability_FixedIn_Feature WHERE vulnerability_id = $1 AND feature_id = $2)
+			RETURNING id
+		)
+		SELECT false, id FROM Vulnerability_FixedIn_Feature WHERE vulnerability_id = $1 AND feature_id = $2
+		UNION
+		SELECT true, id FROM new_fixedinfeature`
 
 	searchFeatureVersionByFeature = `SELECT id, version FROM FeatureVersion WHERE feature_id = $1`
 

database/pgsql/vulnerability.go

@@ -433,18 +433,25 @@ func (pgSQL *pgSQL) insertVulnerabilityFixedInFeatureVersions(tx *sql.Tx, vulner
 
 	for _, fv := range fixedIn {
 		var fixedInID int
+		var created bool
 
-		// Insert Vulnerability_FixedIn_Feature.
+		// Find or create entry in Vulnerability_FixedIn_Feature.
 		err = tx.QueryRow(
-			insertVulnerabilityFixedInFeature,
+			soiVulnerabilityFixedInFeature,
 			vulnerabilityID, fv.Feature.ID,
 			&fv.Version,
-		).Scan(&fixedInID)
+		).Scan(&created, &fixedInID)
 
 		if err != nil {
 			return handleError("insertVulnerabilityFixedInFeature", err)
 		}
 
+		if !created {
+			// The relationship between the feature and the vulnerability already
+			// existed, no need to update Vulnerability_Affects_FeatureVersion.
+			continue
+		}
+
 		// Insert Vulnerability_Affects_FeatureVersion.
 		err = linkVulnerabilityToFeatureVersions(tx, fixedInID, vulnerabilityID, fv.Feature.ID, fv.Version)
 		if err != nil {

database/pgsql/vulnerability_test.go

@@ -226,25 +226,24 @@ func TestInsertVulnerability(t *testing.T) {
 	v1.Description = "TestInsertVulnerabilityLink2"
 	v1.Link = "TestInsertVulnerabilityLink2"
 	v1.Severity = types.High
-	// Update f3 in f4, add fixed in f5, add fixed in f6 which already exists, removes fixed in f7 by
-	// adding f8 which is f7 but with MinVersion.
-	v1.FixedIn = []database.FeatureVersion{f4, f5, f6, f8}
+	// Update f3 in f4, add fixed in f5, add fixed in f6 which already exists,
+	// removes fixed in f7 by adding f8 which is f7 but with MinVersion, and
+	// add fixed by f5 a second time (duplicated).
+	v1.FixedIn = []database.FeatureVersion{f4, f5, f6, f8, f5}
 
 	err = datastore.InsertVulnerabilities([]database.Vulnerability{v1}, true)
 	if assert.Nil(t, err) {
 		v1f, err := datastore.FindVulnerability(n1.Name, v1.Name)
 		if assert.Nil(t, err) {
+			// Remove f8 from the struct for comparison as it was just here to cancel f7.
+			// Remove one of the f5 too as it was twice in the struct but the database
+			// implementation should have dedup'd it.
+			v1.FixedIn = v1.FixedIn[:len(v1.FixedIn)-2]
+
 			// We already had f1 before the update.
 			// Add it to the struct for comparison.
 			v1.FixedIn = append(v1.FixedIn, f1)
 
-			// Removes f8 from the struct for comparison as it was just here to cancel f7.
-			for i := 0; i < len(v1.FixedIn); i++ {
-				if v1.FixedIn[i].Feature.Name == f8.Feature.Name {
-					v1.FixedIn = append(v1.FixedIn[:i], v1.FixedIn[i+1:]...)
-				}
-			}
-
 			equalsVuln(t, &v1, &v1f)
 		}
 	}