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 1 commit
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

change PopQueryBuilder to QueryBuilder

  • Loading branch information...
mikena-truss committed May 24, 2019
commit c9c3780c06bfb4f062efa4e04c2bd6f08a0fd47a
@@ -9,7 +9,7 @@ import (
adminops "github.com/transcom/mymove/pkg/gen/adminapi/adminoperations"
"github.com/transcom/mymove/pkg/gen/restapi"
"github.com/transcom/mymove/pkg/handlers"
query2 "github.com/transcom/mymove/pkg/services/query"
"github.com/transcom/mymove/pkg/services/query"
"github.com/transcom/mymove/pkg/services/user"
)

@@ -24,10 +24,10 @@ func NewAdminAPIHandler(context handlers.HandlerContext) http.Handler {

adminAPI := adminops.NewMymoveAPI(adminSpec)

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

@@ -6,24 +6,24 @@ import (
officeuserop "github.com/transcom/mymove/pkg/gen/adminapi/adminoperations/office"
"github.com/transcom/mymove/pkg/handlers"
"github.com/transcom/mymove/pkg/services/query"
user2 "github.com/transcom/mymove/pkg/services/user"
"github.com/transcom/mymove/pkg/services/user"
"github.com/transcom/mymove/pkg/testdatagen"
)

func (suite *HandlerSuite) TestIndexOfficeUsersHandler() {
user := testdatagen.MakeDefaultUser(suite.DB())
officeUser := testdatagen.MakeDefaultUser(suite.DB())
req := httptest.NewRequest("GET", "/office_users", nil)
req = suite.AuthenticateUserRequest(req, user)
req = suite.AuthenticateUserRequest(req, officeUser)

params := officeuserop.IndexOfficeUsersParams{
HTTPRequest: req,
}

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

@@ -13,16 +13,16 @@ import (
// allowed comparators for this query builder implementation
const equals = "="

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 29, 2019

Author Contributor

If someone has a fancier enum way of doing this, please let me know. I messed around with it a bit, but didn't love with what I ended up with. Essentially an enum with a .Comparator method that was a switch statement returning the symbols.


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

// NewPopQueryBuilder returns a new query builder implemented with pop
// NewQueryBuilder returns a new query builder implemented with pop
// constructor is for Dependency Injection frameworks requiring a function instead of struct
func NewPopQueryBuilder(db *pop.Connection) *PopQueryBuilder {
return &PopQueryBuilder{db}
func NewQueryBuilder(db *pop.Connection) Builder {
return Builder{db}
}

// Lookup to check if a specific string is inside the db field tags of the type
@@ -74,7 +74,7 @@ func filteredQuery(query *pop.Query, filters []services.QueryFilter, t reflect.T

// FetchOne fetches a single model record using pop's First method
// Will return error if model is not pointer to struct
func (p *PopQueryBuilder) FetchOne(model interface{}, filters []services.QueryFilter) error {
func (p Builder) FetchOne(model interface{}, filters []services.QueryFilter) error {
t := reflect.TypeOf(model)
if t.Kind() != reflect.Ptr {

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss May 29, 2019

Author Contributor

I've added type checks here. Note that we currently don't have these checks when using pop directly. We just test to make sure this doesn't happen. It didn't seem to hurt to give that info though, so we don't panic.

return errors.New("Model should be pointer to struct")
@@ -93,7 +93,7 @@ func (p *PopQueryBuilder) FetchOne(model interface{}, filters []services.QueryFi

// FetchMany fetches multiple model records using pop's All method
// Will return error if model is not pointer to slice of structs
func (p *PopQueryBuilder) FetchMany(model interface{}, filters []services.QueryFilter) error {
func (p Builder) FetchMany(model interface{}, filters []services.QueryFilter) error {
t := reflect.TypeOf(model)
if t.Kind() != reflect.Ptr {
return errors.New("Model should be pointer to slice of structs")
@@ -12,18 +12,18 @@ import (
"github.com/transcom/mymove/pkg/testingsuite"
)

type PopQueryBuilderSuite struct {
type QueryBuilderSuite struct {
testingsuite.PopTestSuite
logger Logger
}

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

func TestUserSuite(t *testing.T) {

hs := &PopQueryBuilderSuite{
hs := &QueryBuilderSuite{
PopTestSuite: testingsuite.NewPopTestSuite(),
logger: zap.NewNop(), // Use a no-op logger during testing
}
@@ -48,9 +48,9 @@ func (f testQueryFilter) Value() string {
return f.value
}

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

suite.T().Run("fetches one with filter", func(t *testing.T) {
@@ -123,11 +123,11 @@ func (suite *PopQueryBuilderSuite) TestFetchOne() {

}

func (suite *PopQueryBuilderSuite) TestFetchMany() {
func (suite *QueryBuilderSuite) TestFetchMany() {
// this should be stubbed out with a model that is agnostic to our code
// similar to how the pop repo tests might work
user := testdatagen.MakeDefaultOfficeUser(suite.DB())
builder := NewPopQueryBuilder(suite.DB())
builder := NewQueryBuilder(suite.DB())
var actualUsers models.OfficeUsers

suite.T().Run("fetches many with filter", func(t *testing.T) {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.