Sc 10284/update handler #73
Conversation
|
This pull request has been linked to Shortcut Story #10284: Implement Tenant Resource Update Handlers. |
pkg/tenant/tenants.go
Outdated
| } | ||
|
|
||
| // Get the tenant ID from the URL and return a 400 if the tenant | ||
| // does not exist. |
There was a problem hiding this comment.
| // does not exist. | |
| // ID is not a ULID |
| ID: tenantID, | ||
| Name: tenant.Name, | ||
| EnvironmentType: tenant.EnvironmentType, | ||
| } |
There was a problem hiding this comment.
The pattern we've mostly used in GDS for Update handlers is to first try to retrieve the original record, then update the record that was retrieved and store it back to the database.
I think this would go a long way to make this endpoint more resilient to future changes, since it prevents us from accidentally overwriting internal data on the tenant in the database. We may also have metadata fields like created_at, modified_at timestamps that we don't want to override. This would also allow us to handle the "tenant not found" and "error calling the database" errors separately to give the caller more information about what went wrong.
There was a problem hiding this comment.
@pdeziel I made an update to retrieve the tenant from the db once the tenant ID is parsed from the URL. However, if the change isn't in line with what you had in mind please let me know.
There was a problem hiding this comment.
Great, now that we are retrieving the tenant from the db can we use that instead of the req (so modifying the model directly) when storing to the database?
Also, I think it's probably better to put the db.RetrieveTenant after the 400 checks, so that we don't have to make a request to the database unless the request is formatted correctly (this will save a bit of time on the error paths).
| suite.requireError(err, http.StatusBadRequest, "tenant environment type is required", "expected error when tenant environent type does not exist") | ||
|
|
||
| req := &api.Tenant{ | ||
| ID: "01ARZ3NDEKTSV4RRFFQ69G5FAV", |
There was a problem hiding this comment.
Can we change this to a different ID so that we're testing that the ID in the request does not override the ID on the tenant object?
| ID: tenantID, | ||
| Name: tenant.Name, | ||
| EnvironmentType: tenant.EnvironmentType, | ||
| } |
There was a problem hiding this comment.
Great, now that we are retrieving the tenant from the db can we use that instead of the req (so modifying the model directly) when storing to the database?
Also, I think it's probably better to put the db.RetrieveTenant after the 400 checks, so that we don't have to make a request to the database unless the request is formatted correctly (this will save a bit of time on the error paths).
pkg/tenant/tenants.go
Outdated
| // tenant record cannot be updated. | ||
| if err := db.UpdateTenant(c.Request.Context(), req); err != nil { | ||
| log.Error().Err(err).Msg("could not save tenant") | ||
| c.JSON(http.StatusNotFound, api.ErrorResponse("could not update tenant")) |
There was a problem hiding this comment.
| c.JSON(http.StatusNotFound, api.ErrorResponse("could not update tenant")) | |
| c.JSON(http.StatusInternalServerError, api.ErrorResponse("could not update tenant")) |
There was a problem hiding this comment.
At this point we have already verified that the tenant exists so if we get an error here then it's probably a server side error.
Scope of changes
Adds TenantUpdate server-side handler that will update tenant records in the Tenant API along with corresponding tests.
Fixes SC-10284
Type of change
Acceptance criteria
Describe how reviewers can test this change to be sure that it works correctly. Add a checklist if possible.
Author checklist
Reviewer(s) checklist