chore: Remove getIdentityByDID route #1558

dwertent · 2024-08-14T20:14:34Z

Overview

Fixed issues with rendering Swagger documentation.
Addressed some linter warnings.

Swagger Documentation Issue

The problem occurred when rendering the Swagger documentation. The code is designed to list API paths and generate documentation based on those paths. However, an issue arose in the way the Find function was used. This function is agnostic to the specific value of the API keys, which led to the swagger generator pulling in the wrong documentation.

Solution

I considered the following potential solutions:

Implement a custom Find function that is aware of the API key value.
Combine the API functions so that identities/{ID} can accept either a UUID or a DID.

I chose the second solution for the following reasons:

A different organization maintains the Find function, and it may take time to implement a custom version.
Since the API paths for handling DID and UUID are the same, it's logical to use a single function to handle both types of requests.

Side Note

I recommend deprecating DID support in favor of using UUIDs exclusively. This would simplify our API by providing a single, consistent definition for ID, reducing potential confusion and complexity in future development.

This PR fixes #1528

Signed-off-by: dwertent <[email protected]>

…Identity routes The route parameter name "id" was updated in the getIdentityByID and patchUpdateIdentity routes to improve clarity and consistency. Signed-off-by: dwertent <[email protected]>

EnriqueL8

Based on your description the kin-openapi tool is not smart enough to differentiate between the same API paths but with different path parameters so the second one in the order will override the first one. In this case /identities/{did} overrides /identities/{iid} which results in the issue. You have decided to combine both those API calls into one generic /identities/{id}.

This introduces a slightly breaking change as the name of the path parameter is now different so the swagger will be different but the requests should still work. The description now is generic for both ID and DID.

The side note is not valid as DIDs are different to the IDs used internal by FireFly. A Decentralized identity (DID) is used to enable decentralised verification and is pinned on-chain, associated with a key. The IDs used here are purely a way for FireFly to uniquely identify in the DB the identities that this FireFly cares about and it's only present in the context of one single FireFly instance.

EnriqueL8 · 2024-08-15T08:59:04Z

internal/apiserver/server.go

-	as.namespacedContractSwaggerGenerator(hf, r, mgr, as.apiPublicURL, `/api/swagger.json`, ffapi.OpenAPIFormatJSON)
-	as.namespacedContractSwaggerGenerator(hf, r, mgr, as.apiPublicURL, `/api/openapi.json`, ffapi.OpenAPIFormatJSON)
-	as.namespacedContractSwaggerGenerator(hf, r, mgr, as.apiPublicURL, `/api/swagger.yaml`, ffapi.OpenAPIFormatYAML)
-	as.namespacedContractSwaggerGenerator(hf, r, mgr, as.apiPublicURL, `/api/openapi.yaml`, ffapi.OpenAPIFormatYAML)


We need to keep this change

I think it's just the linter is saying that the arguments are not being used in the function - ideally we fix the bug there and make it work properly, maybe not as part of this PR

I will revert my changes and open an issue so we can address this later

EnriqueL8 · 2024-08-15T08:59:13Z

internal/apiserver/server.go

@@ -300,7 +300,7 @@ func (as *apiServer) namespacedSwaggerUI(hf *ffapi.HandlerFactory, r *mux.Router
 	}))
 }

-func (as *apiServer) namespacedContractSwaggerGenerator(hf *ffapi.HandlerFactory, r *mux.Router, mgr namespace.Manager, publicURL, relativePath string, format ffapi.OpenAPIFormat) {


Same here keep this

This just seems to be a bug that the publicURL is maybe not used?

EnriqueL8 · 2024-08-15T09:00:30Z

internal/apiserver/server.go

@@ -288,7 +288,7 @@ func (as *apiServer) nsOpenAPIHandlerFactory(req *http.Request, publicURL string
 	}
 }

