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

Proof of concept on admin query interface #2120

Merged
merged 37 commits into from Jun 3, 2019
Merged
Changes from 10 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
09805c5
exploratory work for query interace
mikena-truss May 8, 2019
9c9e115
Merge branch 'master' into mw-add-query-interface
mikena-truss May 9, 2019
f35a400
add list fetcher
mikena-truss May 10, 2019
fafc674
move DB tests to builder
mikena-truss May 10, 2019
0bc888e
checkpoint before messing with interface pointer
mikena-truss May 10, 2019
df3aca3
remove unused mock
mikena-truss May 10, 2019
64d6b1f
add tests for office user fetcher
mikena-truss May 10, 2019
f4d47a8
add list tests
mikena-truss May 10, 2019
5d15e49
fix some linting mistakes
mikena-truss May 10, 2019
a660c46
office user fetcher lint fixes
mikena-truss May 10, 2019
b312abb
Merge branch 'master' into mw-add-query-interface
mikena-truss May 20, 2019
82e8673
Merge branch 'master' into mw-add-query-interface
mikena-truss May 21, 2019
5450a94
add type checking for reflection
mikena-truss May 21, 2019
ebaecda
write tests for type checks
mikena-truss May 21, 2019
ceed57a
user same pattern for FetchOne as FetchMany
mikena-truss May 22, 2019
abdddaa
comments + test fixes
mikena-truss May 22, 2019
985f04a
fix office user fetcher to conform to FetchOne
mikena-truss May 22, 2019
cd6aa19
first pass at adding filters
mikena-truss May 22, 2019
0cb9775
Merge branch 'master' into mw-add-query-interface
mikena-truss May 24, 2019
2e4b0a1
refactor query filter to use interface pattern in service layer
mikena-truss May 24, 2019
953823f
remove custom error type for now
mikena-truss May 24, 2019
7556621
test for invalid comparators
mikena-truss May 24, 2019
84afdda
pass the services into the handler
mikena-truss May 24, 2019
c9c3780
change PopQueryBuilder to QueryBuilder
mikena-truss May 24, 2019
a3e8729
return a pointer in query builder constructor
mikena-truss May 24, 2019
bda112b
add integration test
mikena-truss May 24, 2019
c10a1bc
add handler unit test with mock
mikena-truss May 24, 2019
168b67a
add test for error
mikena-truss May 24, 2019
d48cd88
convert value to an interface{} so db connector can get underlying type
mikena-truss May 28, 2019
9aa2c93
Merge branch 'master' into mw-add-query-interface
mikena-truss May 31, 2019
82478c8
continue over bad columns
mikena-truss May 31, 2019
a6b89a9
Merge branch 'master' into mw-add-query-interface
mikena-truss May 31, 2019
faa56cd
fix filters typo
mikena-truss May 31, 2019
82fc0a8
Merge branch 'master' into mw-add-query-interface
mikena-truss Jun 3, 2019
ea531e8
replace reflection error with constant
mikena-truss Jun 3, 2019
de65d2d
add link to OWASP guidelines
mikena-truss Jun 3, 2019
8142d6e
grammer updates
mikena-truss Jun 3, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -0,0 +1,15 @@
package query

import (
"go.uber.org/zap"
)

// Logger is an interface that describes the logging requirements of this package.
type Logger interface {
Debug(msg string, fields ...zap.Field)
Info(msg string, fields ...zap.Field)
Warn(msg string, fields ...zap.Field)
Error(msg string, fields ...zap.Field)
Fatal(msg string, fields ...zap.Field)
WithOptions(opts ...zap.Option) *zap.Logger
}
@@ -0,0 +1,70 @@
package query

import (
"fmt"
"reflect"

"github.com/gobuffalo/pop"
)

// PopQueryBuilder is a wrapper aroudn pop
// with more flexible query patterns to MilMove
type PopQueryBuilder struct {
db *pop.Connection
}

// NewPopQueryBuilder returns a new query builder implemented with pop
func NewPopQueryBuilder(db *pop.Connection) *PopQueryBuilder {

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 10, 2019

Author Contributor

Constructor is for dependency injection that requires a function.

This comment has been minimized.

Copy link
@kahlouie

kahlouie May 15, 2019

Contributor

What about adding this to the comment section above the function definition? And in all future places that do similar things? I imagine that's still important to know as we move forward

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 15, 2019

Author Contributor

Will do. I also have to add a lot more comments when I polish this all up

return &PopQueryBuilder{db}
}

