Repository and Service pattern

[sirn]

During development of other projects, which were using the similar stack and infrastructure as Go-Boilerplate, we found several design issue with the way Repository is currently designed. Currently Repository implicitly passing gorm.DB and implements primitives such as Find, ByIDs, ByIsEnabled, Preload. This makes repository hard to extend, and unable to encapsulate a TRANSACTION without defining extra functions for that purpose (e.g., FindTx where it accepts transaction with first argument). For example:

handler.PostRepository.Get(
	repositories.ByID(id),
	repositories.Limit(limit),
)

In this example, gorm.DB is not explicitly passed to PostRepository and thus unable to utilises transaction at all. In the template, there is invalid usage of transaction, e.g. in Create:

go-boilerplate/templates/database/repository.tmpl

Lines 53 to 62 in b80c410

	// Create ... Create new entity
	func (repository *{{ .Name.Singular.Title }}Repository) Create(model *models.{{ .Name.Singular.Title }}) (*models.{{ .Name.Singular.Title }}, error) {
	err := repository.database.Transaction(func(tx *gorm.DB) error {
	return repository.database.Create(&model).Error
	})
	if err != nil {
	return nil, err
	}
	return repository.First(ByID(model.ID))
	}

At a glance, this code has two problems:

• A single query TRANSACTION does not guarantee data integrity (i.e. it is equivalent of not doing TRANSACTION at all). This is only useful in the case where multiple rows were to be created from a single Create (e.g. create with associations). However, Gorm already does this by default making the transaction here redundant.
• Transaction is not really used in this code, since Gorm create a new database session with Transaction Context when calling Begin (see finisher_api.go) which means the returned tx should be used instead of a generic gorm.DB. Since Repository assumes gorm.DB for all its operation (passed via NewRepository), this makes transaction not usable.

Additionally, in case of SQL standard-compliant SQL servers (anything but MySQL/MariaDB), TRANSACTION does more than grouping INSERT/UPDATE operations together for easier rollback, but also provides isolation for SELECT queries, ensuring data is not changing after BEGIN between the first and subsequent selects preventing subtle race condition issues (see Transaction Isolation).

Apart from that, we found several more design patterns that are hard to accomplish with the current repository design:

• Since Repository is mapped 1:1 to the database model, it can be complicated when business logic involves multiple models (e.g. OrderRepository requires calling OrderRequestRepository, even both could be grouped as Order)
• As Repository implements SQL primitives, this moves business logic back to Handler, which makes Repository somewhat redundant, and there are many abstraction leakage in the Repository layer (e.g. the need to handle gorm.ErrRecordNotFound directly in Handler). This design may make migration to microservices harder in a long run.

Suggestions

My suggestion for Repository refactor based on experience working with other projects codebase and team feedback, is that it should act more like a Service, in that they should be grouped based on business function. For example, in case of OrderRepository, with current structure we'd have:

• OrderRepository
  • Get (get Order by its ID)
  • GetAll (get all Orders)
  • GetByOrderRequestID (get Order by order_request_id)
  • Create (create an Order)
  • ...
• OrderRequestRepository
  • Get (get OrderRequest by its ID)
  • GetAll (get all OrderRequests)
  • Create (create an OrderRequest)
  • ...

In this case, since Repository is providing a primitives for querying data, business logic is instead implemented on the Handler. Some functions that may also resulted in being implemented in Handler instead, which may result duplicated code if such code were to be reused elsewhere (e.g., a Pay function).

If we were to refactor this code based on few rules:

• Service is created based on its business function
• Service may access multiple models
• Service must only return a localized errors (e.g., OrderNotFound rather than gorm.ErrRecordNotFound)
• Model is only visible to repository

This repository would be refactored into:

• OrderService
  • Create (create an OrderRequest)
  • Get (get an OrderRequest and associate Order)
  • PayWithCardID (make a payment with external service, then create Order)
  • ...

In this case, Handler would only handle processing request parameters (e.g., validating params, pagination) and creating a response struct (in case of API). In order to accommodate for migration to microservice in the future, we might also have few more rules:

• Model can only belong to single service
• Service only communicates using wrapped object, and not directly the model (e.g. DTO)

In other words,

I think by switching Repository to use Service pattern, we will be able to have much cleaner architecture for our services, as boundaries are clearly defined, and can migrate to service much easier later on. e.g., by simply wrapping a service with a Remote Facade.

[takaaki-mizuno]

Thanks for your feedback. I totally agree with your opinion about the transaction part. The current structure is not good for making transactions.

However, I still hesitate to remove the repository layer because the repository layer can conceal "where the data comes from ( database / API ...). Because the model itself is already a DTO and doesn't have to tie with the database tables.

Now I am thinking about how to support transactions in the right way on the repository layer.

[boolafish]

One issue though, is testing. Coupling data access layer and service layer you will need to test business logic and your SQL logic all together. Could be okay, but ideally they can be tested separately (and easier).

TIL about the Gorm transaction, such a non-user-friendly interface 😢

But not fully focusing on the service <> repo discussion, I do feel that the opn-generator might be a bit too opinionated so sometimes if the default generated code does not work out, engineer (aka, I) would write new functions, and it might be hard to clean up the unused codes 🤔