Skip to content

Conversation

@awrichar
Copy link
Contributor

See #583

This is more of a starting point - the functionality works, but I'm pretty certain the API spelling is not what we want.

I was struggling with the best way to provide this functionality - it seems like it must be a query param, because / is a valid char for query params but not for path params. Assuming we do use a query param, it would also be nice to flag it as "required" from the Swagger perspective.

Side note: you can already query the /namespaces/{ns}/identities endpoint with a query param for did, but you have to know the namespace you're looking for in that case.

See hyperledger#583

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
o, r := newTestAPIServer()
nmn := &networkmapmocks.Manager{}
o.On("NetworkMap").Return(nmn)
req := httptest.NewRequest("GET", "/api/v1/network/did?did=did:firefly:org/org_1", nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this looks weird 🙃

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was reading https://w3c-ccg.github.io/did-resolution/#example-0

Wondering about:
/api/v1/network/identities/did:firefly:org/org_1

I think that could work. Given @awrichar is out on vacation, I might make this proposal in code for @nguyer to review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - that's done @nguyer, including sorting the swagger generator so it can handle the custom Gorilla mux syntax.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2022

Codecov Report

Merging #617 (a20e09f) into main (bdf4b4a) will not change coverage.
The diff coverage is 100.00%.

❗ Current head a20e09f differs from pull request most recent head fd13992. Consider uploading reports for the commit fd13992 to get more accurate results

@@            Coverage Diff            @@
##              main      #617   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          309       310    +1     
  Lines        18705     18720   +15     
=========================================
+ Hits         18705     18720   +15     
Impacted Files Coverage Δ
internal/networkmap/manager.go 100.00% <ø> (ø)
internal/apiserver/route_get_net_did.go 100.00% <100.00%> (ø)
internal/apiserver/route_get_net_node.go 100.00% <100.00%> (ø)
internal/apiserver/route_get_net_org.go 100.00% <100.00%> (ø)
internal/networkmap/data_query.go 100.00% <100.00%> (ø)
internal/oapispec/openapi3.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06f0ed3...fd13992. Read the comment docs.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
…s by DID/org-name in identity registration

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst
Copy link
Contributor

peterbroadhurst commented Mar 21, 2022

Made a couple more enhancements:

  • The DID resolution API uses the same cached lookup API that other code paths use in the codebase.
  • Creation of a new identity can also use a DID (or org name) for the parent rather than having to use the UUID directly

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@@ -0,0 +1,42 @@
// Copyright © 2021 Kaleido, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised the linter didn't complain about the year here

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
@nguyer nguyer merged commit 1eff364 into hyperledger:main Mar 21, 2022
@nguyer nguyer deleted the did branch March 21, 2022 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants