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 33 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

Some generated files are not rendered by default. Learn more.

Some generated files are not rendered by default. Learn more.

@@ -9,6 +9,8 @@ import (
"github.com/transcom/mymove/pkg/gen/adminapi"
adminops "github.com/transcom/mymove/pkg/gen/adminapi/adminoperations"
"github.com/transcom/mymove/pkg/handlers"
"github.com/transcom/mymove/pkg/services/query"
"github.com/transcom/mymove/pkg/services/user"
)

// NewAdminAPIHandler returns a handler for the admin API
@@ -22,7 +24,12 @@ func NewAdminAPIHandler(context handlers.HandlerContext) http.Handler {

adminAPI := adminops.NewMymoveAPI(adminSpec)

adminAPI.OfficeIndexOfficeUsersHandler = IndexOfficeUsersHandler{context}
queryBuilder := query.NewQueryBuilder(context.DB())
adminAPI.OfficeIndexOfficeUsersHandler = IndexOfficeUsersHandler{
HandlerContext: context,
NewQueryFilter: query.NewQueryFilter,
OfficeUserListFetcher: user.NewOfficeUserListFetcher(queryBuilder),
}

return adminAPI.Serve(nil)
}
@@ -4,15 +4,40 @@ import (
"github.com/go-openapi/runtime/middleware"

officeuserop "github.com/transcom/mymove/pkg/gen/adminapi/adminoperations/office"
"github.com/transcom/mymove/pkg/gen/adminmessages"
"github.com/transcom/mymove/pkg/handlers"
"github.com/transcom/mymove/pkg/models"
"github.com/transcom/mymove/pkg/services"
)

// TODO: fill this in
func payloadForOfficeUserModel(o models.OfficeUser) *adminmessages.OfficeUser {
return &adminmessages.OfficeUser{ID: *handlers.FmtUUID(o.ID)}
}

// IndexOfficeUsersHandler returns a list of office users via GET /office_users
type IndexOfficeUsersHandler struct {
handlers.HandlerContext
services.NewQueryFilter
services.OfficeUserListFetcher
}

// Handle retrieves a list of office users
func (h IndexOfficeUsersHandler) Handle(params officeuserop.IndexOfficeUsersParams) middleware.Responder {

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 29, 2019

Author Contributor

This PR needs to be broken up a bit, so we should handle API marshalling into this handler here: https://www.pivotaltracker.com/story/show/164884924

return officeuserop.NewIndexOfficeUsersOK()
// Here is where NewQueryFilter will be used to create Filters from the 'filter' query param
queryFilters := []services.QueryFilter{

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 29, 2019

Author Contributor

So, I realized that in order to utilize the type benefits in the DB connector, these column values still need a type. So the param to a Where query may be interface{}, but passing a date in string format or a date in time format do different things. Which makes sense.

The consequence, is that we'll need to either:

  1. explicitly list types when creating filters
  2. marshall the data using reflection and type info in the model

2 seems like a totally reasonable thing to do. In fact, it's what all the swagger generated code is doing anyways. The only difference here is that we hit the limits of Open API specs and have to do some of the work ourselves.

More info on the OpenAPI issues here: OAI/OpenAPI-Specification#1706

h.NewQueryFilter("id", "=", "d874d002-5582-4a91-97d3-786e8f66c763"),
}

officeUsers, err := h.OfficeUserListFetcher.FetchOfficeUserList(queryFilters)
if err != nil {
return handlers.ResponseForError(h.Logger(), err)
}

payload := make(adminmessages.OfficeUsers, len(officeUsers))
for i, s := range officeUsers {
payload[i] = payloadForOfficeUserModel(s)
}

return officeuserop.NewIndexOfficeUsersOK().WithPayload(payload)
}
@@ -1,24 +1,113 @@
package adminapi

import (
"net/http"
"net/http/httptest"
"testing"

"github.com/gofrs/uuid"
"github.com/stretchr/testify/mock"

"github.com/transcom/mymove/mocks"
officeuserop "github.com/transcom/mymove/pkg/gen/adminapi/adminoperations/office"
"github.com/transcom/mymove/pkg/handlers"
"github.com/transcom/mymove/pkg/models"
"github.com/transcom/mymove/pkg/services"
"github.com/transcom/mymove/pkg/services/query"
"github.com/transcom/mymove/pkg/services/user"
"github.com/transcom/mymove/pkg/testdatagen"
)

func newMockQueryFilterBuilder(filter *mocks.QueryFilter) services.NewQueryFilter {
return func(column string, comparator string, value interface{}) services.QueryFilter {
return filter
}
}

