Added batch update for statuses #658
Conversation
andresuribe87
requested review from
decentralgabe and
nitro-neal
as code owners
pkg/server/router/credential.go
Outdated
| @@ -339,6 +339,72 @@ type UpdateCredentialStatusResponse struct { | |||
| Suspended bool `json:"suspended"` | |||
| } | |||
|
|
|||
| type SingleUpdateCredentialStatusRequest struct { | |||
| // ID of the credential who's status should be updated. | |||
| ID string `json:"id"` | |||
pkg/server/router/credential.go
Outdated
| serviceReq := routerReq.toServiceRequest(routerReq.ID) | ||
| req.Requests = append(req.Requests, serviceReq) | ||
| } | ||
| return &req |
There was a problem hiding this comment.
No reason. Shouldn't make a difference. Changes to non-ptr.
pkg/server/server.go
Outdated
| @@ -218,6 +218,7 @@ func CredentialAPI(rg *gin.RouterGroup, service svcframework.Service, webhookSer | |||
| // Credential Status | |||
| credentialAPI.GET("/:id"+StatusPrefix, credRouter.GetCredentialStatus) | |||
| credentialAPI.PUT("/:id"+StatusPrefix, credRouter.UpdateCredentialStatus) | |||
| credentialAPI.PUT(StatusPrefix+"/batch", credRouter.BatchUpdateCredentialStatus) | |||
There was a problem hiding this comment.
change /batch to a const which can be used elsewhere
| @@ -31,6 +31,109 @@ func TestCredentialAPI(t *testing.T) { | |||
|
|
|||
| for _, test := range testutil.TestDatabases { | |||
| t.Run(test.Name, func(tt *testing.T) { | |||
| tt.Run("Batch Update Credential Status", func(ttt *testing.T) { | |||
There was a problem hiding this comment.
we need a few different test cases here
- batch of 0
- batch of 1-n (all valid)
- batch of 1-n (mixed invalid and valid credentials, invalid = cred doesn't exist, cred status already set)
Another test we might need here and elsewhere is performing an update where the key no longer exists (revoked/deleted)
There was a problem hiding this comment.
Added tests for all batch behaviors.
Another test we might need here and elsewhere is performing an update where the key no longer exists (revoked/deleted)
I'm unsure about some of the details of the behavior you're trying to test. Mind creating an issue and we can discuss?
|
|
||
| type Status struct { | ||
| // ID of the credentials who's status this object represents. | ||
| ID string `json:"id,omitempty"` |
There was a problem hiding this comment.
No, the usage in UpdateCredentialStatusResponse is optional.
pkg/service/credential/service.go
Outdated
| return nil, errors.Wrap(err, "reading credential") | ||
| } | ||
|
|
||
| if gotCred.Credential.CredentialStatus == nil { |
There was a problem hiding this comment.
I would recommend a new helper like HasCredentialStatus which can also check that gotCred is not nil and gotCred.Credential is not nil, or else we're at risk for NPEs
pkg/service/credential/service.go
Outdated
| return nil, sdkutil.LoggingNewErrorf("credential %q has no credentialStatus field", gotCred.LocalCredentialID) | ||
| } | ||
|
|
||
| statusPurpose := gotCred.Credential.CredentialStatus.(map[string]any)["statusPurpose"].(string) |
There was a problem hiding this comment.
GetStatusPurpose method would be helpful
pkg/server/router/credential.go
Outdated
| @@ -339,6 +339,72 @@ type UpdateCredentialStatusResponse struct { | |||
| Suspended bool `json:"suspended"` | |||
| } | |||
|
|
|||
| type SingleUpdateCredentialStatusRequest struct { | |||
| // ID of the credential who's status should be updated. | |||
| ID string `json:"id"` | |||
pkg/server/router/credential.go
Outdated
| serviceReq := routerReq.toServiceRequest(routerReq.ID) | ||
| req.Requests = append(req.Requests, serviceReq) | ||
| } | ||
| return &req |
There was a problem hiding this comment.
No reason. Shouldn't make a difference. Changes to non-ptr.
pkg/server/server.go
Outdated
| @@ -218,6 +218,7 @@ func CredentialAPI(rg *gin.RouterGroup, service svcframework.Service, webhookSer | |||
| // Credential Status | |||
| credentialAPI.GET("/:id"+StatusPrefix, credRouter.GetCredentialStatus) | |||
| credentialAPI.PUT("/:id"+StatusPrefix, credRouter.UpdateCredentialStatus) | |||
| credentialAPI.PUT(StatusPrefix+"/batch", credRouter.BatchUpdateCredentialStatus) | |||
| @@ -31,6 +31,109 @@ func TestCredentialAPI(t *testing.T) { | |||
|
|
|||
| for _, test := range testutil.TestDatabases { | |||
| t.Run(test.Name, func(tt *testing.T) { | |||
| tt.Run("Batch Update Credential Status", func(ttt *testing.T) { | |||
There was a problem hiding this comment.
Added tests for all batch behaviors.
Another test we might need here and elsewhere is performing an update where the key no longer exists (revoked/deleted)
I'm unsure about some of the details of the behavior you're trying to test. Mind creating an issue and we can discuss?
pkg/service/credential/service.go
Outdated
| return nil, errors.Wrap(err, "reading credential") | ||
| } | ||
|
|
||
| if gotCred.Credential.CredentialStatus == nil { |
pkg/service/credential/service.go
Outdated
| return nil, sdkutil.LoggingNewErrorf("credential %q has no credentialStatus field", gotCred.LocalCredentialID) | ||
| } | ||
|
|
||
| statusPurpose := gotCred.Credential.CredentialStatus.(map[string]any)["statusPurpose"].(string) |
|
|
||
| type Status struct { | ||
| // ID of the credentials who's status this object represents. | ||
| ID string `json:"id,omitempty"` |
There was a problem hiding this comment.
No, the usage in UpdateCredentialStatusResponse is optional.
Codecov Report
@@ Coverage Diff @@
## main #658 +/- ##
==========================================
- Coverage 25.72% 25.55% -0.17%
==========================================
Files 56 56
Lines 6254 6299 +45
==========================================
+ Hits 1609 1610 +1
- Misses 4369 4413 +44
Partials 276 276
|
Overview
This PR adds a batch endpoint for updating statuses of credentials. It is the last PR that fixes #347
Description
Added the
v1/credentials/status/batchendpoint. See the swagger spec file for documentation on the request/response schemas.How Has This Been Tested?
Unit and integration tests.