Don't use context.Context for optional parameters

[romshark]

Problem

You seem to be using context.Context for optional parameters such as in database.Query() which can be considered an API design flaw for the following arguments:

Official recommendations violation:

The recommendations in the official package documentation clearly state:

Use context Values only for request-scoped data that transits processes and APIs, not for passing optional parameters to functions.

And this statement makes total sense if you consider the disadvantages described below.

Performance impact

Contexts are dynamic entities and for every optional parameter passed (for every query we perform) we're creating a new context object every time we're making a query also involving dynamic type casting. This has an obvious performance impact and produces unnecessary garbage that's then to be collected by the GC. Even though those impacts are rather small - this is not how we write code in a statically typed language like Go.

Usability issues (non-reusable options)

As the documentation clearly state: contexts are request scoped and must thus not be stored.

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it.

Though we tend to reuse optional parameters (declaring them globally, or reusing them for multiple queries in a row etc.) to avoid allocating them over and over again for each request for either usability and/or performance reasons.

Readability issues

Even though this is rather a matter of taste - most would still prefer something like:

db.Query(ctx, `query`, nil, arango.QueryOptions{
	BatchID: "someid",
	QueryBatchSize: 64,
})

Rather than:

ctx = arango.WithBatchID(context.Background(), "someid")
ctx = arango.WithQueryBatchSize(ctx, 64)
arango.db.Query(ctx, `query`, nil)

Solution Proposal

Specialized methods

Keep in mind, that some optional parameters like WaitForSync could be turned into separate specialized methods such as: db.QuerySync(context.Context, string, Parameters, Options) in this case. This only applies to rather substantial options.

Optional parameters interface

Working code example: https://play.golang.org/p/u-r-Ivu6f24

If a method really needs optional parameters then consider passing them by interfaces.

Options interface:

type Options interface {
	// Clone returns an exact deep copy of the options.
	// This helps avoiding mutability problems
	// since Go doesn't yet have immutable data structures
	Clone() Options

	// Validate returns nil, if all options are correct,
	// otherwise returns an error explaining the mistake
	Validate() error
	
	// SetDefaults sets the default values for unset options if necessary
	SetDefaults()
}

Database.Query method options:

// QueryOptions implements the Options interface
type QueryOptions struct {
	BatchID        string   // len < 1 when unset, always valid
	QueryBatchSize int64    // < 1 when unset, > 4096 when invalid
	ImportDetails  []string // nil when unset, < 1 when invalid
}

// Clone implements the Options interface
func (qo *QueryOptions) Clone() Options {
	// Copy the slice since it's a reference-type
	importDetails := make([]string, len(qo.ImportDetails))
	copy(importDetails, qo.ImportDetails)

	return &QueryOptions{
		BatchID:        qo.BatchID,
		QueryBatchSize: qo.QueryBatchSize,
		ImportDetails:  importDetails,
	}
}

// Validate implements the Options interface
func (qo *QueryOptions) Validate() error {
	if qo.ImportDetails != nil && len(qo.ImportDetails) < 1 {
		return fmt.Errorf(
			"Invalid value for option ImportDetails: empty",
		)
	}
	if qo.QueryBatchSize > 4096 {
		return fmt.Errorf(
			"Invalid QueryBatchSize option (%d): too big",
			qo.QueryBatchSize,
		)
	}
	return nil
}

// SetDefaults implements the Options interface
func (qo *QueryOptions) SetDefaults() {
	// Set default query batch size if it's not set
	if qo.QueryBatchSize < 1 {
		qo.QueryBatchSize = 32
	}
}

Reading:

func (db *database) Query(
	ctx context.Context,
	query string,
	bindVars map[string]interface{},
	options Options,
) (Cursor, error) {
	switch opts := options.(type) {
	case *QueryOptions:
		if opts == nil {
			return nil, fmt.Errorf("QueryOptions is nil")
		}

		// Validate the options
		if err := options.Validate(); err != nil {
			return nil, fmt.Errorf("Invalid options: %s", err)
		}

		// Get a copy of the options to avoid unexpected mutations
		options = options.Clone()

		// Set unset fields if necessary
		options.SetDefaults()

	case nil:
		// No options set, use default options
		options = &QueryOptions{
			BatchID:        "",
			QueryBatchSize: 32,
			ImportDetails:  nil,
		}

	default:
		// Unexpected options type
		return nil, fmt.Errorf(
			"Invalid options type: %s, expected: *QueryOptions",
			reflect.TypeOf(options),
		)
	}
	queryOptions := options.(*QueryOptions)

	// Use the options
	log.Print("BatchID: ", queryOptions.BatchID)
	log.Print("QueryBatchSize: ", queryOptions.QueryBatchSize)
	log.Print("ImportDetails: ", queryOptions.ImportDetails)

	return nil, nil
}

Usage:

db.Query(ctx, `...`, nil, arango.QueryOptions{
	QueryBatchSize: 128,
	ImportDetails: []string{"detail1", "detail2"},
})

Usage: (no options)

db.Query(ctx, `...`, nil, nil)

P.S.

I'm not fully aware of the library internals thus I can't claim for sure that you're abusing context.Context though it certainly looks so from the standpoint of a library user, because contexts should really only be used for 2 things:

• cancelation & deadlines
• passing data through (third-party) APIs

For example a good use-case would be passing connection authentication information from your network layer through a third-party GraphQL engine to your resolver functions, that's where contexts really shine, because otherwise this would not be possible due to the third-party library not being aware of the stuff you'd like to pass through it. For anything else than that - contexts are really bad!

I didn't have the time to look through the implementation of the Database interface but I hope that it does respect cancelable and timed contexts, otherwise there's no need in taking the context in the first place.

[djmcgreal-cc]

There's also somehow queryRequest with a nested Options inside, which matches query parameters available in ArangoDB, but have no way of setting them via Contexts. Context values must be documented and are hard to find, whereas if Options were exposed to the API, it would be self-documenting.

For example, I'm trying to get the query profile, and whilst there's an Options.Profile, there seems to be no context value that allows me to configure it.

[weslenng]

Using context.Context for optional parameters is a really weird design decision

[jwierzbo]

In V2 it's being moved to the Client configuration structure: https://github.com/arangodb/go-driver/pull/597