From fa532a5b3e3df237ce3deb53d56e31066a58ed20 Mon Sep 17 00:00:00 2001 From: Michael Adams Date: Thu, 4 Jan 2018 18:21:21 +0000 Subject: [PATCH 01/41] Added label to dimension model, expanded dimension list creation function to include label --- api/dataset.go | 4 ++++ models/dimension.go | 1 + 2 files changed, 5 insertions(+) diff --git a/api/dataset.go b/api/dataset.go index 02326aa1..7d6d7245 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -672,10 +672,13 @@ func (api *DatasetAPI) getDimensions(w http.ResponseWriter, r *http.Request) { } func (api *DatasetAPI) createListOfDimensions(versionDoc *models.Version, dimensions []bson.M) ([]models.Dimension, error) { + // Get dimension description from the version document and add to hash map dimensionDescriptions := make(map[string]string) + dimensionLabels := make(map[string]string) for _, details := range versionDoc.Dimensions { dimensionDescriptions[details.Name] = details.Description + dimensionLabels[details.Name] = details.Label } var results []models.Dimension @@ -693,6 +696,7 @@ func (api *DatasetAPI) createListOfDimensions(versionDoc *models.Version, dimens // Add description to dimension from hash map dimension.Description = dimensionDescriptions[dimension.Name] + dimension.Label = dimensionLabels[dimension.Name] results = append(results, dimension) } diff --git a/models/dimension.go b/models/dimension.go index 5f92d7ed..92c236b9 100644 --- a/models/dimension.go +++ b/models/dimension.go @@ -19,6 +19,7 @@ type Dimension struct { Links DimensionLink `bson:"links,omitempty" json:"links,omitempty"` Name string `bson:"name,omitempty" json:"dimension,omitempty"` LastUpdated time.Time `bson:"last_updated,omitempty" json:"-"` + Label string `bson:"label,omitempty" json:"label,omitempty"` } // DimensionLink contains all links needed for a dimension From b391d83e3cdbcf673108997aae46d59fed8d81cc Mon Sep 17 00:00:00 2001 From: Michael Adams Date: Mon, 8 Jan 2018 20:48:37 +0000 Subject: [PATCH 02/41] Added PUT endpoint for label and/or description --- api/api.go | 3 +++ instance/instance.go | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/api/api.go b/api/api.go index 245ee6aa..ee4b6e24 100644 --- a/api/api.go +++ b/api/api.go @@ -71,6 +71,7 @@ func routes(host, secretKey string, router *mux.Router, dataStore store.DataStor api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}", api.privateAuth.Check(api.putVersion)).Methods("PUT") api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}/metadata", api.getMetadata).Methods("GET") api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}/dimensions", api.getDimensions).Methods("GET") + api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}/dimensions/{dimension}", api.privateAuth.Check(api.putDimension)).Methods("PUT") api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}/dimensions/{dimension}/options", api.getDimensionOptions).Methods("GET") instance := instance.Store{Host: api.host, Storer: api.dataStore.Backend} @@ -78,6 +79,7 @@ func routes(host, secretKey string, router *mux.Router, dataStore store.DataStor api.router.HandleFunc("/instances", api.privateAuth.Check(instance.Add)).Methods("POST") api.router.HandleFunc("/instances/{id}", api.privateAuth.Check(instance.Get)).Methods("GET") api.router.HandleFunc("/instances/{id}", api.privateAuth.Check(instance.Update)).Methods("PUT") + api.router.HandleFunc("/instances/{id}/dimensions/{dimension}", api.privateAuth.Check(instance.UpdateDimension)).Methods("PUT") api.router.HandleFunc("/instances/{id}/events", api.privateAuth.Check(instance.AddEvent)).Methods("POST") api.router.HandleFunc("/instances/{id}/inserted_observations/{inserted_observations}", api.privateAuth.Check(instance.UpdateObservations)).Methods("PUT") @@ -86,6 +88,7 @@ func routes(host, secretKey string, router *mux.Router, dataStore store.DataStor api.router.HandleFunc("/instances/{id}/dimensions", api.privateAuth.Check(dimension.Add)).Methods("POST") api.router.HandleFunc("/instances/{id}/dimensions/{dimension}/options", dimension.GetUnique).Methods("GET") api.router.HandleFunc("/instances/{id}/dimensions/{dimension}/options/{value}/node_id/{node_id}", api.privateAuth.Check(dimension.AddNodeID)).Methods("PUT") + return &api } diff --git a/instance/instance.go b/instance/instance.go index 4bb95915..2f0eb61d 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -109,6 +109,49 @@ func (s *Store) Add(w http.ResponseWriter, r *http.Request) { log.Debug("add instance", log.Data{"instance": instance}) } +func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { + vars := mux.Vars(r) + ID := vars["id"] + dimension := vars["dimension"] + + bytes, err := ioutil.ReadAll(r.Body) + if err != nil { + log.Error(err, nil) + } + + var dim *models.CodeList + + err = json.Unmarshal(bytes, &dim) + if err != nil { + log.Error(err, nil) + } + + instance, err := s.GetInstance(ID) + if err != nil { + log.Error(err, nil) + } + + for i := 0; i < len(instance.Dimensions); i++ { + if instance.Dimensions[i].Name == dimension { + if dim.Label != "" { + instance.Dimensions[i].Label = dim.Label + } + if dim.Description != "" { + instance.Dimensions[i].Description = dim.Description + } + } + } + + if err = s.UpdateInstance(ID, instance); err != nil { + log.Error(err, nil) + handleErrorType(err, w) + return + } + + log.Debug("updated dimension", log.Data{"instance": ID, "dimension": dimension}) + +} + //Update a specific instance func (s *Store) Update(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) From 67745e08d453792a3f5f98eb2867a41dc48622ac Mon Sep 17 00:00:00 2001 From: Michael Adams Date: Mon, 8 Jan 2018 20:55:03 +0000 Subject: [PATCH 03/41] Removed code from failed first attempt --- api/api.go | 1 - 1 file changed, 1 deletion(-) diff --git a/api/api.go b/api/api.go index ee4b6e24..0ee9a490 100644 --- a/api/api.go +++ b/api/api.go @@ -71,7 +71,6 @@ func routes(host, secretKey string, router *mux.Router, dataStore store.DataStor api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}", api.privateAuth.Check(api.putVersion)).Methods("PUT") api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}/metadata", api.getMetadata).Methods("GET") api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}/dimensions", api.getDimensions).Methods("GET") - api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}/dimensions/{dimension}", api.privateAuth.Check(api.putDimension)).Methods("PUT") api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}/dimensions/{dimension}/options", api.getDimensionOptions).Methods("GET") instance := instance.Store{Host: api.host, Storer: api.dataStore.Backend} From a250fcf3df6fb54b434880c64fd1a4377b2955c5 Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Tue, 9 Jan 2018 14:13:55 +0000 Subject: [PATCH 04/41] Added comments and statusForbidden response after publication --- instance/instance.go | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/instance/instance.go b/instance/instance.go index 2f0eb61d..d32d4ea6 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -109,30 +109,45 @@ func (s *Store) Add(w http.ResponseWriter, r *http.Request) { log.Debug("add instance", log.Data{"instance": instance}) } +// UpdateDimension updates label and/or description for a specific dimension in an instance func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { + vars := mux.Vars(r) ID := vars["id"] dimension := vars["dimension"] - bytes, err := ioutil.ReadAll(r.Body) + // Get instance + instance, err := s.GetInstance(ID) if err != nil { log.Error(err, nil) } - var dim *models.CodeList + // Early return if instance is already published + if instance.State == "published" { + w.WriteHeader(http.StatusForbidden) + return + } - err = json.Unmarshal(bytes, &dim) + // Read and unmarshal request body + bytes, err := ioutil.ReadAll(r.Body) if err != nil { log.Error(err, nil) } - instance, err := s.GetInstance(ID) + var dim *models.CodeList + + err = json.Unmarshal(bytes, &dim) if err != nil { log.Error(err, nil) } + // Update instance-dimension for i := 0; i < len(instance.Dimensions); i++ { + + // For the chosen dimension if instance.Dimensions[i].Name == dimension { + + // Assign update info, conditionals to allow updating of both or either without blanking other if dim.Label != "" { instance.Dimensions[i].Label = dim.Label } @@ -140,8 +155,10 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { instance.Dimensions[i].Description = dim.Description } } + } + // Update instance if err = s.UpdateInstance(ID, instance); err != nil { log.Error(err, nil) handleErrorType(err, w) From bad3a7c9e03b4df2ebb118b63ccbfa75e4169718 Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Tue, 9 Jan 2018 15:44:41 +0000 Subject: [PATCH 05/41] Final --- instance/instance.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instance/instance.go b/instance/instance.go index d32d4ea6..90d92fca 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -109,8 +109,8 @@ func (s *Store) Add(w http.ResponseWriter, r *http.Request) { log.Debug("add instance", log.Data{"instance": instance}) } -// UpdateDimension updates label and/or description for a specific dimension in an instance -func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { +// UpdateDimensionLabelDesc updates label and/or description for a specific dimension within an instance +func (s *Store) UpdateDimensionLabelDesc(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) ID := vars["id"] From 53426a409f734f8e0214341dce55ecdc4bf76ea4 Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Tue, 9 Jan 2018 17:22:23 +0000 Subject: [PATCH 06/41] Corrections: Re-added codelist label field and corrected function name --- instance/instance.go | 4 ++-- models/instance.go | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/instance/instance.go b/instance/instance.go index 90d92fca..50289333 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -109,8 +109,8 @@ func (s *Store) Add(w http.ResponseWriter, r *http.Request) { log.Debug("add instance", log.Data{"instance": instance}) } -// UpdateDimensionLabelDesc updates label and/or description for a specific dimension within an instance -func (s *Store) UpdateDimensionLabelDesc(w http.ResponseWriter, r *http.Request) { +// UpdateDimension updates label and/or description for a specific dimension within an instance +func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) ID := vars["id"] diff --git a/models/instance.go b/models/instance.go index f5b307e7..14a61dd4 100644 --- a/models/instance.go +++ b/models/instance.go @@ -33,6 +33,7 @@ type CodeList struct { HRef string `json:"href"` ID string `json:"id"` Name string `json:"name"` + Label string `json:"label"` } // InstanceLinks holds all links for an instance From ff121294917473749a339e4aaed985a350285c33 Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Tue, 9 Jan 2018 17:30:32 +0000 Subject: [PATCH 07/41] Startd updating swaggerpec for new API route --- swagger.yaml | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/swagger.yaml b/swagger.yaml index 7f3f5049..8dd6f7e2 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -384,6 +384,32 @@ paths: description: "No dimensions found for version of an edition of a dataset using the id, edition and version provided" 500: $ref: '#/responses/InternalError' + /datasets/{id}/editions/{edition}/versions/{version}/dimensions/{dimension}: + get: + tags: + - "Private user" + summary: "Update label and/or description for a single dimension" + description: "Update label and/or description for a single dimension" + parameters: + - $ref: '#/parameters/id' + - $ref: '#/parameters/dimension' + - $ref: '#/parameters/limit' + - $ref: '#/parameters/offset' + responses: + 400: + description: | + Invalid request, reasons can be one of the following: + * invalid request body + * dataset id was incorrect + * edition was incorrect + 401: + description: "Unauthorised to update version of dataset" + 403: + description: "Forbidden to overwrite version of dataset, already published" + 404: + description: "Version was not found for a dataset using the id and edition provided" + 500: + $ref: '#/responses/InternalError' /datasets/{id}/editions/{edition}/versions/{version}/dimensions/{dimension}/options: get: tags: From bc1cdd33639ac046dce28e8fea0d3d24645a7b72 Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Tue, 9 Jan 2018 17:51:23 +0000 Subject: [PATCH 08/41] changed to use published state constant rather than 'published' string --- instance/instance.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instance/instance.go b/instance/instance.go index 50289333..52f0f5a9 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -123,7 +123,7 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { } // Early return if instance is already published - if instance.State == "published" { + if instance.State == models.PublishedState { w.WriteHeader(http.StatusForbidden) return } From 2fa0a7b981a9ecafacab91626f78f3c5f674c9f5 Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Fri, 19 Jan 2018 11:37:24 +0000 Subject: [PATCH 09/41] Imporoved error logging --- instance/instance.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/instance/instance.go b/instance/instance.go index 52f0f5a9..18c2b817 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -119,7 +119,8 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { // Get instance instance, err := s.GetInstance(ID) if err != nil { - log.Error(err, nil) + log.ErrorC("Failed to GET instance when attempting to update a dimension of that instance.", err, log.Data{"instance": ID}) + handleErrorType(err, w) } // Early return if instance is already published @@ -131,14 +132,16 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { // Read and unmarshal request body bytes, err := ioutil.ReadAll(r.Body) if err != nil { - log.Error(err, nil) + log.ErrorC("Error reading response.body, expecting {\"label\":foo', \"description\":foo} or one of", err, log.Data{"response body": r.Body}) + handleErrorType(err, w) } var dim *models.CodeList err = json.Unmarshal(bytes, &dim) if err != nil { - log.Error(err, nil) + log.ErrorC("Failing to unmarshall bytes to models.codelist", err, log.Data{"bytes": bytes}) + handleErrorType(err, w) } // Update instance-dimension @@ -160,7 +163,7 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { // Update instance if err = s.UpdateInstance(ID, instance); err != nil { - log.Error(err, nil) + log.ErrorC("Failed to update instace with new dimension label/description.", err, log.Data{"instance": ID, "update": instance}) handleErrorType(err, w) return } From e794107844efef13d0f1535b92e1b98b41b331b2 Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Fri, 19 Jan 2018 11:47:15 +0000 Subject: [PATCH 10/41] Imporoved error logging --- instance/instance.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instance/instance.go b/instance/instance.go index 18c2b817..bca7784d 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -140,7 +140,7 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { err = json.Unmarshal(bytes, &dim) if err != nil { - log.ErrorC("Failing to unmarshall bytes to models.codelist", err, log.Data{"bytes": bytes}) + log.ErrorC("Failing to model models.Codelist resource based on request", err, log.Data{"instance": ID, "dimension": dimension}) handleErrorType(err, w) } From 5e10a2bd5475ce92769e9f4078d7e19752e9d4dd Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Fri, 19 Jan 2018 11:59:45 +0000 Subject: [PATCH 11/41] lowercased id, added log message for 'already published' --- instance/instance.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/instance/instance.go b/instance/instance.go index bca7784d..1f8b4139 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -113,18 +113,19 @@ func (s *Store) Add(w http.ResponseWriter, r *http.Request) { func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) - ID := vars["id"] + id := vars["id"] dimension := vars["dimension"] // Get instance - instance, err := s.GetInstance(ID) + instance, err := s.GetInstance(id) if err != nil { - log.ErrorC("Failed to GET instance when attempting to update a dimension of that instance.", err, log.Data{"instance": ID}) + log.ErrorC("Failed to GET instance when attempting to update a dimension of that instance.", err, log.Data{"instance": id}) handleErrorType(err, w) } // Early return if instance is already published if instance.State == models.PublishedState { + log.Debug("unable to update instance, already published", nil) w.WriteHeader(http.StatusForbidden) return } @@ -140,7 +141,7 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { err = json.Unmarshal(bytes, &dim) if err != nil { - log.ErrorC("Failing to model models.Codelist resource based on request", err, log.Data{"instance": ID, "dimension": dimension}) + log.ErrorC("Failing to model models.Codelist resource based on request", err, log.Data{"instance": id, "dimension": dimension}) handleErrorType(err, w) } @@ -157,18 +158,19 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { if dim.Description != "" { instance.Dimensions[i].Description = dim.Description } + break } } // Update instance - if err = s.UpdateInstance(ID, instance); err != nil { - log.ErrorC("Failed to update instace with new dimension label/description.", err, log.Data{"instance": ID, "update": instance}) + if err = s.UpdateInstance(id, instance); err != nil { + log.ErrorC("Failed to update instace with new dimension label/description.", err, log.Data{"instance": id, "update": instance}) handleErrorType(err, w) return } - log.Debug("updated dimension", log.Data{"instance": ID, "dimension": dimension}) + log.Debug("updated dimension", log.Data{"instance": id, "dimension": dimension}) } From 8ae72dd018f7256d8b50f23269d911f3de382ae3 Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Fri, 19 Jan 2018 12:19:39 +0000 Subject: [PATCH 12/41] Switched to using range operator --- instance/instance.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instance/instance.go b/instance/instance.go index 1f8b4139..f956aa4b 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -146,7 +146,7 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { } // Update instance-dimension - for i := 0; i < len(instance.Dimensions); i++ { + for i := range instance.Dimensions { // For the chosen dimension if instance.Dimensions[i].Name == dimension { From 0f562ab87b013ff2a33ab07b6915e28a4c5d48cc Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Fri, 19 Jan 2018 12:55:35 +0000 Subject: [PATCH 13/41] Better log.Data variables --- instance/instance.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/instance/instance.go b/instance/instance.go index f956aa4b..b0ee73c3 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -125,7 +125,7 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { // Early return if instance is already published if instance.State == models.PublishedState { - log.Debug("unable to update instance, already published", nil) + log.Debug("unable to update instance/version, already published", nil) w.WriteHeader(http.StatusForbidden) return } @@ -133,8 +133,8 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { // Read and unmarshal request body bytes, err := ioutil.ReadAll(r.Body) if err != nil { - log.ErrorC("Error reading response.body, expecting {\"label\":foo', \"description\":foo} or one of", err, log.Data{"response body": r.Body}) - handleErrorType(err, w) + log.ErrorC("Error reading response.body.", err, nil) + http.Error(w, err.Error(), http.StatusBadRequest) } var dim *models.CodeList @@ -142,7 +142,7 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { err = json.Unmarshal(bytes, &dim) if err != nil { log.ErrorC("Failing to model models.Codelist resource based on request", err, log.Data{"instance": id, "dimension": dimension}) - handleErrorType(err, w) + http.Error(w, err.Error(), http.StatusBadRequest) } // Update instance-dimension @@ -165,7 +165,7 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { // Update instance if err = s.UpdateInstance(id, instance); err != nil { - log.ErrorC("Failed to update instace with new dimension label/description.", err, log.Data{"instance": id, "update": instance}) + log.ErrorC("Failed to update instance with new dimension label/description.", err, log.Data{"instance": id, "dimension": dimension}) handleErrorType(err, w) return } From 1ebceb14c848ccd867d15c7ed0558062f3341e76 Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Fri, 19 Jan 2018 12:55:56 +0000 Subject: [PATCH 14/41] Updated swagger spec --- swagger.yaml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/swagger.yaml b/swagger.yaml index 8dd6f7e2..1470044b 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -609,6 +609,31 @@ paths: $ref: '#/responses/InstanceNotFound' 500: $ref: '#/responses/InternalError' + /instances/{instance_id}/dimensions/{dimension}: + put: + tags: + - "Private" + summary: "Update dimension description and/or label" + description: "Update the label and/or description of a dimension within an instance, by providing dimension name and properties to over write" + parameters: + - $ref: '#/parameters/instance_id' + - $ref: '#/parameters/dimension' + security: + - InternalAPIKey: [] + responses: + 200: + description: "The instance has been updated" + 400: + $ref: '#/responses/InvalidRequestError' + 401: + $ref: '#/responses/UnauthorisedError' + 403: + $ref: '#/responses/ForbiddenError' + 404: + $ref: '#/responses/InstanceNotFound' + 500: + $ref: '#/responses/InternalError' + /instances/{instance_id}/dimensions/{dimension}/options: get: tags: From 22522b10ea86deae7a67219dbab64560301da2bc Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Fri, 19 Jan 2018 12:59:58 +0000 Subject: [PATCH 15/41] Removed unintentional blank line in swagger spec --- swagger.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/swagger.yaml b/swagger.yaml index 1470044b..fe409835 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -633,7 +633,6 @@ paths: $ref: '#/responses/InstanceNotFound' 500: $ref: '#/responses/InternalError' - /instances/{instance_id}/dimensions/{dimension}/options: get: tags: From e5163cd1c8a1e25fc28685366211ea8d6df886fc Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Fri, 19 Jan 2018 13:05:02 +0000 Subject: [PATCH 16/41] removed early incorrect commit to swagger spec --- swagger.yaml | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/swagger.yaml b/swagger.yaml index fe409835..c0afc2c3 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -384,32 +384,6 @@ paths: description: "No dimensions found for version of an edition of a dataset using the id, edition and version provided" 500: $ref: '#/responses/InternalError' - /datasets/{id}/editions/{edition}/versions/{version}/dimensions/{dimension}: - get: - tags: - - "Private user" - summary: "Update label and/or description for a single dimension" - description: "Update label and/or description for a single dimension" - parameters: - - $ref: '#/parameters/id' - - $ref: '#/parameters/dimension' - - $ref: '#/parameters/limit' - - $ref: '#/parameters/offset' - responses: - 400: - description: | - Invalid request, reasons can be one of the following: - * invalid request body - * dataset id was incorrect - * edition was incorrect - 401: - description: "Unauthorised to update version of dataset" - 403: - description: "Forbidden to overwrite version of dataset, already published" - 404: - description: "Version was not found for a dataset using the id and edition provided" - 500: - $ref: '#/responses/InternalError' /datasets/{id}/editions/{edition}/versions/{version}/dimensions/{dimension}/options: get: tags: From e209b5cae906547b002c0fa9ecf665d88995c756 Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Fri, 19 Jan 2018 13:53:51 +0000 Subject: [PATCH 17/41] Added return on encountering errors --- instance/instance.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/instance/instance.go b/instance/instance.go index b0ee73c3..6ed6b59d 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -121,6 +121,7 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { if err != nil { log.ErrorC("Failed to GET instance when attempting to update a dimension of that instance.", err, log.Data{"instance": id}) handleErrorType(err, w) + return } // Early return if instance is already published @@ -135,6 +136,7 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { if err != nil { log.ErrorC("Error reading response.body.", err, nil) http.Error(w, err.Error(), http.StatusBadRequest) + return } var dim *models.CodeList @@ -143,6 +145,7 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { if err != nil { log.ErrorC("Failing to model models.Codelist resource based on request", err, log.Data{"instance": id, "dimension": dimension}) http.Error(w, err.Error(), http.StatusBadRequest) + return } // Update instance-dimension From 6b3cc1500d0afd3b0cc630122e100afee829099d Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Mon, 22 Jan 2018 14:16:51 +0000 Subject: [PATCH 18/41] Added tests for first 3 dimension update failures --- instance/instance_external_test.go | 62 ++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/instance/instance_external_test.go b/instance/instance_external_test.go index 051a1d6b..645f16ab 100644 --- a/instance/instance_external_test.go +++ b/instance/instance_external_test.go @@ -446,3 +446,65 @@ func TestInsertedObservationsReturnsNotFound(t *testing.T) { So(len(mockedDataStore.UpdateObservationInsertedCalls()), ShouldEqual, 1) }) } + +func TestUpdateDimensionFailure(t *testing.T) { + t.Parallel() + + Convey("when the instance does not exist return status not found", t, func() { + r := createRequestWithToken("PUT", "http://localhost:22000/instances/dimensions/age", nil) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetInstanceFunc: func(id string) (*models.Instance, error) { + return nil, errs.ErrInstanceNotFound + }, + } + + instance := &instance.Store{Host: host, Storer: mockedDataStore} + instance.UpdateDimension(w, r) + + So(w.Code, ShouldEqual, http.StatusNotFound) + So(len(mockedDataStore.GetInstanceCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateInstanceCalls()), ShouldEqual, 0) + }) + + Convey("when the instance containing the dimension is already published", t, func() { + body := strings.NewReader(`{"label": "My amazing dimension"}`) + r := createRequestWithToken("PUT", "http://localhost:21800/instances/123/dimensions/age", body) + w := httptest.NewRecorder() + + currentInstanceTestData := &models.Instance{ + State: models.PublishedState, + } + + mockedDataStore := &storetest.StorerMock{ + GetInstanceFunc: func(id string) (*models.Instance, error) { + return currentInstanceTestData, nil + }, + } + + instance := &instance.Store{Host: host, Storer: mockedDataStore} + instance.UpdateDimension(w, r) + + So(w.Code, ShouldEqual, http.StatusForbidden) + So(len(mockedDataStore.GetInstanceCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateInstanceCalls()), ShouldEqual, 0) + }) + + Convey("When the response body does not contain valid json", t, func() { + body := strings.NewReader("{") + r := createRequestWithToken("PUT", "http://localhost:21800/instances/123/dimensions/age", body) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetInstanceFunc: func(id string) (*models.Instance, error) { + return &models.Instance{}, nil + }, + } + + instance := &instance.Store{Host: host, Storer: mockedDataStore} + instance.UpdateDimension(w, r) + + So(w.Code, ShouldEqual, http.StatusBadRequest) + }) +} From f9ea648931a71661483b9b22ea5187e7e1bac6e6 Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Mon, 22 Jan 2018 16:10:51 +0000 Subject: [PATCH 19/41] Added update returns ok test, cleaned up --- instance/instance_external_test.go | 45 ++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/instance/instance_external_test.go b/instance/instance_external_test.go index 645f16ab..54e3e1bc 100644 --- a/instance/instance_external_test.go +++ b/instance/instance_external_test.go @@ -447,10 +447,9 @@ func TestInsertedObservationsReturnsNotFound(t *testing.T) { }) } -func TestUpdateDimensionFailure(t *testing.T) { +func TestUpdateDimensionReturnsNotFound(t *testing.T) { t.Parallel() - - Convey("when the instance does not exist return status not found", t, func() { + Convey("When update dimension return status not found", t, func() { r := createRequestWithToken("PUT", "http://localhost:22000/instances/dimensions/age", nil) w := httptest.NewRecorder() @@ -467,10 +466,12 @@ func TestUpdateDimensionFailure(t *testing.T) { So(len(mockedDataStore.GetInstanceCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateInstanceCalls()), ShouldEqual, 0) }) +} - Convey("when the instance containing the dimension is already published", t, func() { - body := strings.NewReader(`{"label": "My amazing dimension"}`) - r := createRequestWithToken("PUT", "http://localhost:21800/instances/123/dimensions/age", body) +func TestUpdateDimensionReturnsForbidden(t *testing.T) { + t.Parallel() + Convey("When update dimension returns forbidden (for already published) ", t, func() { + r := createRequestWithToken("PUT", "http://localhost:22000/instances/123/dimensions/age", nil) w := httptest.NewRecorder() currentInstanceTestData := &models.Instance{ @@ -490,10 +491,13 @@ func TestUpdateDimensionFailure(t *testing.T) { So(len(mockedDataStore.GetInstanceCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateInstanceCalls()), ShouldEqual, 0) }) +} - Convey("When the response body does not contain valid json", t, func() { +func TestUpdateDimensionReturnsBadRequest(t *testing.T) { + t.Parallel() + Convey("When update dimension returns bad request", t, func() { body := strings.NewReader("{") - r := createRequestWithToken("PUT", "http://localhost:21800/instances/123/dimensions/age", body) + r := createRequestWithToken("PUT", "http://localhost:22000/instances/123/dimensions/age", body) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ @@ -508,3 +512,28 @@ func TestUpdateDimensionFailure(t *testing.T) { So(w.Code, ShouldEqual, http.StatusBadRequest) }) } + +func TestUpdateDimensionReturnsOk(t *testing.T) { + t.Parallel() + Convey("When update dimension fails to update an instance", t, func() { + body := strings.NewReader("{}") + r := createRequestWithToken("PUT", "http://localhost:22000/instances/123/dimensions/age", body) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetInstanceFunc: func(id string) (*models.Instance, error) { + return &models.Instance{}, nil + }, + UpdateInstanceFunc: func(id string, i *models.Instance) error { + return nil + }, + } + + instance := &instance.Store{Host: host, Storer: mockedDataStore} + instance.UpdateDimension(w, r) + + So(w.Code, ShouldEqual, http.StatusOK) + So(len(mockedDataStore.GetInstanceCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateInstanceCalls()), ShouldEqual, 1) + }) +} From 50add4830b898076dac1ab7dca38b2d9e04e9edc Mon Sep 17 00:00:00 2001 From: Carl Hembrough Date: Tue, 23 Jan 2018 15:43:10 +0000 Subject: [PATCH 20/41] Search builder integration. --- instance/instance.go | 14 ++++++++++++++ models/instance.go | 13 ++++++++++--- mongo/instance_store.go | 28 +++++++++++++++++++++++++--- store/datastore.go | 1 + 4 files changed, 50 insertions(+), 6 deletions(-) diff --git a/instance/instance.go b/instance/instance.go index fb4e27c5..78995cb1 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -398,6 +398,20 @@ func (s *Store) UpdateImportTask(w http.ResponseWriter, r *http.Request) { } } + if tasks.BuildSearchTasks != nil { + for _, task := range tasks.BuildSearchTasks { + if task.State != "" { + if task.State != models.CompletedState { + validationErrs = append(validationErrs, fmt.Errorf("bad request - invalid task state value: %v", task.State)) + } else if err := s.UpdateBuildSearchTaskState(id, task.DimensionName, task.State); err != nil { + log.Error(err, nil) + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + } + } + } + if len(validationErrs) > 0 { for _, err := range validationErrs { log.Error(err, nil) diff --git a/models/instance.go b/models/instance.go index 7b239b05..273f8946 100644 --- a/models/instance.go +++ b/models/instance.go @@ -27,16 +27,17 @@ type Instance struct { ImportTasks *InstanceImportTasks `bson:"import_tasks,omitempty" json:"import_tasks"` } -// InstanceImportTasks +// InstanceImportTasks represents all of the tasks required to complete an import job. type InstanceImportTasks struct { ImportObservations *ImportObservationsTask `bson:"import_observations,omitempty" json:"import_observations"` BuildHierarchyTasks []*BuildHierarchyTask `bson:"build_hierarchies,omitempty" json:"build_hierarchies"` + BuildSearchTasks []*BuildSearchTask `bson:"build_search,omitempty" json:"build_search"` } // ImportObservationsTask represents the task of importing instance observation data into the database. type ImportObservationsTask struct { - State string `bson:"state,omitempty" json:"state,omitempty"` - InsertedObservations int `bson:"total_inserted_observations" json:"total_inserted_observations,omitempty"` + State string `json:"state,omitempty"` + InsertedObservations int `json:"total_inserted_observations"` } // BuildHierarchyTask represents a task of importing a single hierarchy. @@ -46,6 +47,12 @@ type BuildHierarchyTask struct { CodeListID string `bson:"code_list_id,omitempty" json:"code_list_id,omitempty"` } +// BuildSearchTask represents a task of importing a single hierarchy into search. +type BuildSearchTask struct { + State string `bson:"state,omitempty" json:"state,omitempty"` + DimensionName string `bson:"dimension_name,omitempty" json:"dimension_name,omitempty"` +} + // CodeList for a dimension within an instance type CodeList struct { Description string `json:"description"` diff --git a/mongo/instance_store.go b/mongo/instance_store.go index 3d47d163..3ce75232 100644 --- a/mongo/instance_store.go +++ b/mongo/instance_store.go @@ -128,7 +128,9 @@ func (m *Mongo) UpdateObservationInserted(id string, observationInserted int64) err := s.DB(m.Database).C(instanceCollection).Update(bson.M{"id": id}, bson.M{ "$inc": bson.M{"import_tasks.import_observations.total_inserted_observations": observationInserted}, - "$set": bson.M{"last_updated": time.Now().UTC()}}) + "$set": bson.M{"last_updated": time.Now().UTC()}, + }, + ) if err == mgo.ErrNotFound { return errs.ErrInstanceNotFound @@ -150,7 +152,8 @@ func (m *Mongo) UpdateImportObservationsTaskState(id string, state string) error bson.M{ "$set": bson.M{"import_tasks.import_observations.state": state}, "$currentDate": bson.M{"last_updated": true}, - }) + }, + ) if err == mgo.ErrNotFound { return errs.ErrInstanceNotFound @@ -169,7 +172,7 @@ func (m *Mongo) UpdateBuildHierarchyTaskState(id, dimension, state string) (err defer s.Close() selector := bson.M{ - "id": id, + "id": id, "import_tasks.build_hierarchies.dimension_name": dimension, } @@ -181,3 +184,22 @@ func (m *Mongo) UpdateBuildHierarchyTaskState(id, dimension, state string) (err err = s.DB(m.Database).C(instanceCollection).Update(selector, update) return } + +// UpdateBuildSearchTaskState updates the state of a build search task. +func (m *Mongo) UpdateBuildSearchTaskState(id, dimension, state string) (err error) { + s := m.Session.Copy() + defer s.Close() + + selector := bson.M{ + "id": id, + "import_tasks.build_search.dimension_name": dimension, + } + + update := bson.M{ + "$set": bson.M{"import_tasks.build_search.$.state": state}, + "$currentDate": bson.M{"last_updated": true}, + } + + err = s.DB(m.Database).C(instanceCollection).Update(selector, update) + return +} diff --git a/store/datastore.go b/store/datastore.go index 3c335797..06b3a847 100644 --- a/store/datastore.go +++ b/store/datastore.go @@ -43,6 +43,7 @@ type Storer interface { UpdateObservationInserted(ID string, observationInserted int64) error UpdateImportObservationsTaskState(id, state string) error UpdateBuildHierarchyTaskState(id, dimension, state string) error + UpdateBuildSearchTaskState(id, dimension, state string) error UpdateVersion(ID string, version *models.Version) error UpsertContact(ID string, update interface{}) error UpsertDataset(ID string, datasetDoc *models.DatasetUpdate) error From 8ba962ab85e599ef4714c03d41dc7f455f39a0e7 Mon Sep 17 00:00:00 2001 From: Matt Guest Date: Mon, 29 Jan 2018 12:17:18 +0000 Subject: [PATCH 21/41] Stop reverting publish datasets back to different states --- Makefile | 3 +- instance/instance.go | 72 +++++++++++++++--------------- instance/instance_external_test.go | 25 +++++++++++ 3 files changed, 64 insertions(+), 36 deletions(-) diff --git a/Makefile b/Makefile index 6bda65d3..b4fae539 100644 --- a/Makefile +++ b/Makefile @@ -12,7 +12,8 @@ build: go build -o $(BUILD_ARCH)/$(BIN_DIR)/dp-dataset-api main.go debug: HUMAN_LOG=1 go run main.go - +acceptance: build + MONGODB_DATABASE=test HUMAN_LOG=1 go run main.go test: go test -cover $(shell go list ./... | grep -v /vendor/) diff --git a/instance/instance.go b/instance/instance.go index fb4e27c5..ed78ac6b 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -134,35 +134,37 @@ func (s *Store) Update(w http.ResponseWriter, r *http.Request) { instance.Links = updateLinks(instance, currentInstance) logData := log.Data{"instance_id": id, "current_state": currentInstance.State, "requested_state": instance.State} - switch instance.State { - case models.CompletedState: - if err = validateInstanceUpdate(models.SubmittedState, currentInstance, instance); err != nil { - log.Error(err, logData) - http.Error(w, err.Error(), http.StatusForbidden) - return - } - case models.EditionConfirmedState: - if err = validateInstanceUpdate(models.CompletedState, currentInstance, instance); err != nil { - log.Error(err, logData) - http.Error(w, err.Error(), http.StatusForbidden) - return - } - case models.AssociatedState: - if err = validateInstanceUpdate(models.EditionConfirmedState, currentInstance, instance); err != nil { - log.Error(err, logData) - http.Error(w, err.Error(), http.StatusForbidden) - return - } + if instance.State != currentInstance.State { + switch instance.State { + case models.CompletedState: + if err = validateInstanceUpdate(models.SubmittedState, currentInstance, instance); err != nil { + log.Error(err, logData) + http.Error(w, err.Error(), http.StatusForbidden) + return + } + case models.EditionConfirmedState: + if err = validateInstanceUpdate(models.CompletedState, currentInstance, instance); err != nil { + log.Error(err, logData) + http.Error(w, err.Error(), http.StatusForbidden) + return + } + case models.AssociatedState: + if err = validateInstanceUpdate(models.EditionConfirmedState, currentInstance, instance); err != nil { + log.Error(err, logData) + http.Error(w, err.Error(), http.StatusForbidden) + return + } - // TODO Update dataset.next state to associated and add collection id - case models.PublishedState: - if err = validateInstanceUpdate(models.AssociatedState, currentInstance, instance); err != nil { - log.Error(err, logData) - http.Error(w, err.Error(), http.StatusForbidden) - return - } + // TODO Update dataset.next state to associated and add collection id + case models.PublishedState: + if err = validateInstanceUpdate(models.AssociatedState, currentInstance, instance); err != nil { + log.Error(err, logData) + http.Error(w, err.Error(), http.StatusForbidden) + return + } - // TODO Update both edition and dataset states to published + // TODO Update both edition and dataset states to published + } } if instance.State == models.EditionConfirmedState { @@ -286,16 +288,16 @@ func (s *Store) getEdition(datasetID, edition, instanceID string) (*models.Editi } func validateInstanceUpdate(expectedState string, currentInstance, instance *models.Instance) error { - if currentInstance.State != expectedState { - err := fmt.Errorf("Unable to update resource, expected resource to have a state of %s", expectedState) - return err - } - if instance.State == models.EditionConfirmedState && currentInstance.Edition == "" && instance.Edition == "" { - err := errors.New("Unable to update resource, missing a value for the edition") - return err + var err error + if currentInstance.State == models.PublishedState { + err = fmt.Errorf("Unable to update resource state, as the version has been published") + } else if currentInstance.State != expectedState { + err = fmt.Errorf("Unable to update resource, expected resource to have a state of %s", expectedState) + } else if instance.State == models.EditionConfirmedState && currentInstance.Edition == "" && instance.Edition == "" { + err = errors.New("Unable to update resource, missing a value for the edition") } - return nil + return err } func (s *Store) defineInstanceLinks(instance *models.Instance, editionDoc *models.Edition) *models.InstanceLinks { diff --git a/instance/instance_external_test.go b/instance/instance_external_test.go index 3ccbf40d..540ca753 100644 --- a/instance/instance_external_test.go +++ b/instance/instance_external_test.go @@ -599,3 +599,28 @@ func TestStore_UpdateImportTask_ReturnsInternalError(t *testing.T) { So(len(mockedDataStore.UpdateImportObservationsTaskStateCalls()), ShouldEqual, 1) }) } + +func TestUpdateInstanceReturnsErrorWhenStateIsPublished(t *testing.T) { + t.Parallel() + Convey("when an instance has a state of published, then put request to change to it to completed ", t, func() { + body := strings.NewReader(`{"state":"completed"}`) + r := createRequestWithToken("PUT", "http://localhost:21800/instances/123", body) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetInstanceFunc: func(id string) (*models.Instance, error) { + return &models.Instance{State: models.PublishedState}, nil + }, + UpdateInstanceFunc: func(id string, i *models.Instance) error { + return nil + }, + } + + instance := &instance.Store{Host: host, Storer: mockedDataStore} + instance.Update(w, r) + + So(w.Code, ShouldEqual, http.StatusForbidden) + So(len(mockedDataStore.GetInstanceCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateInstanceCalls()), ShouldEqual, 1) + }) +} From fc3162b55e7dde4ea868d1afad3de969d0f436e3 Mon Sep 17 00:00:00 2001 From: Carl Hembrough Date: Mon, 29 Jan 2018 14:53:39 +0000 Subject: [PATCH 22/41] Search builder integration --- instance/instance.go | 4 +-- models/instance.go | 10 +++--- mongo/instance_store.go | 4 +-- store/datastoretest/datastore.go | 55 ++++++++++++++++++++++++++++++++ 4 files changed, 64 insertions(+), 9 deletions(-) diff --git a/instance/instance.go b/instance/instance.go index 78995cb1..07760236 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -398,8 +398,8 @@ func (s *Store) UpdateImportTask(w http.ResponseWriter, r *http.Request) { } } - if tasks.BuildSearchTasks != nil { - for _, task := range tasks.BuildSearchTasks { + if tasks.BuildSearchIndexTasks != nil { + for _, task := range tasks.BuildSearchIndexTasks { if task.State != "" { if task.State != models.CompletedState { validationErrs = append(validationErrs, fmt.Errorf("bad request - invalid task state value: %v", task.State)) diff --git a/models/instance.go b/models/instance.go index 273f8946..1c640566 100644 --- a/models/instance.go +++ b/models/instance.go @@ -29,9 +29,9 @@ type Instance struct { // InstanceImportTasks represents all of the tasks required to complete an import job. type InstanceImportTasks struct { - ImportObservations *ImportObservationsTask `bson:"import_observations,omitempty" json:"import_observations"` - BuildHierarchyTasks []*BuildHierarchyTask `bson:"build_hierarchies,omitempty" json:"build_hierarchies"` - BuildSearchTasks []*BuildSearchTask `bson:"build_search,omitempty" json:"build_search"` + ImportObservations *ImportObservationsTask `bson:"import_observations,omitempty" json:"import_observations"` + BuildHierarchyTasks []*BuildHierarchyTask `bson:"build_hierarchies,omitempty" json:"build_hierarchies"` + BuildSearchIndexTasks []*BuildSearchIndexTask `bson:"build_search_indexes,omitempty" json:"build_search_indexes"` } // ImportObservationsTask represents the task of importing instance observation data into the database. @@ -47,8 +47,8 @@ type BuildHierarchyTask struct { CodeListID string `bson:"code_list_id,omitempty" json:"code_list_id,omitempty"` } -// BuildSearchTask represents a task of importing a single hierarchy into search. -type BuildSearchTask struct { +// BuildSearchIndexTask represents a task of importing a single search index into search. +type BuildSearchIndexTask struct { State string `bson:"state,omitempty" json:"state,omitempty"` DimensionName string `bson:"dimension_name,omitempty" json:"dimension_name,omitempty"` } diff --git a/mongo/instance_store.go b/mongo/instance_store.go index 3ce75232..d32a5d3a 100644 --- a/mongo/instance_store.go +++ b/mongo/instance_store.go @@ -172,7 +172,7 @@ func (m *Mongo) UpdateBuildHierarchyTaskState(id, dimension, state string) (err defer s.Close() selector := bson.M{ - "id": id, + "id": id, "import_tasks.build_hierarchies.dimension_name": dimension, } @@ -191,7 +191,7 @@ func (m *Mongo) UpdateBuildSearchTaskState(id, dimension, state string) (err err defer s.Close() selector := bson.M{ - "id": id, + "id": id, "import_tasks.build_search.dimension_name": dimension, } diff --git a/store/datastoretest/datastore.go b/store/datastoretest/datastore.go index 3ccd3139..2ce1f81e 100755 --- a/store/datastoretest/datastore.go +++ b/store/datastoretest/datastore.go @@ -32,6 +32,7 @@ var ( lockStorerMockGetVersions sync.RWMutex lockStorerMockPing sync.RWMutex lockStorerMockUpdateBuildHierarchyTaskState sync.RWMutex + lockStorerMockUpdateBuildSearchTaskState sync.RWMutex lockStorerMockUpdateDataset sync.RWMutex lockStorerMockUpdateDatasetWithAssociation sync.RWMutex lockStorerMockUpdateDimensionNodeID sync.RWMutex @@ -112,6 +113,9 @@ var ( // UpdateBuildHierarchyTaskStateFunc: func(id string, dimension string, state string) error { // panic("TODO: mock out the UpdateBuildHierarchyTaskState method") // }, +// UpdateBuildSearchTaskStateFunc: func(id string, dimension string, state string) error { +// panic("TODO: mock out the UpdateBuildSearchTaskState method") +// }, // UpdateDatasetFunc: func(ID string, dataset *models.Dataset) error { // panic("TODO: mock out the UpdateDataset method") // }, @@ -215,6 +219,9 @@ type StorerMock struct { // UpdateBuildHierarchyTaskStateFunc mocks the UpdateBuildHierarchyTaskState method. UpdateBuildHierarchyTaskStateFunc func(id string, dimension string, state string) error + // UpdateBuildSearchTaskStateFunc mocks the UpdateBuildSearchTaskState method. + UpdateBuildSearchTaskStateFunc func(id string, dimension string, state string) error + // UpdateDatasetFunc mocks the UpdateDataset method. UpdateDatasetFunc func(ID string, dataset *models.Dataset) error @@ -391,6 +398,15 @@ type StorerMock struct { // State is the state argument value. State string } + // UpdateBuildSearchTaskState holds details about calls to the UpdateBuildSearchTaskState method. + UpdateBuildSearchTaskState []struct { + // Id is the id argument value. + Id string + // Dimension is the dimension argument value. + Dimension string + // State is the state argument value. + State string + } // UpdateDataset holds details about calls to the UpdateDataset method. UpdateDataset []struct { // ID is the ID argument value. @@ -1177,6 +1193,45 @@ func (mock *StorerMock) UpdateBuildHierarchyTaskStateCalls() []struct { return calls } +// UpdateBuildSearchTaskState calls UpdateBuildSearchTaskStateFunc. +func (mock *StorerMock) UpdateBuildSearchTaskState(id string, dimension string, state string) error { + if mock.UpdateBuildSearchTaskStateFunc == nil { + panic("moq: StorerMock.UpdateBuildSearchTaskStateFunc is nil but Storer.UpdateBuildSearchTaskState was just called") + } + callInfo := struct { + Id string + Dimension string + State string + }{ + Id: id, + Dimension: dimension, + State: state, + } + lockStorerMockUpdateBuildSearchTaskState.Lock() + mock.calls.UpdateBuildSearchTaskState = append(mock.calls.UpdateBuildSearchTaskState, callInfo) + lockStorerMockUpdateBuildSearchTaskState.Unlock() + return mock.UpdateBuildSearchTaskStateFunc(id, dimension, state) +} + +// UpdateBuildSearchTaskStateCalls gets all the calls that were made to UpdateBuildSearchTaskState. +// Check the length with: +// len(mockedStorer.UpdateBuildSearchTaskStateCalls()) +func (mock *StorerMock) UpdateBuildSearchTaskStateCalls() []struct { + Id string + Dimension string + State string +} { + var calls []struct { + Id string + Dimension string + State string + } + lockStorerMockUpdateBuildSearchTaskState.RLock() + calls = mock.calls.UpdateBuildSearchTaskState + lockStorerMockUpdateBuildSearchTaskState.RUnlock() + return calls +} + // UpdateDataset calls UpdateDatasetFunc. func (mock *StorerMock) UpdateDataset(ID string, dataset *models.Dataset) error { if mock.UpdateDatasetFunc == nil { From dd987e945a7a2a8b4402a362415115aa87d2b72c Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Mon, 29 Jan 2018 17:35:40 +0000 Subject: [PATCH 23/41] Corrected changes to swagger spec --- swagger.yaml | 47 ++++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/swagger.yaml b/swagger.yaml index c0afc2c3..1528147c 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -584,29 +584,30 @@ paths: 500: $ref: '#/responses/InternalError' /instances/{instance_id}/dimensions/{dimension}: - put: - tags: - - "Private" - summary: "Update dimension description and/or label" - description: "Update the label and/or description of a dimension within an instance, by providing dimension name and properties to over write" - parameters: - - $ref: '#/parameters/instance_id' - - $ref: '#/parameters/dimension' - security: - - InternalAPIKey: [] - responses: - 200: - description: "The instance has been updated" - 400: - $ref: '#/responses/InvalidRequestError' - 401: - $ref: '#/responses/UnauthorisedError' - 403: - $ref: '#/responses/ForbiddenError' - 404: - $ref: '#/responses/InstanceNotFound' - 500: - $ref: '#/responses/InternalError' + put: + tags: + - "Private user" + summary: "Update dimension description and/or label" + description: "Update the label and/or description of a dimension within an instance, by providing dimension name and properties to over write" + parameters: + - $ref: '#/parameters/instance_id' + - $ref: '#/parameters/dimension' + - $ref: '#/parameters/instance' + security: + - InternalAPIKey: [] + responses: + 200: + description: "The instance has been updated" + 400: + $ref: '#/responses/InvalidRequestError' + 401: + $ref: '#/responses/UnauthorisedError' + 403: + $ref: '#/responses/ForbiddenError' + 404: + $ref: '#/responses/InstanceNotFound' + 500: + $ref: '#/responses/InternalError' /instances/{instance_id}/dimensions/{dimension}/options: get: tags: From 8135f81085685e6d4df4c78c4441613eb12b0f65 Mon Sep 17 00:00:00 2001 From: Matt Guest Date: Thu, 1 Feb 2018 10:22:03 +0000 Subject: [PATCH 24/41] Added additional guards around instance endpoints for published data --- api/api.go | 23 +++++++++++++---------- instance/instance.go | 29 +++++++++++++++++++++++++++++ instance/instance_external_test.go | 3 +-- 3 files changed, 43 insertions(+), 12 deletions(-) diff --git a/api/api.go b/api/api.go index 8c94a1e7..ccad57d0 100644 --- a/api/api.go +++ b/api/api.go @@ -83,20 +83,23 @@ func routes(host, secretKey string, router *mux.Router, dataStore store.DataStor api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}/dimensions", api.getDimensions).Methods("GET") api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}/dimensions/{dimension}/options", api.getDimensionOptions).Methods("GET") - instance := instance.Store{Host: api.host, Storer: api.dataStore.Backend} - api.router.HandleFunc("/instances", api.privateAuth.Check(instance.GetList)).Methods("GET") - api.router.HandleFunc("/instances", api.privateAuth.Check(instance.Add)).Methods("POST") - api.router.HandleFunc("/instances/{id}", api.privateAuth.Check(instance.Get)).Methods("GET") - api.router.HandleFunc("/instances/{id}", api.privateAuth.Check(instance.Update)).Methods("PUT") - api.router.HandleFunc("/instances/{id}/events", api.privateAuth.Check(instance.AddEvent)).Methods("POST") - api.router.HandleFunc("/instances/{id}/inserted_observations/{inserted_observations}", api.privateAuth.Check(instance.UpdateObservations)).Methods("PUT") - api.router.HandleFunc("/instances/{id}/import_tasks", api.privateAuth.Check(instance.UpdateImportTask)).Methods("PUT") + instanceAPI := instance.Store{Host: api.host, Storer: api.dataStore.Backend} + publishChecker := instance.InstancePublishCheck{Datastore: dataStore.Backend} + api.router.HandleFunc("/instances", api.privateAuth.Check(instanceAPI.GetList)).Methods("GET") + api.router.HandleFunc("/instances", api.privateAuth.Check(instanceAPI.Add)).Methods("POST") + api.router.HandleFunc("/instances/{id}", api.privateAuth.Check(instanceAPI.Get)).Methods("GET") + api.router.HandleFunc("/instances/{id}", api.privateAuth.Check(publishChecker.Check(instanceAPI.Update))).Methods("PUT") + api.router.HandleFunc("/instances/{id}/events", api.privateAuth.Check(instanceAPI.AddEvent)).Methods("POST") + api.router.HandleFunc("/instances/{id}/inserted_observations/{inserted_observations}", + api.privateAuth.Check(publishChecker.Check(instanceAPI.UpdateObservations))).Methods("PUT") + api.router.HandleFunc("/instances/{id}/import_tasks", api.privateAuth.Check(publishChecker.Check(instanceAPI.UpdateImportTask))).Methods("PUT") dimension := dimension.Store{Storer: api.dataStore.Backend} api.router.HandleFunc("/instances/{id}/dimensions", dimension.GetNodes).Methods("GET") - api.router.HandleFunc("/instances/{id}/dimensions", api.privateAuth.Check(dimension.Add)).Methods("POST") + api.router.HandleFunc("/instances/{id}/dimensions", api.privateAuth.Check(publishChecker.Check(dimension.Add))).Methods("POST") api.router.HandleFunc("/instances/{id}/dimensions/{dimension}/options", dimension.GetUnique).Methods("GET") - api.router.HandleFunc("/instances/{id}/dimensions/{dimension}/options/{value}/node_id/{node_id}", api.privateAuth.Check(dimension.AddNodeID)).Methods("PUT") + api.router.HandleFunc("/instances/{id}/dimensions/{dimension}/options/{value}/node_id/{node_id}", + api.privateAuth.Check(publishChecker.Check(dimension.AddNodeID))).Methods("PUT") return &api } diff --git a/instance/instance.go b/instance/instance.go index ed78ac6b..8245c340 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -490,3 +490,32 @@ func writeBody(w http.ResponseWriter, bytes []byte) { http.Error(w, err.Error(), http.StatusInternalServerError) } } + +// InstancePublishCheck Checks if an instance has been published +type InstancePublishCheck struct { + Datastore store.Storer +} + +// Check wraps a HTTP handle. Checks that the state is not published +func (d *InstancePublishCheck) Check(handle func(http.ResponseWriter, *http.Request)) http.HandlerFunc { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + + vars := mux.Vars(r) + id := vars["id"] + instance, err := d.Datastore.GetInstance(id) + if err != nil { + log.Error(err, nil) + handleErrorType(err, w) + return + } + + if instance.State == models.PublishedState { + err = errors.New("unable to update instance as it has been published") + log.Error(err, log.Data{"instance": instance}) + http.Error(w, err.Error(), http.StatusForbidden) + return + + } + handle(w, r) + }) +} diff --git a/instance/instance_external_test.go b/instance/instance_external_test.go index 540ca753..02a17d60 100644 --- a/instance/instance_external_test.go +++ b/instance/instance_external_test.go @@ -401,7 +401,7 @@ func TestUpdateCompletedInstanceToCompletedReturnsForbidden(t *testing.T) { ID: "4567", }, }, - State: models.CompletedState, + State: models.PublishedState, } mockedDataStore := &storetest.StorerMock{ @@ -621,6 +621,5 @@ func TestUpdateInstanceReturnsErrorWhenStateIsPublished(t *testing.T) { So(w.Code, ShouldEqual, http.StatusForbidden) So(len(mockedDataStore.GetInstanceCalls()), ShouldEqual, 1) - So(len(mockedDataStore.UpdateInstanceCalls()), ShouldEqual, 1) }) } From ec854fdb580696cadf6f0cb6113392c7d4e2f286 Mon Sep 17 00:00:00 2001 From: Matt Guest Date: Thu, 1 Feb 2018 11:24:02 +0000 Subject: [PATCH 25/41] Fixed a bug that only logged default values --- config/config.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/config/config.go b/config/config.go index a8d8e71a..c87d5497 100644 --- a/config/config.go +++ b/config/config.go @@ -55,7 +55,8 @@ func Get() (*Configuration, error) { sanitized := *cfg sanitized.SecretKey = "" + err := envconfig.Process("", cfg) log.Info("config on startup", log.Data{"config": sanitized}) - return cfg, envconfig.Process("", cfg) + return cfg, err } From a52970572bb54ee08d23276d2b2402469458de4a Mon Sep 17 00:00:00 2001 From: Carl Hembrough Date: Mon, 5 Feb 2018 13:26:49 +0000 Subject: [PATCH 26/41] Fix observations inserted model to use int64, and not omitempty --- models/instance.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/instance.go b/models/instance.go index 1c640566..7d8624bb 100644 --- a/models/instance.go +++ b/models/instance.go @@ -36,8 +36,8 @@ type InstanceImportTasks struct { // ImportObservationsTask represents the task of importing instance observation data into the database. type ImportObservationsTask struct { - State string `json:"state,omitempty"` - InsertedObservations int `json:"total_inserted_observations"` + State string `bson:"state,omitempty" json:"state,omitempty"` + InsertedObservations int64 `bson:"total_inserted_observations" json:"total_inserted_observations"` } // BuildHierarchyTask represents a task of importing a single hierarchy. From dcce6d78ee199de3c5acdaea8745213871ffe94e Mon Sep 17 00:00:00 2001 From: Carl Hembrough Date: Mon, 5 Feb 2018 15:27:34 +0000 Subject: [PATCH 27/41] Fix query to update search import state --- mongo/instance_store.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mongo/instance_store.go b/mongo/instance_store.go index d32a5d3a..4ddbad3e 100644 --- a/mongo/instance_store.go +++ b/mongo/instance_store.go @@ -192,11 +192,11 @@ func (m *Mongo) UpdateBuildSearchTaskState(id, dimension, state string) (err err selector := bson.M{ "id": id, - "import_tasks.build_search.dimension_name": dimension, + "import_tasks.build_search_indexes.dimension_name": dimension, } update := bson.M{ - "$set": bson.M{"import_tasks.build_search.$.state": state}, + "$set": bson.M{"import_tasks.build_search_indexes.$.state": state}, "$currentDate": bson.M{"last_updated": true}, } From b50dfcea3b9856a611b1b28f7e0a98391ffdedd0 Mon Sep 17 00:00:00 2001 From: mikeAdams Date: Mon, 5 Feb 2018 15:34:28 +0000 Subject: [PATCH 28/41] Updated copyright year --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 7e913907..3d404ac5 100644 --- a/README.md +++ b/README.md @@ -65,6 +65,6 @@ See [CONTRIBUTING](CONTRIBUTING.md) for details. ### License -Copyright © 2016-2017, Office for National Statistics (https://www.ons.gov.uk) +Copyright © 2016-2018, Office for National Statistics (https://www.ons.gov.uk) Released under MIT license, see [LICENSE](LICENSE.md) for details From 4e4f876509d9ca0af3440cfa4437a593a54a5d46 Mon Sep 17 00:00:00 2001 From: Eleanor Deal Date: Mon, 5 Feb 2018 17:08:35 +0000 Subject: [PATCH 29/41] Fail dimension update if dimension is not found --- apierrors/errors.go | 1 + instance/instance.go | 11 ++++++-- instance/instance_external_test.go | 44 ++++++++++++++++++++++++++---- 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/apierrors/errors.go b/apierrors/errors.go index 4fd97b8d..a72d43c0 100644 --- a/apierrors/errors.go +++ b/apierrors/errors.go @@ -8,6 +8,7 @@ var ( ErrEditionNotFound = errors.New("Edition not found") ErrVersionNotFound = errors.New("Version not found") ErrDimensionNodeNotFound = errors.New("Dimension node not found") + ErrDimensionNotFound = errors.New("Dimension not found") ErrDimensionsNotFound = errors.New("Dimensions not found") ErrInstanceNotFound = errors.New("Instance not found") ) diff --git a/instance/instance.go b/instance/instance.go index 6f35379a..eba390a5 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -149,11 +149,12 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { } // Update instance-dimension + notFound := true for i := range instance.Dimensions { // For the chosen dimension if instance.Dimensions[i].Name == dimension { - + notFound = false // Assign update info, conditionals to allow updating of both or either without blanking other if dim.Label != "" { instance.Dimensions[i].Label = dim.Label @@ -166,6 +167,12 @@ func (s *Store) UpdateDimension(w http.ResponseWriter, r *http.Request) { } + if notFound { + log.ErrorC("dimension not found", errs.ErrDimensionNotFound, log.Data{"instance": id, "dimension": dimension}) + handleErrorType(errs.ErrDimensionNotFound, w) + return + } + // Update instance if err = s.UpdateInstance(id, instance); err != nil { log.ErrorC("Failed to update instance with new dimension label/description.", err, log.Data{"instance": id, "dimension": dimension}) @@ -537,7 +544,7 @@ func unmarshalInstance(reader io.Reader, post bool) (*models.Instance, error) { func handleErrorType(err error, w http.ResponseWriter) { status := http.StatusInternalServerError - if err == errs.ErrDatasetNotFound || err == errs.ErrEditionNotFound || err == errs.ErrVersionNotFound || err == errs.ErrDimensionNodeNotFound || err == errs.ErrInstanceNotFound { + if err == errs.ErrDatasetNotFound || err == errs.ErrEditionNotFound || err == errs.ErrVersionNotFound || err == errs.ErrDimensionNotFound || err == errs.ErrDimensionNodeNotFound || err == errs.ErrInstanceNotFound { status = http.StatusNotFound } diff --git a/instance/instance_external_test.go b/instance/instance_external_test.go index 1f58559a..2a2a8bd0 100644 --- a/instance/instance_external_test.go +++ b/instance/instance_external_test.go @@ -17,7 +17,7 @@ import ( ) const secretKey = "coffee" -const host = "http://locahost:8080" +const host = "http://localhost:8080" var internalError = errors.New("internal error") @@ -600,7 +600,6 @@ func TestStore_UpdateImportTask_ReturnsInternalError(t *testing.T) { }) } - func TestUpdateDimensionReturnsNotFound(t *testing.T) { t.Parallel() Convey("When update dimension return status not found", t, func() { @@ -667,16 +666,48 @@ func TestUpdateDimensionReturnsBadRequest(t *testing.T) { }) } +func TestUpdateDimensionReturnsNotFoundWithWrongName(t *testing.T) { + t.Parallel() + Convey("When update dimension fails to update an instance", t, func() { + body := strings.NewReader(`{"label":"notages"}`) + r := createRequestWithToken("PUT", "http://localhost:22000/instances/123/dimensions/notage", body) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + GetInstanceFunc: func(id string) (*models.Instance, error) { + return &models.Instance{State: models.EditionConfirmedState, + InstanceID: "123", + Dimensions: []models.CodeList{{Name: "age", ID: "age"}}}, nil + }, + UpdateInstanceFunc: func(id string, i *models.Instance) error { + return nil + }, + } + + instance := &instance.Store{Host: host, Storer: mockedDataStore} + router := mux.NewRouter() + router.HandleFunc("/instances/{id}/dimensions/{dimension}", instance.UpdateDimension).Methods("PUT") + + router.ServeHTTP(w, r) + + So(w.Code, ShouldEqual, http.StatusNotFound) + So(len(mockedDataStore.GetInstanceCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateInstanceCalls()), ShouldEqual, 0) + }) +} + func TestUpdateDimensionReturnsOk(t *testing.T) { t.Parallel() Convey("When update dimension fails to update an instance", t, func() { - body := strings.NewReader("{}") + body := strings.NewReader(`{"label":"ages"}`) r := createRequestWithToken("PUT", "http://localhost:22000/instances/123/dimensions/age", body) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ GetInstanceFunc: func(id string) (*models.Instance, error) { - return &models.Instance{}, nil + return &models.Instance{State: models.EditionConfirmedState, + InstanceID: "123", + Dimensions: []models.CodeList{{Name: "age", ID: "age"}}}, nil }, UpdateInstanceFunc: func(id string, i *models.Instance) error { return nil @@ -684,7 +715,10 @@ func TestUpdateDimensionReturnsOk(t *testing.T) { } instance := &instance.Store{Host: host, Storer: mockedDataStore} - instance.UpdateDimension(w, r) + router := mux.NewRouter() + router.HandleFunc("/instances/{id}/dimensions/{dimension}", instance.UpdateDimension).Methods("PUT") + + router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusOK) So(len(mockedDataStore.GetInstanceCalls()), ShouldEqual, 1) From e42b8e0a7d0aa241f3d3bb14624ee9458bcd8dd0 Mon Sep 17 00:00:00 2001 From: Carl Hembrough Date: Tue, 6 Feb 2018 09:54:07 +0000 Subject: [PATCH 30/41] Add search index task model to swagger spec --- swagger.yaml | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/swagger.yaml b/swagger.yaml index 8b5bf1bf..895c4366 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -1191,6 +1191,17 @@ definitions: code_list_id: type: string description: "The ID of the codelist that this hierarchy represents" + build_search_indexes: + type: array + items: + type: object + properties: + state: + type: string + description: "The state of the import observations task" + dimension_name: + type: string + description: "The name of the dimension the search index represents" Codelist: type: object properties: @@ -1802,4 +1813,4 @@ definitions: type: string href: description: "A URL to the version this resource relates to" - type: string \ No newline at end of file + type: string From 1dd659eed2d217d8c63ef508245595757f311519 Mon Sep 17 00:00:00 2001 From: Carl Hembrough Date: Tue, 6 Feb 2018 10:25:07 +0000 Subject: [PATCH 31/41] Add tests for updating the state of a build search index task --- instance/instance_external_test.go | 50 ++++++++++++++++++++++++++++++ instance/instance_internal_test.go | 22 ++++++++++++- 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/instance/instance_external_test.go b/instance/instance_external_test.go index 3ccbf40d..a37b31fc 100644 --- a/instance/instance_external_test.go +++ b/instance/instance_external_test.go @@ -599,3 +599,53 @@ func TestStore_UpdateImportTask_ReturnsInternalError(t *testing.T) { So(len(mockedDataStore.UpdateImportObservationsTaskStateCalls()), ShouldEqual, 1) }) } + +func TestStore_UpdateImportTask_UpdateBuildSearchIndexTask_InvalidState(t *testing.T) { + + t.Parallel() + Convey("update to an import task with an invalid state returns http 400 response", t, func() { + body := strings.NewReader(`{"build_search_indexes":[{"state":"notvalid"}]}`) + r := createRequestWithToken("PUT", "http://localhost:21800/instances/123/import_tasks", body) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + UpdateBuildSearchTaskStateFunc: func(id string, dimension string, state string) error { + return nil + }, + } + + instance := &instance.Store{Host: host, Storer: mockedDataStore} + + instance.UpdateImportTask(w, r) + + So(w.Code, ShouldEqual, http.StatusBadRequest) + So(len(mockedDataStore.UpdateImportObservationsTaskStateCalls()), ShouldEqual, 0) + So(len(mockedDataStore.UpdateBuildHierarchyTaskStateCalls()), ShouldEqual, 0) + So(len(mockedDataStore.UpdateBuildSearchTaskStateCalls()), ShouldEqual, 0) + }) +} + +func TestStore_UpdateImportTask_UpdateBuildSearchIndexTask(t *testing.T) { + + t.Parallel() + Convey("update to an import task returns http 200 response if no errors occur", t, func() { + body := strings.NewReader(`{"build_search_indexes":[{"state":"completed"}]}`) + r := createRequestWithToken("PUT", "http://localhost:21800/instances/123/import_tasks", body) + w := httptest.NewRecorder() + + mockedDataStore := &storetest.StorerMock{ + UpdateBuildSearchTaskStateFunc: func(id string, dimension string, state string) error { + return nil + }, + } + + instance := &instance.Store{Host: host, Storer: mockedDataStore} + + instance.UpdateImportTask(w, r) + + So(w.Code, ShouldEqual, http.StatusOK) + So(len(mockedDataStore.UpdateImportObservationsTaskStateCalls()), ShouldEqual, 0) + So(len(mockedDataStore.UpdateBuildHierarchyTaskStateCalls()), ShouldEqual, 0) + So(len(mockedDataStore.UpdateBuildSearchTaskStateCalls()), ShouldEqual, 1) + }) +} diff --git a/instance/instance_internal_test.go b/instance/instance_internal_test.go index dc6f9c72..71b184ad 100644 --- a/instance/instance_internal_test.go +++ b/instance/instance_internal_test.go @@ -115,7 +115,7 @@ func TestUnmarshalImportTaskWithInvalidJson(t *testing.T) { }) } -func TestUnmarshalImportTask(t *testing.T) { +func TestUnmarshalImportTask_ImportObservations(t *testing.T) { Convey("Create an import observation task with valid json", t, func() { task, err := unmarshalImportTasks(strings.NewReader(`{"import_observations":{"state":"completed"}}`)) So(err, ShouldBeNil) @@ -124,3 +124,23 @@ func TestUnmarshalImportTask(t *testing.T) { So(task.ImportObservations.State, ShouldEqual, "completed") }) } + +func TestUnmarshalImportTask_BuildHierarchies(t *testing.T) { + Convey("Create an import observation task with valid json", t, func() { + task, err := unmarshalImportTasks(strings.NewReader(`{"build_hierarchies":[{"state":"completed"}]}`)) + So(err, ShouldBeNil) + So(task, ShouldNotBeNil) + So(task.BuildHierarchyTasks, ShouldNotBeNil) + So(task.BuildHierarchyTasks[0].State, ShouldEqual, "completed") + }) +} + +func TestUnmarshalImportTask_BuildSearch(t *testing.T) { + Convey("Create an import observation task with valid json", t, func() { + task, err := unmarshalImportTasks(strings.NewReader(`{"build_search_indexes":[{"state":"completed"}]}`)) + So(err, ShouldBeNil) + So(task, ShouldNotBeNil) + So(task.BuildSearchIndexTasks, ShouldNotBeNil) + So(task.BuildSearchIndexTasks[0].State, ShouldEqual, "completed") + }) +} From 3d14f4b27dae04367974c28d7ac4941f667ecf19 Mon Sep 17 00:00:00 2001 From: Carl Hembrough Date: Tue, 6 Feb 2018 10:56:25 +0000 Subject: [PATCH 32/41] Add custom error messages for http responses instead of err.error() --- instance/instance.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/instance/instance.go b/instance/instance.go index 07760236..ee903e82 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -378,7 +378,7 @@ func (s *Store) UpdateImportTask(w http.ResponseWriter, r *http.Request) { validationErrs = append(validationErrs, fmt.Errorf("bad request - invalid task state value for import observations: %v", tasks.ImportObservations.State)) } else if err := s.UpdateImportObservationsTaskState(id, tasks.ImportObservations.State); err != nil { log.Error(err, nil) - http.Error(w, err.Error(), http.StatusInternalServerError) + http.Error(w, "Failed to update import observations task state", http.StatusInternalServerError) return } } @@ -391,7 +391,7 @@ func (s *Store) UpdateImportTask(w http.ResponseWriter, r *http.Request) { validationErrs = append(validationErrs, fmt.Errorf("bad request - invalid task state value: %v", task.State)) } else if err := s.UpdateBuildHierarchyTaskState(id, task.DimensionName, task.State); err != nil { log.Error(err, nil) - http.Error(w, err.Error(), http.StatusInternalServerError) + http.Error(w, "Failed to update build hierarchy task state", http.StatusInternalServerError) return } } @@ -405,7 +405,7 @@ func (s *Store) UpdateImportTask(w http.ResponseWriter, r *http.Request) { validationErrs = append(validationErrs, fmt.Errorf("bad request - invalid task state value: %v", task.State)) } else if err := s.UpdateBuildSearchTaskState(id, task.DimensionName, task.State); err != nil { log.Error(err, nil) - http.Error(w, err.Error(), http.StatusInternalServerError) + http.Error(w, "Failed to update build search index task state", http.StatusInternalServerError) return } } From 5f2d59031922c13dd35b03d80569ccbbee832f9b Mon Sep 17 00:00:00 2001 From: nshumoogum Date: Tue, 6 Feb 2018 18:53:27 +0000 Subject: [PATCH 33/41] Use check function on PUT version endpoint Check function returns a status of forbidden if state is published --- api/api.go | 15 +++---- api/dataset.go | 50 +++++++++++++++++++---- api/dataset_test.go | 97 +++++++++++++++++++++----------------------- apierrors/errors.go | 1 + instance/instance.go | 8 ++-- models/dataset.go | 3 ++ 6 files changed, 104 insertions(+), 70 deletions(-) diff --git a/api/api.go b/api/api.go index ccad57d0..8953dcb3 100644 --- a/api/api.go +++ b/api/api.go @@ -70,6 +70,7 @@ func routes(host, secretKey string, router *mux.Router, dataStore store.DataStor api.router.HandleFunc("/healthcheck", api.healthCheck).Methods("GET") + versionPublishChecker := PublishCheck{Datastore: dataStore.Backend} api.router.HandleFunc("/datasets", api.getDatasets).Methods("GET") api.router.HandleFunc("/datasets/{id}", api.privateAuth.Check(api.addDataset)).Methods("POST") api.router.HandleFunc("/datasets/{id}", api.getDataset).Methods("GET") @@ -78,28 +79,28 @@ func routes(host, secretKey string, router *mux.Router, dataStore store.DataStor api.router.HandleFunc("/datasets/{id}/editions/{edition}", api.getEdition).Methods("GET") api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions", api.getVersions).Methods("GET") api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}", api.getVersion).Methods("GET") - api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}", api.privateAuth.Check(api.putVersion)).Methods("PUT") + api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}", api.privateAuth.Check(versionPublishChecker.Check(api.putVersion))).Methods("PUT") api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}/metadata", api.getMetadata).Methods("GET") api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}/dimensions", api.getDimensions).Methods("GET") api.router.HandleFunc("/datasets/{id}/editions/{edition}/versions/{version}/dimensions/{dimension}/options", api.getDimensionOptions).Methods("GET") instanceAPI := instance.Store{Host: api.host, Storer: api.dataStore.Backend} - publishChecker := instance.InstancePublishCheck{Datastore: dataStore.Backend} + instancePublishChecker := instance.PublishCheck{Datastore: dataStore.Backend} api.router.HandleFunc("/instances", api.privateAuth.Check(instanceAPI.GetList)).Methods("GET") api.router.HandleFunc("/instances", api.privateAuth.Check(instanceAPI.Add)).Methods("POST") api.router.HandleFunc("/instances/{id}", api.privateAuth.Check(instanceAPI.Get)).Methods("GET") - api.router.HandleFunc("/instances/{id}", api.privateAuth.Check(publishChecker.Check(instanceAPI.Update))).Methods("PUT") + api.router.HandleFunc("/instances/{id}", api.privateAuth.Check(instancePublishChecker.Check(instanceAPI.Update))).Methods("PUT") api.router.HandleFunc("/instances/{id}/events", api.privateAuth.Check(instanceAPI.AddEvent)).Methods("POST") api.router.HandleFunc("/instances/{id}/inserted_observations/{inserted_observations}", - api.privateAuth.Check(publishChecker.Check(instanceAPI.UpdateObservations))).Methods("PUT") - api.router.HandleFunc("/instances/{id}/import_tasks", api.privateAuth.Check(publishChecker.Check(instanceAPI.UpdateImportTask))).Methods("PUT") + api.privateAuth.Check(instancePublishChecker.Check(instanceAPI.UpdateObservations))).Methods("PUT") + api.router.HandleFunc("/instances/{id}/import_tasks", api.privateAuth.Check(instancePublishChecker.Check(instanceAPI.UpdateImportTask))).Methods("PUT") dimension := dimension.Store{Storer: api.dataStore.Backend} api.router.HandleFunc("/instances/{id}/dimensions", dimension.GetNodes).Methods("GET") - api.router.HandleFunc("/instances/{id}/dimensions", api.privateAuth.Check(publishChecker.Check(dimension.Add))).Methods("POST") + api.router.HandleFunc("/instances/{id}/dimensions", api.privateAuth.Check(instancePublishChecker.Check(dimension.Add))).Methods("POST") api.router.HandleFunc("/instances/{id}/dimensions/{dimension}/options", dimension.GetUnique).Methods("GET") api.router.HandleFunc("/instances/{id}/dimensions/{dimension}/options/{value}/node_id/{node_id}", - api.privateAuth.Check(publishChecker.Check(dimension.AddNodeID))).Methods("PUT") + api.privateAuth.Check(instancePublishChecker.Check(dimension.AddNodeID))).Methods("PUT") return &api } diff --git a/api/dataset.go b/api/dataset.go index e7891582..c79a12a0 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -8,6 +8,7 @@ import ( errs "github.com/ONSdigital/dp-dataset-api/apierrors" "github.com/ONSdigital/dp-dataset-api/models" + "github.com/ONSdigital/dp-dataset-api/store" "github.com/ONSdigital/go-ns/log" "github.com/gorilla/mux" "github.com/pkg/errors" @@ -396,12 +397,12 @@ func (api *DatasetAPI) putVersion(w http.ResponseWriter, r *http.Request) { version := vars["version"] versionDoc, err := models.CreateVersion(r.Body) + defer r.Body.Close() if err != nil { log.ErrorC("failed to model version resource based on request", err, log.Data{"dataset_id": datasetID, "edition": edition, "version": version}) http.Error(w, err.Error(), http.StatusBadRequest) return } - defer r.Body.Close() if err = api.dataStore.Backend.CheckDatasetExists(datasetID, ""); err != nil { log.ErrorC("failed to find dataset", err, log.Data{"dataset_id": datasetID, "edition": edition, "version": version}) @@ -422,14 +423,6 @@ func (api *DatasetAPI) putVersion(w http.ResponseWriter, r *http.Request) { return } - // Check current state of version document - if currentVersion.State == models.PublishedState { - err = fmt.Errorf("unable to update document, already published") - log.Error(err, nil) - http.Error(w, err.Error(), http.StatusForbidden) - return - } - // Combine update version document to existing version document newVersion := createNewVersionDoc(currentVersion, versionDoc) log.Debug("combined current version document with update request", log.Data{"dataset_id": datasetID, "edition": edition, "version": version, "updated_version": newVersion}) @@ -896,6 +889,45 @@ func handleErrorType(docType string, err error, w http.ResponseWriter) { } } +// PublishCheck Checks if an version has been published +type PublishCheck struct { + Datastore store.Storer +} + +// Check wraps a HTTP handle. Checks that the state is not published +func (d *PublishCheck) Check(handle func(http.ResponseWriter, *http.Request)) http.HandlerFunc { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + + vars := mux.Vars(r) + id := vars["id"] + edition := vars["edition"] + version := vars["version"] + + versionDoc, err := d.Datastore.GetVersion(id, edition, version, "") + if err != nil { + if err != errs.ErrVersionNotFound { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + // If document cannot be found do not handle error + handle(w, r) + return + } + + if versionDoc != nil { + if versionDoc.State == models.PublishedState { + err = errors.New("unable to update version as it has been published") + log.Error(err, log.Data{"version": versionDoc}) + http.Error(w, err.Error(), http.StatusForbidden) + return + } + } + + handle(w, r) + return + }) +} + func setJSONContentType(w http.ResponseWriter) { w.Header().Set("Content-Type", "application/json") } diff --git a/api/dataset_test.go b/api/dataset_test.go index beee92e3..bd1a07f9 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -13,16 +13,16 @@ import ( "gopkg.in/mgo.v2/bson" - errs "github.com/ONSdigital/dp-dataset-api/apierrors" "github.com/ONSdigital/dp-dataset-api/mocks" "github.com/ONSdigital/dp-dataset-api/models" "github.com/ONSdigital/dp-dataset-api/store" "github.com/ONSdigital/dp-dataset-api/store/datastoretest" - "github.com/ONSdigital/go-ns/log" + "github.com/ian-kent/go-log/log" "github.com/ONSdigital/dp-dataset-api/url" "github.com/gorilla/mux" + errs "github.com/ONSdigital/dp-dataset-api/apierrors" . "github.com/smartystreets/goconvey/convey" ) @@ -930,7 +930,7 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { So(w.Code, ShouldEqual, http.StatusOK) So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) - So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 3) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 2) So(len(mockedDataStore.UpdateEditionCalls()), ShouldEqual, 0) So(len(mockedDataStore.UpsertDatasetCalls()), ShouldEqual, 0) @@ -981,7 +981,7 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { So(w.Code, ShouldEqual, http.StatusOK) So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) - So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 3) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 2) So(len(mockedDataStore.UpdateDatasetWithAssociationCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateEditionCalls()), ShouldEqual, 0) @@ -1041,7 +1041,7 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { So(w.Code, ShouldEqual, http.StatusOK) So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) - So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateDatasetWithAssociationCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateEditionCalls()), ShouldEqual, 0) @@ -1124,7 +1124,7 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { So(w.Code, ShouldEqual, http.StatusOK) So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) - So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 3) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 2) So(len(mockedDataStore.UpdateEditionCalls()), ShouldEqual, 2) So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 2) @@ -1188,7 +1188,7 @@ func TestPutVersionGenerateDownloadsError(t *testing.T) { So(mockedDataStore.CheckEditionExistsCalls()[0].ID, ShouldEqual, "123") So(mockedDataStore.CheckEditionExistsCalls()[0].EditionID, ShouldEqual, "2017") - So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) So(mockedDataStore.GetVersionCalls()[0].DatasetID, ShouldEqual, "123") So(mockedDataStore.GetVersionCalls()[0].EditionID, ShouldEqual, "2017") So(mockedDataStore.GetVersionCalls()[0].Version, ShouldEqual, "1") @@ -1239,6 +1239,7 @@ func TestPutEmptyVersion(t *testing.T) { }) Convey("and the updated version is as expected", func() { + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 1) So(mockedDataStore.UpdateVersionCalls()[0].Version.Downloads, ShouldBeNil) }) @@ -1291,7 +1292,7 @@ func TestPutEmptyVersion(t *testing.T) { So(mockedDataStore.CheckEditionExistsCalls()[0].EditionID, ShouldEqual, "2017") So(mockedDataStore.CheckEditionExistsCalls()[0].State, ShouldEqual, "") - So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) So(mockedDataStore.GetVersionCalls()[0].DatasetID, ShouldEqual, "123") So(mockedDataStore.GetVersionCalls()[0].EditionID, ShouldEqual, "2017") So(mockedDataStore.GetVersionCalls()[0].Version, ShouldEqual, "1") @@ -1322,7 +1323,10 @@ func TestPutVersionReturnsError(t *testing.T) { w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ GetVersionFunc: func(string, string, string, string) (*models.Version, error) { - return &models.Version{}, nil + return &models.Version{State: models.AssociatedState}, nil + }, + CheckDatasetExistsFunc: func(string, string) error { + return nil }, } @@ -1330,7 +1334,8 @@ func TestPutVersionReturnsError(t *testing.T) { api.router.ServeHTTP(w, r) So(w.Body.String(), ShouldEqual, "Failed to parse json body\n") So(w.Code, ShouldEqual, http.StatusBadRequest) - So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 0) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 0) So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) }) @@ -1348,10 +1353,10 @@ func TestPutVersionReturnsError(t *testing.T) { So(err, ShouldBeNil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - CheckDatasetExistsFunc: func(string, string) error { - return errInternal + GetVersionFunc: func(string, string, string, string) (*models.Version, error) { + return nil, errInternal }, - CheckEditionExistsFunc: func(string, string, string) error { + CheckDatasetExistsFunc: func(string, string) error { return nil }, } @@ -1360,8 +1365,8 @@ func TestPutVersionReturnsError(t *testing.T) { api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusInternalServerError) So(w.Body.String(), ShouldEqual, "internal error\n") - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) - So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 0) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) + So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 0) So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) }) @@ -1379,6 +1384,9 @@ func TestPutVersionReturnsError(t *testing.T) { So(err, ShouldBeNil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ + GetVersionFunc: func(string, string, string, string) (*models.Version, error) { + return &models.Version{}, errs.ErrVersionNotFound + }, CheckDatasetExistsFunc: func(string, string) error { return errs.ErrDatasetNotFound }, @@ -1391,6 +1399,7 @@ func TestPutVersionReturnsError(t *testing.T) { api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusBadRequest) So(w.Body.String(), ShouldEqual, "Dataset not found\n") + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 0) So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) @@ -1410,6 +1419,9 @@ func TestPutVersionReturnsError(t *testing.T) { So(err, ShouldBeNil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ + GetVersionFunc: func(string, string, string, string) (*models.Version, error) { + return &models.Version{}, errs.ErrVersionNotFound + }, CheckDatasetExistsFunc: func(string, string) error { return nil }, @@ -1422,9 +1434,9 @@ func TestPutVersionReturnsError(t *testing.T) { api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusBadRequest) So(w.Body.String(), ShouldEqual, "Edition not found\n") + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) - So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 0) So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) }) @@ -1442,15 +1454,15 @@ func TestPutVersionReturnsError(t *testing.T) { So(err, ShouldBeNil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ + GetVersionFunc: func(string, string, string, string) (*models.Version, error) { + return &models.Version{}, errs.ErrVersionNotFound + }, CheckDatasetExistsFunc: func(string, string) error { return nil }, CheckEditionExistsFunc: func(string, string, string) error { return nil }, - GetVersionFunc: func(string, string, string, string) (*models.Version, error) { - return nil, errs.ErrVersionNotFound - }, UpdateVersionFunc: func(string, *models.Version) error { return nil }, @@ -1460,9 +1472,9 @@ func TestPutVersionReturnsError(t *testing.T) { api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusNotFound) So(w.Body.String(), ShouldEqual, "Version not found\n") + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) - So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 0) So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) }) @@ -1480,8 +1492,10 @@ func TestPutVersionReturnsError(t *testing.T) { So(err, ShouldBeNil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - CheckDatasetExistsFunc: func(string, string) error { - return nil + GetVersionFunc: func(string, string, string, string) (*models.Version, error) { + return &models.Version{ + State: "associated", + }, nil }, } @@ -1489,7 +1503,7 @@ func TestPutVersionReturnsError(t *testing.T) { api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusUnauthorized) So(w.Body.String(), ShouldEqual, "No authentication header provided\n") - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 0) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 0) So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) }) @@ -1507,18 +1521,12 @@ func TestPutVersionReturnsError(t *testing.T) { So(err, ShouldBeNil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - CheckDatasetExistsFunc: func(string, string) error { - return nil - }, - CheckEditionExistsFunc: func(string, string, string) error { - return nil - }, GetVersionFunc: func(string, string, string, string) (*models.Version, error) { return &models.Version{ State: models.PublishedState, }, nil }, - UpdateVersionFunc: func(string, *models.Version) error { + CheckDatasetExistsFunc: func(string, string) error { return nil }, } @@ -1526,11 +1534,9 @@ func TestPutVersionReturnsError(t *testing.T) { api := GetAPIWithMockedDatastore(mockedDataStore, generatorMock) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusForbidden) - So(w.Body.String(), ShouldEqual, "unable to update document, already published\n") - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) - So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) + So(w.Body.String(), ShouldEqual, "unable to update version as it has been published\n") So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) - So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 0) + So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 0) }) Convey("When the version document has already been published return status forbidden", t, func() { @@ -1547,18 +1553,12 @@ func TestPutVersionReturnsError(t *testing.T) { So(err, ShouldBeNil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - CheckDatasetExistsFunc: func(string, string) error { - return nil - }, - CheckEditionExistsFunc: func(string, string, string) error { - return nil - }, GetVersionFunc: func(string, string, string, string) (*models.Version, error) { return &models.Version{ State: models.PublishedState, }, nil }, - UpdateVersionFunc: func(string, *models.Version) error { + CheckDatasetExistsFunc: func(string, string) error { return nil }, } @@ -1566,12 +1566,9 @@ func TestPutVersionReturnsError(t *testing.T) { api := GetAPIWithMockedDatastore(mockedDataStore, generatorMock) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusForbidden) - So(w.Body.String(), ShouldEqual, "unable to update document, already published\n") - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) - So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) + So(w.Body.String(), ShouldEqual, "unable to update version as it has been published\n") So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) - So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 0) - So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) + So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 0) }) Convey("When the request body is invalid return status bad request", t, func() { @@ -1588,15 +1585,15 @@ func TestPutVersionReturnsError(t *testing.T) { So(err, ShouldBeNil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ + GetVersionFunc: func(string, string, string, string) (*models.Version, error) { + return &models.Version{State: "associated"}, nil + }, CheckDatasetExistsFunc: func(string, string) error { return nil }, CheckEditionExistsFunc: func(string, string, string) error { return nil }, - GetVersionFunc: func(string, string, string, string) (*models.Version, error) { - return &models.Version{}, nil - }, UpdateVersionFunc: func(string, *models.Version) error { return nil }, @@ -1606,9 +1603,9 @@ func TestPutVersionReturnsError(t *testing.T) { api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusBadRequest) So(w.Body.String(), ShouldEqual, "Missing collection_id for association between version and a collection\n") + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) - So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 0) So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) }) diff --git a/apierrors/errors.go b/apierrors/errors.go index 4fd97b8d..0d5081cd 100644 --- a/apierrors/errors.go +++ b/apierrors/errors.go @@ -10,4 +10,5 @@ var ( ErrDimensionNodeNotFound = errors.New("Dimension node not found") ErrDimensionsNotFound = errors.New("Dimensions not found") ErrInstanceNotFound = errors.New("Instance not found") + ErrInternalServerError = errors.New("") ) diff --git a/instance/instance.go b/instance/instance.go index 8245c340..279d587e 100644 --- a/instance/instance.go +++ b/instance/instance.go @@ -491,13 +491,13 @@ func writeBody(w http.ResponseWriter, bytes []byte) { } } -// InstancePublishCheck Checks if an instance has been published -type InstancePublishCheck struct { +// PublishCheck Checks if an instance has been published +type PublishCheck struct { Datastore store.Storer } // Check wraps a HTTP handle. Checks that the state is not published -func (d *InstancePublishCheck) Check(handle func(http.ResponseWriter, *http.Request)) http.HandlerFunc { +func (d *PublishCheck) Check(handle func(http.ResponseWriter, *http.Request)) http.HandlerFunc { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) @@ -514,8 +514,8 @@ func (d *InstancePublishCheck) Check(handle func(http.ResponseWriter, *http.Requ log.Error(err, log.Data{"instance": instance}) http.Error(w, err.Error(), http.StatusForbidden) return - } + handle(w, r) }) } diff --git a/models/dataset.go b/models/dataset.go index df05bd3b..d25d5c55 100644 --- a/models/dataset.go +++ b/models/dataset.go @@ -8,6 +8,7 @@ import ( "strconv" "time" + "github.com/ONSdigital/go-ns/log" "github.com/pkg/errors" "github.com/satori/go.uuid" ) @@ -212,6 +213,8 @@ func CreateVersion(reader io.Reader) (*Version, error) { return nil, errors.New("Failed to read message body") } + log.Info("version", log.Data{"bytes": string(bytes)}) + var version Version // Create unique id version.ID = uuid.NewV4().String() From e6f76539fa3ca6f8ed241e6c226098a68756193c Mon Sep 17 00:00:00 2001 From: nshumoogum Date: Wed, 7 Feb 2018 11:34:28 +0000 Subject: [PATCH 34/41] Minor fixes Includes: * Remove unnecessary return * Use correct logging package, `go-ns/log` * Remove unused error message from apierrors package * bindAddr no longer hidden * Update a unit test * Remove log line (accidently commited) --- api/dataset.go | 1 - api/dataset_test.go | 2 +- apierrors/errors.go | 1 - config/config.go | 2 +- instance/instance_external_test.go | 8 ++++---- models/dataset.go | 3 --- 6 files changed, 6 insertions(+), 11 deletions(-) diff --git a/api/dataset.go b/api/dataset.go index 5f81f2c7..82eb6b18 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -928,7 +928,6 @@ func (d *PublishCheck) Check(handle func(http.ResponseWriter, *http.Request)) ht } handle(w, r) - return }) } diff --git a/api/dataset_test.go b/api/dataset_test.go index bd1a07f9..347b252a 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -17,7 +17,7 @@ import ( "github.com/ONSdigital/dp-dataset-api/models" "github.com/ONSdigital/dp-dataset-api/store" "github.com/ONSdigital/dp-dataset-api/store/datastoretest" - "github.com/ian-kent/go-log/log" + "github.com/ONSdigital/go-ns/log" "github.com/ONSdigital/dp-dataset-api/url" "github.com/gorilla/mux" diff --git a/apierrors/errors.go b/apierrors/errors.go index f1849531..a72d43c0 100644 --- a/apierrors/errors.go +++ b/apierrors/errors.go @@ -11,5 +11,4 @@ var ( ErrDimensionNotFound = errors.New("Dimension not found") ErrDimensionsNotFound = errors.New("Dimensions not found") ErrInstanceNotFound = errors.New("Instance not found") - ErrInternalServerError = errors.New("") ) diff --git a/config/config.go b/config/config.go index 4c2bcf02..260997d8 100644 --- a/config/config.go +++ b/config/config.go @@ -9,7 +9,7 @@ import ( // Configuration structure which hold information for configuring the import API type Configuration struct { - BindAddr string `envconfig:"BIND_ADDR" json:"-"` + BindAddr string `envconfig:"BIND_ADDR"` KafkaAddr []string `envconfig:"KAFKA_ADDR" json:"-"` GenerateDownloadsTopic string `envconfig:"GENERATE_DOWNLOADS_TOPIC"` CodeListAPIURL string `envconfig:"CODE_LIST_API_URL"` diff --git a/instance/instance_external_test.go b/instance/instance_external_test.go index 6e141dc5..6a90c31e 100644 --- a/instance/instance_external_test.go +++ b/instance/instance_external_test.go @@ -374,7 +374,7 @@ func TestUpdatePublishedInstanceToCompletedReturnsForbidden(t *testing.T) { return currentInstanceTestData, nil }, UpdateInstanceFunc: func(id string, i *models.Instance) error { - return internalError + return nil }, } @@ -387,7 +387,7 @@ func TestUpdatePublishedInstanceToCompletedReturnsForbidden(t *testing.T) { }) } -func TestUpdateCompletedInstanceToCompletedReturnsForbidden(t *testing.T) { +func TestUpdateEditionConfirmedInstanceToCompletedReturnsForbidden(t *testing.T) { t.Parallel() Convey("update to an instance returns an internal error", t, func() { body := strings.NewReader(`{"state":"completed"}`) @@ -401,7 +401,7 @@ func TestUpdateCompletedInstanceToCompletedReturnsForbidden(t *testing.T) { ID: "4567", }, }, - State: models.PublishedState, + State: models.EditionConfirmedState, } mockedDataStore := &storetest.StorerMock{ @@ -409,7 +409,7 @@ func TestUpdateCompletedInstanceToCompletedReturnsForbidden(t *testing.T) { return currentInstanceTestData, nil }, UpdateInstanceFunc: func(id string, i *models.Instance) error { - return internalError + return nil }, } diff --git a/models/dataset.go b/models/dataset.go index d25d5c55..df05bd3b 100644 --- a/models/dataset.go +++ b/models/dataset.go @@ -8,7 +8,6 @@ import ( "strconv" "time" - "github.com/ONSdigital/go-ns/log" "github.com/pkg/errors" "github.com/satori/go.uuid" ) @@ -213,8 +212,6 @@ func CreateVersion(reader io.Reader) (*Version, error) { return nil, errors.New("Failed to read message body") } - log.Info("version", log.Data{"bytes": string(bytes)}) - var version Version // Create unique id version.ID = uuid.NewV4().String() From 5a2bc9ba44f9492b02fdcd613784538d67f16b23 Mon Sep 17 00:00:00 2001 From: Eleanor Deal Date: Wed, 7 Feb 2018 17:55:16 +0000 Subject: [PATCH 35/41] Get dimensions for unpublished version when authenticated --- api/dataset.go | 14 +++++++++++++- api/dataset_test.go | 15 ++++++++++++--- mongo/dimension_store.go | 10 ++-------- store/datastore.go | 2 +- store/datastoretest/datastore.go | 32 ++++++++++---------------------- 5 files changed, 38 insertions(+), 35 deletions(-) diff --git a/api/dataset.go b/api/dataset.go index 82eb6b18..15960099 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -745,7 +745,19 @@ func (api *DatasetAPI) getDimensionOptions(w http.ResponseWriter, r *http.Reques versionID := vars["version"] dimension := vars["dimension"] - results, err := api.dataStore.Backend.GetDimensionOptions(datasetID, editionID, versionID, dimension) + var state string + if r.Header.Get(internalToken) != api.internalToken { + state = models.PublishedState + } + + version, err := api.dataStore.Backend.GetVersion(datasetID, editionID, versionID, state) + if err != nil { + log.ErrorC("failed to get version", err, log.Data{"dataset_id": datasetID, "edition": editionID, "version": versionID}) + handleErrorType(versionDocType, err, w) + return + } + + results, err := api.dataStore.Backend.GetDimensionOptions(version, dimension) if err != nil { log.ErrorC("failed to get a list of dimension options", err, log.Data{"dataset_id": datasetID, "edition": editionID, "version": versionID, "dimension": dimension}) handleErrorType(dimensionOptionDocType, err, w) diff --git a/api/dataset_test.go b/api/dataset_test.go index 347b252a..dac6f68d 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -1699,7 +1699,10 @@ func TestGetDimensionOptionsReturnsOk(t *testing.T) { r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/dimensions/age/options", nil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - GetDimensionOptionsFunc: func(datasetID, editionID, versionID, dimensions string) (*models.DimensionOptionResults, error) { + GetVersionFunc: func(datasetID, edition, version, state string) (*models.Version, error) { + return &models.Version{}, nil + }, + GetDimensionOptionsFunc: func(version *models.Version, dimensions string) (*models.DimensionOptionResults, error) { return &models.DimensionOptionResults{}, nil }, } @@ -1716,7 +1719,10 @@ func TestGetDimensionOptionsReturnsErrors(t *testing.T) { r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/dimensions/age/options", nil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - GetDimensionOptionsFunc: func(datasetID, editionID, versionID, dimensions string) (*models.DimensionOptionResults, error) { + GetVersionFunc: func(datasetID, edition, version, state string) (*models.Version, error) { + return &models.Version{}, nil + }, + GetDimensionOptionsFunc: func(version *models.Version, dimensions string) (*models.DimensionOptionResults, error) { return nil, errs.ErrDatasetNotFound }, } @@ -1730,7 +1736,10 @@ func TestGetDimensionOptionsReturnsErrors(t *testing.T) { r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/dimensions/age/options", nil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - GetDimensionOptionsFunc: func(datasetID, editionID, versionID, dimensions string) (*models.DimensionOptionResults, error) { + GetVersionFunc: func(datasetID, edition, version, state string) (*models.Version, error) { + return &models.Version{}, nil + }, + GetDimensionOptionsFunc: func(version *models.Version, dimensions string) (*models.DimensionOptionResults, error) { return nil, errInternal }, } diff --git a/mongo/dimension_store.go b/mongo/dimension_store.go index 514d2a56..20266abf 100644 --- a/mongo/dimension_store.go +++ b/mongo/dimension_store.go @@ -89,19 +89,13 @@ func (m *Mongo) GetDimensions(datasetID, versionID string) ([]bson.M, error) { } // GetDimensionOptions returns all dimension options for a dimensions within a dataset. -func (m *Mongo) GetDimensionOptions(datasetID, editionID, versionID, dimension string) (*models.DimensionOptionResults, error) { +func (m *Mongo) GetDimensionOptions(version *models.Version, dimension string) (*models.DimensionOptionResults, error) { s := m.Session.Copy() defer s.Close() - version, err := m.GetVersion(datasetID, editionID, versionID, models.PublishedState) - if err != nil { - return nil, err - } - var values []models.PublicDimensionOption iter := s.DB(m.Database).C(dimensionOptions).Find(bson.M{"instance_id": version.ID, "name": dimension}).Iter() - err = iter.All(&values) - if err != nil { + if err := iter.All(&values); err != nil { return nil, err } diff --git a/store/datastore.go b/store/datastore.go index 3c335797..b8d43634 100644 --- a/store/datastore.go +++ b/store/datastore.go @@ -26,7 +26,7 @@ type Storer interface { GetDatasets() ([]models.DatasetUpdate, error) GetDimensionNodesFromInstance(ID string) (*models.DimensionNodeResults, error) GetDimensions(datasetID, versionID string) ([]bson.M, error) - GetDimensionOptions(datasetID, editionID, versionID, dimension string) (*models.DimensionOptionResults, error) + GetDimensionOptions(version *models.Version, dimension string) (*models.DimensionOptionResults, error) GetEdition(ID, editionID, state string) (*models.Edition, error) GetEditions(ID, state string) (*models.EditionResults, error) GetInstances(filters []string) (*models.InstanceResults, error) diff --git a/store/datastoretest/datastore.go b/store/datastoretest/datastore.go index 3ccd3139..07f6539f 100755 --- a/store/datastoretest/datastore.go +++ b/store/datastoretest/datastore.go @@ -76,7 +76,7 @@ var ( // GetDimensionNodesFromInstanceFunc: func(ID string) (*models.DimensionNodeResults, error) { // panic("TODO: mock out the GetDimensionNodesFromInstance method") // }, -// GetDimensionOptionsFunc: func(datasetID string, editionID string, versionID string, dimension string) (*models.DimensionOptionResults, error) { +// GetDimensionOptionsFunc: func(version *models.Version, dimension string) (*models.DimensionOptionResults, error) { // panic("TODO: mock out the GetDimensionOptions method") // }, // GetDimensionsFunc: func(datasetID string, versionID string) ([]bson.M, error) { @@ -180,7 +180,7 @@ type StorerMock struct { GetDimensionNodesFromInstanceFunc func(ID string) (*models.DimensionNodeResults, error) // GetDimensionOptionsFunc mocks the GetDimensionOptions method. - GetDimensionOptionsFunc func(datasetID string, editionID string, versionID string, dimension string) (*models.DimensionOptionResults, error) + GetDimensionOptionsFunc func(version *models.Version, dimension string) (*models.DimensionOptionResults, error) // GetDimensionsFunc mocks the GetDimensions method. GetDimensionsFunc func(datasetID string, versionID string) ([]bson.M, error) @@ -301,12 +301,8 @@ type StorerMock struct { } // GetDimensionOptions holds details about calls to the GetDimensionOptions method. GetDimensionOptions []struct { - // DatasetID is the datasetID argument value. - DatasetID string - // EditionID is the editionID argument value. - EditionID string - // VersionID is the versionID argument value. - VersionID string + // Version is the version argument value. + Version *models.Version // Dimension is the dimension argument value. Dimension string } @@ -742,40 +738,32 @@ func (mock *StorerMock) GetDimensionNodesFromInstanceCalls() []struct { } // GetDimensionOptions calls GetDimensionOptionsFunc. -func (mock *StorerMock) GetDimensionOptions(datasetID string, editionID string, versionID string, dimension string) (*models.DimensionOptionResults, error) { +func (mock *StorerMock) GetDimensionOptions(version *models.Version, dimension string) (*models.DimensionOptionResults, error) { if mock.GetDimensionOptionsFunc == nil { panic("moq: StorerMock.GetDimensionOptionsFunc is nil but Storer.GetDimensionOptions was just called") } callInfo := struct { - DatasetID string - EditionID string - VersionID string + Version *models.Version Dimension string }{ - DatasetID: datasetID, - EditionID: editionID, - VersionID: versionID, + Version: version, Dimension: dimension, } lockStorerMockGetDimensionOptions.Lock() mock.calls.GetDimensionOptions = append(mock.calls.GetDimensionOptions, callInfo) lockStorerMockGetDimensionOptions.Unlock() - return mock.GetDimensionOptionsFunc(datasetID, editionID, versionID, dimension) + return mock.GetDimensionOptionsFunc(version, dimension) } // GetDimensionOptionsCalls gets all the calls that were made to GetDimensionOptions. // Check the length with: // len(mockedStorer.GetDimensionOptionsCalls()) func (mock *StorerMock) GetDimensionOptionsCalls() []struct { - DatasetID string - EditionID string - VersionID string + Version *models.Version Dimension string } { var calls []struct { - DatasetID string - EditionID string - VersionID string + Version *models.Version Dimension string } lockStorerMockGetDimensionOptions.RLock() From 5596e5926125df30ac9cc4e49193be3ad49398e0 Mon Sep 17 00:00:00 2001 From: Eleanor Deal Date: Thu, 8 Feb 2018 10:00:15 +0000 Subject: [PATCH 36/41] Update tests to reflect new check for version --- api/dataset_test.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/api/dataset_test.go b/api/dataset_test.go index dac6f68d..0d95faa1 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -1695,7 +1695,7 @@ func TestGetDimensionsReturnsErrors(t *testing.T) { func TestGetDimensionOptionsReturnsOk(t *testing.T) { t.Parallel() - Convey("", t, func() { + Convey("When a valid dimension is provided then a list of options can be returned successfully", t, func() { r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/dimensions/age/options", nil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ @@ -1715,15 +1715,12 @@ func TestGetDimensionOptionsReturnsOk(t *testing.T) { func TestGetDimensionOptionsReturnsErrors(t *testing.T) { t.Parallel() - Convey("", t, func() { + Convey("When the version doesn't exist in a request for dimension options, then return not found", t, func() { r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/dimensions/age/options", nil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ GetVersionFunc: func(datasetID, edition, version, state string) (*models.Version, error) { - return &models.Version{}, nil - }, - GetDimensionOptionsFunc: func(version *models.Version, dimensions string) (*models.DimensionOptionResults, error) { - return nil, errs.ErrDatasetNotFound + return nil, errs.ErrVersionNotFound }, } @@ -1732,7 +1729,7 @@ func TestGetDimensionOptionsReturnsErrors(t *testing.T) { So(w.Code, ShouldEqual, http.StatusNotFound) }) - Convey("", t, func() { + Convey("When an internal error causes failure to retrieve dimension options, then return internal server error", t, func() { r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123/editions/2017/versions/1/dimensions/age/options", nil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ From c69c47ba5285c1da51f600cdf67edbe9e420e1c7 Mon Sep 17 00:00:00 2001 From: Eleanor Deal Date: Thu, 8 Feb 2018 10:40:35 +0000 Subject: [PATCH 37/41] Fix header key name and remove redundant test --- api/dataset_test.go | 40 ++++------------------------------------ 1 file changed, 4 insertions(+), 36 deletions(-) diff --git a/api/dataset_test.go b/api/dataset_test.go index 0d95faa1..8f74a7ad 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -212,7 +212,7 @@ func TestGetEditionsReturnsError(t *testing.T) { Convey("When the dataset does not exist return status bad request", t, func() { r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123-456/editions", nil) - r.Header.Add("internal_token", "coffee") + r.Header.Add("internal-token", "coffee") w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ CheckDatasetExistsFunc: func(datasetID, state string) error { @@ -229,7 +229,7 @@ func TestGetEditionsReturnsError(t *testing.T) { Convey("When no editions exist against an existing dataset return status not found", t, func() { r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123-456/editions", nil) - r.Header.Add("internal_token", "coffee") + r.Header.Add("internal-token", "coffee") w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ CheckDatasetExistsFunc: func(datasetID, state string) error { @@ -309,7 +309,7 @@ func TestGetEditionReturnsError(t *testing.T) { Convey("When the dataset does not exist return status bad request", t, func() { r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123-456/editions/678", nil) - r.Header.Add("internal_token", "coffee") + r.Header.Add("internal-token", "coffee") w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ CheckDatasetExistsFunc: func(datasetID, state string) error { @@ -326,7 +326,7 @@ func TestGetEditionReturnsError(t *testing.T) { Convey("When edition does not exist for a dataset return status not found", t, func() { r := httptest.NewRequest("GET", "http://localhost:22000/datasets/123-456/editions/678", nil) - r.Header.Add("internal_token", "coffee") + r.Header.Add("internal-token", "coffee") w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ CheckDatasetExistsFunc: func(datasetID, state string) error { @@ -1507,38 +1507,6 @@ func TestPutVersionReturnsError(t *testing.T) { So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) }) - Convey("Given the version doc is 'published', when we try to set state to 'completed', then we see a status of forbidden", t, func() { - generatorMock := &mocks.DownloadsGeneratorMock{ - GenerateFunc: func(string, string, string, string) error { - return nil - }, - } - - var b string - b = versionPayload - r, err := http.NewRequest("PUT", "http://localhost:22000/datasets/123/editions/2017/versions/1", bytes.NewBufferString(b)) - r.Header.Add("internal-token", "coffee") - So(err, ShouldBeNil) - w := httptest.NewRecorder() - mockedDataStore := &storetest.StorerMock{ - GetVersionFunc: func(string, string, string, string) (*models.Version, error) { - return &models.Version{ - State: models.PublishedState, - }, nil - }, - CheckDatasetExistsFunc: func(string, string) error { - return nil - }, - } - - api := GetAPIWithMockedDatastore(mockedDataStore, generatorMock) - api.router.ServeHTTP(w, r) - So(w.Code, ShouldEqual, http.StatusForbidden) - So(w.Body.String(), ShouldEqual, "unable to update version as it has been published\n") - So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 0) - }) - Convey("When the version document has already been published return status forbidden", t, func() { generatorMock := &mocks.DownloadsGeneratorMock{ GenerateFunc: func(string, string, string, string) error { From af0af0f2552fcce169efe93ae30e81aae14962ad Mon Sep 17 00:00:00 2001 From: Eleanor Deal Date: Fri, 9 Feb 2018 10:40:34 +0000 Subject: [PATCH 38/41] Update logging to include authenticated flag --- api/dataset.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/dataset.go b/api/dataset.go index 15960099..cd12694f 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -746,13 +746,15 @@ func (api *DatasetAPI) getDimensionOptions(w http.ResponseWriter, r *http.Reques dimension := vars["dimension"] var state string + authenticated := true if r.Header.Get(internalToken) != api.internalToken { state = models.PublishedState + authenticated = false } version, err := api.dataStore.Backend.GetVersion(datasetID, editionID, versionID, state) if err != nil { - log.ErrorC("failed to get version", err, log.Data{"dataset_id": datasetID, "edition": editionID, "version": versionID}) + log.ErrorC("failed to get version", err, log.Data{"dataset_id": datasetID, "edition": editionID, "version": versionID, "authenticated": authenticated}) handleErrorType(versionDocType, err, w) return } From becd0de8f917772aca76fe93e8a4d27e923f8b12 Mon Sep 17 00:00:00 2001 From: Eleanor Deal Date: Wed, 21 Feb 2018 09:59:32 +0000 Subject: [PATCH 39/41] Simplify dataset link assignment --- api/dataset.go | 55 +++++++++++---------------------------------- api/dataset_test.go | 4 ++-- 2 files changed, 15 insertions(+), 44 deletions(-) diff --git a/api/dataset.go b/api/dataset.go index cd12694f..1ed3fda6 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -311,30 +311,18 @@ func (api *DatasetAPI) addDataset(w http.ResponseWriter, r *http.Request) { defer r.Body.Close() dataset.State = models.CreatedState + dataset.ID = datasetID - var accessRights string - if dataset.Links != nil { - if dataset.Links.AccessRights != nil { - if dataset.Links.AccessRights.HRef != "" { - accessRights = dataset.Links.AccessRights.HRef - } - } + if dataset.Links == nil { + dataset.Links = &models.DatasetLinks{} } - dataset.ID = datasetID - dataset.Links = &models.DatasetLinks{ - Editions: &models.LinkObject{ - HRef: fmt.Sprintf("%s/datasets/%s/editions", api.host, datasetID), - }, - Self: &models.LinkObject{ - HRef: fmt.Sprintf("%s/datasets/%s", api.host, datasetID), - }, - } - - if accessRights != "" { - dataset.Links.AccessRights = &models.LinkObject{ - HRef: accessRights, - } + dataset.Links.Editions = &models.LinkObject{ + HRef: fmt.Sprintf("%s/datasets/%s/editions", api.host, datasetID), + } + + dataset.Links.Self = &models.LinkObject{ + HRef: fmt.Sprintf("%s/datasets/%s", api.host, datasetID), } dataset.LastUpdated = time.Now() @@ -598,30 +586,13 @@ func (api *DatasetAPI) publishDataset(id string, version *models.Version) error return err } - var accessRights string + currentDataset.Next.CollectionID = version.CollectionID - if currentDataset.Next.Links != nil { - if currentDataset.Next.Links.AccessRights != nil { - accessRights = currentDataset.Next.Links.AccessRights.HRef - } + currentDataset.Next.Links.LatestVersion = &models.LinkObject{ + ID: version.Links.Version.ID, + HRef: version.Links.Version.HRef, } - currentDataset.Next.CollectionID = version.CollectionID - currentDataset.Next.Links = &models.DatasetLinks{ - AccessRights: &models.LinkObject{ - HRef: accessRights, - }, - Editions: &models.LinkObject{ - HRef: fmt.Sprintf("%s/datasets/%s/editions", api.host, version.Links.Dataset.ID), - }, - LatestVersion: &models.LinkObject{ - ID: version.Links.Version.ID, - HRef: version.Links.Version.HRef, - }, - Self: &models.LinkObject{ - HRef: fmt.Sprintf("%s/datasets/%s", api.host, version.Links.Dataset.ID), - }, - } currentDataset.Next.State = models.PublishedState currentDataset.Next.LastUpdated = time.Now() diff --git a/api/dataset_test.go b/api/dataset_test.go index 8f74a7ad..c5c61777 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -1105,8 +1105,8 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { return &models.DatasetUpdate{ ID: "123", - Next: &models.Dataset{}, - Current: &models.Dataset{}, + Next: &models.Dataset{Links: &models.DatasetLinks{}}, + Current: &models.Dataset{Links: &models.DatasetLinks{}}, }, nil }, UpsertDatasetFunc: func(string, *models.DatasetUpdate) error { From 23462dd77cd20eeb8bed38653cf70240e5fca807 Mon Sep 17 00:00:00 2001 From: Eleanor Deal Date: Wed, 21 Feb 2018 12:14:06 +0000 Subject: [PATCH 40/41] Update dataset details ensures state changes from published --- api/dataset.go | 9 ++++- api/dataset_test.go | 47 +++++++++++++++-------- mongo/dataset_store.go | 10 +++-- mongo/dataset_test.go | 6 +-- store/datastore.go | 2 +- store/datastoretest/datastore.go | 66 +++++++++++++++++--------------- 6 files changed, 86 insertions(+), 54 deletions(-) diff --git a/api/dataset.go b/api/dataset.go index 1ed3fda6..33bd253f 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -367,7 +367,14 @@ func (api *DatasetAPI) putDataset(w http.ResponseWriter, r *http.Request) { } defer r.Body.Close() - if err := api.dataStore.Backend.UpdateDataset(datasetID, dataset); err != nil { + currentDataset, err := api.dataStore.Backend.GetDataset(datasetID) + if err != nil { + log.ErrorC("failed to find dataset", err, log.Data{"dataset_id": datasetID}) + handleErrorType(datasetDocType, err, w) + return + } + + if err := api.dataStore.Backend.UpdateDataset(datasetID, dataset, currentDataset.Next.State); err != nil { log.ErrorC("failed to update dataset resource", err, log.Data{"dataset_id": datasetID}) handleErrorType(datasetDocType, err, w) return diff --git a/api/dataset_test.go b/api/dataset_test.go index c5c61777..ddd80173 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -648,7 +648,7 @@ func TestPostDatasetsReturnsCreated(t *testing.T) { return nil }, } - mockedDataStore.UpsertDataset("123", &models.DatasetUpdate{}) + mockedDataStore.UpsertDataset("123", &models.DatasetUpdate{Next: &models.Dataset{}}) api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}) api.router.ServeHTTP(w, r) @@ -762,7 +762,10 @@ func TestPutDatasetReturnsSuccessfully(t *testing.T) { So(err, ShouldBeNil) w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - UpdateDatasetFunc: func(string, *models.Dataset) error { + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{Next: &models.Dataset{}}, nil + }, + UpdateDatasetFunc: func(string, *models.Dataset, string) error { return nil }, } @@ -770,11 +773,12 @@ func TestPutDatasetReturnsSuccessfully(t *testing.T) { dataset := &models.Dataset{ Title: "CPI", } - mockedDataStore.UpdateDataset("123", dataset) + mockedDataStore.UpdateDataset("123", dataset, models.CreatedState) api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusOK) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 2) }) } @@ -790,7 +794,10 @@ func TestPutDatasetReturnsError(t *testing.T) { w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - UpdateDatasetFunc: func(string, *models.Dataset) error { + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{Next: &models.Dataset{}}, nil + }, + UpdateDatasetFunc: func(string, *models.Dataset, string) error { return errBadRequest }, } @@ -798,7 +805,8 @@ func TestPutDatasetReturnsError(t *testing.T) { api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusBadRequest) - So(len(mockedDataStore.UpsertVersionCalls()), ShouldEqual, 0) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) + So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 0) }) Convey("When the api cannot connect to datastore return an internal server error", t, func() { @@ -810,7 +818,10 @@ func TestPutDatasetReturnsError(t *testing.T) { w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - UpdateDatasetFunc: func(string, *models.Dataset) error { + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{Next: &models.Dataset{State: models.CreatedState}}, nil + }, + UpdateDatasetFunc: func(string, *models.Dataset, string) error { return errInternal }, } @@ -818,11 +829,12 @@ func TestPutDatasetReturnsError(t *testing.T) { dataset := &models.Dataset{ Title: "CPI", } - mockedDataStore.UpdateDataset("123", dataset) + mockedDataStore.UpdateDataset("123", dataset, models.CreatedState) api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 2) }) @@ -835,20 +847,19 @@ func TestPutDatasetReturnsError(t *testing.T) { w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - UpdateDatasetFunc: func(string, *models.Dataset) error { + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return nil, errs.ErrDatasetNotFound + }, + UpdateDatasetFunc: func(string, *models.Dataset, string) error { return errs.ErrDatasetNotFound }, } - dataset := &models.Dataset{ - Title: "CPI", - } - mockedDataStore.UpdateDataset("123", dataset) - api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusNotFound) - So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 2) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 0) }) Convey("When the request does not contain a valid internal token return status unauthorised", t, func() { @@ -859,7 +870,10 @@ func TestPutDatasetReturnsError(t *testing.T) { w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - UpdateDatasetFunc: func(string, *models.Dataset) error { + GetDatasetFunc: func(string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{Next: &models.Dataset{}}, nil + }, + UpdateDatasetFunc: func(string, *models.Dataset, string) error { return nil }, } @@ -867,6 +881,7 @@ func TestPutDatasetReturnsError(t *testing.T) { api := GetAPIWithMockedDatastore(mockedDataStore, &mocks.DownloadsGeneratorMock{}) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusUnauthorized) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) So(len(mockedDataStore.UpdateDatasetCalls()), ShouldEqual, 0) }) } @@ -1117,7 +1132,7 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { mockedDataStore.UpdateVersion("a1b2c3", &models.Version{}) mockedDataStore.UpdateEdition("123", "2017", &models.Version{State: "published"}) mockedDataStore.GetDataset("123") - mockedDataStore.UpsertDataset("123", &models.DatasetUpdate{}) + mockedDataStore.UpsertDataset("123", &models.DatasetUpdate{Next: &models.Dataset{}}) api := GetAPIWithMockedDatastore(mockedDataStore, generatorMock) api.router.ServeHTTP(w, r) diff --git a/mongo/dataset_store.go b/mongo/dataset_store.go index bc1aac51..a4fd57b1 100644 --- a/mongo/dataset_store.go +++ b/mongo/dataset_store.go @@ -282,11 +282,11 @@ func buildVersionQuery(id, editionID, state string, versionID int) bson.M { } // UpdateDataset updates an existing dataset document -func (m *Mongo) UpdateDataset(id string, dataset *models.Dataset) (err error) { +func (m *Mongo) UpdateDataset(id string, dataset *models.Dataset, currentState string) (err error) { s := m.Session.Copy() defer s.Close() - updates := createDatasetUpdateQuery(id, dataset) + updates := createDatasetUpdateQuery(id, dataset, currentState) update := bson.M{"$set": updates, "$setOnInsert": bson.M{"next.last_updated": time.Now()}} if err = s.DB(m.Database).C("datasets").UpdateId(id, update); err != nil { if err == mgo.ErrNotFound { @@ -298,7 +298,7 @@ func (m *Mongo) UpdateDataset(id string, dataset *models.Dataset) (err error) { return nil } -func createDatasetUpdateQuery(id string, dataset *models.Dataset) bson.M { +func createDatasetUpdateQuery(id string, dataset *models.Dataset, currentState string) bson.M { updates := make(bson.M, 0) log.Debug("building update query for dataset resource", log.Data{"dataset_id": id, "dataset": dataset, "updates": updates}) @@ -385,6 +385,10 @@ func createDatasetUpdateQuery(id string, dataset *models.Dataset) bson.M { if dataset.State != "" { updates["next.state"] = dataset.State + } else { + if currentState == models.PublishedState { + updates["next.state"] = models.CreatedState + } } if dataset.Theme != "" { diff --git a/mongo/dataset_test.go b/mongo/dataset_test.go index 422553be..8e33e82d 100644 --- a/mongo/dataset_test.go +++ b/mongo/dataset_test.go @@ -226,7 +226,7 @@ func TestDatasetUpdateQuery(t *testing.T) { URI: "http://ons.gov.uk/dataset/123/landing-page", } - selector := createDatasetUpdateQuery("123", dataset) + selector := createDatasetUpdateQuery("123", dataset, models.CreatedState) So(selector, ShouldNotBeNil) So(selector, ShouldResemble, expectedUpdate) }) @@ -239,7 +239,7 @@ func TestDatasetUpdateQuery(t *testing.T) { expectedUpdate := bson.M{ "next.national_statistic": &nationalStatistic, } - selector := createDatasetUpdateQuery("123", dataset) + selector := createDatasetUpdateQuery("123", dataset, models.CreatedState) So(selector, ShouldNotBeNil) So(selector, ShouldResemble, expectedUpdate) }) @@ -247,7 +247,7 @@ func TestDatasetUpdateQuery(t *testing.T) { Convey("When national statistic is not set", t, func() { dataset := &models.Dataset{} - selector := createDatasetUpdateQuery("123", dataset) + selector := createDatasetUpdateQuery("123", dataset, models.CreatedState) So(selector, ShouldNotBeNil) So(selector, ShouldResemble, bson.M{}) }) diff --git a/store/datastore.go b/store/datastore.go index c60e1aca..4b800541 100644 --- a/store/datastore.go +++ b/store/datastore.go @@ -35,7 +35,7 @@ type Storer interface { GetUniqueDimensionValues(ID, dimension string) (*models.DimensionValues, error) GetVersion(datasetID, editionID, version, state string) (*models.Version, error) GetVersions(datasetID, editionID, state string) (*models.VersionResults, error) - UpdateDataset(ID string, dataset *models.Dataset) error + UpdateDataset(ID string, dataset *models.Dataset, currentState string) error UpdateDatasetWithAssociation(ID, state string, version *models.Version) error UpdateDimensionNodeID(dimension *models.DimensionOption) error UpdateEdition(datasetID, edition string, latestVersion *models.Version) error diff --git a/store/datastoretest/datastore.go b/store/datastoretest/datastore.go index 2df345d0..ce639670 100755 --- a/store/datastoretest/datastore.go +++ b/store/datastoretest/datastore.go @@ -116,7 +116,7 @@ var ( // UpdateBuildSearchTaskStateFunc: func(id string, dimension string, state string) error { // panic("TODO: mock out the UpdateBuildSearchTaskState method") // }, -// UpdateDatasetFunc: func(ID string, dataset *models.Dataset) error { +// UpdateDatasetFunc: func(ID string, dataset *models.Dataset, currentState string) error { // panic("TODO: mock out the UpdateDataset method") // }, // UpdateDatasetWithAssociationFunc: func(ID string, state string, version *models.Version) error { @@ -223,7 +223,7 @@ type StorerMock struct { UpdateBuildSearchTaskStateFunc func(id string, dimension string, state string) error // UpdateDatasetFunc mocks the UpdateDataset method. - UpdateDatasetFunc func(ID string, dataset *models.Dataset) error + UpdateDatasetFunc func(ID string, dataset *models.Dataset, currentState string) error // UpdateDatasetWithAssociationFunc mocks the UpdateDatasetWithAssociation method. UpdateDatasetWithAssociationFunc func(ID string, state string, version *models.Version) error @@ -387,8 +387,8 @@ type StorerMock struct { } // UpdateBuildHierarchyTaskState holds details about calls to the UpdateBuildHierarchyTaskState method. UpdateBuildHierarchyTaskState []struct { - // Id is the id argument value. - Id string + // ID is the id argument value. + ID string // Dimension is the dimension argument value. Dimension string // State is the state argument value. @@ -396,8 +396,8 @@ type StorerMock struct { } // UpdateBuildSearchTaskState holds details about calls to the UpdateBuildSearchTaskState method. UpdateBuildSearchTaskState []struct { - // Id is the id argument value. - Id string + // ID is the id argument value. + ID string // Dimension is the dimension argument value. Dimension string // State is the state argument value. @@ -409,6 +409,8 @@ type StorerMock struct { ID string // Dataset is the dataset argument value. Dataset *models.Dataset + // CurrentState is the currentState argument value. + CurrentState string } // UpdateDatasetWithAssociation holds details about calls to the UpdateDatasetWithAssociation method. UpdateDatasetWithAssociation []struct { @@ -435,8 +437,8 @@ type StorerMock struct { } // UpdateImportObservationsTaskState holds details about calls to the UpdateImportObservationsTaskState method. UpdateImportObservationsTaskState []struct { - // Id is the id argument value. - Id string + // ID is the id argument value. + ID string // State is the state argument value. State string } @@ -1148,11 +1150,11 @@ func (mock *StorerMock) UpdateBuildHierarchyTaskState(id string, dimension strin panic("moq: StorerMock.UpdateBuildHierarchyTaskStateFunc is nil but Storer.UpdateBuildHierarchyTaskState was just called") } callInfo := struct { - Id string + ID string Dimension string State string }{ - Id: id, + ID: id, Dimension: dimension, State: state, } @@ -1166,12 +1168,12 @@ func (mock *StorerMock) UpdateBuildHierarchyTaskState(id string, dimension strin // Check the length with: // len(mockedStorer.UpdateBuildHierarchyTaskStateCalls()) func (mock *StorerMock) UpdateBuildHierarchyTaskStateCalls() []struct { - Id string + ID string Dimension string State string } { var calls []struct { - Id string + ID string Dimension string State string } @@ -1187,11 +1189,11 @@ func (mock *StorerMock) UpdateBuildSearchTaskState(id string, dimension string, panic("moq: StorerMock.UpdateBuildSearchTaskStateFunc is nil but Storer.UpdateBuildSearchTaskState was just called") } callInfo := struct { - Id string + ID string Dimension string State string }{ - Id: id, + ID: id, Dimension: dimension, State: state, } @@ -1205,12 +1207,12 @@ func (mock *StorerMock) UpdateBuildSearchTaskState(id string, dimension string, // Check the length with: // len(mockedStorer.UpdateBuildSearchTaskStateCalls()) func (mock *StorerMock) UpdateBuildSearchTaskStateCalls() []struct { - Id string + ID string Dimension string State string } { var calls []struct { - Id string + ID string Dimension string State string } @@ -1221,33 +1223,37 @@ func (mock *StorerMock) UpdateBuildSearchTaskStateCalls() []struct { } // UpdateDataset calls UpdateDatasetFunc. -func (mock *StorerMock) UpdateDataset(ID string, dataset *models.Dataset) error { +func (mock *StorerMock) UpdateDataset(ID string, dataset *models.Dataset, currentState string) error { if mock.UpdateDatasetFunc == nil { panic("moq: StorerMock.UpdateDatasetFunc is nil but Storer.UpdateDataset was just called") } callInfo := struct { - ID string - Dataset *models.Dataset + ID string + Dataset *models.Dataset + CurrentState string }{ - ID: ID, - Dataset: dataset, + ID: ID, + Dataset: dataset, + CurrentState: currentState, } lockStorerMockUpdateDataset.Lock() mock.calls.UpdateDataset = append(mock.calls.UpdateDataset, callInfo) lockStorerMockUpdateDataset.Unlock() - return mock.UpdateDatasetFunc(ID, dataset) + return mock.UpdateDatasetFunc(ID, dataset, currentState) } // UpdateDatasetCalls gets all the calls that were made to UpdateDataset. // Check the length with: // len(mockedStorer.UpdateDatasetCalls()) func (mock *StorerMock) UpdateDatasetCalls() []struct { - ID string - Dataset *models.Dataset + ID string + Dataset *models.Dataset + CurrentState string } { var calls []struct { - ID string - Dataset *models.Dataset + ID string + Dataset *models.Dataset + CurrentState string } lockStorerMockUpdateDataset.RLock() calls = mock.calls.UpdateDataset @@ -1370,10 +1376,10 @@ func (mock *StorerMock) UpdateImportObservationsTaskState(id string, state strin panic("moq: StorerMock.UpdateImportObservationsTaskStateFunc is nil but Storer.UpdateImportObservationsTaskState was just called") } callInfo := struct { - Id string + ID string State string }{ - Id: id, + ID: id, State: state, } lockStorerMockUpdateImportObservationsTaskState.Lock() @@ -1386,11 +1392,11 @@ func (mock *StorerMock) UpdateImportObservationsTaskState(id string, state strin // Check the length with: // len(mockedStorer.UpdateImportObservationsTaskStateCalls()) func (mock *StorerMock) UpdateImportObservationsTaskStateCalls() []struct { - Id string + ID string State string } { var calls []struct { - Id string + ID string State string } lockStorerMockUpdateImportObservationsTaskState.RLock() From 41bb3d70325835f1252aa2d99fdecebfcc095e06 Mon Sep 17 00:00:00 2001 From: Eleanor Deal Date: Wed, 21 Feb 2018 17:01:27 +0000 Subject: [PATCH 41/41] Add publish option for datasets independent of versions --- api/dataset.go | 43 +++++++++++++---------- api/dataset_test.go | 85 +++++++++++++++++++++------------------------ 2 files changed, 64 insertions(+), 64 deletions(-) diff --git a/api/dataset.go b/api/dataset.go index 33bd253f..0d4c07e8 100644 --- a/api/dataset.go +++ b/api/dataset.go @@ -374,10 +374,18 @@ func (api *DatasetAPI) putDataset(w http.ResponseWriter, r *http.Request) { return } - if err := api.dataStore.Backend.UpdateDataset(datasetID, dataset, currentDataset.Next.State); err != nil { - log.ErrorC("failed to update dataset resource", err, log.Data{"dataset_id": datasetID}) - handleErrorType(datasetDocType, err, w) - return + if dataset.State == models.PublishedState { + if err := api.publishDataset(currentDataset, nil); err != nil { + log.ErrorC("failed to update dataset document to published", err, log.Data{"dataset_id": datasetID}) + handleErrorType(versionDocType, err, w) + return + } + } else { + if err := api.dataStore.Backend.UpdateDataset(datasetID, dataset, currentDataset.Next.State); err != nil { + log.ErrorC("failed to update dataset resource", err, log.Data{"dataset_id": datasetID}) + handleErrorType(datasetDocType, err, w) + return + } } setJSONContentType(w) @@ -399,7 +407,8 @@ func (api *DatasetAPI) putVersion(w http.ResponseWriter, r *http.Request) { return } - if err = api.dataStore.Backend.CheckDatasetExists(datasetID, ""); err != nil { + currentDataset, err := api.dataStore.Backend.GetDataset(datasetID) + if err != nil { log.ErrorC("failed to find dataset", err, log.Data{"dataset_id": datasetID, "edition": edition, "version": version}) handleErrorType(versionDocType, err, w) return @@ -442,7 +451,7 @@ func (api *DatasetAPI) putVersion(w http.ResponseWriter, r *http.Request) { } // Pass in newVersion variable to include relevant data needed for update on dataset API (e.g. links) - if err := api.publishDataset(datasetID, newVersion); err != nil { + if err := api.publishDataset(currentDataset, newVersion); err != nil { log.ErrorC("failed to update dataset document once version state changes to publish", err, log.Data{"dataset_id": datasetID, "edition": edition, "version": version}) handleErrorType(versionDocType, err, w) return @@ -586,18 +595,14 @@ func createNewVersionDoc(currentVersion *models.Version, version *models.Version return version } -func (api *DatasetAPI) publishDataset(id string, version *models.Version) error { - currentDataset, err := api.dataStore.Backend.GetDataset(id) - if err != nil { - log.ErrorC("unable to update dataset", err, log.Data{"dataset_id": id}) - return err - } - - currentDataset.Next.CollectionID = version.CollectionID +func (api *DatasetAPI) publishDataset(currentDataset *models.DatasetUpdate, version *models.Version) error { + if version != nil { + currentDataset.Next.CollectionID = version.CollectionID - currentDataset.Next.Links.LatestVersion = &models.LinkObject{ - ID: version.Links.Version.ID, - HRef: version.Links.Version.HRef, + currentDataset.Next.Links.LatestVersion = &models.LinkObject{ + ID: version.Links.Version.ID, + HRef: version.Links.Version.HRef, + } } currentDataset.Next.State = models.PublishedState @@ -613,8 +618,8 @@ func (api *DatasetAPI) publishDataset(id string, version *models.Version) error Next: currentDataset.Next, } - if err := api.dataStore.Backend.UpsertDataset(id, newDataset); err != nil { - log.ErrorC("unable to update dataset", err, log.Data{"dataset_id": id}) + if err := api.dataStore.Backend.UpsertDataset(currentDataset.ID, newDataset); err != nil { + log.ErrorC("unable to update dataset", err, log.Data{"dataset_id": currentDataset.ID}) return err } diff --git a/api/dataset_test.go b/api/dataset_test.go index ddd80173..c628525b 100644 --- a/api/dataset_test.go +++ b/api/dataset_test.go @@ -903,8 +903,8 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - CheckDatasetExistsFunc: func(string, string) error { - return nil + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil }, CheckEditionExistsFunc: func(string, string, string) error { return nil @@ -943,7 +943,7 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusOK) - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 3) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 2) @@ -968,8 +968,8 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - CheckDatasetExistsFunc: func(string, string) error { - return nil + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil }, CheckEditionExistsFunc: func(string, string, string) error { return nil @@ -994,7 +994,7 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusOK) - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 3) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 2) @@ -1022,8 +1022,8 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - CheckDatasetExistsFunc: func(string, string) error { - return nil + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil }, CheckEditionExistsFunc: func(string, string, string) error { return nil @@ -1054,7 +1054,7 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { } So(w.Code, ShouldEqual, http.StatusOK) - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 1) @@ -1079,9 +1079,6 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { w := httptest.NewRecorder() mockedDataStore := &storetest.StorerMock{ - CheckDatasetExistsFunc: func(string, string) error { - return nil - }, CheckEditionExistsFunc: func(string, string, string) error { return nil }, @@ -1137,7 +1134,6 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { api := GetAPIWithMockedDatastore(mockedDataStore, generatorMock) api.router.ServeHTTP(w, r) So(w.Code, ShouldEqual, http.StatusOK) - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 3) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 2) @@ -1161,8 +1157,8 @@ func TestPutVersionGenerateDownloadsError(t *testing.T) { GetVersionFunc: func(datasetID string, editionID string, version string, state string) (*models.Version, error) { return &v, nil }, - CheckDatasetExistsFunc: func(ID string, state string) error { - return nil + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil }, CheckEditionExistsFunc: func(ID string, editionID string, state string) error { return nil @@ -1196,8 +1192,8 @@ func TestPutVersionGenerateDownloadsError(t *testing.T) { Convey("and the expected store calls are made with the expected parameters", func() { genCalls := mockDownloadGenerator.GenerateCalls() - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) - So(mockedDataStore.CheckDatasetExistsCalls()[0].ID, ShouldEqual, "123") + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(mockedDataStore.GetDatasetCalls()[0].ID, ShouldEqual, "123") So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) So(mockedDataStore.CheckEditionExistsCalls()[0].ID, ShouldEqual, "123") @@ -1230,8 +1226,8 @@ func TestPutEmptyVersion(t *testing.T) { GetVersionFunc: func(datasetID string, editionID string, version string, state string) (*models.Version, error) { return &v, nil }, - CheckDatasetExistsFunc: func(ID string, state string) error { - return nil + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil }, CheckEditionExistsFunc: func(ID string, editionID string, state string) error { return nil @@ -1267,8 +1263,8 @@ func TestPutEmptyVersion(t *testing.T) { v.Downloads = xlsDownload return &v, nil }, - CheckDatasetExistsFunc: func(ID string, state string) error { - return nil + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil }, CheckEditionExistsFunc: func(ID string, editionID string, state string) error { return nil @@ -1298,9 +1294,8 @@ func TestPutEmptyVersion(t *testing.T) { }) Convey("and the expected external calls are made with the correct parameters", func() { - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) - So(mockedDataStore.CheckDatasetExistsCalls()[0].ID, ShouldEqual, "123") - So(mockedDataStore.CheckDatasetExistsCalls()[0].State, ShouldEqual, "") + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(mockedDataStore.GetDatasetCalls()[0].ID, ShouldEqual, "123") So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) So(mockedDataStore.CheckEditionExistsCalls()[0].ID, ShouldEqual, "123") @@ -1340,8 +1335,8 @@ func TestPutVersionReturnsError(t *testing.T) { GetVersionFunc: func(string, string, string, string) (*models.Version, error) { return &models.Version{State: models.AssociatedState}, nil }, - CheckDatasetExistsFunc: func(string, string) error { - return nil + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil }, } @@ -1350,7 +1345,7 @@ func TestPutVersionReturnsError(t *testing.T) { So(w.Body.String(), ShouldEqual, "Failed to parse json body\n") So(w.Code, ShouldEqual, http.StatusBadRequest) So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 0) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) }) @@ -1371,8 +1366,8 @@ func TestPutVersionReturnsError(t *testing.T) { GetVersionFunc: func(string, string, string, string) (*models.Version, error) { return nil, errInternal }, - CheckDatasetExistsFunc: func(string, string) error { - return nil + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil }, } @@ -1381,7 +1376,7 @@ func TestPutVersionReturnsError(t *testing.T) { So(w.Code, ShouldEqual, http.StatusInternalServerError) So(w.Body.String(), ShouldEqual, "internal error\n") So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 0) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) }) @@ -1402,8 +1397,8 @@ func TestPutVersionReturnsError(t *testing.T) { GetVersionFunc: func(string, string, string, string) (*models.Version, error) { return &models.Version{}, errs.ErrVersionNotFound }, - CheckDatasetExistsFunc: func(string, string) error { - return errs.ErrDatasetNotFound + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return nil, errs.ErrDatasetNotFound }, CheckEditionExistsFunc: func(string, string, string) error { return nil @@ -1415,7 +1410,7 @@ func TestPutVersionReturnsError(t *testing.T) { So(w.Code, ShouldEqual, http.StatusBadRequest) So(w.Body.String(), ShouldEqual, "Dataset not found\n") So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 0) So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) }) @@ -1437,8 +1432,8 @@ func TestPutVersionReturnsError(t *testing.T) { GetVersionFunc: func(string, string, string, string) (*models.Version, error) { return &models.Version{}, errs.ErrVersionNotFound }, - CheckDatasetExistsFunc: func(string, string) error { - return nil + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil }, CheckEditionExistsFunc: func(string, string, string) error { return errs.ErrEditionNotFound @@ -1450,7 +1445,7 @@ func TestPutVersionReturnsError(t *testing.T) { So(w.Code, ShouldEqual, http.StatusBadRequest) So(w.Body.String(), ShouldEqual, "Edition not found\n") So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) }) @@ -1472,8 +1467,8 @@ func TestPutVersionReturnsError(t *testing.T) { GetVersionFunc: func(string, string, string, string) (*models.Version, error) { return &models.Version{}, errs.ErrVersionNotFound }, - CheckDatasetExistsFunc: func(string, string) error { - return nil + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil }, CheckEditionExistsFunc: func(string, string, string) error { return nil @@ -1488,7 +1483,7 @@ func TestPutVersionReturnsError(t *testing.T) { So(w.Code, ShouldEqual, http.StatusNotFound) So(w.Body.String(), ShouldEqual, "Version not found\n") So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 0) So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) @@ -1541,8 +1536,8 @@ func TestPutVersionReturnsError(t *testing.T) { State: models.PublishedState, }, nil }, - CheckDatasetExistsFunc: func(string, string) error { - return nil + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil }, } @@ -1551,7 +1546,7 @@ func TestPutVersionReturnsError(t *testing.T) { So(w.Code, ShouldEqual, http.StatusForbidden) So(w.Body.String(), ShouldEqual, "unable to update version as it has been published\n") So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 1) - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 0) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 0) }) Convey("When the request body is invalid return status bad request", t, func() { @@ -1571,8 +1566,8 @@ func TestPutVersionReturnsError(t *testing.T) { GetVersionFunc: func(string, string, string, string) (*models.Version, error) { return &models.Version{State: "associated"}, nil }, - CheckDatasetExistsFunc: func(string, string) error { - return nil + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil }, CheckEditionExistsFunc: func(string, string, string) error { return nil @@ -1587,7 +1582,7 @@ func TestPutVersionReturnsError(t *testing.T) { So(w.Code, ShouldEqual, http.StatusBadRequest) So(w.Body.String(), ShouldEqual, "Missing collection_id for association between version and a collection\n") So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) - So(len(mockedDataStore.CheckDatasetExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 0) So(len(generatorMock.GenerateCalls()), ShouldEqual, 0)