-
Notifications
You must be signed in to change notification settings - Fork 208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Remove getIdentityByDID route #1558
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
// Copyright © 2022 Kaleido, Inc. | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// | ||
|
@@ -20,31 +20,53 @@ | |
"net/http" | ||
"strings" | ||
|
||
"github.com/google/uuid" | ||
"github.com/hyperledger/firefly-common/pkg/ffapi" | ||
"github.com/hyperledger/firefly-common/pkg/i18n" | ||
"github.com/hyperledger/firefly/internal/coremsgs" | ||
"github.com/hyperledger/firefly/pkg/core" | ||
) | ||
|
||
var getIdentityByID = &ffapi.Route{ | ||
Name: "getIdentityByID", | ||
Path: "identities/{iid}", | ||
var getIdentityByIDOrDID = &ffapi.Route{ | ||
Name: "getIdentityByIDOrDID", | ||
Path: "identities/{id:.+}", // Allow any character in the ID (including slashes), othrwise the DID will be split into multiple path segments | ||
Method: http.MethodGet, | ||
PathParams: []*ffapi.PathParam{ | ||
{Name: "iid", Example: "id", Description: coremsgs.APIParamsIdentityID}, | ||
{Name: "id", Example: "id", Description: coremsgs.APIParamsIdentityIDs}, | ||
}, | ||
QueryParams: []*ffapi.QueryParam{ | ||
{Name: "fetchverifiers", Example: "true", Description: coremsgs.APIParamsFetchVerifiers, IsBool: true}, | ||
}, | ||
Description: coremsgs.APIEndpointsGetIdentityByID, | ||
Description: coremsgs.APIEndpointsGetIdentityByIDOrDID, | ||
JSONInputValue: nil, | ||
JSONOutputValue: func() interface{} { return &core.Identity{} }, | ||
JSONOutputCodes: []int{http.StatusOK}, | ||
Extensions: &coreExtensions{ | ||
CoreJSONHandler: func(r *ffapi.APIRequest, cr *coreRequest) (output interface{}, err error) { | ||
if strings.EqualFold(r.QP["fetchverifiers"], "true") { | ||
return cr.or.NetworkMap().GetIdentityByIDWithVerifiers(cr.ctx, r.PP["iid"]) | ||
switch id := r.PP["id"]; { | ||
case isUUID(id): | ||
if strings.EqualFold(r.QP["fetchverifiers"], "true") { | ||
return cr.or.NetworkMap().GetIdentityByIDWithVerifiers(cr.ctx, id) | ||
} | ||
return cr.or.NetworkMap().GetIdentityByID(cr.ctx, id) | ||
case isDID(id): | ||
if strings.EqualFold(r.QP["fetchverifiers"], "true") { | ||
return cr.or.NetworkMap().GetIdentityByDIDWithVerifiers(cr.ctx, id) | ||
} | ||
return cr.or.NetworkMap().GetIdentityByDID(cr.ctx, id) | ||
} | ||
return cr.or.NetworkMap().GetIdentityByID(cr.ctx, r.PP["iid"]) | ||
return nil, i18n.NewError(cr.ctx, i18n.MsgUnknownIdentityType) | ||
}, | ||
}, | ||
} | ||
|
||
// isUUID checks if the given string is a UUID | ||
func isUUID(s string) bool { | ||
_, err := uuid.Parse(s) | ||
return err == nil | ||
} | ||
|
||
// isDID checks if the given string is a potential DID | ||
func isDID(s string) bool { | ||
return strings.HasPrefix(s, "did:") | ||
} | ||
Comment on lines
+70
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep the same level of check and change this to a regex expression - I think we should have one for DIDs already in the codebase There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we have the DID validation happening later in the code, I preferred to determine if this a DID string or not. Because if this is a DID but a not valid one, I want the user to receive the proper error code (vs in this case the error code will be "unknown format"). wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah because you are not catching the error in the switch statement, you could catch the error there if you wish to? It just feels since you have moved the validation here make sense to construct the correct error instead of relying on the function underneath to throw the right thing - but will leave up to you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use the google UUID library directly, use the one in firefly-common
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, we are not validating the input, rather we are just filtering the input, so I didn't think we need to call the UUID validator with the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just more of a consistency and dependency thing to use the firefly-common library for this