Golang Rest API

Question

I want to ask if there is a better way to arrange this. Main concern is whether the store is set up in a good way and if passing Pointer to ProductRepository is a good idea or there are better ways but criticism towards all of it is welcome. I'm relatively new to Go. I have this folder structure

.
├── Makefile
├── apiserver
├── cmd
│ └── apiserver
│ └── main.go
├── configs
│ └── apiserver.toml
├── go.mod
├── go.sum
└── internal
 └── app
 ├── apiserver
 │ ├── apiserver.go
 │ ├── config.go
 │ └── server.go
 ├── handlers
 │ ├── getAll.go
 │ └── getOne.go
 ├── model
 │ └── product.go
 └── store
 ├── product_repository.go
 └── store.go

My server.go file looks like

package apiserver
import (
 "net/http"
 "github.com/gorilla/mux"
 "github.com/sirupsen/logrus"
 "github.com/vSterlin/sw-store/internal/app/handlers"
 "github.com/vSterlin/sw-store/internal/app/store"
)
type server struct {
 router *mux.Router
 logger *logrus.Logger
 store *store.Store
}
func newServer(store *store.Store) *server {
 s := &server{
 router: mux.NewRouter(),
 logger: logrus.New(),
 store: store,
 }
 s.configureRouter()
 return s
}
func (s *server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 s.router.ServeHTTP(w, r)
}
func (s *server) configureRouter() {
 pr := s.store.Product()
 s.router.HandleFunc("/products", handlers.GetAllHandler(pr)).Methods("GET")
 s.router.HandleFunc("/products/{id}", handlers.GetOneHandler(pr)).Methods("GET")
}

apiserver.go which starts up

package apiserver
import (
 "database/sql"
 "net/http"
 // Postgres driver
 _ "github.com/lib/pq"
 "github.com/vSterlin/sw-store/internal/app/store"
)
// Start starts up the server
func Start(config *Config) error {
 db, err := newDB(config.DatabaseURL)
 if err != nil {
 return nil
 }
 defer db.Close()
 store := store.New(db)
 srv := newServer(store)
 return http.ListenAndServe(config.BindAddr, srv)
}
func newDB(databaseURL string) (*sql.DB, error) {
 db, err := sql.Open("postgres", databaseURL)
 if err != nil {
 return nil, err
 }
 if err := db.Ping(); err != nil {
 return nil, err
 }
 return db, nil
}

product_repository.go

package store
import (
 "github.com/vSterlin/sw-store/internal/app/model"
)
type ProductRepository struct {
 store *Store
}
func (pr *ProductRepository) FindAll() ([]*model.Product, error) {
 rows, err := pr.store.db.Query("SELECT * FROM products;")
 if err != nil {
 return nil, err
 }
 pmArr := []*model.Product{}
 for rows.Next() {
 pm := &model.Product{}
 rows.Scan(&pm.ID, &pm.Name, &pm.Price, &pm.Description, &pm.CreatedAt, &pm.UpdatedAt)
 pmArr = append(pmArr, pm)
 }
 return pmArr, nil
}
func (pr *ProductRepository) FindById(id int) (*model.Product, error) {
 row := pr.store.db.QueryRow("SELECT * FROM products WHERE id=1ドル;", id)
 pm := &model.Product{}
 err := row.Scan(&pm.ID, &pm.Name, &pm.Price, &pm.Description, &pm.CreatedAt, &pm.UpdatedAt)
 if err != nil {
 return nil, err
 }
 return pm, nil
}

and store.go is

package store
import (
 "database/sql"
)
type Store struct {
 db *sql.DB
 productRepository *ProductRepository
}
func New(db *sql.DB) *Store {
 return &Store{
 db: db,
 }
}
func (s *Store) Product() *ProductRepository {
 if s.productRepository != nil {
 return s.productRepository
 }
 s.productRepository = &ProductRepository{
 store: s,
 }
 return s.productRepository
}

Answer 1

The first thing I noticed about your folder structure is that all of your internal packages are contained within this app directory. There is no point to this. An internal package, by definition, cannot be imported outside of your project, so any package under internal by definition is part of the application you're building. It's less effort to type import "github.com/vSterlin/sw-store/internal/model", and to me, it's arguably more communicative: From project sw-store, I'm importing the "internal model" package. That says all it needs to say.

