From 7f5df077823409408d66a455e53d3e22979f4f23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arturo=20Filast=C3=B2?= Date: Wed, 12 Sep 2018 18:47:57 +0200 Subject: [PATCH] Fix a very annoying logic bug in URL insertion Write test case for it --- internal/database/actions.go | 64 +++++++++++++-------------- internal/database/actions_test.go | 31 ++++++++++++- nettests/nettests.go | 2 +- nettests/websites/web_connectivity.go | 1 + 4 files changed, 63 insertions(+), 35 deletions(-) diff --git a/internal/database/actions.go b/internal/database/actions.go index 84699ed..bb0ea74 100644 --- a/internal/database/actions.go +++ b/internal/database/actions.go @@ -202,48 +202,48 @@ func CreateNetwork(sess sqlbuilder.Database, location *utils.LocationInfo) (*Net // CreateOrUpdateURL will create a new URL entry to the urls table if it doesn't // exists, otherwise it will update the category code of the one already in // there. -func CreateOrUpdateURL(sess sqlbuilder.Database, url string, categoryCode string, countryCode string) (int64, error) { - var urlID int64 - - res, err := sess.Update("urls").Set( - "url", url, - "category_code", categoryCode, - "country_code", countryCode, - ).Where("url = ? AND country_code = ?", url, countryCode).Exec() +func CreateOrUpdateURL(sess sqlbuilder.Database, urlStr string, categoryCode string, countryCode string) (int64, error) { + var url URL + tx, err := sess.NewTx(nil) if err != nil { - log.Error("Failed to write to the URL table") + log.WithError(err).Error("failed to create transaction") return 0, err } - affected, err := res.RowsAffected() + res := tx.Collection("urls").Find( + db.Cond{"url": urlStr, "country_code": countryCode}, + ) + err = res.One(&url) - if err != nil { - log.Error("Failed to get affected row count") - return 0, err - } - if affected == 0 { - newID, err := sess.Collection("urls").Insert( - URL{ - URL: sql.NullString{String: url, Valid: true}, - CategoryCode: sql.NullString{String: categoryCode, Valid: true}, - CountryCode: sql.NullString{String: countryCode, Valid: true}, - }) - if err != nil { + if err == db.ErrNoMoreRows { + url = URL{ + URL: sql.NullString{String: urlStr, Valid: true}, + CategoryCode: sql.NullString{String: categoryCode, Valid: true}, + CountryCode: sql.NullString{String: countryCode, Valid: true}, + } + newID, insErr := tx.Collection("urls").Insert(url) + if insErr != nil { log.Error("Failed to insert into the URLs table") - return 0, err + return 0, insErr } - urlID = newID.(int64) + url.ID = sql.NullInt64{Int64: newID.(int64), Valid: true} + } else if err != nil { + log.WithError(err).Error("Failed to get single result") + return 0, err } else { - lastID, err := res.LastInsertId() - if err != nil { - log.Error("failed to get URL ID") - return 0, err - } - urlID = lastID + url.CategoryCode = sql.NullString{String: categoryCode, Valid: true} + res.Update(url) } - log.Debugf("returning url %d", urlID) - return urlID, nil + err = tx.Commit() + if err != nil { + log.WithError(err).Error("Failed to write to the URL table") + return 0, err + } + + log.Debugf("returning url %d", url.ID.Int64) + + return url.ID.Int64, nil } // AddTestKeys writes the summary to the measurement diff --git a/internal/database/actions_test.go b/internal/database/actions_test.go index 504f175..608aa3d 100644 --- a/internal/database/actions_test.go +++ b/internal/database/actions_test.go @@ -132,12 +132,27 @@ func TestURLCreation(t *testing.T) { t.Fatal(err) } - newID1, err := CreateOrUpdateURL(sess, "https://google.com", "SRCH", "XX") + newID1, err := CreateOrUpdateURL(sess, "https://google.com", "GMB", "XX") if err != nil { t.Fatal(err) } - newID2, err := CreateOrUpdateURL(sess, "https://google.com", "GMB", "XX") + newID2, err := CreateOrUpdateURL(sess, "https://google.com", "SRCH", "XX") + if err != nil { + t.Fatal(err) + } + + newID3, err := CreateOrUpdateURL(sess, "https://facebook.com", "GRP", "XX") + if err != nil { + t.Fatal(err) + } + + newID4, err := CreateOrUpdateURL(sess, "https://facebook.com", "GMP", "XX") + if err != nil { + t.Fatal(err) + } + + newID5, err := CreateOrUpdateURL(sess, "https://google.com", "SRCH", "XX") if err != nil { t.Fatal(err) } @@ -145,6 +160,18 @@ func TestURLCreation(t *testing.T) { if newID2 != newID1 { t.Error("inserting the same URL with different category code should produce the same result") } + + if newID3 == newID1 { + t.Error("inserting different URL should produce different ids") + } + + if newID4 != newID3 { + t.Error("inserting the same URL with different category code should produce the same result") + } + + if newID5 != newID1 { + t.Error("the ID of google should still be the same") + } } func TestPerformanceTestKeys(t *testing.T) { diff --git a/nettests/nettests.go b/nettests/nettests.go index 8da672c..c01b290 100644 --- a/nettests/nettests.go +++ b/nettests/nettests.go @@ -176,12 +176,12 @@ func (c *Controller) Init(nt *mk.Nettest) error { nt.On("status.measurement_start", func(e mk.Event) { log.Debugf(color.RedString(e.Key)) - idx := e.Value.Idx urlID := sql.NullInt64{Int64: 0, Valid: false} if c.inputIdxMap != nil { urlID = sql.NullInt64{Int64: c.inputIdxMap[idx], Valid: true} } + log.Debugf("👁 %d %s %d", idx, e.Value.Input, urlID.Int64) msmt, err := database.CreateMeasurement(c.Ctx.DB, reportID, testName, resultID, reportFilePath, urlID) if err != nil { log.WithError(err).Error("Failed to create measurement") diff --git a/nettests/websites/web_connectivity.go b/nettests/websites/web_connectivity.go index 6427bd4..4121207 100644 --- a/nettests/websites/web_connectivity.go +++ b/nettests/websites/web_connectivity.go @@ -59,6 +59,7 @@ func lookupURLs(ctl *nettests.Controller) ([]string, map[int64]int64, error) { if err != nil { log.Error("failed to add to the URL table") } + log.Debugf("Mapped URL %s to idx %d and urlID %d", url.URL, idx, urlID) urlIDMap[int64(idx)] = urlID urls = append(urls, url.URL) }