freerware / work Goto Github PK

BACKGROUND

when I initially created this repository and started work on it, Travis CI was the quickest and easiest route for me. since then, GitHub actions has become more mature and prolific. switching to it would provide the following benefits:

• free
• everything is visible in GitHub (less context switching)
• less complex
  • TravisCI has since deprecated travisci.org, which is what this repository used.
  • don't have to worry about plans.
  • don't have to worry about repository access.

PROPOSAL

switch to GitHub Actions.

BACKGROUND

As brought up in #57, the project lost steam just prior to releasing V4. this version bring along a lot of benefit to usability and feature sets.

PROPOSAL

Get #55 out the door, and then cut a new beta release. Once that is out and things look solid, cut V4.

In particular, we should rename work.UnitLogger (unit.Logger) to work.UnitZapLogger (unit.ZapLogger), and work.UnitScope (unit.Scope) to work.UnitTallyMetricScope (unit.TallyMetricScope).

This approach opens the door for us to utilize work.UnitLogger and work.UnitMetricScope in a way that is more standardized if such a consensus arrives. For example, logr is an attempt of the community to consolidate on a consistent logging interface.

Let's get things up to speed.

Does this use a batch request? There isn't any reference to one.

Description

When leveraging SQL work units, I am experiencing a panic when I do not provide a tally.Scope via work.UnitScope(...).

...

tri-fitness/genesis/vendor/github.com/freerware/work.(*sqlUnit).rollback.func1
        /home/jon/go/src/tri-fitness/genesis/vendor/github.com/freerware/work/sql_unit.go:109
tri-fitness/genesis/vendor/github.com/freerware/work.(*sqlUnit).rollback
        /home/jon/go/src/tri-fitness/genesis/vendor/github.com/freerware/work/sql_unit.go:118
tri-fitness/genesis/vendor/github.com/freerware/work.(*sqlUnit).applyInserts
        /home/jon/go/src/tri-fitness/genesis/vendor/github.com/freerware/work/sql_unit.go:125
tri-fitness/genesis/vendor/github.com/freerware/work.(*sqlUnit).Save
        /home/jon/go/src/tri-fitness/genesis/vendor/github.com/freerware/work/sql_unit.go:191
tri-fitness/genesis/application.(*AccountService).Create
        /home/jon/go/src/tri-fitness/genesis/application/account.go:80
tri-fitness/genesis/api/resources.(*AccountResource).CreateAndAppend
        /home/jon/go/src/tri-fitness/genesis/api/resources/account.go:181

...

It appears that the issue is caused by this bit of code on line 123 in unit.go:

122 func (u *unit) startTimer(name string) func() {¬                                                                                                                                          
123 ▹   var stopFunc func()¬                                                                                                                                                                  
124 ▹   if u.hasScope() {¬                                                                                                                                                                    
125 ▹   ▹   stopFunc = u.scope.Timer(name).Start().Stop¬                                                                                                                                      
126 ▹   }¬                                                                                                                                                                                    
127 ▹   return stopFunc¬                                                                                                                                                                      
128 }¬ 

The zero value of func is nil, as defined in the Go specification. The consumer of this method attempts to call the return function, which may actually be nil, which causes the observed panic.

Steps to Reproduce

1. Create a work unit without using the work.UnitScope(...) option.
2. [Optional] Track some changes using Alter, Add, or Remove.
3. Call Save.

Expected Behavior

I expect that a panic does not occur when attempting to utilize work units without the work.UnitScope(...) option.

Actual Behavior

As detailed in the description, I encounter a panic every time I call Save, regardless if a rollback does or does not occur (as both paths leverage the offending code).

Versions

$ go version && dep version
go version go1.12.9 linux/amd64
dep:
 version     : v0.5.0
 build date  : 2018-07-26
 git hash    : 224a564
 go version  : go1.10.3
 go compiler : gc
 platform    : linux/amd64
 features    : ImportDuringSolve=false

