Skip to content
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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@ $(eval $(call makemock, internal/apiserver, Server, apiser
$(eval $(call makemock, internal/events/websockets, WebSocketsNamespaced, websocketsmocks))

firefly-nocgo: ${GOFILES}
CGO_ENABLED=0 $(VGO) build -o ${BINARY_NAME}-nocgo -ldflags "-X main.buildDate=$(DATE) -X main.buildVersion=$(BUILD_VERSION) -X 'github.com/hyperledger/firefly/cmd.BuildVersionOverride=$(BUILD_VERSION)' -X 'github.com/hyperledger/firefly/cmd.BuildDate=$(DATE)' -X 'github.com/hyperledger/firefly/cmd.BuildCommit=$(GIT_REF)'" -tags=prod -tags=prod -v
CGO_ENABLED=0 $(VGO) build -o ${BINARY_NAME}-nocgo -ldflags "-X main.buildDate=$(DATE) -X main.buildVersion=$(BUILD_VERSION) -X 'github.com/hyperledger/firefly/cmd.BuildVersionOverride=$(BUILD_VERSION)' -X 'github.com/hyperledger/firefly/cmd.BuildDate=$(DATE)' -X 'github.com/hyperledger/firefly/cmd.BuildCommit=$(GIT_REF)'" -tags=prod -v
firefly: ${GOFILES}
$(VGO) build -o ${BINARY_NAME} -ldflags "-X main.buildDate=$(DATE) -X main.buildVersion=$(BUILD_VERSION) -X 'github.com/hyperledger/firefly/cmd.BuildVersionOverride=$(BUILD_VERSION)' -X 'github.com/hyperledger/firefly/cmd.BuildDate=$(DATE)' -X 'github.com/hyperledger/firefly/cmd.BuildCommit=$(GIT_REF)'" -tags=prod -tags=prod -v
$(VGO) build -o ${BINARY_NAME} -ldflags "-X main.buildDate=$(DATE) -X main.buildVersion=$(BUILD_VERSION) -X 'github.com/hyperledger/firefly/cmd.BuildVersionOverride=$(BUILD_VERSION)' -X 'github.com/hyperledger/firefly/cmd.BuildDate=$(DATE)' -X 'github.com/hyperledger/firefly/cmd.BuildCommit=$(GIT_REF)'" -tags=prod -v
go-mod-tidy: .ALWAYS
$(VGO) mod tidy
build: firefly-nocgo firefly
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ require (
github.com/ghodss/yaml v1.0.0
github.com/go-resty/resty/v2 v2.11.0
github.com/golang-migrate/migrate/v4 v4.17.0
github.com/google/uuid v1.5.0
github.com/gorilla/mux v1.8.1
github.com/gorilla/websocket v1.5.1
github.com/hyperledger/firefly-common v1.4.6
Expand Down Expand Up @@ -44,7 +45,6 @@ require (
github.com/fsnotify/fsnotify v1.7.0 // indirect
github.com/go-openapi/jsonpointer v0.20.2 // indirect
github.com/go-openapi/swag v0.22.7 // indirect
github.com/google/uuid v1.5.0 // indirect
github.com/hashicorp/errwrap v1.1.0 // indirect
github.com/hashicorp/go-multierror v1.1.1 // indirect
github.com/hashicorp/hcl v1.0.0 // indirect
Expand Down
4 changes: 2 additions & 2 deletions internal/apiserver/ffi2swagger.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func (swg *ffiSwaggerGen) Build(ctx context.Context, api *core.ContractAPI, ffi

func addFFIMethod(ctx context.Context, routes []*ffapi.Route, method *fftypes.FFIMethod, hasLocation bool) []*ffapi.Route {
description := method.Description
if method.Details != nil && len(method.Details) > 0 {
if len(method.Details) > 0 {
additionalDetailsHeader := i18n.Expand(ctx, coremsgs.APISmartContractDetails)
description = fmt.Sprintf("%s\n\n%s:\n\n%s", description, additionalDetailsHeader, buildDetailsTable(ctx, method.Details))
}
Expand Down Expand Up @@ -117,7 +117,7 @@ func addFFIMethod(ctx context.Context, routes []*ffapi.Route, method *fftypes.FF

func addFFIEvent(ctx context.Context, routes []*ffapi.Route, event *fftypes.FFIEvent, hasLocation bool) []*ffapi.Route {
description := event.Description
if event.Details != nil && len(event.Details) > 0 {
if len(event.Details) > 0 {
additionalDetailsHeader := i18n.Expand(ctx, coremsgs.APISmartContractDetails)
description = fmt.Sprintf("%s\n\n%s:\n\n%s", description, additionalDetailsHeader, buildDetailsTable(ctx, event.Details))
}
Expand Down
50 changes: 0 additions & 50 deletions internal/apiserver/route_get_identity_by_did.go

This file was deleted.

59 changes: 0 additions & 59 deletions internal/apiserver/route_get_identity_by_did_test.go

This file was deleted.

38 changes: 30 additions & 8 deletions internal/apiserver/route_get_identity_by_id.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright © 2022 Kaleido, Inc.

Check failure on line 1 in internal/apiserver/route_get_identity_by_id.go

View workflow job for this annotation

GitHub Actions / build

Expected:2024, Actual: 2022 Kaleido, Inc. (goheader)
//
// SPDX-License-Identifier: Apache-2.0
//
Expand All @@ -20,31 +20,53 @@
"net/http"
"strings"

"github.com/google/uuid"
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

"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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

32 changes: 32 additions & 0 deletions internal/apiserver/route_get_identity_by_id_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,35 @@ func TestGetIdentityByIDWithVerifiers(t *testing.T) {

assert.Equal(t, 200, res.Result().StatusCode)
}

func TestGetIdentityByDID(t *testing.T) {
o, r := newTestAPIServer()
o.On("Authorize", mock.Anything, mock.Anything).Return(nil)
nmn := &networkmapmocks.Manager{}
o.On("NetworkMap").Return(nmn)
req := httptest.NewRequest("GET", "/api/v1/identities/did:firefly:org/org_1", nil)
req.Header.Set("Content-Type", "application/json; charset=utf-8")
res := httptest.NewRecorder()

nmn.On("GetIdentityByDID", mock.Anything, "did:firefly:org/org_1").
Return(&core.Identity{}, nil)
r.ServeHTTP(res, req)

assert.Equal(t, 200, res.Result().StatusCode)
}

func TestGetIdentityByDIDWithVerifiers(t *testing.T) {
o, r := newTestAPIServer()
o.On("Authorize", mock.Anything, mock.Anything).Return(nil)
nmn := &networkmapmocks.Manager{}
o.On("NetworkMap").Return(nmn)
req := httptest.NewRequest("GET", "/api/v1/identities/did:firefly:org/org_1?fetchverifiers", nil)
req.Header.Set("Content-Type", "application/json; charset=utf-8")
res := httptest.NewRecorder()

nmn.On("GetIdentityByDIDWithVerifiers", mock.Anything, "did:firefly:org/org_1").
Return(&core.IdentityWithVerifiers{}, nil)
r.ServeHTTP(res, req)

assert.Equal(t, 200, res.Result().StatusCode)
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,4 @@ func TestGetSubscriptionEventsFilteredNoSequenceIDsProvided(t *testing.T) {

r.ServeHTTP(res, req)
assert.Equal(t, 200, res.Result().StatusCode)
}
}
7 changes: 3 additions & 4 deletions internal/apiserver/route_patch_update_identity.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ import (

var patchUpdateIdentity = &ffapi.Route{
Name: "patchUpdateIdentity",
Path: "identities/{iid}",
Path: "identities/{id}",
Method: http.MethodPatch,
PathParams: []*ffapi.PathParam{
{Name: "iid", Description: coremsgs.APIParamsIdentityID},
{Name: "id", Description: coremsgs.APIParamsIdentityID},
},
QueryParams: []*ffapi.QueryParam{
{Name: "confirm", Description: coremsgs.APIConfirmMsgQueryParam, IsBool: true},
Expand All @@ -43,8 +43,7 @@ var patchUpdateIdentity = &ffapi.Route{
CoreJSONHandler: func(r *ffapi.APIRequest, cr *coreRequest) (output interface{}, err error) {
waitConfirm := strings.EqualFold(r.QP["confirm"], "true")
r.SuccessStatus = syncRetcode(waitConfirm)
org, err := cr.or.NetworkMap().UpdateIdentity(cr.ctx, r.PP["iid"], r.Input.(*core.IdentityUpdateDTO), waitConfirm)
return org, err
return cr.or.NetworkMap().UpdateIdentity(cr.ctx, r.PP["id"], r.Input.(*core.IdentityUpdateDTO), waitConfirm)
},
},
}
3 changes: 1 addition & 2 deletions internal/apiserver/routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,7 @@ var routes = append(
getGroupByHash,
getGroups,
getIdentities,
getIdentityByDID,
getIdentityByID,
getIdentityByIDOrDID,
getIdentityDID,
getIdentityVerifiers,
getMsgByID,
Expand Down
3 changes: 2 additions & 1 deletion internal/coremsgs/en_api_translations.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ var (
APIParamsGroupHash = ffm("api.params.groupID", "The hash of the group")
APIParamsFetchVerifiers = ffm("api.params.fetchVerifiers", "When set, the API will return the verifier for this identity")
APIParamsIdentityID = ffm("api.params.identityID", "The identity ID, which is a UUID generated by FireFly")
APIParamsIdentityIDs = ffm("api.params.identityIDs", "The identity ID, which is a UUID generated by FireFly or the identity DID")
APIParamsMessageID = ffm("api.params.messageID", "The message ID")
APIParamsDID = ffm("api.params.DID", "The identity DID")
APIParamsNodeNameOrID = ffm("api.params.nodeNameOrID", "The name or ID of the node")
Expand Down Expand Up @@ -107,7 +108,7 @@ var (
APIEndpointsGetGroupByHash = ffm("api.endpoints.getGroupByHash", "Gets a group by its ID (hash)")
APIEndpointsGetGroups = ffm("api.endpoints.getGroups", "Gets a list of groups")
APIEndpointsGetIdentities = ffm("api.endpoints.getIdentities", "Gets a list of all identities that have been registered in the namespace")
APIEndpointsGetIdentityByID = ffm("api.endpoints.getIdentityByID", "Gets an identity by its ID")
APIEndpointsGetIdentityByIDOrDID = ffm("api.endpoints.getIdentityByIDOrDID", "Gets an identity by its ID or DID")
APIEndpointsGetIdentityDID = ffm("api.endpoints.getIdentityDID", "Gets the DID for an identity based on its ID")
APIEndpointsGetIdentityVerifiers = ffm("api.endpoints.getIdentityVerifiers", "Gets the verifiers for an identity")
APIEndpointsGetMsgByID = ffm("api.endpoints.getMsgByID", "Gets a message by its ID")
Expand Down
5 changes: 2 additions & 3 deletions internal/database/sqlcommon/event_sql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func TestGetEventsInSequenceRangeE2EWithDB(t *testing.T) {
Type: core.EventTypeMessageConfirmed,
Reference: fftypes.NewUUID(),
Correlator: fftypes.NewUUID(),
Topic: fmt.Sprintf("topic%d", i % 2),
Topic: fmt.Sprintf("topic%d", i%2),
Created: fftypes.Now(),
}
err := s.InsertEvent(ctx, event)
Expand Down Expand Up @@ -322,10 +322,9 @@ func TestGetEventsInSequenceRangeBuildQueryFail(t *testing.T) {

func TestGetEventsInSequenceRangeShouldCallGetEventsWhenNoSequencedProvidedAndThrowAnError(t *testing.T) {
s, mock := newMockProvider().init()
mock.ExpectQuery("SELECT .*").WillReturnRows(sqlmock.NewRows([]string{"id", }).AddRow("only one"))
mock.ExpectQuery("SELECT .*").WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("only one"))
f := database.EventQueryFactory.NewFilter(context.Background()).And()
_, _, err := s.GetEventsInSequenceRange(context.Background(), "ns1", f, -1, -1)
assert.NotNil(t, err)
assert.NoError(t, mock.ExpectationsWereMet())
}

2 changes: 1 addition & 1 deletion internal/events/event_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,7 @@ func TestEventFilterOnSubscriptionMatchesEventType(t *testing.T) {
filteredEvents, _ = em.FilterHistoricalEventsOnSubscription(context.Background(), events, subscription)
assert.NotNil(t, filteredEvents)
assert.Equal(t, 1, len(filteredEvents))

listenerUuid := fftypes.NewUUID()

events[0].Event.Topic = ""
Expand Down
2 changes: 1 addition & 1 deletion internal/events/subscription_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ func (sm *subscriptionManager) connectionClosed(ei events.Plugin, connID string)
sm.mux.Lock()
conn, ok := sm.connections[connID]
if ok && conn.ei != ei {
log.L(sm.ctx).Warnf(i18n.ExpandWithCode(sm.ctx, i18n.MessageKey(coremsgs.MsgMismatchedTransport), connID, ei.Name(), conn.ei.Name()))
log.L(sm.ctx).Warnln(i18n.ExpandWithCode(sm.ctx, i18n.MessageKey(coremsgs.MsgMismatchedTransport), connID, ei.Name(), conn.ei.Name()))
sm.mux.Unlock()
return
}
Expand Down
2 changes: 1 addition & 1 deletion internal/events/websockets/websockets_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1219,4 +1219,4 @@ func TestHandleStartWrongNamespace(t *testing.T) {
err := wc.handleStart(startMessage)
assert.Error(t, err)
assert.Regexp(t, "FF10462", err)
}
}
6 changes: 3 additions & 3 deletions internal/reference/reference.go
Original file line number Diff line number Diff line change
Expand Up @@ -790,22 +790,22 @@ func generateObjectReferenceMarkdown(ctx context.Context, descRequired bool, exa
if typeReferenceDoc.Description != nil {
buff.Write(typeReferenceDoc.Description)
}
if typeReferenceDoc.Example != nil && len(typeReferenceDoc.Example) > 0 {
if len(typeReferenceDoc.Example) > 0 {
if sectionCount > 1 {
buff.WriteString("### Example\n\n```json\n")
}
buff.Write(typeReferenceDoc.Example)
buff.WriteString("\n```\n\n")
}
if typeReferenceDoc.FieldDescriptions != nil && len(typeReferenceDoc.FieldDescriptions) > 0 {
if len(typeReferenceDoc.FieldDescriptions) > 0 {
if sectionCount > 1 {
buff.WriteString("### Field Descriptions\n\n")
}
buff.Write(typeReferenceDoc.FieldDescriptions)
buff.WriteString("\n")
}

if typeReferenceDoc.SubFieldTables != nil && len(typeReferenceDoc.SubFieldTables) > 0 {
if len(typeReferenceDoc.SubFieldTables) > 0 {
buff.Write(typeReferenceDoc.SubFieldTables)
}

Expand Down
Loading
Loading