-func (as *apiServer) namespacedSwaggerHandler(hf *ffapi.HandlerFactory, r *mux.Router, publicURL, relativePath string, format ffapi.OpenAPIFormat) {


probably better to fix the bug and not remove this

EnriqueL8 · 2024-08-15T10:14:29Z

internal/apiserver/route_get_identity_by_id.go

+func isDID(s string) bool {
+	return strings.HasPrefix(s, "did:")
+}


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

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?

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

EnriqueL8 · 2024-08-15T10:14:50Z

internal/apiserver/route_get_identity_by_id.go

@@ -20,17 +20,19 @@ import (
 	"net/http"
 	"strings"

+	"github.com/google/uuid"


We don't use the google UUID library directly, use the one in firefly-common

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.

just more of a consistency and dependency thing to use the firefly-common library for this

EnriqueL8 · 2024-08-15T10:15:52Z

internal/coremsgs/en_api_translations.go

@@ -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")
+	APIEndpointsGetIdentityByID                 = ffm("api.endpoints.getIdentityByID", "Gets an identity by its ID (UUID/DID)")


Suggested change

APIEndpointsGetIdentityByID = ffm("api.endpoints.getIdentityByID", "Gets an identity by its ID (UUID/DID)")

APIEndpointsGetIdentityByID = ffm("api.endpoints.getIdentityByID", "Gets an identity by its ID or DID")

internal/syncasync/sync_async_bridge.go

internal/apiserver/route_get_identity_by_id.go

EnriqueL8 · 2024-08-15T10:22:19Z

internal/apiserver/route_get_identity_by_id.go

 	"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{


Suggested change

var getIdentityByID = &ffapi.Route{

var getIdentityByIDOrDID = &ffapi.Route{

dwertent · 2024-08-15T13:23:56Z

Summarizing your review:

This PR should not break backward compatibility because iid/DID are not query params, mainly placeholders.
I will revert unused code to the linter fixes I made.

Signed-off-by: dwertent <[email protected]>

EnriqueL8 · 2024-08-15T15:14:28Z

@dwertent build failing and e2e tests as well

EnriqueL8 · 2024-08-15T15:15:09Z

something wrong with the UUID verification restclient.go:118: <!! {"error":"FF00126: Unknown identity type: %!!(MISSING)s(MISSING)"}

dwertent · 2024-08-16T15:00:54Z

This PR is no longer needed as we've resolved the issue with a different approach. I will open a new PR to update the FireFly-common tag once it's published.

dwertent added 2 commits August 14, 2024 15:28

chore: Remove getIdentityByDID route

0ae9414

Signed-off-by: dwertent <[email protected]>

chore: Update route parameter name in getIdentityByID and patchUpdate…

bedbcf2

…Identity routes The route parameter name "id" was updated in the getIdentityByID and patchUpdateIdentity routes to improve clarity and consistency. Signed-off-by: dwertent <[email protected]>

dwertent requested a review from a team as a code owner August 14, 2024 20:14

EnriqueL8 reviewed Aug 15, 2024

View reviewed changes

dwertent added 2 commits August 15, 2024 11:00

wip: reverting linter fixes

ab093be

Signed-off-by: dwertent <[email protected]>

wip: renaming the api function

0ebc1d8

Signed-off-by: dwertent <[email protected]>

dwertent mentioned this pull request Aug 16, 2024

chore: Refactor SwaggerGen to support multiple values with the same path hyperledger/firefly-common#145

Open

dwertent closed this Aug 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Remove getIdentityByDID route #1558

chore: Remove getIdentityByDID route #1558

dwertent commented Aug 14, 2024

EnriqueL8 left a comment

EnriqueL8 Aug 15, 2024

EnriqueL8 Aug 15, 2024

dwertent Aug 15, 2024

EnriqueL8 Aug 15, 2024

EnriqueL8 Aug 15, 2024

EnriqueL8 Aug 15, 2024

EnriqueL8 Aug 15, 2024

dwertent Aug 15, 2024

EnriqueL8 Aug 15, 2024

EnriqueL8 Aug 15, 2024

dwertent Aug 15, 2024

EnriqueL8 Aug 15, 2024

EnriqueL8 Aug 15, 2024

EnriqueL8 Aug 15, 2024

dwertent commented Aug 15, 2024

EnriqueL8 commented Aug 15, 2024

EnriqueL8 commented Aug 15, 2024

dwertent commented Aug 16, 2024

	APIEndpointsGetIdentityByID = ffm("api.endpoints.getIdentityByID", "Gets an identity by its ID (UUID/DID)")
	APIEndpointsGetIdentityByID = ffm("api.endpoints.getIdentityByID", "Gets an identity by its ID or DID")

	var getIdentityByID = &ffapi.Route{
	var getIdentityByIDOrDID = &ffapi.Route{

chore: Remove getIdentityByDID route #1558

chore: Remove getIdentityByDID route #1558

Conversation

dwertent commented Aug 14, 2024

Overview

Swagger Documentation Issue

Solution

Side Note

EnriqueL8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwertent commented Aug 15, 2024

EnriqueL8 commented Aug 15, 2024

EnriqueL8 commented Aug 15, 2024

dwertent commented Aug 16, 2024