From 989314efef23d0c10212e2a760fd7397c7241870 Mon Sep 17 00:00:00 2001 From: Joshua Medley Date: Thu, 15 Dec 2022 11:21:26 +0000 Subject: [PATCH] add XLSX to download list and remove debug --- api/publish_state_checker.go | 8 ++++++++ api/versions.go | 35 ++++++++--------------------------- models/dataset.go | 18 +++++++++++++++++- models/dataset_test.go | 14 +++++++++++++- mongo/dataset_store.go | 11 ----------- 5 files changed, 46 insertions(+), 40 deletions(-) diff --git a/api/publish_state_checker.go b/api/publish_state_checker.go index 93169d19..20461d2e 100644 --- a/api/publish_state_checker.go +++ b/api/publish_state_checker.go @@ -96,6 +96,14 @@ func (d *PublishCheck) Check(handle func(http.ResponseWriter, *http.Request), ac } } + if versionDoc.Downloads.XLSX != nil && versionDoc.Downloads.XLSX.Public != "" { + newVersion.Downloads.XLSX = &models.DownloadObject{ + Public: versionDoc.Downloads.XLSX.Public, + Size: versionDoc.Downloads.XLSX.Size, + HRef: versionDoc.Downloads.XLSX.HRef, + } + } + if versionDoc.Downloads.TXT != nil && versionDoc.Downloads.TXT.Public != "" { newVersion.Downloads.TXT = &models.DownloadObject{ Public: versionDoc.Downloads.TXT.Public, diff --git a/api/versions.go b/api/versions.go index 86685381..41059c45 100644 --- a/api/versions.go +++ b/api/versions.go @@ -7,7 +7,6 @@ import ( "io" "net/http" "strings" - "time" "github.com/ONSdigital/dp-api-clients-go/v2/headers" errs "github.com/ONSdigital/dp-dataset-api/apierrors" @@ -58,7 +57,7 @@ func (v VersionDetails) baseLogData() log.Data { return log.Data{"dataset_id": v.datasetID, "edition": v.edition, "version": v.version} } -//getVersions returns a list of versions, the total count of versions that match the query parameters and an error +// getVersions returns a list of versions, the total count of versions that match the query parameters and an error func (api *DatasetAPI) getVersions(w http.ResponseWriter, r *http.Request, limit, offset int) (interface{}, int, error) { ctx := r.Context() vars := mux.Vars(r) @@ -235,13 +234,11 @@ func (api *DatasetAPI) putVersion(w http.ResponseWriter, r *http.Request) { "version": vars["version"], } - t0 := time.Now() currentDataset, currentVersion, versionUpdate, err := api.updateVersion(ctx, r.Body, versionDetails) if err != nil { handleVersionAPIErr(ctx, err, w, data) return } - log.Info(ctx, "DEBUG full updateVersion", log.Data{"time": time.Since(t0)}) // If update was to add downloads do not try to publish/associate version if vars[hasDownloads] != trueStringified { @@ -383,38 +380,29 @@ func (api *DatasetAPI) updateVersion(ctx context.Context, body io.ReadCloser, ve return nil, nil, nil, err } - t0 := time.Now() // reads http header and creates struct for new versionNumber versionUpdate, err := models.CreateVersion(body, versionDetails.datasetID) if err != nil { log.Error(ctx, "putVersion endpoint: failed to model version resource based on request", err, data) return nil, nil, nil, errs.ErrUnableToParseJSON } - log.Info(ctx, "DEBUG createVersion from body", log.Data{"time": time.Since(t0), "reqID": reqID}) - t0 = time.Now() currentDataset, err := api.dataStore.Backend.GetDataset(ctx, versionDetails.datasetID) if err != nil { log.Error(ctx, "putVersion endpoint: datastore.getDataset returned an error", err, data) return nil, nil, nil, err } - log.Info(ctx, "DEBUG GetDataset from mongo", log.Data{"time": time.Since(t0), "reqID": reqID}) - t0 = time.Now() if err = api.dataStore.Backend.CheckEditionExists(ctx, versionDetails.datasetID, versionDetails.edition, ""); err != nil { log.Error(ctx, "putVersion endpoint: failed to find edition of dataset", err, data) return nil, nil, nil, err } - log.Info(ctx, "DEBUG CheckEditionExists from mongo", log.Data{"time": time.Since(t0), "reqID": reqID}) - t0 = time.Now() currentVersion, err := api.dataStore.Backend.GetVersion(ctx, versionDetails.datasetID, versionDetails.edition, versionNumber, "") - if err != nil { log.Error(ctx, "putVersion endpoint: datastore.GetVersion returned an error", err, data) return nil, nil, nil, err } - log.Info(ctx, "DEBUG GetVersion from mongo", log.Data{"time": time.Since(t0), "reqID": reqID}) var combinedVersionUpdate *models.Version @@ -422,23 +410,17 @@ func (api *DatasetAPI) updateVersion(ctx context.Context, body io.ReadCloser, ve // then it validates the new model, and performs the update in MongoDB, passing the existing model ETag (if it exists) to be used in the query selector // Note that the combined version update does not mutate versionUpdate because multiple retries might generate a different value depending on the currentVersion at that point. var doUpdate = func() error { - t0 = time.Now() combinedVersionUpdate, err = populateNewVersionDoc(currentVersion, versionUpdate) if err != nil { return err } - log.Info(ctx, "DEBUG populateNewVersionDoc (merge mongoDB doc and update)", log.Data{"time": time.Since(t0), "reqID": reqID}) + data["updated_version"] = combinedVersionUpdate - log.Info(ctx, "putVersion endpoint: combined current version document with update request", data) - t0 = time.Now() if err = models.ValidateVersion(combinedVersionUpdate); err != nil { log.Error(ctx, "putVersion endpoint: failed validation check for version update", err) return err } - log.Info(ctx, "DEBUG ValidateVersion", log.Data{"time": time.Since(t0), "reqID": reqID}) - - t0 = time.Now() eTag := headers.IfMatchAnyETag if currentVersion.ETag != "" { @@ -448,22 +430,17 @@ func (api *DatasetAPI) updateVersion(ctx context.Context, body io.ReadCloser, ve if _, err := api.dataStore.Backend.UpdateVersion(ctx, currentVersion, combinedVersionUpdate, eTag); err != nil { return err } - log.Info(ctx, "DEBUG UpdateVersion to MongoDB", log.Data{"time": time.Since(t0), "reqID": reqID, "etag": eTag}) return nil } // acquire instance lock to prevent race conditions on instance collection - t0 = time.Now() lockID, err := api.dataStore.Backend.AcquireInstanceLock(ctx, currentVersion.ID) if err != nil { return nil, nil, nil, err } - log.Info(ctx, "DEBUG AcquireInstanceLock", log.Data{"time": time.Since(t0), "reqID": reqID}) defer func() { - t0 = time.Now() api.dataStore.Backend.UnlockInstance(ctx, lockID) - log.Info(ctx, "DEBUG UnlockInstance", log.Data{"time": time.Since(t0), "reqID": reqID}) }() // Try to perform the update. If there was a race condition and another caller performed the update @@ -475,13 +452,11 @@ func (api *DatasetAPI) updateVersion(ctx context.Context, body io.ReadCloser, ve if err := doUpdate(); err != nil { if err == errs.ErrDatasetNotFound { log.Info(ctx, "instance document in database corresponding to dataset version was modified before the lock was acquired, retrying...", data) - t0 = time.Now() currentVersion, err = api.dataStore.Backend.GetVersion(ctx, versionDetails.datasetID, versionDetails.edition, versionNumber, "") if err != nil { log.Error(ctx, "putVersion endpoint: datastore.GetVersion returned an error", err, data) return nil, nil, nil, err } - log.Info(ctx, "DEBUG (retry) GetVersion from mongo", log.Data{"time": time.Since(t0), "reqID": reqID}) if err = doUpdate(); err != nil { log.Error(ctx, "putVersion endpoint: failed to update version document on 2nd attempt", err, data) @@ -706,6 +681,8 @@ func populateNewVersionDoc(currentVersion *models.Version, originalVersion *mode // TODO - Data Integrity - Updating downloads should be locked down to services // with permissions to do so, currently a user could update these fields + + log.Info(context.Background(), "DEBUG", log.Data{"downloads": version.Downloads, "currentDownloads": currentVersion.Downloads}) if version.Downloads == nil { version.Downloads = currentVersion.Downloads } else { @@ -713,6 +690,10 @@ func populateNewVersionDoc(currentVersion *models.Version, originalVersion *mode version.Downloads.XLS = currentVersion.Downloads.XLS } + if version.Downloads.XLSX == nil && currentVersion.Downloads != nil { + version.Downloads.XLSX = currentVersion.Downloads.XLSX + } + if version.Downloads.CSV == nil && currentVersion.Downloads != nil { version.Downloads.CSV = currentVersion.Downloads.CSV } diff --git a/models/dataset.go b/models/dataset.go index bb807f5e..b5829c8e 100644 --- a/models/dataset.go +++ b/models/dataset.go @@ -243,9 +243,10 @@ type Alert struct { } // DownloadList represents a list of objects of containing information on the downloadable files. -// Items are in a specific order and should not be changed (xlsx, csv, txt, csvw) +// Items are in a specific order and should not be changed (xls, xlsx, csv, txt, csvw) type DownloadList struct { XLS *DownloadObject `bson:"xls,omitempty" json:"xls,omitempty"` + XLSX *DownloadObject `bson:"xlsx,omitempty" json:"xlsx,omitempty"` CSV *DownloadObject `bson:"csv,omitempty" json:"csv,omitempty"` TXT *DownloadObject `bson:"txt,omitempty" json:"txt,omitempty"` CSVW *DownloadObject `bson:"csvw,omitempty" json:"csvw,omitempty"` @@ -363,6 +364,7 @@ func CreateVersion(reader io.Reader, datasetID string) (*Version, error) { return nil, errs.ErrUnableToReadMessage } + log.Info(context.Background(), "DEBUG", log.Data{"body_create_version": string(b)}) var version Version version.ID = uuid.NewV4().String() version.DatasetID = datasetID @@ -372,6 +374,8 @@ func CreateVersion(reader io.Reader, datasetID string) (*Version, error) { return nil, errs.ErrUnableToParseJSON } + log.Info(context.Background(), "DEBUG", log.Data{"unmarshaled": version}) + return &version, nil } @@ -638,6 +642,18 @@ func ValidateVersion(version *Version) error { } } + if version.Downloads.XLSX != nil { + if version.Downloads.XLSX.HRef == "" { + missingFields = append(missingFields, "Downloads.XLSX.HRef") + } + if version.Downloads.XLSX.Size == "" { + missingFields = append(missingFields, "Downloads.XLSX.Size") + } + if _, err := strconv.Atoi(version.Downloads.XLSX.Size); err != nil { + invalidFields = append(invalidFields, "Downloads.XLSX.Size not a number") + } + } + if version.Downloads.CSV != nil { if version.Downloads.CSV.HRef == "" { missingFields = append(missingFields, "Downloads.CSV.HRef") diff --git a/models/dataset_test.go b/models/dataset_test.go index 4a69f024..60e8b5ad 100644 --- a/models/dataset_test.go +++ b/models/dataset_test.go @@ -547,6 +547,9 @@ func TestValidateVersion(t *testing.T) { v.Downloads = &DownloadList{XLS: &DownloadObject{HRef: "", Size: "2"}} assertVersionDownloadError(fmt.Errorf("missing mandatory fields: %v", []string{"Downloads.XLS.HRef"}), v) + v.Downloads = &DownloadList{XLSX: &DownloadObject{HRef: "", Size: "2"}} + assertVersionDownloadError(fmt.Errorf("missing mandatory fields: %v", []string{"Downloads.XLSX.HRef"}), v) + v.Downloads = &DownloadList{CSV: &DownloadObject{HRef: "", Size: "2"}} assertVersionDownloadError(fmt.Errorf("missing mandatory fields: %v", []string{"Downloads.CSV.HRef"}), v) @@ -559,6 +562,9 @@ func TestValidateVersion(t *testing.T) { v.Downloads = &DownloadList{XLS: &DownloadObject{HRef: "/", Size: ""}} assertVersionDownloadError(fmt.Errorf("missing mandatory fields: %v", []string{"Downloads.XLS.Size"}), v) + v.Downloads = &DownloadList{XLSX: &DownloadObject{HRef: "/", Size: ""}} + assertVersionDownloadError(fmt.Errorf("missing mandatory fields: %v", []string{"Downloads.XLSX.Size"}), v) + v.Downloads = &DownloadList{CSV: &DownloadObject{HRef: "/", Size: ""}} assertVersionDownloadError(fmt.Errorf("missing mandatory fields: %v", []string{"Downloads.CSV.Size"}), v) @@ -571,6 +577,9 @@ func TestValidateVersion(t *testing.T) { v.Downloads = &DownloadList{XLS: &DownloadObject{HRef: "/", Size: "bob"}} assertVersionDownloadError(fmt.Errorf("invalid fields: %v", []string{"Downloads.XLS.Size not a number"}), v) + v.Downloads = &DownloadList{XLSX: &DownloadObject{HRef: "/", Size: "bob"}} + assertVersionDownloadError(fmt.Errorf("invalid fields: %v", []string{"Downloads.XLSX.Size not a number"}), v) + v.Downloads = &DownloadList{CSV: &DownloadObject{HRef: "/", Size: "bob"}} assertVersionDownloadError(fmt.Errorf("invalid fields: %v", []string{"Downloads.CSV.Size not a number"}), v) @@ -1141,6 +1150,9 @@ func TestVersionDownloadsOrder(t *testing.T) { Convey("Given a Downloads struct", t, func() { d := DownloadList{ XLS: &DownloadObject{ + HRef: "XLS", + }, + XLSX: &DownloadObject{ HRef: "XLSX", }, CSV: &DownloadObject{ @@ -1160,7 +1172,7 @@ func TestVersionDownloadsOrder(t *testing.T) { t.Log(string(b)) Convey("The downloads should be in the expected order", func() { - expected := `{"xls":{"href":"XLSX"},"csv":{"href":"CSV"},"txt":{"href":"TXT"},"csvw":{"href":"CSVW"}}` + expected := `{"xls":{"href":"XLS"},"xlsx":{"href":"XLSX"},"csv":{"href":"CSV"},"txt":{"href":"TXT"},"csvw":{"href":"CSVW"}}` So(string(b), ShouldResemble, expected) }) }) diff --git a/mongo/dataset_store.go b/mongo/dataset_store.go index 1eac07d5..38334c3e 100644 --- a/mongo/dataset_store.go +++ b/mongo/dataset_store.go @@ -89,15 +89,7 @@ func (m *Mongo) GetDataset(ctx context.Context, id string) (*models.DatasetUpdat // GetEditions retrieves all edition documents for a dataset func (m *Mongo) GetEditions(ctx context.Context, id, state string, offset, limit int, authorised bool) ([]*models.EditionUpdate, int, error) { - - log.Info(context.TODO(), "[DEBUG] getting editions", log.Data{}) - selector := buildEditionsQuery(id, state, authorised) - log.Info(context.TODO(), "[DEBUG] query details", log.Data{ - "editionsCollection": m.ActualCollectionName(config.EditionsCollection), - "selector": selector, - "database": m.Database, - }) // get total count and paginated values according to provided offset and limit results := []*models.EditionUpdate{} @@ -115,8 +107,6 @@ func (m *Mongo) GetEditions(ctx context.Context, id, state string, offset, limit } func buildEditionsQuery(id, state string, authorised bool) bson.M { - - log.Info(context.TODO(), "[DEBUG] building query", log.Data{"id": id, "state": state, "authorised": authorised}) // all queries must get the dataset by id selector := bson.M{ "next.links.dataset.id": id, @@ -137,7 +127,6 @@ func buildEditionsQuery(id, state string, authorised bool) bson.M { // GetEdition retrieves an edition document for a dataset func (m *Mongo) GetEdition(ctx context.Context, id, editionID, state string) (*models.EditionUpdate, error) { - selector := buildEditionQuery(id, editionID, state) var edition models.EditionUpdate