I am currently using v2.0.0 of work.

Additional Context

Nope!

Description

Another well known design pattern presented by Martin Fowler is known as the Identity Map. Although very straightforward, this pattern can be incredibly powerful, as it reduce unnecessary round trips between you application and it's data store.

I propose being able to inject an Identity Map into the implementers of work.Unit. Doing so would allow the work units to manage the state of the identity map.

Example Usage

The Identity Map can be injected by creating a new option:

var (

    ...

    // UnitIdentityMap specifies the option to provide an identity map for the work unit.
    UnitIdentityMap = func(identityMap map[interface{}]Entity) Option {
        return func(o *UnitOptions) {
            o.IdentityMap = identityMap
        }
    }

    ...
)

where Entity is the following interface:

// Entity represents an entity managed by the work unit.
type Entity interface {
    ID() interface{}
}

The work.Unit interface would need to be adapted slightly:

type Unit interface {

    // Register tracks the provided entities as clean.
    Register(...Entity) error

    // Add marks the provided entities as new additions.
    Add(...Entity) error

    // Alter marks the provided entities as modifications.
    Alter(...Entity) error

    // Remove marks the provided entities as removals.
    Remove(...Entity) error

    // Save commits the new additions, modifications, and removals
    // within the work unit to a persistent store.
    Save() error
}

Lastly, each implementer of work.Unit will then appropriately store the provided entities to Register(...) into the Identity Map. If an entity is passed into Alter(...) or Remove(...) and it also exists in the Identity Map, than it's entry will be removed. Calling Save() on the work unit will clear the Identity Map if successful.

Since the consumer of the work provides the Identity Map, they have free will to use it as see fit. in particular, the consumer should check the Identity Map before performing retrieval operations against their data store. some like the following:

...

// Get retrieves the account with the UUID provided from the repository.
func (a AccountRepository) Get(uuid u.UUID) (Account, error) {
  query := NewFindByUUIDQuery(uuid, a.identityMap) // <-- pass in the identity map to query object.
  account, err := query.Execute() // <--- checks the identity map before issuing query.
  if err != nil {
    return Account{}, err
  }
  if err = a.unit.Register(account); err != nil { // <-- Register(...) will update the identity map.
    return Account{}, err
  }
  return account, nil
}

...

Description

It may be desirable for consumers of work to perform concurrent operations that interact with work units. One such prominent example is for Register(...). Register(...) is called during database retrieval operations, and retrieval operations are typically parallelized when there is a lot of data to fetch.

Example Usage

There would be no interface changes. Instead, the data structures used internally would need to support concurrent writes/reads, such as sync.Map. Doing so would allow the following trivial example:

...
var w work.Unit
w = ...
func register(entity interface{}) {
  w.Register(entity)
}
go register(Foo{})
go register(Bar{})
...

I'm interested in your library implementing UoW.
Could I help to move the lib in the stable state?
Are there a roadmap or features that need to be added in?

PROPOSAL

Given Go has a robust toolchain that provides benchmarking out of the box, and also because we have already put together a simple "benchmarking suite" here for v4, we should run benchmarks and detect performance regressions when a push is performed.

Since we have migrated over to GitHub Actions, this should be pretty straightforward. This action looks promising.

BACKGROUND

As it stands currently, consuming code must create instances abiding by the unit.DataMapper interface and pass those instances in via the unit.DataMappers option. Although this approach functionally operates well, being constrained to this approach has some downsides:

• in situations where the data mapping logic is straightforward and doesn't require building instances, consumers are having to craft these instances purely to adapt their code to fit the API.
• it doesn't make use of functional programming elements of Go, in that functions are first class concepts and can be passed around and referenced.

PROPOSAL

there are still use cases for support unit.DataMapper and unit.DataMappers going forward. in particular complex codebases already are using OO principles to encapsulate their data mapping responsibilities (via Repository, Data Mapper, etc.). these types may already be designed to track state, in which a struct would be required.

