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

feat: Create/Get Profile Endpoint and store the profile data #10

Merged
merged 2 commits into from
Jan 20, 2020

Conversation

talwinder50
Copy link
Member

closes #2

Signed-off-by: talwinder.kaur talwinder.kaur@securekey.com

@cla-bot cla-bot bot added the cla-signed label Jan 15, 2020
@todo
Copy link

todo bot commented Jan 15, 2020

to be replaced by getting profile ID

https://github.com/trustbloc/edge-service/blob/bf9e2e781da4409842226d40ed2c828d2b54b155/pkg/restapi/vc/operation/operations.go#L217-L220


This comment was generated by todo based on a TODO comment in bf9e2e7 in #10. cc @talwinder50.

@codecov
Copy link

codecov bot commented Jan 15, 2020

Codecov Report

Merging #10 into master will decrease coverage by 4.93%.
The diff coverage is 87.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
- Coverage   94.59%   89.65%   -4.94%     
==========================================
  Files           4        5       +1     
  Lines          74      145      +71     
==========================================
+ Hits           70      130      +60     
- Misses          2        8       +6     
- Partials        2        7       +5
Impacted Files Coverage Δ
pkg/restapi/vc/controller.go 77.77% <60%> (-22.23%) ⬇️
pkg/restapi/vc/operation/profile.go 77.77% <77.77%> (ø)
pkg/restapi/vc/operation/operations.go 92.92% <92.53%> (-2.91%) ⬇️

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 c05bf23...281b8e2. Read the comment docs.

"github.com/trustbloc/edge-service/pkg/storage"
)

// MockVC mock did exchange service
Copy link
Contributor

Choose a reason for hiding this comment

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

this mock is referencing exchange service and protocols, lets discuss this

@@ -0,0 +1,90 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this memory store reused from other projects?

Copy link
Collaborator

@DRK3 DRK3 Jan 16, 2020

Choose a reason for hiding this comment

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

Firas, Sandra and I just talked - I am going to ask @troyronda if we can make another repo for just storage, since it looks like we're going to be using the same storage in the edv repo and this one

When couchdb support is ready, it'll go into that new repo as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes i reused it from aries-framework-go, agree it would make more sense to put that in separate project. I have pointed to edv for now.

@@ -111,11 +181,61 @@ func createCredential(data *CreateCrendential) (*verifiable.Credential, error) {
return validatedCred, nil
}

func (c *Operation) createProfile(pr *ProfileRequest) (*ProfileResponse, error) {
if pr.DID == "" || pr.URI == "" {
return nil, fmt.Errorf("profile request is bad")
Copy link
Contributor

Choose a reason for hiding this comment

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

error should be more informative: missing DID and/or URI information

Copy link
Member Author

Choose a reason for hiding this comment

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

done

)

// MockStoreProvider mock store provider.
type MockStoreProvider struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are mocks from edv too?

// Name return service name
func (m *MockVC) Name() string {
if m.ProtocolName != "" {
return m.ProtocolName
Copy link
Contributor

Choose a reason for hiding this comment

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

What is protocol name?

cmd/vc-rest/go.mod Show resolved Hide resolved
cmd/vc-rest/go.sum Show resolved Hide resolved
cmd/vc-rest/startcmd/start.go Show resolved Hide resolved
go.mod Show resolved Hide resolved
_, err = rw.Write([]byte(fmt.Sprintf("Received invalid request: %s", err.Error())))

if err != nil {
log.Errorf("Failed to write response for credential profile failure (unable to read request): %s", err.Error())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change this log error to "Failed to write response for invalid request received" or something similar so it matches the pattern of the other log error messages in this file

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to have common function for error response - it will make your unit-testing easier.
func (c *Operation) writeErrorResponse(rw http.ResponseWriter, status int, msg string) { rw.WriteHeader(status) if _, err := rw.Write([]byte(msg)); err != nil { log.Errorf("Unable to send error message, %s", err) } }

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Comment on lines 31 to 37
//
// Args:
//
//
// Returns:
//
// error: error
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't need this part of the comment. Or perhaps expand on it so it provides some more useful info if you think it's needed

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Comment on lines 51 to 59
//
// Args:
//
// id: profile id
//
// Returns:
//
// profile found
// error: error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need this part of the comment. Or perhaps expand on it so it provides some more useful info if you think it's needed

Copy link
Member Author

Choose a reason for hiding this comment

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

done

pkg/restapi/vc/operation/persistence.go Outdated Show resolved Hide resolved
pkg/restapi/vc/operation/persistence.go Outdated Show resolved Hide resolved
pkg/restapi/vc/operation/persistence.go Outdated Show resolved Hide resolved
@@ -89,7 +93,7 @@ func startEdgeService(parameters *vcRestParameters) error {
router.HandleFunc(handler.Path(), handler.Handle()).Methods(handler.Method())
}

err := parameters.srv.ListenAndServe(parameters.hostURL, router)
err = parameters.srv.ListenAndServe(parameters.hostURL, router)
Copy link
Contributor

Choose a reason for hiding this comment

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

you can return here

Copy link
Member Author

Choose a reason for hiding this comment

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

done

_, err = rw.Write([]byte(fmt.Sprintf("Received invalid request: %s", err.Error())))

if err != nil {
log.Errorf("Failed to write response for credential profile failure (unable to read request): %s", err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to have common function for error response - it will make your unit-testing easier.
func (c *Operation) writeErrorResponse(rw http.ResponseWriter, status int, msg string) { rw.WriteHeader(status) if _, err := rw.Write([]byte(msg)); err != nil { log.Errorf("Unable to send error message, %s", err) } }

pkg/restapi/vc/operation/operations.go Outdated Show resolved Hide resolved
pkg/restapi/vc/operation/operations.go Outdated Show resolved Hide resolved
closes: trustbloc#1


Signed-off-by: talwinder.kaur <talwinder.kaur@securekey.com>
@todo
Copy link

todo bot commented Jan 20, 2020

to be replaced by getting profile ID issue-47

https://github.com/trustbloc/edge-service/blob/dbcf514ed3e67eb2056349d9b8a9c7b727fdf98f/pkg/restapi/vc/operation/operations.go#L147-L150


This comment was generated by todo based on a TODO comment in dbcf514 in #10. cc @talwinder50.

if err != nil {
log.Errorf("Unable to send error response, %s", err)
if err == errProfileNotFound {
rw.WriteHeader(http.StatusNotFound)
Copy link
Contributor

Choose a reason for hiding this comment

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

header is also written in write error response, no need to write twice

Copy link
Member Author

Choose a reason for hiding this comment

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

done

return
}

rw.WriteHeader(http.StatusBadRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to write header

Copy link
Member Author

Choose a reason for hiding this comment

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

done

}

func getIssueDate() time.Time {
issueDate := time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day(),
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to call time.Now() this many times
also, can you use time.Now().UTC()

Copy link
Member Author

Choose a reason for hiding this comment

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

i have tried time.Now().UTC() and it worked. I remember it didn't work before, so i have updated now.

@@ -111,11 +158,65 @@ func createCredential(data *CreateCrendential) (*verifiable.Credential, error) {
return validatedCred, nil
}

func (c *Operation) createProfile(pr *ProfileRequest) (*ProfileResponse, error) {
if pr.DID == "" && pr.URI == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if pr.DID is blank but pr.URI isn't, is any error thrown? If not, you'll probably want to catch that.

closes trustbloc#2

Signed-off-by: talwinder.kaur <talwinder.kaur@securekey.com>
@talwinder50 talwinder50 merged commit 8e96b30 into trustbloc:master Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Credential Issuer Rest API
3 participants