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

myst-100 tequilapi create identity / register identity #62

Merged
merged 13 commits into from Dec 21, 2017

Conversation

Projects
None yet
4 participants
@shroomist
Copy link
Contributor

commented Dec 19, 2017

No description provided.

)

type identityManager struct {
keystoreManager keystoreInterface
discovery server.Client

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Currently name was MysteriumClient

}

func toRegisterRequest(
req *http.Request, params httprouter.Params) (id dto.Identity, isRegisterReq identityRegistrationDto, err error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

All args should be newline

idmEnd := NewIdentitiesEndpoint(idm)
router.GET("/identities", idmEnd.List)
router.POST("/identities", idmEnd.Create)
//router.GET("/identities/:id/registration", idmEnd.IsRegister)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Dont commit unused code

var identityUrl = "/identities/123/registration"

func TestRegisterExistingIdentityRequest(t *testing.T) {
req, err := http.NewRequest(

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Weird formating

}

func (endpoint *identitiesApi) Register(writer http.ResponseWriter, request *http.Request, params httprouter.Params) {
id, registerReq, err := toRegisterRequest(request, params)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Dont mix identityId reading into same fucntion

req, err := http.NewRequest(
http.MethodPut,
identityUrl,
bytes.NewBufferString(

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Weird formatting

"github.com/mysterium/node/identity"
"github.com/stretchr/testify/assert"
)

func TestListIdentities(t *testing.T) {
var identityUrl = "/identities/123/registration"

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member
  • It could be constant
  • Dont mislead with wrong URL . I would put just "" or "/test"
handlerFunc := NewIdentitiesEndpoint(mockIdm).Register
handlerFunc(resp, req, nil)

assert.Equal(t, 501, resp.Code)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

We should freeze error DTO with test also

@@ -68,3 +71,8 @@ func (idm *identityManager) HasIdentity(identityString string) bool {

return err == nil
}

func (idm *identityManager) Register(id dto.Identity) (err error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Dont mix RestClient into identity manager, it started doing 2 responsibilities.
Just call MysteriumClient.RegisterIdentity where you need.

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 19, 2017

Member

What if registration will be doing more than just calling our api endpoint? for example broadcasting event? identity manager as a facade service for all identity management functionality seems to be the right plays to keep all related code in one place. Doing 2 or more responsibilities should viewed as providing some kind of services to caller like register , create, find etc.
If we use low level actions (call mysterium api) directly, refactoring will be problematic later as caller will need to know more and more implementation details about registering new identity.

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 19, 2017

Member

@Waldz This is more a question to you :)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

But it's strange&inefficient to load MysteriuRestClient, for action like GetIdentity
Later I see responsibilieties like this:
IdentityManager - jungle with local identities
IdentityServiceClient - jungle with identity endpoint

req, err := http.NewRequest(
http.MethodPost,
"/identities",
bytes.NewBufferString(

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Weird formating


assert.JSONEq(
t,
`

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Weird formatting

req, err := http.NewRequest(
http.MethodPost,
"/identities",
bytes.NewBufferString(

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Weird formating


func validateRegistrationRequest(regReq identityRegistrationDto) (err error) {
if regReq.Registered == false {
err = errors.New("Deregistration not supported")

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Unregister not supported

}
err = validateRegistrationRequest(registerReq)
if err != nil {
utils.SendError(writer, err, 501)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member
  • is it really validation?
  • If so, validation errors should be 400 + formatted always with utils.SendValidationErrorMessage()
}
id, err := endpoint.idm.CreateNewIdentity(createReq.Password)
if err != nil {
utils.SendError(writer, err, http.StatusInternalServerError) // This should never happen

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Useless comment


func (idm *identityManager) Register(id dto.Identity) (err error) {
err = idm.discovery.RegisterIdentity(id)
return

This comment has been minimized.

Copy link
@donce

donce Dec 19, 2017

Contributor
return idm.discovery.RegisterIdentity(id)

func (idm *identityManager) Register(id dto.Identity) (err error) {
err = idm.discovery.RegisterIdentity(id)
return

This comment has been minimized.

Copy link
@donce

donce Dec 19, 2017

Contributor

Is named err value really useful here? This seems more straightforward.

return idm.discovery.RegisterIdentity(id)
isRegisterReq = identityRegistrationDto{}
err = json.NewDecoder(req.Body).Decode(&isRegisterReq)
if err != nil {
return

This comment has been minimized.

Copy link
@donce

donce Dec 19, 2017

Contributor

Useless if - maybe it should be return nil, err?

shroomist added some commits Dec 19, 2017

Merge branch 'master' of https://github.com/MysteriumNetwork/node int…
…o feature/MYST-100-tequilapi-create-id

# Conflicts:
#	identity/manager.go
#	identity/manager_fake.go
Merge branch 'master' of https://github.com/MysteriumNetwork/node int…
…o feature/MYST-100-tequilapi-create-id

# Conflicts:
#	identity/manager.go
#	identity/manager_fake.go
return
}

idDto := identityDto{string(id)}

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Dont cast, but map ;)

@@ -33,6 +46,86 @@ func (endpoint *identitiesApi) List(writer http.ResponseWriter, request *http.Re
utils.WriteAsJson(idsSerializable, writer)
}

func RegisterIdentitiesEndpoint(router *httprouter.Router, idm identity.IdentityManagerInterface) {
router.GET("/identities", NewIdentitiesEndpoint(idm).List)
func (endpoint *identitiesApi) Create(writer http.ResponseWriter, request *http.Request, _ httprouter.Params) {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

writer -> response is better name

http.MethodPut,
identityUrl,
bytes.NewBufferString(
`{

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

misformatted


assert.JSONEq(
t,
`{

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

Align JSON formatting style with project style see openvpn/service_discovery/dto/service_definition_test.go

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 20, 2017

Author Contributor

no such file. I don't understand how you want to format this.

@shroomist

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2017

gofmt fixing imports for me. shall I revert?

@shroomist shroomist force-pushed the feature/MYST-100-tequilapi-create-id branch 4 times, most recently from 7ffbe48 to d49e83f Dec 20, 2017

@shroomist

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2017

guys, can we make this work? please approve.

@shroomist

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2017

formatting comments fixed, anything else from @tadovas or @ignasbernotas?
can we merge this?

@Waldz

Waldz approved these changes Dec 21, 2017

@Waldz Waldz merged commit dcd217c into master Dec 21, 2017

@Waldz Waldz deleted the feature/MYST-100-tequilapi-create-id branch Dec 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.