Writing the code to reuse

[sriramkp]

I am new to Golang and Echo.

I really appreciate your ideas/suggestions to improve the code with below aspects:

1. Minimizing the lines of code
2. Reuse the code with different model struct or error handling.

func CreatePromotionalArea(c echo.Context) error {

	var response model.Status
	areaModel := db.PromoArea{}
	if err := c.Bind(&areaModel); err != nil {

		response.Code = http.StatusBadRequest
		response.Msg = "Request model is not formed correctly!"
		log.Errorf("%s - Error: %s - Input: %#v\n", response.Msg, err, areaModel)
		return c.JSON(response.Code, response)
	}

	createResult, err := areaModel.CreatePromotionArea()
	if err != nil {

		response.Code = http.StatusInternalServerError
		response.Msg = "Failed to create!"
		log.Errorf("%s - Error: %#v - Input: %#v\n", response.Msg, err, areaModel)
	} else {
		response.Code = http.StatusOK
		response.Msg = createResult
	}

	return c.JSON(response.Code, response)
}

[aldas]

You do not need so detailed error handling in handlers. You could just return err and let global/default error handler do the work. See https://echo.labstack.com/guide/error-handling/
For example c.Bind() already returns errors of type echo.HTTPError, that have http.StatusBadRequest as status code - no need to repeat that.
With your own global error handler your could have more centralized logging also for errors.

Here is how defaultErrorHandler is implemented:

echo/echo.go

Line 352 in 1ac4a8f

func (e *Echo) DefaultHTTPErrorHandler(err error, c Context) {

p.s. if you swallow all errors in handler your middleware are not aware that error occurred.

[aldas]

This is my personal opinion and may not be shared by others.

using binding payload directly to object is asking for problems. For example if you are using JSON and introduce new (public) field in your struct that is not meant to be unmarshaled to you could have attacker providing that field and therefore filling it with unwanted data.

This is probably overkill for small applications but I tend to create controllerlike structs that take dependencies from constructor so I do not need to use global instances and therefore can test/mock stuff more easily

For example for HTTP layer

type HTTPHandlers struct {
	db DB // in my own code a service for that domain.
}

func RegisterPromoAreaHandlers(parentRoute *echo.Group, db DB) *HTTPHandlers {
	h := &HTTPHandlers{db: db}

	promo := parentRoute.Group("/promo")
	promo.POST("", h.createPromotionalArea)
	//promo.GET("/:id", h.getPromotionalArea)

	return h
}

func (h *HTTPHandlers) createPromotionalArea(c echo.Context) error {
	payload := PromoArea{} // could/should be different type of struct so transport layer can be different from business domain
	if err := c.Bind(&payload); err != nil {
		return err
	}

	promo, err := h.db.StorePromoArea(
		c.Request().Context(),
		PromoArea{
			// explicitly map fields so no newly added field would slip through
			Name: payload.Name,
		},
	)
	if err != nil {
		return err
	}
	return c.JSON(http.StatusCreated, promo) // could/should be different type of struct so transport layer can be different from business domain
}

Database or service you pass in RegisterPromoAreaHandlers is interface. In that case you can mock it.

type DB interface {
	StorePromoArea(ctx context.Context, p PromoArea) (PromoArea, error)
}

and simple main for api structure

func main() {
	e := echo.New()

	db := &db{}
	// connect to db

	api := e.Group("/api")
	RegisterPromoAreaHandlers(api, db)
	// register other handlers for other domains
	
	if err := e.Start(":8080"); err != http.ErrServerClosed {
		log.Fatal(err)
	}
}

and your handler test can be something like that with mocking service/db layer

import (
	"context"
	"github.com/labstack/echo/v4"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/mock"
	"net/http"
	"net/http/httptest"
	"strings"
	"testing"
)

type dbMock struct {
	mock.Mock
}

func (m *dbMock) StorePromoArea(ctx context.Context, p PromoArea) (PromoArea, error) {
	args := m.Called(ctx, p)
	return args.Get(0).(PromoArea), args.Error(1)
}

func TestHTTPHandlers_createPromotionalArea(t *testing.T) {
	var testCases = []struct {
		name        string
		whenPayload string
		dBWhen      PromoArea
		dBReturn    PromoArea
		dBError     error
		expect      string
		expectError string
	}{
		{
			name:        "ok, happy case",
			whenPayload: `{"ID": 1, "name": "test"}`, // ID could be sent
			dBWhen:      PromoArea{Name: "test"}, // but not be sent to db
			dBReturn:    PromoArea{ID: 999, Name: "test"},
			expect:      `{"ID":999, "Name":"test"}`,
		},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			db := new(dbMock)
			db.On("StorePromoArea", mock.Anything, tc.dBWhen).Return(tc.dBReturn, tc.dBError)

			req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(tc.whenPayload))
			req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON)
			rec := httptest.NewRecorder()
			e := echo.New()
			c := e.NewContext(req, rec)

			h := HTTPHandlers{db: db}
			err := h.createPromotionalArea(c)

			assert.JSONEq(t, tc.expect, string(rec.Body.Bytes()))
			if tc.expectError != "" {
				assert.EqualError(t, err, tc.expectError)
			} else {
				assert.NoError(t, err)
			}
		})
	}
}

[sriramkp]

Hi, I am facing conversion issues in the below testcase code.

h := HTTPHandlers{db: db}

Error Message: Cannot convert expression of type '*DbMock' to type 'DB'

Its causing the error because DB interface is having StorePromoArea method.

[aldas]

Is your dbMock struct implementing all DB interface methods?

[sriramkp]

You are absolutely right! I didn't implement all the methods. Thankyou a lot!

[sriramkp]

Can we use the methods from other packages without exporting (ie. the function name starting with small case)?

err := h.createPromotionalArea(c)

[aldas]

no, are you having problems with circular dependencies? Thats why you use interfaces and dependency injection.

note: unless interfaces is used in many packages it would be better to create it in package that needs it and not in package that provides implementation. When you create interface near place you use it you can only include methods that you use in it - therefore having smaller interfaces and making mocking etc easier in that package
note. there is Go proverb Accept interfaces, return structs. search internetz for it meaning.

Basically - create package for your separate domain. inside that package create interface for other package/domain dependecy (ala service) you want to use and use that interface as type for creator functions.

By that you would up in package structure as

• application/higher-abstraction-level-domain
  • domain1
  • domain2

For example application package ties everything together. This is where echo instance could be created and http server started. This is place where you use/instantiate top level dependencies that use pass into other package methods like RegisterPromoAreaHandlers

NB: in Go you can not compile code that has circular dependencies ala application uses domain1 and domain1 uses application