func (suite *HandlerSuite) TestIndexOfficeUsersHandler() {
user := testdatagen.MakeDefaultUser(suite.DB())
// replace this with generated UUID when filter param is built out
uuidString := "d874d002-5582-4a91-97d3-786e8f66c763"
id, _ := uuid.FromString(uuidString)
assertions := testdatagen.Assertions{
OfficeUser: models.OfficeUser{
ID: id,
},
}
testdatagen.MakeOfficeUser(suite.DB(), assertions)
testdatagen.MakeDefaultOfficeUser(suite.DB())

requestUser := testdatagen.MakeDefaultUser(suite.DB())
req := httptest.NewRequest("GET", "/office_users", nil)
req = suite.AuthenticateUserRequest(req, user)
req = suite.AuthenticateUserRequest(req, requestUser)

params := officeuserop.IndexOfficeUsersParams{
HTTPRequest: req,
}
// test that everything is wired up

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 29, 2019

Author Contributor

This is really an integration test, not a handler unit test. I don't want to try and solve every problem in this PR, however I'm going to circle back later with an ADR about how we might want to handle backend integration tests. Now that we've decoupled our code, this is more necessary.

suite.T().Run("integration test ok response", func(t *testing.T) {
params := officeuserop.IndexOfficeUsersParams{
HTTPRequest: req,
}

queryBuilder := query.NewQueryBuilder(suite.DB())
handler := IndexOfficeUsersHandler{
HandlerContext: handlers.NewHandlerContext(suite.DB(), suite.TestLogger()),
NewQueryFilter: query.NewQueryFilter,
OfficeUserListFetcher: user.NewOfficeUserListFetcher(queryBuilder),
}

response := handler.Handle(params)

suite.IsType(&officeuserop.IndexOfficeUsersOK{}, response)
okResponse := response.(*officeuserop.IndexOfficeUsersOK)
suite.Len(okResponse.Payload, 1)
suite.Equal(uuidString, okResponse.Payload[0].ID.String())

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 29, 2019

Author Contributor

let me circle around and remove the uuidString in favor of id

})

queryFilter := mocks.QueryFilter{}
newQueryFilter := newMockQueryFilterBuilder(&queryFilter)

suite.T().Run("successful response", func(t *testing.T) {
officeUser := models.OfficeUser{ID: id}
params := officeuserop.IndexOfficeUsersParams{
HTTPRequest: req,
}
officeUserListFetcher := &mocks.OfficeUserListFetcher{}
officeUserListFetcher.On("FetchOfficeUserList",
mock.Anything,
).Return(models.OfficeUsers{officeUser}, nil).Once()
handler := IndexOfficeUsersHandler{
HandlerContext: handlers.NewHandlerContext(suite.DB(), suite.TestLogger()),
NewQueryFilter: newQueryFilter,
OfficeUserListFetcher: officeUserListFetcher,
}

response := handler.Handle(params)

suite.IsType(&officeuserop.IndexOfficeUsersOK{}, response)
okResponse := response.(*officeuserop.IndexOfficeUsersOK)
suite.Len(okResponse.Payload, 1)
suite.Equal(uuidString, okResponse.Payload[0].ID.String())
})

suite.T().Run("unsuccesful response when fetch fails", func(t *testing.T) {
params := officeuserop.IndexOfficeUsersParams{
HTTPRequest: req,
}
expectedError := models.ErrFetchNotFound
officeUserListFetcher := &mocks.OfficeUserListFetcher{}
officeUserListFetcher.On("FetchOfficeUserList",
mock.Anything,
).Return(nil, expectedError).Once()
handler := IndexOfficeUsersHandler{
HandlerContext: handlers.NewHandlerContext(suite.DB(), suite.TestLogger()),
NewQueryFilter: newQueryFilter,
OfficeUserListFetcher: officeUserListFetcher,
}

handler := IndexOfficeUsersHandler{handlers.NewHandlerContext(suite.DB(), suite.TestLogger())}
response := handler.Handle(params)
response := handler.Handle(params)

suite.Assertions.IsType(&officeuserop.IndexOfficeUsersOK{}, response)
expectedResponse := &handlers.ErrResponse{
Code: http.StatusNotFound,
Err: expectedError,
}
suite.Equal(expectedResponse, response)
})
}
@@ -0,0 +1,13 @@
package services

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 29, 2019

Author Contributor

Note this isn't following the service object pattern, but this seems ok in services. It's a dependency used for handlers/services to communicate.

Originally, I had it in its own package. However, that mean that the QueryFilter was defined there and in services so that services could pass the filters to the builder. Interfaces with the same methods don't satisfy each other though and it caused a leaky abstraction of sorts for them to communicate.


// QueryFilter is an interface to allow passing filter values into query interfaces
// Ex `FetchMany` takes a list of filters
type QueryFilter interface {
Column() string
Comparator() string
Value() interface{}
}

// NewQueryFilter is a function type definition for building a QueryFilter
// Should allow handlers to parse query params into QueryFilters for services
type NewQueryFilter func(column string, comparator string, value interface{}) QueryFilter
@@ -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
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.