That being said, you may want to have a read through the code review comments on the official golang repo. It does link to some other resources about package names, for example. There's a recommendation to avoid package names that don't communicate much of anything. I understand that a model, especially if you've worked in an MVC style framework, has a meaning. I'm not entirely sold on the name, but that's a matter of personal preference, I suppose.

---

Bigger issues

My real concern in the code you posted is the apiserver.go file. Why is the package apiserver aware of what underlying storage solution we're using? Why is it even connecting to the database directly? How are you going to test your code, if an unexported function is always there trying to connect to a DB? You're passing around the raw types. The server expects a *store.Store argument. How can you unit test that? This type expects a DB connection, which it receives from the apiserver package. That's a bit of a mess.

I noticed you have a config.go file. Consider creating a separate config package, where you can neatly organise your config values on a per-package basis:

package config
type Config struct {
 Server
 Store
}
type Server struct {
 Port string // etc...
}
type Store struct {
 Driver string // e.g. "postgres"
 DSN string // etc...
}
func New() (*Config, error) {
 // parse config from file/env vars/wherever
 return &Config{}, nil
}
func Defaults() *Config {
 return &Config{
 Server: Server{
 Port: ":8081",
 },
 Store: Store{
 Driver: "postgres",
 DSN: "foo@localhost:5432/dbname",
 },
 }
}

Now each package can have a constructor function that takes in a specific config type, and that package is responsible for interpreting that config, and make sense of it. That way, if you ever need to change the storage you're using from PG to MSSQL or whatever, you don't need to change the apiserver package. That package should be completely unaffected by such a change.

package store
import (
 "database/sql"
 "github.com/vSterlin/sw-store/internal/config"
 _ "github.com/lib/pq"
)
func New(c config.Store) (*Store, error) {
 db, err := sql.Open(c.Driver, c.DSN)
 if err != nil {
 return nil, err
 }
 return &Store{db: db}, nil
}

Now any code responsible for connecting to a DB is contained within a single package.

As for your repositories, you're basically allowing them to access the raw connection directly on an unexported field of your Store type. That, too, seems off. Once again: how are you going to unit-test any of this? What if you're having to support different types of storage (PG, MSSQL, etc...?). What you're essentially looking for is something that has the functions Query and QueryRow (probably a couple of other things, but I'm just looking at the code you provided).

As such, I'd define an interface alongside each repository. For clarity, I'm going to assume the repositories are defined in a separate package, too. This is to emphasise that the interface is to be defined along side the repository, the type that uses the dependency, not the type that implements the interface:

package repository
//go:generate go run github.com/golang/mock/mockgen -destination mocks/store_mock.go -package mocks github.com/vSterlin/sw-store/internal/repository ProductStore
type ProductStore interface {
 Query(q string) (*sql.Rows, error)
 QueryRow(q string, args ...interface{}) *sql.Row
}
type PRepo struct {
 s ProductStore
}
func NewProduct(s ProductStore) *PRepo {
 return &PRepo{
 s: s,
 }
}

So now, in your store package, you would create the repositories like so:

func (s *Store) Product() *PRepo {
 if s.prepo != nil {
 return s.prepo
 }
 s.prepo = repository.NewProduct(s.db) // implements interface
 return s.prepo
}

You may have noticed the go:generate comment on the interface. This allows you to run a simple go generate ./internal/repository/... command and it will generate a type for you that perfectly implements the interface your repo depends on. This makes the code in that file unit-testable.

---

Closing connections

The one thing you may be wondering is where the db.Close() call should go now. It was deferred in your start function originally. Well, that's quite simple: you just add it to the store.Store type (a stuttering name, BTW, you ought to fix that). Just defer your Close call there.

---

There's a lot more stuff we could cover here, like using context, the pro's and con's of using the package structure you're doing, what type of testing we really want/need to write, etc...

I think, based on the code you've posted here, this review should be enough to get you started, though.

Have fun.

• Sterlin V: I'm not quite sure how I would defer a connection Close() there? I have it inside apiserver.go file in Start function. Not sure where else I would put it.
• Sterlin V: Could you please tell me how to write a test for a handler? Here’s the link to the GetAll handler. Also what other tests are useful?GetAll handler
• Sterlin V: Also not exactly sure why is it better to create interface for ProductStore alongside repository and do it for each repo rather than creating an interface for Store, like so type PRepo struct { s Store }? Assuming I defined type Store interface { Query()... }.