however, in instances where that isn't the case, let's provide the ability for consumers to pass functions instead. below is some rough psuedocode of what i'm aiming for:

// entities.
f := Foo{}

// type names.
ft := unit.TypeNameOf(f)

// 🎉
opts = []unit.Option{
  unit.DB(db),
  unit.InsertFunc(ft, func(context.Context, MapperContext, ...unit.Entity{}){
    // insertion data mapper logic.
  }),
  unit.UpdateFunc(ft, func(context.Context, unit.MapperContext, ...unit.Entity{}){
    // update data mapper logic.
  }),
  unit.DeleteFunc(ft, func(context.Context, unit.MapperContext, ...unit.Entity{}){
    // deletion data mapper logic.
  }),
}
unit, err := unit.New(opts...)

if the unit.DataMappers option is specified alongside any of these new options, an error would be returned during unit construction.

another callout is that this approach will likely yield the positive side-effect that we can adapt the code to handle both usage of unit.DataMapper and these functions, as we could simply pass the corresponding function on unit.DataMapper when the unit.DataMappers option is used.

Description

It may be desirable for consumers of work to pass in and instance of context.Context when interacting with work units. Various database drivers support APIs that utilize context.Context, and downstream services that are called during work unit saves may also leverage them. In addition, GitHub wrote a blog post detailing how the context actually plays a critical role within the sql package that can impact runtime behavior for the better.

Example Usage

The work.Unit, work.DataMapper, and work.SQLDataMapper would need to be altered to include additional methods hear allow a context.Context parameter to be passed as the first argument.

...
ctx := context.Background()
w := work.NewSQLUnit(...)
entities := ...

if err := w.Add(ctx, entities...); err != nil {
  panic(err)
}
...

Description

The file headers are still stuck in 2019 😄 . As much as we may not want to acknowledge 2020, we should update them to let people know we are hip.

Currently the README.md doesn't show on go.dev. We should fix that.

Description

Currently there is some boilerplate code that checks whether a logger or a metric scope has been provided to the work units. Instead, this code can be simplified if the "no operation" type from zap ( zap.NewNop() ) and tally ( tally.NoopScope ).

Hello,

sqlUnit is strongly connected with database/sql because of *sql.Tx.
However, sql.Tx could be placed in an interface with Begin, Commit and Rollback functions.
That gives the ability to change databases without changes the unit implementation.

What do you think about the idea?

package main

import (
    "context"
    "database/sql"
    "github.com/DATA-DOG/go-sqlmock"
)

type TrM interface {
    Do(ctx context.Context, fn func(ctx context.Context, tx interface{}) error) error
}

type Mapper interface {
    Insert(ctx context.Context, tx interface{}, additions ...interface{}) error
}

type trm struct {
    db *sql.DB
}

func (t trm) Do(ctx context.Context, fn func(ctx context.Context, tx interface{}) error) error {
    tx, _ := t.db.BeginTx(ctx, nil)
    defer tx.Commit()

    return fn(ctx, tx)
}

type sqlMapper struct{}

func (s sqlMapper) Insert(_ context.Context, txAny interface{}, _ ...interface{}) error {
    tx, _ := txAny.(*sql.Tx)

    _ = tx
    // tx.Exec(...)

    return nil
}

type unit struct {
    trm       TrM
    mapper    Mapper
    additions []interface{}
}

func (u *unit) Save(ctx context.Context) error {
    return u.trm.Do(ctx, func(ctx context.Context, tx interface{}) error {
        return u.mapper.Insert(ctx, tx, u.additions...)
    })
}

func main() {
    db, mock, _ := sqlmock.New()
    mock.ExpectBegin()
    mock.ExpectCommit()

    u := &unit{
        trm:    trm{db: db},
        mapper: sqlMapper{},
    }

    u.Save(context.Background())

    if err := mock.ExpectationsWereMet(); err != nil {
        panic(err)
    }
}

