Merge branch 'develop' into release/1.53.0

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,

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,62 +380,47 @@ 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
 
 	// doUpdate is an aux function that combines the existing version document with the update received in the body request,
 	// 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,13 +681,19 @@ 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 {
 		if version.Downloads.XLS == nil && currentVersion.Downloads != nil {
 			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
 		}

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")

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)
 			})
 		})

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