From de87669b8703ecf338f6eba297fd202b189bdc44 Mon Sep 17 00:00:00 2001 From: janderson2 Date: Thu, 9 Dec 2021 19:00:02 +0000 Subject: [PATCH] Fix full file download not generating The full file download is not generating due to a mismatch between the dataset type expected by the dataset API and the type stored in the recipes. Also attempt to surface the errors that are currently getting swallowed by the catch all internal error message. --- api/versions.go | 7 +- api/versions_test.go | 208 +++++++++++++++++++++++++++++++++++++------ models/dataset.go | 2 +- 3 files changed, 186 insertions(+), 31 deletions(-) diff --git a/api/versions.go b/api/versions.go index 03007bb8..6b659e5b 100644 --- a/api/versions.go +++ b/api/versions.go @@ -480,10 +480,11 @@ func (api *DatasetAPI) publishVersion(ctx context.Context, currentDataset *model } generator, ok := api.downloadGenerators[t] if !ok { + fmt.Printf("we are here: %s", t) return fmt.Errorf("no downloader available for type %s", t) } // Send Kafka message. The generator which is used depends on the type defined in VersionDoc. - if err := generator.Generate(ctx, versionDetails.datasetID, versionUpdate.ID, versionDetails.edition, versionDetails.version); err != nil{ + if err := generator.Generate(ctx, versionDetails.datasetID, versionUpdate.ID, versionDetails.edition, versionDetails.version); err != nil { data["instance_id"] = versionUpdate.ID data["state"] = versionUpdate.State data["type"] = t.String() @@ -528,7 +529,7 @@ func (api *DatasetAPI) associateVersion(ctx context.Context, currentVersion, ver return fmt.Errorf("no downloader available for type %s", t.String()) } // ToDo outsidecoder - Pass the right mock and get Generate to fail by returning an error.New("Error message") - if err := generator.Generate(ctx, versionDetails.datasetID, versionDoc.ID, versionDetails.edition, versionDetails.version); err != nil{ + if err := generator.Generate(ctx, versionDetails.datasetID, versionDoc.ID, versionDetails.edition, versionDetails.version); err != nil { data["instance_id"] = versionDoc.ID data["state"] = versionDoc.State log.Error(ctx, "putVersion endpoint: error while attempting to generate full dataset version downloads on version association", err, data) @@ -672,7 +673,7 @@ func handleVersionAPIErr(ctx context.Context, err error, w http.ResponseWriter, case strings.HasPrefix(err.Error(), "invalid version requested"): status = http.StatusBadRequest default: - err = errs.ErrInternalServer + err = fmt.Errorf("%s: %s", errs.ErrInternalServer.Error(), err.Error()) status = http.StatusInternalServerError } diff --git a/api/versions_test.go b/api/versions_test.go index 2f7dcd84..399ce7a7 100644 --- a/api/versions_test.go +++ b/api/versions_test.go @@ -591,7 +591,7 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { datasetPermissions := getAuthorisationHandlerMock() permissions := getAuthorisationHandlerMock() - Convey("put version with CMD type", func() { + Convey("put version with filterable type", func() { mockedDataStore := &storetest.StorerMock{ GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { @@ -602,7 +602,51 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { }, GetVersionFunc: func(string, string, int, string) (*models.Version, error) { return &models.Version{ - Type: models.Filterable.String(), + Type: "filterable", + }, nil + }, + UpdateVersionFunc: func(string, *models.Version) error { + return nil + }, + UpdateDatasetWithAssociationFunc: func(string, string, *models.Version) error { + return nil + }, + } + + api := GetAPIWithCMDMocks(mockedDataStore, generatorMock, datasetPermissions, permissions) + api.Router.ServeHTTP(w, r) + + So(w.Code, ShouldEqual, http.StatusOK) + So(datasetPermissions.Required.Calls, ShouldEqual, 1) + So(permissions.Required.Calls, ShouldEqual, 0) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateDatasetWithAssociationCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpsertEditionCalls()), ShouldEqual, 0) + So(len(mockedDataStore.SetInstanceIsPublishedCalls()), ShouldEqual, 0) + So(len(mockedDataStore.UpsertDatasetCalls()), ShouldEqual, 0) + So(len(generatorMock.GenerateCalls()), ShouldEqual, 1) + + Convey("then the request body has been drained", func() { + _, err := r.Body.Read(make([]byte, 1)) + So(err, ShouldEqual, io.EOF) + }) + }) + + Convey("put version with v4 type", func() { + + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil + }, + CheckEditionExistsFunc: func(string, string, string) error { + return nil + }, + GetVersionFunc: func(string, string, int, string) (*models.Version, error) { + return &models.Version{ + Type: "v4", }, nil }, UpdateVersionFunc: func(string, *models.Version) error { @@ -637,25 +681,25 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { Convey("put version with Cantabular type", func() { - mockedDataStore := &storetest.StorerMock{ - GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { - return &models.DatasetUpdate{}, nil - }, - CheckEditionExistsFunc: func(string, string, string) error { - return nil - }, - GetVersionFunc: func(string, string, int, string) (*models.Version, error) { - return &models.Version{ - Type: models.CantabularTable.String(), - }, nil - }, - UpdateVersionFunc: func(string, *models.Version) error { - return nil - }, - UpdateDatasetWithAssociationFunc: func(string, string, *models.Version) error { - return nil - }, - } + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil + }, + CheckEditionExistsFunc: func(string, string, string) error { + return nil + }, + GetVersionFunc: func(string, string, int, string) (*models.Version, error) { + return &models.Version{ + Type: models.CantabularTable.String(), + }, nil + }, + UpdateVersionFunc: func(string, *models.Version) error { + return nil + }, + UpdateDatasetWithAssociationFunc: func(string, string, *models.Version) error { + return nil + }, + } api := GetAPIWithCantabularMocks(mockedDataStore, generatorMock, datasetPermissions, permissions) api.Router.ServeHTTP(w, r) @@ -765,7 +809,7 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { datasetPermissions := getAuthorisationHandlerMock() permissions := getAuthorisationHandlerMock() - Convey("And the datatype is CMD", func(){ + Convey("And the datatype is CMD", func() { mockedDataStore := &storetest.StorerMock{ CheckEditionExistsFunc: func(string, string, string) error { return nil @@ -802,7 +846,7 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { }, }, State: models.EditionConfirmedState, - Type: models.Filterable.String(), + Type: models.Filterable.String(), }, nil }, UpdateVersionFunc: func(string, *models.Version) error { @@ -870,7 +914,6 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { }) }) - Convey("And the datatype is Cantabular", func() { mockedDataStore := &storetest.StorerMock{ @@ -909,7 +952,7 @@ func TestPutVersionReturnsSuccessfully(t *testing.T) { }, }, State: models.EditionConfirmedState, - Type: models.CantabularTable.String(), + Type: models.CantabularTable.String(), }, nil }, UpdateVersionFunc: func(string, *models.Version) error { @@ -1174,13 +1217,14 @@ func TestPutVersionGenerateDownloadsError(t *testing.T) { }) }) } + //v.Type Cant and one for Associated func TestPutEmptyVersion(t *testing.T) { getVersionAssociatedModel := func(datasetType models.DatasetType) models.Version { var v models.Version err := json.Unmarshal([]byte(versionAssociatedPayload), &v) // - So(err, ShouldBeNil) // + So(err, ShouldBeNil) // v.Type = datasetType.String() v.State = models.AssociatedState // return v @@ -1748,6 +1792,116 @@ func TestPutVersionReturnsError(t *testing.T) { }) }) + Convey("When the request body is invalid return status bad request", t, func() { + generatorMock := &mocks.DownloadsGeneratorMock{ + GenerateFunc: func(context.Context, string, string, string, string) error { + return nil + }, + } + + b := `{"instance_id":"a1b2c3","edition":"2017","license":"ONS","release_date":"2017-04-04","state":"associated"}` + r := createRequestWithAuth("PUT", "http://localhost:22000/datasets/123/editions/2017/versions/1", bytes.NewBufferString(b)) + + w := httptest.NewRecorder() + mockedDataStore := &storetest.StorerMock{ + GetVersionFunc: func(string, string, int, string) (*models.Version, error) { + return &models.Version{State: "associated"}, nil + }, + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil + }, + CheckEditionExistsFunc: func(string, string, string) error { + return nil + }, + UpdateVersionFunc: func(string, *models.Version) error { + return nil + }, + } + + datasetPermissions := getAuthorisationHandlerMock() + permissions := getAuthorisationHandlerMock() + api := GetAPIWithCMDMocks(mockedDataStore, generatorMock, datasetPermissions, permissions) + 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(datasetPermissions.Required.Calls, ShouldEqual, 1) + So(permissions.Required.Calls, ShouldEqual, 0) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 0) + So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) + + Convey("then the request body has been drained", func() { + _, err := r.Body.Read(make([]byte, 1)) + So(err, ShouldEqual, io.EOF) + }) + }) + + Convey("Given a version with state of associated", t, func() { + generatorMock := &mocks.DownloadsGeneratorMock{ + GenerateFunc: func(context.Context, string, string, string, string) error { + return nil + }, + } + + b := versionAssociatedPayload + r := createRequestWithAuth("PUT", "http://localhost:22000/datasets/123/editions/2017/versions/1", bytes.NewBufferString(b)) + + w := httptest.NewRecorder() + + datasetPermissions := getAuthorisationHandlerMock() + permissions := getAuthorisationHandlerMock() + + Convey("when the PUT version is called with an invalid type", func() { + mockedDataStore := &storetest.StorerMock{ + GetDatasetFunc: func(datasetID string) (*models.DatasetUpdate, error) { + return &models.DatasetUpdate{}, nil + }, + CheckEditionExistsFunc: func(string, string, string) error { + return nil + }, + GetVersionFunc: func(string, string, int, string) (*models.Version, error) { + return &models.Version{ + Type: "thisisinvalid", + }, nil + }, + UpdateVersionFunc: func(string, *models.Version) error { + return nil + }, + UpdateDatasetWithAssociationFunc: func(string, string, *models.Version) error { + return nil + }, + } + + api := GetAPIWithCMDMocks(mockedDataStore, generatorMock, datasetPermissions, permissions) + api.Router.ServeHTTP(w, r) + + Convey("then an error is returned", func() { + So(w.Code, ShouldEqual, http.StatusInternalServerError) + So(w.Body.String(), ShouldEqual, "internal error: error getting type of version: invalid dataset type\n") + So(datasetPermissions.Required.Calls, ShouldEqual, 1) + So(permissions.Required.Calls, ShouldEqual, 0) + So(len(mockedDataStore.GetVersionCalls()), ShouldEqual, 2) + So(len(mockedDataStore.GetDatasetCalls()), ShouldEqual, 1) + So(len(mockedDataStore.CheckEditionExistsCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateVersionCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpdateDatasetWithAssociationCalls()), ShouldEqual, 1) + So(len(mockedDataStore.UpsertEditionCalls()), ShouldEqual, 0) + So(len(mockedDataStore.SetInstanceIsPublishedCalls()), ShouldEqual, 0) + So(len(mockedDataStore.UpsertDatasetCalls()), ShouldEqual, 0) + So(len(generatorMock.GenerateCalls()), ShouldEqual, 0) + }) + + Convey("and the request body has been drained", func() { + _, err := r.Body.Read(make([]byte, 1)) + So(err, ShouldEqual, io.EOF) + }) + }) + }) + Convey("When setting the instance node to published fails", t, func() { generatorMock := &mocks.DownloadsGeneratorMock{ GenerateFunc: func(context.Context, string, string, string, string) error { @@ -2490,5 +2644,5 @@ func TestDetachVersionReturnsError(t *testing.T) { func assertInternalServerErr(w *httptest.ResponseRecorder) { So(w.Code, ShouldEqual, http.StatusInternalServerError) - So(strings.TrimSpace(w.Body.String()), ShouldEqual, errs.ErrInternalServer.Error()) + So(strings.TrimSpace(w.Body.String()), ShouldStartWith, errs.ErrInternalServer.Error()) } diff --git a/models/dataset.go b/models/dataset.go index 99203233..b103f85f 100644 --- a/models/dataset.go +++ b/models/dataset.go @@ -38,7 +38,7 @@ func (dt DatasetType) String() string { // GetDatasetType returns a dataset type for a given dataset func GetDatasetType(datasetType string) (DatasetType, error) { switch datasetType { - case "filterable", "": + case "filterable", "v4", "": return Filterable, nil case "nomis": return Nomis, nil