Add a sqlx datastore implementation

[marpaia]

Following the patterns laid out here, lets move to sqlx isolated behind a subpackage.

[groob]

My proposal for how to organize database access is shown in this blog post, in particular the section on using an interface.

The idea is simple. First, create an interface that describes what the backend should do:

    // Datastore is a collection of backend interfaces
    type Datastore interface {
        UserStore
        LogStore
    }

    // UserStore contains the methods for manipulating users in a backend.
    type UserStore interface {
        AddUser(name string) (User, error)
        Users() ([]User, error)
        // etc
    }

now we can create a concrete struct, like

   type mysqlStore struct{
       db *sqlx.DB
   }

and implement all the datastore methods using the sqlx package.

Placing all the database methods behind a database has several advantages:

• implement multiple backends without rewriting business logic:
  type inmem struct{} // a mock inmemory database
type rest struct{} // now the methods can be built on top of a REST API
• isolate implementation errors from the user/rest of the app:
  databases returns all sorts of specific errors. By catching them all at this level, we can have return app specific errors like KolideError instead
• create middleware/wrappers around a datastore:
  Because the datastore implementation is decoupled from everything else, we can wrap it in additional behavior
  consider the following example:
  https://play.golang.org/p/jOTGgTYm7o

This pattern is easy to implement, maintain and extend and should be considered for dependencies in general, but especially when importing code that is hard to test or might be replaced later on.

Here's an example from hashicorp/vault:
Backend interface for key/value store:
https://github.com/hashicorp/vault/blob/6efe8c97de9a2ce32f2e976ec9d71b2a466b4460/physical/physical.go#L20

A bunch of implementations of the Backend:
https://github.com/hashicorp/vault/tree/6efe8c97de9a2ce32f2e976ec9d71b2a466b4460/physical

[groob]

Playing a little with the code, here's how an initial refactoring could look like:

current code:

// users.go
func NewUser(db *gorm.DB, username, password, email string, admin, needsPasswordReset bool) (*User, error) {
    salt, hash, err := SaltAndHashPassword(password)
    if err != nil {
        return nil, err
    }
    user := User{
        Username:           username,
        Password:           hash,
        Salt:               salt,
        Email:              email,
        Admin:              admin,
        Enabled:            true,
        NeedsPasswordReset: needsPasswordReset,
    }

    err = db.Create(&user).Error
    if err != nil {
        return nil, err
    }
    return &user, nil
}

refactored

// users.go
// UserStore defines methods used to create users
type UserStore interface {
    NewUser(User) error
}

func NewUser(db UserStore, username, password, email string, admin, needsPasswordReset bool) (*User, error) {
    salt, hash, err := SaltAndHashPassword(password)
    if err != nil {
        return nil, err
    }
    user := User{
        Username:           username,
        Password:           hash,
        Salt:               salt,
        Email:              email,
        Admin:              admin,
        Enabled:            true,
        NeedsPasswordReset: needsPasswordReset,
    }

    return db.NewUser(&u)
}

new adapter package

database implementation lives here instead

// datastore package
package datastore

import (
    "github.com/jinzhu/gorm"
    "github.com/kolide/kolide-ose/app"
)

type gormDB struct {
    db *gorm.DB
}

// extract gorm implementation
func (store gormDB) NewUser(u *app.User) (*app.User, error) {
    err := store.db.Create(u).Error
    if err != nil {
        return nil, err
    }
    return u, nil
}

// now we can add an sqlx version

type mysqlx struct {
   db *sqlx.DB
}

[groob]

One last thought here:

this can get pretty ugly:

type UserStore interface {
    NewUser(User) error

    FindUserByID(id int) (*User, error)
    FindUserByName(name string) (*User, error)
}

To keep the interface relatively small, we can add some useful filters.
Consider this method instead:

type UserStore interface {
    NewUser(User) error

     Users(filters ...userFilter) ([]*User, error)
 }

Here userFilter is a type, and we can pass zero or more filters to it

allUsers, err := db.Users()
filtered, err := db.Users(
    db.Username{"groob"},
    db.BeforeCreatedDate{time.Now().UTC()},
    )

[groob]

@zwass @marpaia mind reviewing my comments above and giving me some feedback?

I'd like to tackle this in three steps:

1. design a minimal Datastore interface
2. refactor code to move all gorm code behind interface with a gormDB implementation
3. write sqlx implementation of the same datastore interface
4. deprecate the gorm one once all behavior is duplicated to work using sqlx

[zwass]

It looks like you hit on one of my concerns with your "one last thought". I think that using filters like you describe could be helpful, but how will we decide which filters to implement? Just implement them as needed?

I am also wondering about creating mocks to conform to the interface. I notice there are a couple of packages for Golang that help with this (https://github.com/golang/mock, https://github.com/stretchr/testify#mock-package). Do you have experience with them? It seems likely that we would want to implement the methods on an as-needed basis, and it would be convenient if the mocks were easy to update alongside.

Lastly, I'm not sure about the nesting of UserStore within DataStore. It seems nice because we could swap out the backend implementation of each type of store, but I can't imagine how often we'd want to do that. I guess the only real cost of it is that each call in a controller would be something like ds.us.NewUser rather than just ds.NewUser.

[groob]

My example of embedding UserStore inside Datastore is just for code cleanliness. It just means that Datastore is an UserStore. Having smaller interface definitions also helps when you're mocking something. When you're mocking user interactions, you only need to implement that subset of methods.

I tend to avoid big testing packages like testify, in favor of relying on stdlib.
mock looks like a easy helper, I use impl to generate interface stubs myself.

Embedding an interface into a struct can also help with quickly mocking just a subset of that interface: https://play.golang.org/p/qhQwgq0CYs

Regarding filters: I think we implement them as the need comes up.

[zwass]

Thanks for the reminder on embedding. I'm not quite used to all this Go syntax yet but I understand what you're doing now.

One thing I really don't understand is why Go lets you compile code with only a partially implemented interface (that then segfaults at runtime if you try to call unimplemented functions), but that is convenient for our purposes here.

Overall, yes I am into this idea.

[groob]

@murphybytes We should probably close this now?