Merge pull request #33 from Quentin-M/insertvulns

database: Improve InsertVulnerabilities.

database/vulnerability.go

@@ -96,7 +96,6 @@ func (av *AbstractVulnerability) ToVulnerability(fixedInNodes []string) *Vulnera
 }
 
 // InsertVulnerabilities inserts or updates several vulnerabilities in the database in one transaction
-// It ensures that a vulnerability can't be fixed by two packages belonging the same Branch.
 // During an update, if the vulnerability was previously fixed by a version in a branch and a new package of that branch is specified, the previous one is deleted
 // Otherwise, it simply adds the defined packages, there is currently no way to delete affected packages.
 //
@@ -110,13 +109,15 @@ func InsertVulnerabilities(vulnerabilities []*Vulnerability) ([]Notification, er
 	var err error
 	t := cayley.NewTransaction()
 	cachedVulnerabilities := make(map[string]*Vulnerability)
+
+	var notifications []Notification
 	newVulnerabilityNotifications := make(map[string]*NewVulnerabilityNotification)
 	vulnerabilityPriorityIncreasedNotifications := make(map[string]*VulnerabilityPriorityIncreasedNotification)
 	vulnerabilityPackageChangedNotifications := make(map[string]*VulnerabilityPackageChangedNotification)
 
 	// Iterate over all the vulnerabilities we need to insert/update
 	for _, vulnerability := range vulnerabilities {
-		// Is the vulnerability already existing ?
+		// Check if the vulnerability already exists
 		existingVulnerability, _ := cachedVulnerabilities[vulnerability.ID]
 		if existingVulnerability == nil {
 			existingVulnerability, err = FindOneVulnerability(vulnerability.ID, FieldVulnerabilityAll)
@@ -128,23 +129,6 @@ func InsertVulnerabilities(vulnerabilities []*Vulnerability) ([]Notification, er
 			}
 		}
 
-		// Don't allow inserting/updating a vulnerability which is fixed in two packages of the same branch
-		if len(vulnerability.FixedInNodes) > 0 {
-			fixedInPackages, err := FindAllPackagesByNodes(vulnerability.FixedInNodes, []string{FieldPackageOS, FieldPackageName})
-			if err != nil {
-				return []Notification{}, err
-			}
-			fixedInBranches := make(map[string]struct{})
-			for _, fixedInPackage := range fixedInPackages {
-				branch := fixedInPackage.Branch()
-				if _, branchExists := fixedInBranches[branch]; branchExists {
-					log.Warningf("could not insert vulnerability %s because it is fixed in two packages of the same branch", vulnerability.ID)
-					return []Notification{}, cerrors.NewBadRequestError("could not insert a vulnerability which is fixed in two packages of the same branch")
-				}
-				fixedInBranches[branch] = struct{}{}
-			}
-		}
-
 		// Insert/Update vulnerability
 		if existingVulnerability == nil {
 			// The vulnerability does not exist, create it
@@ -165,7 +149,6 @@ func InsertVulnerabilities(vulnerabilities []*Vulnerability) ([]Notification, er
 
 			// Insert it
 			vulnerability.Node = vulnerability.GetNode()
-			cachedVulnerabilities[vulnerability.ID] = vulnerability
 
 			t.AddQuad(cayley.Quad(vulnerability.Node, FieldIs, FieldVulnerabilityIsValue, ""))
 			t.AddQuad(cayley.Quad(vulnerability.Node, FieldVulnerabilityID, vulnerability.ID, ""))
@@ -177,7 +160,11 @@ func InsertVulnerabilities(vulnerabilities []*Vulnerability) ([]Notification, er
 			}
 
 			// Add a notification
-			newVulnerabilityNotifications[vulnerability.ID] = &NewVulnerabilityNotification{VulnerabilityID: vulnerability.ID}
+			notification := &NewVulnerabilityNotification{VulnerabilityID: vulnerability.ID}
+			notifications = append(notifications, notification)
+			newVulnerabilityNotifications[vulnerability.ID] = notification
+
+			cachedVulnerabilities[vulnerability.ID] = vulnerability
 		} else {
 			// The vulnerability already exists, update it
 			if vulnerability.Link != "" && existingVulnerability.Link != vulnerability.Link {
@@ -185,6 +172,7 @@ func InsertVulnerabilities(vulnerabilities []*Vulnerability) ([]Notification, er
 				t.AddQuad(cayley.Quad(existingVulnerability.Node, FieldVulnerabilityLink, vulnerability.Link, ""))
 				existingVulnerability.Link = vulnerability.Link
 			}
+
 			if vulnerability.Priority != "" && vulnerability.Priority != types.Unknown && existingVulnerability.Priority != vulnerability.Priority {
 				if !vulnerability.Priority.IsValid() {
 					log.Warningf("could not update a vulnerability which has an invalid priority [ID: %s, Link: %s, Priority: %s]. Valid priorities are: %v.", vulnerability.ID, vulnerability.Link, vulnerability.Priority, types.Priorities)
@@ -197,10 +185,12 @@ func InsertVulnerabilities(vulnerabilities []*Vulnerability) ([]Notification, er
 						// Any priorityChangeNotification already ?
 						if existingPriorityNotification, _ := vulnerabilityPriorityIncreasedNotifications[vulnerability.ID]; existingPriorityNotification != nil {
 							// There is a priority change notification, replace it but keep the old priority field
-							vulnerabilityPriorityIncreasedNotifications[vulnerability.ID] = &VulnerabilityPriorityIncreasedNotification{OldPriority: existingPriorityNotification.OldPriority, NewPriority: vulnerability.Priority, VulnerabilityID: existingVulnerability.ID}
+							existingPriorityNotification.NewPriority = vulnerability.Priority
 						} else {
 							// No previous notification, just add a new one
-							vulnerabilityPriorityIncreasedNotifications[vulnerability.ID] = &VulnerabilityPriorityIncreasedNotification{OldPriority: existingVulnerability.Priority, NewPriority: vulnerability.Priority, VulnerabilityID: existingVulnerability.ID}
+							notification := &VulnerabilityPriorityIncreasedNotification{OldPriority: existingVulnerability.Priority, NewPriority: vulnerability.Priority, VulnerabilityID: existingVulnerability.ID}
+							notifications = append(notifications, notification)
+							vulnerabilityPriorityIncreasedNotifications[vulnerability.ID] = notification
 						}
 					}
 				}
@@ -209,12 +199,15 @@ func InsertVulnerabilities(vulnerabilities []*Vulnerability) ([]Notification, er
 				t.AddQuad(cayley.Quad(existingVulnerability.Node, FieldVulnerabilityPriority, string(vulnerability.Priority), ""))
 				existingVulnerability.Priority = vulnerability.Priority
 			}
+
 			if vulnerability.Description != "" && existingVulnerability.Description != vulnerability.Description {
 				t.RemoveQuad(cayley.Quad(existingVulnerability.Node, FieldVulnerabilityDescription, existingVulnerability.Description, ""))
 				t.AddQuad(cayley.Quad(existingVulnerability.Node, FieldVulnerabilityDescription, vulnerability.Description, ""))
 				existingVulnerability.Description = vulnerability.Description
 			}
-			if len(vulnerability.FixedInNodes) > 0 && len(utils.CompareStringLists(vulnerability.FixedInNodes, existingVulnerability.FixedInNodes)) != 0 {
+
+			newFixedInNodes := utils.CompareStringLists(vulnerability.FixedInNodes, existingVulnerability.FixedInNodes)
+			if len(newFixedInNodes) > 0 {
 				var removedNodes []string
 				var addedNodes []string
 
@@ -222,19 +215,14 @@ func InsertVulnerabilities(vulnerabilities []*Vulnerability) ([]Notification, er
 				if err != nil {
 					return []Notification{}, err
 				}
-				vulnerabilityFixedInPackages, err := FindAllPackagesByNodes(vulnerability.FixedInNodes, []string{FieldPackageOS, FieldPackageName, FieldPackageVersion})
+				newFixedInPackages, err := FindAllPackagesByNodes(newFixedInNodes, []string{FieldPackageOS, FieldPackageName, FieldPackageVersion})
 				if err != nil {
 					return []Notification{}, err
 				}
 
-				for _, p := range vulnerabilityFixedInPackages {
+				for _, p := range newFixedInPackages {
-					// Any already existing link ?
-					fixedInLinkAlreadyExists := false
 					for _, ep := range existingVulnerabilityFixedInPackages {
-						if *p == *ep {
+						if p.Branch() == ep.Branch() {
-							// This exact link already exists, we won't insert it again
-							fixedInLinkAlreadyExists = true
-						} else if p.Branch() == ep.Branch() {
 							// A link to this package branch already exist and is not the same version, we will delete it
 							t.RemoveQuad(cayley.Quad(existingVulnerability.Node, FieldVulnerabilityFixedIn, ep.Node, ""))
 
@@ -250,15 +238,12 @@ func InsertVulnerabilities(vulnerabilities []*Vulnerability) ([]Notification, er
 						}
 					}
 
-					if fixedInLinkAlreadyExists == false {
 					t.AddQuad(cayley.Quad(existingVulnerability.Node, FieldVulnerabilityFixedIn, p.Node, ""))
 					existingVulnerability.FixedInNodes = append(existingVulnerability.FixedInNodes, p.Node)
 					addedNodes = append(addedNodes, p.Node)
 				}
-				}
 
 				// Add notification about the FixedIn modification if the vulnerability is not new
-				if len(removedNodes) > 0 || len(addedNodes) > 0 {
 				if _, newVulnerabilityNotificationExists := newVulnerabilityNotifications[vulnerability.ID]; !newVulnerabilityNotificationExists {
 					// Any VulnerabilityPackageChangedNotification already ?
 					if existingPackageNotification, _ := vulnerabilityPackageChangedNotifications[vulnerability.ID]; existingPackageNotification != nil {
@@ -267,8 +252,9 @@ func InsertVulnerabilities(vulnerabilities []*Vulnerability) ([]Notification, er
 						existingPackageNotification.RemovedFixedInNodes = append(existingPackageNotification.RemovedFixedInNodes, removedNodes...)
 					} else {
 						// No previous notification, just add a new one
-							vulnerabilityPackageChangedNotifications[vulnerability.ID] = &VulnerabilityPackageChangedNotification{VulnerabilityID: vulnerability.ID, AddedFixedInNodes: addedNodes, RemovedFixedInNodes: removedNodes}
+						notification := &VulnerabilityPackageChangedNotification{VulnerabilityID: vulnerability.ID, AddedFixedInNodes: addedNodes, RemovedFixedInNodes: removedNodes}
-						}
+						notifications = append(notifications, notification)
+						vulnerabilityPackageChangedNotifications[vulnerability.ID] = notification
 					}
 				}
 			}
@@ -281,19 +267,7 @@ func InsertVulnerabilities(vulnerabilities []*Vulnerability) ([]Notification, er
 		return []Notification{}, ErrTransaction
 	}
 
-	// Group all notifications
+	return notifications, nil
-	var allNotifications []Notification
-	for _, notification := range newVulnerabilityNotifications {
-		allNotifications = append(allNotifications, notification)
-	}
-	for _, notification := range vulnerabilityPriorityIncreasedNotifications {
-		allNotifications = append(allNotifications, notification)
-	}
-	for _, notification := range vulnerabilityPackageChangedNotifications {
-		allNotifications = append(allNotifications, notification)
-	}
-
-	return allNotifications, nil
 }
 
 // DeleteVulnerability deletes the vulnerability having the given ID

database/vulnerability_test.go

@@ -155,9 +155,7 @@ func TestVulnerability(t *testing.T) {
 	pkg1 = &Package{OS: "testOS", Name: "testpkg1", Version: types.NewVersionUnsafe("1.0")}
 	pkg1b := &Package{OS: "testOS", Name: "testpkg1", Version: types.NewVersionUnsafe("1.1")}
 	InsertPackages([]*Package{pkg1, pkg1b})
-	// # A vulnerability can't be inserted if fixed by two packages of the same branch
+
-	_, err = InsertVulnerabilities([]*Vulnerability{&Vulnerability{ID: "test6", Link: "link6", Priority: types.Medium, Description: "testDescription6", FixedInNodes: []string{pkg1.Node, pkg1b.Node}}})
-	assert.Error(t, err)
 	// # Two updates of the same vulnerability in the same batch with packages of the same branch
 	pkg0 := &Package{OS: "testOS", Name: "testpkg0", Version: types.NewVersionUnsafe("1.0")}
 	InsertPackages([]*Package{pkg0})
@@ -173,10 +171,6 @@ func TestVulnerability(t *testing.T) {
 				assert.NotContains(t, v7.FixedInNodes, pkg1.Node)
 				assert.Contains(t, v7.FixedInNodes, pkg1b.Node)
 			}
-
-			// # A vulnerability can't be updated if fixed by two packages of the same branch
-			_, err = InsertVulnerabilities([]*Vulnerability{&Vulnerability{ID: "test7", FixedInNodes: []string{pkg1.Node, pkg1b.Node}}})
-			assert.Error(t, err)
 		}
 	}
 }