lunemec / go-clean-architecture

13 stars 1 forks source link

Sometimes using services+repository leads to worse code than simple SQL #5

Open lunemec opened 6 years ago

lunemec commented 6 years ago

Case of API. Suppose having API method to list all organizations, and their connected locations and contacts. This is pretty simple to do in Go.

func organizationsHandler(w http.ResponseWriter, req *http.Request) {
    db, err := sql.Open()  // Or have in APP context.
    if err != nil {
        // Handle err
    }
    query := `
    SELECT * FROM organization
        JOIN location ON location.id = organization.location_id
        JOIN contact ON contact.organization_id = organization.id
        JOIN location as contact_location ON location.id = contact.location_id`
    rows, err := db.Query(query)
    if err != nil {
        // Handle err
    }
    defer rows.Close()
    // Read rows
    // Write JSON response
}

But when using services/repository, this is not so simple:

func organizationsHandler(w http.ResponseWriter, req *http.Request) {
    db, err := sql.Open() // Or have in APP context.
    if err != nil {
        // Handle err
    }

    orgRepo := mysql.NewOrganizationRepository(db)
    locRepo := mysql.NewLocationRepository(db)
    contactRepo := mysql.NewContactRepository(db)
    loaderService := service.LoadOrganizations(orgRepo, locRepo, contactRepo)
    data := loaderService.Load()
    // Write data as JSON response.
}

The problem here is loaderService. It is needlessly stupid, having to Get() every table and it's related records. Joins are much simpler and more performant.

What would be the correct solution? Having separate repository with specific SQL which the service would use?

thfre commented 6 years ago

Your first example is perfectly fine. That is not to say you don't need the individual repositories.

lunemec commented 6 years ago

@Phr0ztByte yes, I think so too. But when your entire codebase uses repositories, it seems inconsistent to use SQL's in some places and repositories in some other. Or you mean to wrap the specific SQL in its own repository?

thfre commented 6 years ago

Yes. Personally I have Aggregate repositories. And I may model a bit differently. I would rarely use the contacts and locations. But I might need a count of them. So I make the aggregates part of that model. My organisation will never have a Locations array or contacts array, those I will only use on the screens which displays lists of them.

mvrhov commented 4 years ago

I've solved a problem, where one service needs a ton of data from different repositories by creating a new repository that handles what's needed by that specific service.

The other thing That you usually need is that the views are different and span more than one repository.. Well you could split repositories to read/write ones. (just like if you'd implement command/query segregation)

SergeAx commented 2 years ago

(got here from an old Reddit thread)

This is not how things should be done in a sense of code and architecture cleaniness.

First of all, you don't query three tables simultaneously. They may become a metric ton large, and this query will kill your performance. In fact, if you do things right and introduce timeout into your Context, this handle will just fail every time.

Instead, I would do things like this:

  1. Query orgs, adding paging and sorting, like orgs := orgRepo.List(ctx, page, sort)
  2. Enrich array of orgs with locEnricher.EnrichOrgs(ctx, &orgs, locRepo)
  3. Repeat with contactEnricher.EnrichOrgs(ctx, &orgs, contactRepo)

And by the way, how about dependency injection? Like:

    // Init repos
    orgRepo := mysql.NewOrganizationRepository(db)
    locRepo := mysql.NewLocationRepository(db)
    contactRepo := mysql.NewContactRepository(db)

    // Init services
    locEnricher := service.NewLocationEnricher(locRepo)
    contactEnricher := contact.NewContactEnricher(contactRepo)

    // Init controllers
    orgCtl := api.NewOrgController(orgRepo, locEnricher, contactEnricher)

    http.HandleFunc("/organizations", orgCtl.Handler)
lunemec commented 2 years ago

@SergeAx thanks for your comments. Please note the purpose of this repository was to get some answers from more experienced Go devs. So it is not really "correct", however great summary I found was in this article: https://threedots.tech/post/common-anti-patterns-in-go-web-applications/

IMO that is the GOTO way to do things. Some things are similar to what you say, I just don't like the name enricher personally. Rather to have 1 method that runs the SQL with 3 joins for that specific purpose IMO, and then just pass to the domain logic.