GODRIVER-3522 Add support for the rawData option for time-series bucket access - PR0

[qingyang-hu]

GODRIVER-3522

Summary

Add support for the rawData option for time-series bucket access - PR0

Add support for the following operations:

• aggregate
• count/countDocuments/estimatedDocumentCount
• delete
• distinct
• insert

Background & Motivation

[prestonvasquez]

From the scope:

Customers do not need to be able to set this in their applications. The interface will be marked “for internal use only”.

It sounds like we should add this as a deprecated option to InsertOneOptions and add a SetInternalInsertOneOptions function to the xoptions package, rather than exposing an options-level setter. It sounds like nobody will use this but internal teams, we can pivot in the future if necessary.

opts := options.InsertOne()
_ = xoptions.SetInternalInsertOneOptions(opts, "rawData", true)

[qingyang-hu]

I agree that the rawData option should be internal only. However, I doubt if it should be a client level option because it makes users to set up multiple clients to handle different scenario. IMO, for operation options following the Lister interface with a correlated "optionsBuilder" type, we can have a setter, for example:

// Deprecated: This function is for internal use only. It may be changed or removed in any release.
func (ao *AggregateOptionsBuilder) SetInternalOption(key string, option any) error

What are your thoughts?
cc: @matthewdale

[matthewdale]

I see an internal-only options pattern was added a few months ago: xoptions.SetInternalClientOptions. Can we extend that pattern for this use case?

[qingyang-hu]

The existing pattern for SetInternalClientOptions is meant for ClientOptions. I think that adding rawData directly as a client option will cause inconvenience in most use cases, such as when setting an option for a specific operation.

Another option would be like xoptions.SetInternalAggregateOptions(opts *AggregateOptionsBuilder, key string, option any) error. We add a setter method for each OptionsBuilder in the xoptions package.

[prestonvasquez]

@qingyang-hu This sounds correct:

We add a setter method for each OptionsBuilder in the xoptions package.

[matthewdale]

Seems like there are two good options based on the existing pattern of SetInternalClientOptions. I prefer the first one because it can be extended to support future internal options without any additional API changes. Does that sound reasonable?

Custom key/value pairs with xoptions setter

Add a Custom optionsutil.Options field to every CRUD method options struct (like ClientOptions.Custom). Add a function to xoptions to set custom k/v pairs in Custom.

E.g.

package options

type FindOptions struct {
    // ...
    Custom optionsutil.Options
}

package xoptions

func SetFindOneCustom(f *options.FindOneOptionsBuilder, key string, option any) {
	f.Opts = append(f.Opts, func(opts *FindOneOptions) error {
		opts.Custom = optionsutil.WithValue(opts.Custom, key, option)
		return nil
	})
}

RawData bool option with xoptions setter

Add a RawData *bool field to every CRUD method options struct. Add a function to xoptions to set RawData for each supported option type.

E.g.

package options

type FindOptions struct {
    // ...
    RawData *bool
}

package xoptions

func SetFindOneRawData(f *options.FindOneOptionsBuilder, b bool) {
	f.Opts = append(f.Opts, func(opts *FindOneOptions) error {
		opts.RawData = &b
		return nil
	})
}

[matthewdale]

We should add a note that enabling it on pre-9.0 server versions has no effect. This applies for all SetRawData docs.

Note: This comment is irrelevant if we convert RawData into an internal-only option.

[prestonvasquez]

LGTM with a few suggestions

[prestonvasquez]

[nit] w could reduce LoC here:

	return nil, xoptions.SetInternalCountOptions(opts, key, val.Boolean())

[prestonvasquez]

[nit] suggest putting the key in quotes:

return fmt.Errorf("unexpected type for %q: %T is not %s", key, option, t)

[prestonvasquez]

We shouldn't need to add this deprecation notice to x package API.

[matthewdale]

I suggest renaming the new fields to Internal because it more clearly describes the purpose and matches the naming of the xoptions functions.

[matthewdale]

Looks good! 👍