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-224 Refactor identityHandler, add tests #89

Merged
merged 6 commits into from Jan 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions cmd/mysterium_server/command_run/factory.go
Expand Up @@ -33,15 +33,16 @@ func NewCommandWith(
) *CommandRun {

keystoreInstance := keystore.NewKeyStore(options.DirectoryKeystore, keystore.StandardScryptN, keystore.StandardScryptP)
cache := identity.NewIdentityCache(options.DirectoryKeystore, "remember.json")
identityHandler := NewNodeIdentityHandler(
identity.NewIdentityManager(keystoreInstance),
mysteriumClient,
options.DirectoryKeystore,
cache,
)

return &CommandRun{
identitySelector: func() (identity.Identity, error) {
return identityHandler.Select(options.NodeKey)
return SelectIdentity(identityHandler, options.NodeKey)
},
ipifyClient: ipifyClient,
mysteriumClient: mysteriumClient,
Expand Down
36 changes: 18 additions & 18 deletions cmd/mysterium_server/command_run/identity_handler.go
Expand Up @@ -6,39 +6,39 @@ import (
"github.com/mysterium/node/server"
)

func SelectIdentity(identityHandler *identityHandler, keyOption string) (id identity.Identity, err error) {
if len(keyOption) > 0 {
return identityHandler.UseExisting(keyOption)
}

if id, err = identityHandler.UseLast(); err == nil {
return id, err
}

return identityHandler.UseNew()
}

const nodeIdentityPassword = ""

type identityHandler struct {
manager identity.IdentityManagerInterface
identityApi server.Client
cache *identity.IdentityCache
cache identity.IdentityCacheInterface
}

func NewNodeIdentityHandler(
manager identity.IdentityManagerInterface,
identityApi server.Client,
cacheDir string,
cache identity.IdentityCacheInterface,
) *identityHandler {
return &identityHandler{
manager: manager,
identityApi: identityApi,
cache: identity.NewIdentityCache(cacheDir, "remember.json"),
cache: cache,
}
}

func (ih *identityHandler) Select(identityAddressWanted string) (id identity.Identity, err error) {
if len(identityAddressWanted) > 0 {
return ih.useExisting(identityAddressWanted)
}

if id, err = ih.useLast(); err == nil {
return id, err
}

return ih.useNew()
}

func (ih *identityHandler) useExisting(address string) (id identity.Identity, err error) {
func (ih *identityHandler) UseExisting(address string) (id identity.Identity, err error) {
id, err = ih.manager.GetIdentity(address)
if err != nil {
return
Expand All @@ -48,7 +48,7 @@ func (ih *identityHandler) useExisting(address string) (id identity.Identity, er
return
}

func (ih *identityHandler) useLast() (identity identity.Identity, err error) {
func (ih *identityHandler) UseLast() (identity identity.Identity, err error) {
identity, err = ih.cache.GetIdentity()
if err != nil || !ih.manager.HasIdentity(identity.Address) {
return identity, errors.New("identity not found in cache")
Expand All @@ -57,7 +57,7 @@ func (ih *identityHandler) useLast() (identity identity.Identity, err error) {
return identity, nil
}

func (ih *identityHandler) useNew() (id identity.Identity, err error) {
func (ih *identityHandler) UseNew() (id identity.Identity, err error) {
// if all fails, create a new one
id, err = ih.manager.CreateNewIdentity(nodeIdentityPassword)
if err != nil {
Expand Down
51 changes: 51 additions & 0 deletions cmd/mysterium_server/command_run/identity_handler_test.go
@@ -0,0 +1,51 @@
package command_run

import (
"testing"

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

func Test_identityHandler_UseExisting(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

+100

identityManager := identity.NewIdentityManagerFake()
client := server.NewClientFake()
cache := identity.NewIdentityCacheFake()

handler := NewNodeIdentityHandler(identityManager, client, cache)

id, err := handler.UseExisting("address")
Copy link
Member

Choose a reason for hiding this comment

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

But such value "address" is not mocked in identityManager. is not it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This address is ignored in fake identity manager:

func (fakeIdm *idmFake) GetIdentity(_ string) (Identity, error) {
	return fakeIdm.FakeIdentity1, nil
}

Copy link
Member

Choose a reason for hiding this comment

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

But how can we use existing "address" if it dont exist actually. Could could double check, plz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double checked - everything seems ok, not sure what's confusing you.

assert.Equal(t, identityManager.FakeIdentity1, id)
Copy link
Member

Choose a reason for hiding this comment

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

Could your assert that no identity was registered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

assert.Nil(t, err)
assert.Equal(t, "", client.RegisteredIdentity.Address)
}

func Test_identityHandler_UseLast(t *testing.T) {
identityManager := identity.NewIdentityManagerFake()
client := server.NewClientFake()
cache := identity.NewIdentityCacheFake()

fakeIdentity := identity.FromAddress("abc")
cache.StoreIdentity(fakeIdentity)

handler := NewNodeIdentityHandler(identityManager, client, cache)

id, err := handler.UseLast()
assert.Equal(t, fakeIdentity, id)
Copy link
Member

Choose a reason for hiding this comment

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

Could your assert that no identity was registered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

assert.Nil(t, err)
assert.Equal(t, "", client.RegisteredIdentity.Address)
}

func Test_identityHandler_UseNew(t *testing.T) {
identityManager := identity.NewIdentityManagerFake()
client := server.NewClientFake()
cache := identity.NewIdentityCacheFake()

handler := NewNodeIdentityHandler(identityManager, client, cache)

id, err := handler.UseNew()
assert.Equal(t, identityManager.FakeIdentity2, client.RegisteredIdentity)
assert.Equal(t, identityManager.FakeIdentity2, id)
assert.Nil(t, err)
}
18 changes: 10 additions & 8 deletions identity/cache.go
Expand Up @@ -2,6 +2,7 @@ package identity

import (
"encoding/json"
"errors"
"io/ioutil"
"os"
"path/filepath"
Expand All @@ -15,23 +16,24 @@ type IdentityCache struct {
File string
}

func NewIdentityCache(dir string, jsonFile string) *IdentityCache {
func NewIdentityCache(dir string, jsonFile string) IdentityCacheInterface {
return &IdentityCache{
File: filepath.Join(dir, jsonFile),
}
}

func (ic *IdentityCache) GetIdentity() (identity Identity, err error) {
if ic.cacheExists() {
cache, err := ic.readCache()
if err != nil {
return identity, err
}
if !ic.cacheExists() {
err = errors.New("cache file does not exist")
Copy link
Member

Choose a reason for hiding this comment

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

It's not an error if file is missing. It's same as "identity non found in cache"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. This method is named GetIdentity and it seems a common sense to get an identity or an error.

  2. Looking in identity_handler.go:

	identity, err = ih.cache.GetIdentity()
	if err != nil || !ih.manager.HasIdentity(identity.Address) {
		return identity, errors.New("identity not found in cache")
	}

If cache file does not exist, this results in "identity not found in cache" error either way - with or without this change.
What this change is about is not only making method more predictable, but also avoiding calling
ih.manager.HasIdentity("") for empty identity, which is returned when cache does not exist. If we know that cache file does not exist, we should not check for an empty identity in identity manager.

return
}

return cache.Identity, nil
cache, err := ic.readCache()
if err != nil {
return
}

return
return cache.Identity, nil
}

func (ic *IdentityCache) StoreIdentity(identity Identity) error {
Expand Down
18 changes: 18 additions & 0 deletions identity/cache_fake.go
@@ -0,0 +1,18 @@
package identity

type identityCacheFake struct {
identity Identity
}

func NewIdentityCacheFake() IdentityCacheInterface {
return &identityCacheFake{}
}

func (icf *identityCacheFake) GetIdentity() (identity Identity, err error) {
return icf.identity, nil
}

func (icf *identityCacheFake) StoreIdentity(identity Identity) error {
icf.identity = identity
return nil
}
6 changes: 6 additions & 0 deletions identity/cache_interface.go
@@ -0,0 +1,6 @@
package identity

type IdentityCacheInterface interface {
GetIdentity() (identity Identity, err error)
StoreIdentity(identity Identity) error
}
11 changes: 11 additions & 0 deletions identity/cache_test.go
@@ -1,6 +1,7 @@
package identity

import (
"errors"
"github.com/stretchr/testify/assert"
"os"
"testing"
Expand Down Expand Up @@ -32,6 +33,16 @@ func TestIdentityCache_GetIdentity(t *testing.T) {
assert.Nil(t, err)
}

func TestIdentityCache_GetIdentityWithNoCache(t *testing.T) {
cache := IdentityCache{
File: "does-not-exist",
}

_, err := cache.GetIdentity()

assert.Equal(t, errors.New("cache file does not exist"), err)
}

func TestIdentityCache_cacheExists(t *testing.T) {
identity := FromAddress("0x000000000000000000000000000000000000000A")
cache := IdentityCache{
Expand Down
20 changes: 12 additions & 8 deletions identity/manager_fake.go
@@ -1,26 +1,30 @@
package identity

type idmFake struct{}
type idmFake struct {
FakeIdentity1 Identity
FakeIdentity2 Identity
}

func NewIdentityManagerFake() *idmFake {
return &idmFake{}
return &idmFake{
FromAddress("0x000000000000000000000000000000000000000A"),
FromAddress("0x000000000000000000000000000000000000bEEF"),
}
}

func (fakeIdm *idmFake) CreateNewIdentity(_ string) (Identity, error) {
id := FromAddress("0x000000000000000000000000000000000000bEEF")
return id, nil
return fakeIdm.FakeIdentity2, nil
}
func (fakeIdm *idmFake) GetIdentities() []Identity {
accountList := []Identity{
FromAddress("0x000000000000000000000000000000000000bEEF"),
FromAddress("0x000000000000000000000000000000000000bEEF"),
fakeIdm.FakeIdentity2,
fakeIdm.FakeIdentity2,
}

return accountList
}
func (fakeIdm *idmFake) GetIdentity(_ string) (Identity, error) {
id := FromAddress("0x000000000000000000000000000000000000000A")
return id, nil
return fakeIdm.FakeIdentity1, nil
}
func (fakeIdm *idmFake) HasIdentity(_ string) bool {
return true
Expand Down
4 changes: 3 additions & 1 deletion server/mysterium_api_fake.go
Expand Up @@ -15,7 +15,8 @@ func NewClientFake() *ClientFake {
}

type ClientFake struct {
proposalsMock []dto_discovery.ServiceProposal
RegisteredIdentity identity.Identity
proposalsMock []dto_discovery.ServiceProposal
}

func (client *ClientFake) NodeRegister(proposal dto_discovery.ServiceProposal) (err error) {
Expand All @@ -26,6 +27,7 @@ func (client *ClientFake) NodeRegister(proposal dto_discovery.ServiceProposal) (
}

func (client *ClientFake) RegisterIdentity(identity identity.Identity) (err error) {
client.RegisteredIdentity = identity
log.Info(mysteriumApiLogPrefix, "Fake identity registered: ", identity)

return nil
Expand Down