Feature/dis 4235 prevent spaces in ids dataset api #654
Conversation
franmoore05
left a comment
franmoore05
left a comment
There was a problem hiding this comment.
Providing an edition field as part of the body on a POST /versions and /versions/{number} which contained spaces allowed me to complete the request successfully. The validation should be consistent and fail if an edition is provided in the body with the value "test 123" so this needs to be resolved.
api/static_versions.go
Outdated
|
|
||
| if err := utils.ValidateIDNoSpaces(datasetID); err != nil { | ||
| log.Error(ctx, "addDatasetVersionCondensed endpoint: dataset ID contains spaces", err, logData) | ||
| return nil, models.NewErrorResponse(http.StatusBadRequest, nil, models.NewError(err, errs.ErrSpacesNotAllowedInID.Error(), errs.ErrSpacesNotAllowedInID.Error())) |
There was a problem hiding this comment.
The code and description values required by models.NewError appear to be duplicated, these should be different.
api/static_versions.go
Outdated
|
|
||
| if err := utils.ValidateIDNoSpaces(datasetID); err != nil { | ||
| log.Error(ctx, "createVersion endpoint: dataset ID contains spaces", err, logData) | ||
| return nil, models.NewErrorResponse(http.StatusBadRequest, nil, models.NewError(err, errs.ErrSpacesNotAllowedInID.Error(), errs.ErrSpacesNotAllowedInID.Error())) |
There was a problem hiding this comment.
As previously mentioned, the code and description values required by models.NewError appear to be duplicated, these should be different.
There was a problem hiding this comment.
These are still returning duplicate values. Please see previous comment.
api/static_versions.go
Outdated
|
|
||
| if err := utils.ValidateIDNoSpaces(edition); err != nil { | ||
| log.Error(ctx, "createVersion endpoint: edition ID contains spaces", err, logData) | ||
| return nil, models.NewErrorResponse(http.StatusBadRequest, nil, models.NewError(err, errs.ErrSpacesNotAllowedInID.Error(), errs.ErrSpacesNotAllowedInID.Error())) |
There was a problem hiding this comment.
As above, duplicate values have been provided for code and description.
There was a problem hiding this comment.
These are still returning duplicate values. Please see previous comment.
franmoore05
left a comment
franmoore05
left a comment
There was a problem hiding this comment.
Requesting changes be addressed (clicked the wrong radio button)
api/static_versions.go
Outdated
| return nil, models.NewErrorResponse(http.StatusBadRequest, nil, models.NewError(err, models.JSONUnmarshalError, "failed to unmarshal version")) | ||
| } | ||
|
|
||
| if newVersion.Type == models.Static.String() { |
There was a problem hiding this comment.
Why has this check been added here? If a record is being processed in the static_versions.go file then it would only be of a static type.
There was a problem hiding this comment.
you’re right. Since this code path is only reached for records handled in static_versions.go, the type will always be static. The additional check is unnecessary, so I’ll remove it.
|
|
||
| // Populate distributions (static only) | ||
| // Populate distributions & validate spaces in IDs(static only) | ||
| if version.Type == models.Static.String() { |
There was a problem hiding this comment.
There is already a check for static type on line 371, could this section be moved in there to avoid the duplicate if statements?
What
How to review
Check when creating or updating a dataset/ edition 400 is returned if the IDs have spaces
Use the https://officefornationalstatistics.atlassian.net/wiki/spaces/DIS/pages/60787954/Discoverable+Datasets+-+Basic+API+Calls+-+Publish to create or update datasets/ editions
validate if the AC is met as given https://officefornationalstatistics.atlassian.net/browse/DIS-4235
Who can review
anyone