// InvalidInputError is an error for when query inputs are incorrect
type InvalidInputError struct {

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 10, 2019

Author Contributor

Curious where things like this should live. Previously, we threw them into the model package and read them up in the handler for error responses. I think that needs to change. Probably check with Roci about error work.

This comment has been minimized.

Copy link
@macrael

macrael May 15, 2019

Contributor

In my project, I've been defining the errors in top-level domain, along with the interfaces that my services implement. That way the package can be completely mocked out if need be without importing it.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 15, 2019

Author Contributor

I think we'll need to work on moving them to a common package. The service package that contains only interfaces and is shared across handlers might make sense.

A lot of interfaces (including the query builder one in the office user fetcher) I've been moving to be private to packages using them. For the most part I don't see a downside to doing this other than some slight boilerplate in multiple packages that use the same interface. But only that package knows what dependencies it needs.

This comment has been minimized.

Copy link
@kahlouie

kahlouie May 16, 2019

Contributor

I agree with @macrael, that we should have all of our error types in a top-level domain in one place, so anytime you are dealing with errors (regardless of where it pops up), you know how to chase it down and we can dry up the error types to a core set of errors that we actually use.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 16, 2019

Author Contributor

Maybe we can pass these over to the services package? Maybe an ADR or part of error design, @kahlouie ? Doesn't necessarily need to be defined for this.

input []string
}

// Error returns the error message
func (e *InvalidInputError) Error() string {
return fmt.Sprintf("%v is not valid input", e.input)
}

func getDBColumn(t reflect.Type, field string) (string, bool) {

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 10, 2019

Author Contributor

Here's the reflection code checking the column names. We can extend this to other functions in the future for things like associations or create our own struct fields for access patterns.

for i := 0; i < t.NumField(); i++ {
dbTag, ok := t.Field(i).Tag.Lookup("db")
if ok && dbTag == field {

This comment has been minimized.

Copy link
@reggieriser

reggieriser May 10, 2019

Contributor

Just a note that it looks like Pop doesn't require the db tag to be there:

Note, for each table field, we defined a pop tag matching the field name, but it's not required. If you don't provide a name, Pop will use the name of the struct field to generate one.

We'll either have to handle that possibility or make sure we always use the tag on our models (probably safer to do the former). Might be able to leverage the same API used in Pop.

This comment has been minimized.

Copy link
@reggieriser

reggieriser May 13, 2019

Contributor

We talked about this a little, but just wanted to add a note as a reminder -- would we need a way to indicate that a field is not able to be used as a filter? If so, maybe we can add additional metadata to the tag to note if that column is a legal filter or not.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 15, 2019

Author Contributor

I think we'll need to either flavor the struct tags more or add some authorization checks that do something similar. I don't think this matters for the office user, but maybe we can test it out on a field?

This comment has been minimized.

Copy link
@jim

jim May 16, 2019

Contributor

What is the scenario where we'd need to disallow a field from being used to query? I worry a little about adding too much functionality to the query builder, at least in the beginning.

return dbTag, true
}
}
return "", false
}

// FetchOne fetches a single model record using pop's First method
func (p *PopQueryBuilder) FetchOne(model interface{}, field string, value interface{}) error {

This comment has been minimized.

Copy link
@reggieriser

reggieriser May 13, 2019

Contributor

Any reason why FetchOne shouldn't take multiple filters like the FetchMany? The two methods can probably share the core filter-processing logic and just differ in taking/returning a model vs. a slice.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 15, 2019

Author Contributor

It was the first one I wrote and modeled it off FetchByID. There's no reason we can't expand it though. It should probably follow the same pattern. We can even just have it be a shorthand for calling FetchMany().First() with some checks. Although on a SQL level I imagine pop is using LIMIT

This comment has been minimized.

Copy link
@jim

jim May 16, 2019

Contributor

I think it would be nice for there to be a unified interface between FetchOne and FetchMany, although I can see that if a single field is being checked most often a shortcut could be nice.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 16, 2019

Author Contributor

I'll see how I can rework some of this in a followup commit.

// todo: pointer check on type

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 10, 2019

Author Contributor

I made some notes about reflect type checking. Calling Elem() can cause a runtime panic. We'll want to enforce that we're getting a pointer and in the FetchMany case a pointer to a slice.

column, ok := getDBColumn(reflect.TypeOf(model).Elem(), field)

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 10, 2019

Author Contributor

Notice we always use the returned name here. This is part of the SQL injection defense, since we're interpolating using Sprintf. Regardless of what the user inputs, we use our predefined struct field tags.

Note that this seems a little complex at first. But having reusable code that rarely changes vs sparse code that is constantly changing (our current typed model fetch methods) actually makes it much easier to control access patterns.

https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.md#defense-option-3-whitelist-input-validation

if !ok {
return &InvalidInputError{[]string{field}}
}
columnQuery := fmt.Sprintf("%s = ?", column)
query := p.db.Where(columnQuery, value)
return query.First(model)
}