//go:build go1.18
// +build go1.18

package main

import (
	"context"
	"database/sql"
	"github.com/DATA-DOG/go-sqlmock"
)

type TrM[Tx any] interface {
	Do(ctx context.Context, fn func(ctx context.Context, tx Tx) error) error
}

type Mapper[Tx any] interface {
	Insert(ctx context.Context, tx Tx, additions ...interface{}) error
}

type trm struct {
	db *sql.DB
}

func (t trm) Do(ctx context.Context, fn func(ctx context.Context, tx *sql.Tx) error) error {
	tx, _ := t.db.BeginTx(ctx, nil)
	defer tx.Commit()

	return fn(ctx, tx)
}

type record struct {
	ID int
}

type recordMapper struct{}

func (r recordMapper) Insert(_ context.Context, _ *sql.Tx, _ ...interface{}) error {
	return nil
}

type unit[Tx any] struct {
	trm       TrM[Tx]
	mapper    Mapper[Tx] // replace on map
	additions []interface{}
}

func (u *unit[Tx]) Save(ctx context.Context) error {
	return u.trm.Do(ctx, func(ctx context.Context, tx Tx) error {
		return u.mapper.Insert(ctx, tx, u.additions...)
	})
}

func main() {
	db, mock, _ := sqlmock.New()
	mock.ExpectBegin()
	mock.ExpectCommit()

	u := &unit[*sql.Tx]{
		trm:    trm{db: db},
		mapper: recordMapper{},
	}

	u.Save(context.Background())

	if err := mock.ExpectationsWereMet(); err != nil {
		panic(err)
	}
}

Description

Especially in larger, more complex codebases, it is likely desirable for consumers of work to specify particular actions (AKA “hooks”) before and after particular events occur. My initial brainstorming has brought forward these candidates:

• AfterRegister
• AfterAdd
• AfterAlter
• AfterRemove
• BeforeInserts
• AfterInserts
• BeforeUpdates
• AfterUpdates
• BeforeDeletions
• AfterDeletions
• BeforeRollback
• AfterRollback
• BeforeSave
• AfterSave

Example Usage

The parameters passed into both work.SQLUnit and work.BestEffortUnit would need to accommodate for these actions to be specified as options:

// example of AfterSave(...) action specified as an option.
w := work.NewSQLUnit(mappers, db, work.AfterSave(func(ctx work.UnitContext) {
  ctx.logger.Info(“successful save!”)
})

The definition of the actions could represented like so:

// Action represents an operation that is performed before or after a significant work unit event.
type Action func(UnitContext)

type UnitContext struct {
  ...
}

After investigating the latest documentation for zap, there is a constructor that wraps a zap.Logger with log.Logger. Since log.Logger is a part of the standard library, it would be best to utilize this logger to prevent a tight coupling between work and any particular logging implementation.

As brought up in #56, it isn't explicitly discussed in the documents (README.md) that all instances of a particular type are passed down the data mapper, thus allowing the consumer to perform batch operations.

We should call this out explicitly, whether it be the birth of a FAQ, wiki, or it be added in the README for now.

Should be quick - first should tackle #58 to ensure that we can get CI back up a running first. Additionally might be easier going forward once #40 lands as we won't have to maintain dependencies for V1 and V2 any longer.

Description

By nature, the sequence that occurs in order to successfully save or rollback is well suited for being managed by a stack data structure. In general, construction of the internal stack will occur based on the options provided to the work unit at construction time. During the save process, each sequence will be processed by popping it off of the stack and placed on a separate stack (the "rollback stack") that maintains the reverse order. If a rollback is required, we instead continue to process the rollback stack until it is empty.

Description

In order to maintain flowing syntax, many types are prefixed with "unit", so that it is understood that they are associated to the core work.Unit itself.

It appears the work.Option has been missed, and thus is inconsistent. It should be named to work.UnitOption.