// FetchMany fetches multiple model records using pop's All method
func (p *PopQueryBuilder) FetchMany(model interface{}, filters map[string]interface{}) error {
query := p.db.Q()
invalidFields := make([]string, 0)
t := reflect.TypeOf(model).Elem().Elem() // todo: add slice check
for field, value := range filters {
column, ok := getDBColumn(t, field)
if !ok {
invalidFields = append(invalidFields, field)
}
columnQuery := fmt.Sprintf("%s = ?", column)

This comment has been minimized.

Copy link
@macrael

macrael May 15, 2019

Contributor

I like the ability to pass multiple filters in, but I wonder if in the future we are going to want more comparisons than "=" here, in the future. I don't think it's something we should add until we need, but this is where I fear that our query builder could one day become more complicated than just writing the SQL by hand for every query.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 15, 2019

Author Contributor

It looks like GitHub marked one of my original comments about this as outdated: #2120 (comment)

I think we'll need some sort of Comparator enum.

To the latter point, I don't think there's anything stopping the query builder from evolving into raw sql. Somehow the handler params need to be translated into sql though. The nice part about interfacing it away, is that we can do whatever we want in the implementation.

This comment has been minimized.

Copy link
@jim

jim May 16, 2019

Contributor

I like the flexibility of using an interface{} as the map values, but I dislike that it doesn't help users of this function know what to pass in. It would be nice to develop this a bit further so our development tooling can help us out a bit.

If we used a specific interface, we could then have better autocomplete and also provide more options for queries:

type Filter interface {
  SQL() string
}

We could then have helper functions defined for folks to use:

// qb.Contains returns a struct that implements SQL()
var users []*User
err := qb.FetchMany(users, map[string]queryBuilder.Filter{
    "email": qb.Contains("gmail.com"),
}

We could also get rid of the map by moving the column name into the filter and using variadic functions:

var users []*User
err := qb.FetchMany(users, qb.Contains("email", "gmail.com"), qb.Equal("active", true))

Another idea is to make the filters type-specific to remove additional uses of interface{} in their definitions, which has an additional side effect of forcing type conversion to happen in Go land and not in the database:

var users []*User
err := qb.FetchMany(users, qb.StringContains("email", "gmail.com"), qb.BoolEqual("active", true))

I'm not sure how far to go in this regard (I think sort, etc. could be incorporated, but it will definitely break down at some point), and I have nothing against interface{} generally, but I would love for us to provide a better API than Pop does if it is possible to do so without a ton of additional work.

This comment has been minimized.

Copy link
@kahlouie

kahlouie May 16, 2019

Contributor

I like @jim's suggestion here. It also prevents us from abstracting too much, which allows for us to fully consider the impact of the queries that we can make in the queries we make from SM, TSP, and Office apps.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 16, 2019

Author Contributor

The filters definitely have to be thrown into a struct at the very least. I'll see what I can do here. I worry about expanding the exported functions on the query builder too much. The nice thing about exposing one function is that the dependency surface is much smaller. It goes hand in hand with Go's one function interface pattern. Building up the struct will probably be a decent tradeoff.

This comment has been minimized.

Copy link
@macrael

macrael May 16, 2019

Contributor

I can't tell if you mean that the filters need to be in a struct because it's the only technical option or because it only makes sense that way. I do think that it would be technically possible to use variadic args to implement the interface @jim sketched out.

query = query.Where(columnQuery, value)
}
if len(invalidFields) != 0 {
return &InvalidInputError{invalidFields}
}
return query.All(model)
}
@@ -0,0 +1,83 @@
package query

import (
"testing"

"github.com/stretchr/testify/suite"
"go.uber.org/zap"

"github.com/transcom/mymove/pkg/models"
"github.com/transcom/mymove/pkg/testdatagen"
"github.com/transcom/mymove/pkg/testingsuite"
)

func (suite *PopQueryBuilderSuite) TestFetchMany() {

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 10, 2019

Author Contributor

These tests should become the only unit tests that reference pop. We made want to add some integration tests that use it also, but services and handlers no longer should need to. Instead, they use the query builder as a dependency and mock its behavior in the tests.

// this should be stubbed out with a model that is agnostic to our code
// similar to how the pop repo tests might work
testdatagen.MakeDefaultOfficeUser(suite.DB())
email := "test@example.com"
assertions := testdatagen.Assertions{OfficeUser: models.OfficeUser{Email: email}}
user2 := testdatagen.MakeOfficeUser(suite.DB(), assertions)
builder := NewPopQueryBuilder(suite.DB())
var officeUsers models.OfficeUsers
suite.T().Run("fetches many with filter", func(t *testing.T) {
filters := map[string]interface{}{
"id": user2.ID,
"email": email,
}
err := builder.FetchMany(&officeUsers, filters)

suite.NoError(err)
suite.Len(officeUsers, 1)
suite.Equal(officeUsers[0].Email, email)
})

suite.T().Run("fails with invalid column", func(t *testing.T) {
filters := map[string]interface{}{
"id": user2.ID,
"fake_column": email,
}
err := builder.FetchMany(&officeUsers, filters)

suite.Error(err)
suite.Equal(err.Error(), "[fake_column] is not valid input")
})
}

func (suite *PopQueryBuilderSuite) TestFetchOne() {
user := testdatagen.MakeDefaultOfficeUser(suite.DB())
builder := NewPopQueryBuilder(suite.DB())
var officeUser models.OfficeUser

suite.T().Run("fetches one", func(t *testing.T) {
err := builder.FetchOne(&officeUser, "id", user.ID.String())

suite.NoError(err)
suite.Equal(officeUser.ID, user.ID)
})

suite.T().Run("returns error on invalid column", func(t *testing.T) {
err := builder.FetchOne(&officeUser, "fake_column", user.ID.String())

suite.Error(err)
suite.Equal(err.Error(), "[fake_column] is not valid input")
})
}

type PopQueryBuilderSuite struct {
testingsuite.PopTestSuite
logger Logger
}

func (suite *PopQueryBuilderSuite) SetupTest() {
suite.DB().TruncateAll()
}

func TestUserSuite(t *testing.T) {

hs := &PopQueryBuilderSuite{
PopTestSuite: testingsuite.NewPopTestSuite(),
logger: zap.NewNop(), // Use a no-op logger during testing
}
suite.Run(t, hs)
}
@@ -0,0 +1,13 @@
package services

import "github.com/transcom/mymove/pkg/models"

// OfficeUserFetcher is the exported interface for fetching a single office user
type OfficeUserFetcher interface {
FetchOfficeUser(field string, value interface{}) (models.OfficeUser, error)
}

// OfficeUserListFetcher is the exported interface for fetching multiple office users
type OfficeUserListFetcher interface {

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 29, 2019

Author Contributor

I could use some insight from others here. Add, delete, modify, and get are all going to be very similar to the main apps. However, the list/query features may vary from the existing specs. Do we call this OfficeUserAdminListFetcher or something similar to avoid collisions? This is more apparent when you look at something like Shipments that will already have services built.

FetchOfficeUserList(filters map[string]interface{}) (models.OfficeUsers, error)
}
@@ -0,0 +1,15 @@
package user

import (
"go.uber.org/zap"
)

// Logger is an interface that describes the logging requirements of this package.
type Logger interface {

This comment has been minimized.

Copy link
@macrael

macrael May 15, 2019

Contributor

Is this a pattern being used across mil move? Since it's only being used by the tests, it feels a bit overkill to me to not just use a zap logger.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 15, 2019

Author Contributor

It has become a pattern: #1775

Debug(msg string, fields ...zap.Field)
Info(msg string, fields ...zap.Field)
Warn(msg string, fields ...zap.Field)
Error(msg string, fields ...zap.Field)
Fatal(msg string, fields ...zap.Field)
WithOptions(opts ...zap.Option) *zap.Logger
}
@@ -0,0 +1,26 @@
package user

import (
"github.com/transcom/mymove/pkg/models"
"github.com/transcom/mymove/pkg/services"
)

type officeUserQueryBuilder interface {

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 10, 2019

Author Contributor

I don't think this name needs to be unexported, but I'm unsure why the pattern is to export them...

Notice how it's tied directly to the user service. This is part of the "Do This Instead" pattern defined here: https://blog.chewxy.com/2018/03/18/golang-interfaces/

Since Go interfaces are satisfied implicitly, it doesn't make sense to define interfaces outside of the package their referenced from. The pop query builder will automatically work as a param.

This comment has been minimized.

Copy link
@macrael

macrael May 15, 2019

Contributor

I think I like not exporting until there is a clear reason to export.

Re: where to define your interfaces, I think that it can make sense to define interfaces outside of the package you are implementing them in for mocking purposes. I like this article: https://medium.com/@benbjohnson/standard-package-layout-7cdbc8391fc1 where all "domain models" are defined in the top level package for your app, including service interfaces. That way you can mock services without importing them at all.

That said, I'm not sure that's an important consideration in this case.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 15, 2019

Author Contributor

I agree the main point is to not have them next to the implementation. I've been tying them directly to the usage package. I commented a little above, but the idea being only that package knows what it needs, any outside packages are just increasing the dependency chain.

I can see this causing an issue with shared mocks, but I think the mock implementation would always satisfy the interface regardless of where it is (ie. a test package)?

In this case all the mocks, weirdly, have to vary because of the interface{} being passed.

FetchOne(model interface{}, field string, value interface{}) error
}

type officeUserFetcher struct {
builder officeUserQueryBuilder
}

// FetchOfficeUser fetches an office user for the given field/value pair
func (o officeUserFetcher) FetchOfficeUser(field string, value interface{}) (models.OfficeUser, error) {

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 10, 2019

Author Contributor

This methods becomes pretty light. In the future more will happen here. Like more advanced querying or authorization patterns.

This comment has been minimized.

Copy link
@jim

jim May 16, 2019

Contributor

We could use code generation to generate model-specific funcs that return a specific type. This would get us away from passing in a pointer to work around the type system.

var officeUser models.OfficeUser
error := o.builder.FetchOne(&officeUser, field, value)

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 10, 2019

Author Contributor

Notice that the FetchOne doesn't return the office user. This is similar to how pop does things (modifying the pointer). The reason being, with reflection, you would have to return interface{} and then typecast it (I think).

This comment has been minimized.

Copy link
@macrael

macrael May 15, 2019

Contributor

That's right, and I like that this function does return a model, that way we keep the reflection stuff contained a bit.

return officeUser, error
}

// NewOfficeUserFetcher return an implementaion of the OfficeUserFetcher interface
func NewOfficeUserFetcher(builder officeUserQueryBuilder) services.OfficeUserFetcher {
return officeUserFetcher{builder}
}
@@ -0,0 +1,57 @@
package user

import (
"errors"
"reflect"
"testing"

"github.com/gofrs/uuid"

"github.com/transcom/mymove/pkg/models"
)

type testOfficeUserQueryBuilder struct {
fakeFetchOne func(model interface{}) error
}

func (t *testOfficeUserQueryBuilder) FetchOne(model interface{}, field string, value interface{}) error {

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 10, 2019

Author Contributor

Notice there's no more pop in the services.

m := t.fakeFetchOne(model)
return m
}

func (suite *UserServiceSuite) TestFetchOfficeUser() {

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 10, 2019

Author Contributor

The service tests now just become a reaction to what is declared in them. In this case if the dependency succeeds/fails, what should the service do?

Similar to the main method, it's pretty lightweight for the time being.

suite.T().Run("if the user it fetched, it should be returned", func(t *testing.T) {
id, err := uuid.NewV4()
suite.NoError(err)
fakeFetchOne := func(model interface{}) error {

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 10, 2019

Author Contributor

In order to simulate the dependency behavior, we test what happens when it modifies the passed (empty) office user in the tested method. We can pass this to the fake dependency and force it to be called by the real method.

reflect.ValueOf(model).Elem().FieldByName("ID").Set(reflect.ValueOf(id))
return nil
}

builder := &testOfficeUserQueryBuilder{
fakeFetchOne: fakeFetchOne,
}
fetcher := NewOfficeUserFetcher(builder)

officeUser, err := fetcher.FetchOfficeUser("id", id)

suite.NoError(err)
suite.Equal(id, officeUser.ID)
})

suite.T().Run("if there is an error, we get it with zero office user", func(t *testing.T) {

This comment has been minimized.

Copy link
@macrael

macrael May 15, 2019

Contributor

The only trouble with these tests is that they are no longer actually testing that the database is configured correctly. Until performance becomes a problem I tend to think that writing tests that actually hit the database is more useful than mocking it out. Somewhere there should be a test that a user with all the required data can be saved to the database, with these layers of indirection it's not clear where that test should live.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 15, 2019

Author Contributor

Performance has been an issue with us, especially how we truncate the DB before tests (I think that could be fixed by itself though). I was alluding to this here without outright saying it: #2120 (comment). We'll probably want some golden path integration tests to make sure everything is wired up correctly. I think we can separate these into their own testing package, rather than conflating them with handlers, models, services, etc.

fakeFetchOne := func(model interface{}) error {
return errors.New("Fetch error")
}
builder := &testOfficeUserQueryBuilder{
fakeFetchOne: fakeFetchOne,
}
fetcher := NewOfficeUserFetcher(builder)

officeUser, err := fetcher.FetchOfficeUser("id", 1)

suite.Error(err)
suite.Equal(err.Error(), "Fetch error")
suite.Equal(models.OfficeUser{}, officeUser)
})
}
@@ -0,0 +1,26 @@
package user

import (
"github.com/transcom/mymove/pkg/models"
"github.com/transcom/mymove/pkg/services"
)

type officeUserListQueryBuilder interface {
FetchMany(model interface{}, fitlers map[string]interface{}) error
}

type officeUserListFetcher struct {
builder officeUserListQueryBuilder
}

// FetchOfficeUserList is uses the passed query builder to fetch a list of office users
func (o officeUserListFetcher) FetchOfficeUserList(filters map[string]interface{}) (models.OfficeUsers, error) {
var officeUsers models.OfficeUsers
error := o.builder.FetchMany(&officeUsers, filters)
return officeUsers, error
}

// NewOfficeUserListFetcher returns an implementation of OfficeUserListFetcher
func NewOfficeUserListFetcher(builder officeUserListQueryBuilder) services.OfficeUserListFetcher {
return officeUserListFetcher{builder}
}
@@ -0,0 +1,60 @@
package user

import (
"errors"
"reflect"
"testing"

"github.com/gofrs/uuid"

"github.com/transcom/mymove/pkg/models"
)

type testOfficeUserListQueryBuilder struct {
fakeFetchMany func(model interface{}) error
}

func (t *testOfficeUserListQueryBuilder) FetchMany(model interface{}, filters map[string]interface{}) error {
m := t.fakeFetchMany(model)
return m
}

func (suite *UserServiceSuite) TestFetchOfficeUserList() {
suite.T().Run("if the user it fetched, it should be returned", func(t *testing.T) {

This comment has been minimized.

Copy link
@reggieriser

reggieriser Jun 3, 2019

Contributor

Nitpick: "If the user is fetched..." (another similar typo in the other test).

id, err := uuid.NewV4()
suite.NoError(err)
fakeFetchMany := func(model interface{}) error {
value := reflect.ValueOf(model).Elem()
value.Set(reflect.Append(value, reflect.ValueOf(models.OfficeUser{ID: id})))
return nil
}
builder := &testOfficeUserListQueryBuilder{
fakeFetchMany: fakeFetchMany,
}
fetcher := NewOfficeUserListFetcher(builder)
filters := map[string]interface{}{
"id": id,
}

officeUsers, err := fetcher.FetchOfficeUserList(filters)

suite.NoError(err)
suite.Equal(id, officeUsers[0].ID)
})

suite.T().Run("if there is an error, we get it with no office users", func(t *testing.T) {
fakeFetchMany := func(model interface{}) error {
return errors.New("Fetch error")
}
builder := &testOfficeUserListQueryBuilder{
fakeFetchMany: fakeFetchMany,
}
fetcher := NewOfficeUserListFetcher(builder)

officeUsers, err := fetcher.FetchOfficeUserList(map[string]interface{}{})

suite.Error(err)
suite.Equal(err.Error(), "Fetch error")
suite.Equal(models.OfficeUsers(nil), officeUsers)
})
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.