[PROPOSAL / QUESTION] Is there a plan to improve usability by making the API more idiomatic to the Go ecosystem?

[henvic]

What kind of business use case are you trying to solve? What are your requirements?

Hi. First, thank you for releasing this client.

I had been using the original one for the last couple of weeks and I noticed that it uses some patterns (like) that aren't so commonly found on Go clients (generated or not).

I imagine that having a leaner API using a more traditional (as in Go idiomatic) approach would be a good thing, at least for casual API consumers that are primarily Go developers – although I'm not so sure about people with plenty of experience using OpenSearch.

What is the problem? What is preventing you from meeting the requirements?
Let me try to give you a concrete example:

I imported the package to my code for the first time, and then I started looking around to see how I could do what I needed.
I quickly discovered that I needed to instantiate a client by calling opensearch.NewClient.

Then, the next thing I discovered was the Search function there, except that I immediately got confused as to why it was defined in the confusing API struct in an oddly named api._.go file.

opensearch-go/opensearchapi/api._.go

Lines 31 to 83 in 1aec4d8

	type API struct {
	Cat *Cat
	Cluster *Cluster
	Indices *Indices
	Ingest *Ingest
	Nodes *Nodes
	Remote *Remote
	Snapshot *Snapshot
	Tasks *Tasks
	Bulk Bulk
	ClearScroll ClearScroll
	Count Count
	Create Create
	DanglingIndicesDeleteDanglingIndex DanglingIndicesDeleteDanglingIndex
	DanglingIndicesImportDanglingIndex DanglingIndicesImportDanglingIndex
	DanglingIndicesListDanglingIndices DanglingIndicesListDanglingIndices
	DeleteByQuery DeleteByQuery
	DeleteByQueryRethrottle DeleteByQueryRethrottle
	Delete Delete
	DeleteScript DeleteScript
	Exists Exists
	ExistsSource ExistsSource
	Explain Explain
	FieldCaps FieldCaps
	Get Get
	GetScriptContext GetScriptContext
	GetScriptLanguages GetScriptLanguages
	GetScript GetScript
	GetSource GetSource
	Index Index
	Info Info
	Mget Mget
	Msearch Msearch
	MsearchTemplate MsearchTemplate
	Mtermvectors Mtermvectors
	Ping Ping
	PutScript PutScript
	RankEval RankEval
	Reindex Reindex
	ReindexRethrottle ReindexRethrottle
	RenderSearchTemplate RenderSearchTemplate
	ScriptsPainlessExecute ScriptsPainlessExecute
	Scroll Scroll
	Search Search
	SearchShards SearchShards
	SearchTemplate SearchTemplate
	TermsEnum TermsEnum
	Termvectors Termvectors
	UpdateByQuery UpdateByQuery
	UpdateByQueryRethrottle UpdateByQueryRethrottle
	Update Update
	}

and I started looking for usage examples just after I noticed the following

type Search func(o ...func(*SearchRequest)) (*Response, error)

and then I discovered there are functions defined on top of this SearchRequest type

opensearch-go/opensearchapi/api.search.go

Lines 379 to 809 in 1aec4d8

	// WithContext sets the request context.
	//
	func (f Search) WithContext(v context.Context) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.ctx = v
	}
	}
	// WithBody - The search definition using the Query DSL.
	//
	func (f Search) WithBody(v io.Reader) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Body = v
	}
	}
	// WithIndex - a list of index names to search; use _all to perform the operation on all indices.
	//
	func (f Search) WithIndex(v ...string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Index = v
	}
	}
	// WithDocumentType - a list of document types to search; leave empty to perform the operation on all types.
	//
	func (f Search) WithDocumentType(v ...string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.DocumentType = v
	}
	}
	// WithAllowNoIndices - whether to ignore if a wildcard indices expression resolves into no concrete indices. (this includes `_all` string or when no indices have been specified).
	//
	func (f Search) WithAllowNoIndices(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.AllowNoIndices = &v
	}
	}
	// WithAllowPartialSearchResults - indicate if an error should be returned if there is a partial search failure or timeout.
	//
	func (f Search) WithAllowPartialSearchResults(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.AllowPartialSearchResults = &v
	}
	}
	// WithAnalyzer - the analyzer to use for the query string.
	//
	func (f Search) WithAnalyzer(v string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Analyzer = v
	}
	}
	// WithAnalyzeWildcard - specify whether wildcard and prefix queries should be analyzed (default: false).
	//
	func (f Search) WithAnalyzeWildcard(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.AnalyzeWildcard = &v
	}
	}
	// WithBatchedReduceSize - the number of shard results that should be reduced at once on the coordinating node. this value should be used as a protection mechanism to reduce the memory overhead per search request if the potential number of shards in the request can be large..
	//
	func (f Search) WithBatchedReduceSize(v int) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.BatchedReduceSize = &v
	}
	}
	// WithCcsMinimizeRoundtrips - indicates whether network round-trips should be minimized as part of cross-cluster search requests execution.
	//
	func (f Search) WithCcsMinimizeRoundtrips(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.CcsMinimizeRoundtrips = &v
	}
	}
	// WithDefaultOperator - the default operator for query string query (and or or).
	//
	func (f Search) WithDefaultOperator(v string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.DefaultOperator = v
	}
	}
	// WithDf - the field to use as default where no field prefix is given in the query string.
	//
	func (f Search) WithDf(v string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Df = v
	}
	}
	// WithDocvalueFields - a list of fields to return as the docvalue representation of a field for each hit.
	//
	func (f Search) WithDocvalueFields(v ...string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.DocvalueFields = v
	}
	}
	// WithExpandWildcards - whether to expand wildcard expression to concrete indices that are open, closed or both..
	//
	func (f Search) WithExpandWildcards(v string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.ExpandWildcards = v
	}
	}
	// WithExplain - specify whether to return detailed information about score computation as part of a hit.
	//
	func (f Search) WithExplain(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Explain = &v
	}
	}
	// WithFrom - starting offset (default: 0).
	//
	func (f Search) WithFrom(v int) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.From = &v
	}
	}
	// WithIgnoreThrottled - whether specified concrete, expanded or aliased indices should be ignored when throttled.
	//
	func (f Search) WithIgnoreThrottled(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.IgnoreThrottled = &v
	}
	}
	// WithIgnoreUnavailable - whether specified concrete indices should be ignored when unavailable (missing or closed).
	//
	func (f Search) WithIgnoreUnavailable(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.IgnoreUnavailable = &v
	}
	}
	// WithLenient - specify whether format-based query failures (such as providing text to a numeric field) should be ignored.
	//
	func (f Search) WithLenient(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Lenient = &v
	}
	}
	// WithMaxConcurrentShardRequests - the number of concurrent shard requests per node this search executes concurrently. this value should be used to limit the impact of the search on the cluster in order to limit the number of concurrent shard requests.
	//
	func (f Search) WithMaxConcurrentShardRequests(v int) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.MaxConcurrentShardRequests = &v
	}
	}
	// WithMinCompatibleShardNode - the minimum compatible version that all shards involved in search should have for this request to be successful.
	//
	func (f Search) WithMinCompatibleShardNode(v string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.MinCompatibleShardNode = v
	}
	}
	// WithPreference - specify the node or shard the operation should be performed on (default: random).
	//
	func (f Search) WithPreference(v string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Preference = v
	}
	}
	// WithPreFilterShardSize - a threshold that enforces a pre-filter roundtrip to prefilter search shards based on query rewriting if the number of shards the search request expands to exceeds the threshold. this filter roundtrip can limit the number of shards significantly if for instance a shard can not match any documents based on its rewrite method ie. if date filters are mandatory to match but the shard bounds and the query are disjoint..
	//
	func (f Search) WithPreFilterShardSize(v int) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.PreFilterShardSize = &v
	}
	}
	// WithQuery - query in the lucene query string syntax.
	//
	func (f Search) WithQuery(v string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Query = v
	}
	}
	// WithRequestCache - specify if request cache should be used for this request or not, defaults to index level setting.
	//
	func (f Search) WithRequestCache(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.RequestCache = &v
	}
	}
	// WithRestTotalHitsAsInt - indicates whether hits.total should be rendered as an integer or an object in the rest search response.
	//
	func (f Search) WithRestTotalHitsAsInt(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.RestTotalHitsAsInt = &v
	}
	}
	// WithRouting - a list of specific routing values.
	//
	func (f Search) WithRouting(v ...string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Routing = v
	}
	}
	// WithScroll - specify how long a consistent view of the index should be maintained for scrolled search.
	//
	func (f Search) WithScroll(v time.Duration) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Scroll = v
	}
	}
	// WithSearchType - search operation type.
	//
	func (f Search) WithSearchType(v string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.SearchType = v
	}
	}
	// WithSeqNoPrimaryTerm - specify whether to return sequence number and primary term of the last modification of each hit.
	//
	func (f Search) WithSeqNoPrimaryTerm(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.SeqNoPrimaryTerm = &v
	}
	}
	// WithSize - number of hits to return (default: 10).
	//
	func (f Search) WithSize(v int) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Size = &v
	}
	}
	// WithSort - a list of <field>:<direction> pairs.
	//
	func (f Search) WithSort(v ...string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Sort = v
	}
	}
	// WithSource - true or false to return the _source field or not, or a list of fields to return.
	//
	func (f Search) WithSource(v ...string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Source = v
	}
	}
	// WithSourceExcludes - a list of fields to exclude from the returned _source field.
	//
	func (f Search) WithSourceExcludes(v ...string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.SourceExcludes = v
	}
	}
	// WithSourceIncludes - a list of fields to extract and return from the _source field.
	//
	func (f Search) WithSourceIncludes(v ...string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.SourceIncludes = v
	}
	}
	// WithStats - specific 'tag' of the request for logging and statistical purposes.
	//
	func (f Search) WithStats(v ...string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Stats = v
	}
	}
	// WithStoredFields - a list of stored fields to return as part of a hit.
	//
	func (f Search) WithStoredFields(v ...string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.StoredFields = v
	}
	}
	// WithSuggestField - specify which field to use for suggestions.
	//
	func (f Search) WithSuggestField(v string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.SuggestField = v
	}
	}
	// WithSuggestMode - specify suggest mode.
	//
	func (f Search) WithSuggestMode(v string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.SuggestMode = v
	}
	}
	// WithSuggestSize - how many suggestions to return in response.
	//
	func (f Search) WithSuggestSize(v int) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.SuggestSize = &v
	}
	}
	// WithSuggestText - the source text for which the suggestions should be returned.
	//
	func (f Search) WithSuggestText(v string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.SuggestText = v
	}
	}
	// WithTerminateAfter - the maximum number of documents to collect for each shard, upon reaching which the query execution will terminate early..
	//
	func (f Search) WithTerminateAfter(v int) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.TerminateAfter = &v
	}
	}
	// WithTimeout - explicit operation timeout.
	//
	func (f Search) WithTimeout(v time.Duration) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Timeout = v
	}
	}
	// WithTrackScores - whether to calculate and return scores even if they are not used for sorting.
	//
	func (f Search) WithTrackScores(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.TrackScores = &v
	}
	}
	// WithTrackTotalHits - indicate if the number of documents that match the query should be tracked.
	//
	func (f Search) WithTrackTotalHits(v interface{}) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.TrackTotalHits = v
	}
	}
	// WithTypedKeys - specify whether aggregation and suggester names should be prefixed by their respective types in the response.
	//
	func (f Search) WithTypedKeys(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.TypedKeys = &v
	}
	}
	// WithVersion - specify whether to return document version as part of a hit.
	//
	func (f Search) WithVersion(v bool) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Version = &v
	}
	}
	// WithPretty makes the response body pretty-printed.
	//
	func (f Search) WithPretty() func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Pretty = true
	}
	}
	// WithHuman makes statistical values human-readable.
	//
	func (f Search) WithHuman() func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.Human = true
	}
	}
	// WithErrorTrace includes the stack trace for errors in the response body.
	//
	func (f Search) WithErrorTrace() func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.ErrorTrace = true
	}
	}
	// WithFilterPath filters the properties of the response body.
	//
	func (f Search) WithFilterPath(v ...string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	r.FilterPath = v
	}
	}
	// WithHeader adds the headers to the HTTP request.
	//
	func (f Search) WithHeader(h map[string]string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	if r.Header == nil {
	r.Header = make(http.Header)
	}
	for k, v := range h {
	r.Header.Add(k, v)
	}
	}
	}
	// WithOpaqueID adds the X-Opaque-Id header to the HTTP request.
	//
	func (f Search) WithOpaqueID(s string) func(*SearchRequest) {
	return func(r *SearchRequest) {
	if r.Header == nil {
	r.Header = make(http.Header)
	}
	r.Header.Set("X-Opaque-Id", s)
	}
	}

which are really decorators(-ish?) on top of the much more idiomatic SearchRequest param:

opensearch-go/opensearchapi/api.search.go

Lines 59 to 117 in 1aec4d8

	type SearchRequest struct {
	Index []string
	DocumentType []string
	Body io.Reader
	AllowNoIndices *bool
	AllowPartialSearchResults *bool
	Analyzer string
	AnalyzeWildcard *bool
	BatchedReduceSize *int
	CcsMinimizeRoundtrips *bool
	DefaultOperator string
	Df string
	DocvalueFields []string
	ExpandWildcards string
	Explain *bool
	From *int
	IgnoreThrottled *bool
	IgnoreUnavailable *bool
	Lenient *bool
	MaxConcurrentShardRequests *int
	MinCompatibleShardNode string
	Preference string
	PreFilterShardSize *int
	Query string
	RequestCache *bool
	RestTotalHitsAsInt *bool
	Routing []string
	Scroll time.Duration
	SearchType string
	SeqNoPrimaryTerm *bool
	Size *int
	Sort []string
	Source []string
	SourceExcludes []string
	SourceIncludes []string
	Stats []string
	StoredFields []string
	SuggestField string
	SuggestMode string
	SuggestSize *int
	SuggestText string
	TerminateAfter *int
	Timeout time.Duration
	TrackScores *bool
	TrackTotalHits interface{}
	TypedKeys *bool
	Version *bool
	Pretty bool
	Human bool
	ErrorTrace bool
	FilterPath []string
	Header http.Header
	ctx context.Context
	}

I see several problems with decorator functions like these.

I also noticed that gopls wasn't helping me much discover the API for the library, and I attribute part of the problem to how you're expected to build the code kind indirectly using decorators – making browsing code and documentation slower than it may be.

• There are other usage complications too, but I feel I need more time using it to share a more thoughtful opinion.

What are you proposing? What do you suggest we do to solve the problem or improve the existing situation?

• Primarily, remove these functional/decorator functions and make some adjustments to the API to make it feel more like a traditional Go API.
• Also, improve the high-level documentation (probably after making the API leaner).

In short, my suggestion is to do a clean-up to make the API more idiomatic to the Go ecosystem before releasing a v1 version.
What do you think?

What are your assumptions or prerequisites?
I'm assuming the current API can improve developer experience getting rid of some, apparently unimportant, odd patterns (to the Go programming language), which makes the API itself bigger than it needs to be, and harder to master.

Best regards,
Henrique.

[henvic]

Talking about SearchRequest, we've the following function:

func (r SearchRequest) Do(ctx context.Context, transport Transport) (*Response, error)

I believe that it'd make more sense to have client.Search with a signature such as the following:

func (c *Client) Search(ctx context.Context, params SearchRequestParams) (*SearchResponse, error)

and then something along the lines of:

type SearchResponse struct {
	Took     int                   `json:"_took"`
	TimedOut bool                  `json:"timed_out"`
	Shards   *SearchResponseShards `json:"shards,omitempty"`
	Hits     *SearchResponseHits   `json:"hits,omitempty"`
}

type SearchResponseShards struct {
	Total      int
	Successful int
	Skipped    int
	Failed     int
}

type SearchResponseHits struct {
	Total    SearchResponseHitsTotal
	MaxScore json.Number         `json:"max_score"`
	Hits     []SearchResponseHit `json:"hits,omitempty"`
}

type SearchResponseHitsTotal struct {
	Value    int
	Relation string
}

type SearchResponseHit struct {
	Index  string          `json:"_index"`
	Type   string          `json:"_type"`
	ID     string          `json:"_id"`
	Score  json.Number     `json:"_score"`
	Source json.RawMessage `json:"_source"`
}

The error returned there could be used as follows:

resp, err := client.Search(ctx, params)
if err != nil {
	var serr *opensearch.SearchError
	if errors.As(err, &serr) {
		log.Printf("opensearch error: %s\n", serr.Error.Reason)
		return ...
	}
	// otherwise, not the kind of error we expect...
	return ...
}
// success flow

[VijayanB]

@henvic Thanks for opening an issue. We will take a look at it and will get back soon.

[VijayanB]

@henvic Thanks for detailed proposal. We agree that we should improve the interface, to make API more idiomatic to the Go ecosystem. For first release, our focus is not to make any changes to existing API, hence, users who are migrating from elasticsearch oss to OpenSearch will have less impact. Updating API will break existing users.

We should invest on improving code generation (api structs) to be more go friendly (not in first release). We will be prioritizing this effort based on community feedback/ask.

[wbeckler]

What do people think about adding the idiomatic options and preserving the older accessors? Besides the carrying cost and the confusion of having more than one way to do things, are there more fundamental incompatibilities with both options existing?

[Jakob3xD]

What do people think about adding the idiomatic options and preserving the older accessors?

What is the benefit of keeping both. It will complicate the maintenance and it might confuse users on how to use the lib.

There are several ways on how to build the lib. We should discuss how we want the lib to be used and enforce the style. Therefor I suggest that the lib has one way of doing things. Currently users have two options on how to perform request and there is now guide on which is the preferred one. This results in users having different implementation of the lib. Currently users can create a request and execute it with the Do() function or use the client to execute a request and adding the options via the With functions.

The current implementation of the With functions is weird. Having a newRequestXFunc returning a Request func by looping over its request arguments is odd.

What implementation options do I see:

1. Sub clients using With functions to add arguments.
   This would give users an easy way of performing a request by just calling the client func and adding arguments by calling With functions. Downside I see: Requests like search with a lot of options would be very hard to call as they get really long and might be complicated to read.

2. Sub clients using Request structs.
   Users can execute the request by calling the client and giving a request struct to the func. This is also a simple way for users to execute a request. This is more like the already suggested solution. Downside is, that users most of the time need to at least give an empty request struct to the called func, which can look odd.

3. Sub clients requiring fixed arguments.
   Users can execute the request by calling the client and giving the required definition of arguments.
   This might look simple but users would always need to define all arguments even thought they don't need them.
   For search request this would escalate in an extreme amount of arguments. IMO this is not a real option.

4. Using the current Do() functions on each request.
   This would remove the API definitions from the client. So the user would need to create a Request struct and then call the Do() function handing over the client. With this solution we would only have a lose collections of requests without any mapping to the client. It could make the documentation look weird and might lead to misunderstanding.

This was a short overview of implementations I can think of. There might be more pro and con arguments for each options we could discuss, but right now option 2 seems the best to me. Therefore I created a branch to display the change and the use by adjusting some tests. Moreover I moved the parameters of the request to a new struct and file so we can separate the parameter parsing from the actual request and also prepare the code to be generated once we implement the code generation. I also de-duplicated the request build and execution to a general function as all request do the same. With this change we might also remove the Elasticsearch license stuff as the code changes.

What we might want to discuss is the use of pointers for request. Using pointers is faster and in some cases can shorten the users code, as they can give nil as argument, but we also need to check in most cases if the request is not nil and otherwise return an error. One could also argue that the time benefit of using pointers is negligibly when calling a HTTP API.
main...Jakob3xD:opensearch-go:feature-idiomatic

I would really like to hear @wbeckler @dblock and @VachaShah opinion on this. If you have any questions I am happy to explain things further.

[dblock]

💯 for simplifying, keeping 1 way of doing things, and deleting everything else

I am not a go expert to say which one is best, as a rule of thumb I would say that we want developers to write the least amount of code when using the library. As an example the simplification of error handling was along those lines.

[dtaivpp]

Agreed that it would be best to make it more idiomatic for Go users. I think it would be great if we could soften the blow.

@Jakob3xD the reason to keep both (for a short time or maybe a slow transition) is to give users with dependencies on this client some time to adapt. It would be quite a shock to attempt an update of the client to discover the ways that you had been using it (while funky) had been completely changed version over version.

[Jakob3xD]

the reason to keep both (for a short time or maybe a slow transition) is to give users with dependencies on this client some time to adapt.

The issue I see, we can't make old and new work together without creating a new/different client and duplicating code.
As we already plan to do a braking change I would suggest switching to the new style, so the users rewrite there code once for this braking change/ major version update. And because it is a major version update, I as a client would expect that the lib use might change.

Just to clarify, I am talking about rewriting/changing the whole opensearch-api package. We could go even further and rethink the file names as using . is uncommon.

Without a decision on how we want to struct the lib it make no sense to implement any features as it is not clear how they should look like and where they should be.

For the general structure of the client, I suggest the linked diff. More over we should create a plugins package which contains a sub package for each plugin. So for example:

plugins/osism
plugins/ossecurity
plugins/osml

Names can be discussed, but for example naming it just security might cause issue as security is a common name.
Users could then call resp, secUserResp, err := client.Security.Users.Update(&ossecurity.UsersUpdateRequest{some args}).
We should not move the plugin functions and structs to the opensearchapi package because the responses and errors are different.

[Jakob3xD]

In relation to my listed options and my diff.
Another option would be to create a generic request interface and instead of attaching the request to the client, give the client a .Do() function. Like the net/http lib does. The user could then call resp, err := client.Do(ctx, req, nil).
Downside would be, that we can't return custom Response Types. Instead the user could give the Response struct as pointer to the Do func and we parse it.
This could look like this:

func (c *client)Do(ctx context.Context, req opensearchapi.Request, respStruct any) (*Response, error) {
	resp, err := c.Perform(ctx, req)
	if err != nil {
		return nil, err
	}
	if respStruct != nil {
		if err := json.NewDecoder(resp.Body).Decode(respStruct); err != nil {
			return resp, err
		}
	}
	return resp, nil
}

This would give the users the option to use the lib very low level by giving nil as struc so we don't try to parse it or easy by parsing the response.
In my current diff we would always try to parse the response into the struct and return the reponse with an error if it fails.

[dblock]

• I think namespacing plugin-specific code makes a lot of sense
• I wouldn't worry about common names for now, but I expect the security plugin to be prefixed with "plugins/security" so it's clear that it's a plugin; in the future we will need to support multiple security plugins by extracting the code of the well known security plugin into a separate library
• 100% on even changing file names, be mindful of the fact that we want to automate generating all this boilerplate from API specs
• I think we need both a way to make generic requests and a way to make specific requests - is there a subset of the work here to make generic requests possible that can be standalone?
• As a developer I'd like to write a minimum amount of code, but have the strongest contract in types, not sure what that looks like in go ;)

[Jakob3xD]

I created a POC on how an implementation could look like. @dblock I think, I cover all your points. But there are two things to discuss.
When I talked to you, you suggested some sort of plugin system in combination with code generation. With my POC I split the plugin into another repo to display the change. Each plugin needs to have its own client, so in general, users need to create specific clients for there tasks. We can either have a dynamic plugins system where everything has its own user or we save all plugins in one repo and create one root Client, that can handle all of it.

The other thing is the generic and specif request. With my POC users can either call the Do() function of the client to get the generic unparsed response or call the more specific function which returns the parsed struct. If we support the Do() function, we need to add/keep parameters and overwrite some of the when calling a specific request. For example we always need to nil the filter_path parameter and for cat requests set the format=json parameter.

https://github.com/Jakob3xD/opensearch-golang - This is the equivalent to the current opensearch-go repo
https://github.com/Jakob3xD/opensearch-go-plugin-security - This is a POC security plugin implementation

User code example

package main

import (
	"context"
	"fmt"

	security "github.com/jakob3xd/opensearch-go-plugin-security"
	"github.com/jakob3xd/opensearch-golang/opensearchapi"
)

func main() {
	err := test()
	if err != nil {
		fmt.Println(err)
	}
}
func test() error {
	ctx := context.Background()
	// security
	client, err := security.NewDefaultClient()
	if err != nil {
		return err
	}
	userResp, err := client.Users.Get(ctx, &security.UsersGetReq{User: "admin"})
	if err != nil {
		return err
	}
	fmt.Println(fmt.Sprintf("$#v", userResp))
	// opensearchapi
	osClient, err := opensearchapi.NewDefaultClient()
	if err != nil {
		return err
	}
	catResp, err := osClient.Cat.Indices(ctx, &opensearchapi.CatIndicesReq{})
	if err != nil {
		return err
	}
	fmt.Println(fmt.Sprintf("$#v", catResp))
	return nil
}

[dblock]

Great work! I love the proposed implementation. My big question is why do we need the Do version at all? What can't you do (no pun intended) without a Do version? Paging @Xtansia as well.

For a start I would put the plugins code into a subfolder in this client, knowing that one day we'll separate them into their own libraries. I don't think we can effectively maintain a large set of go clients rn.

[Jakob3xD]

My big question is why do we need the Do version at all? What can't you do (no pun intended) without a Do version?

Currently users can get the response and they can do whatever they want with it. The Do function would preserve this option. Moreover users can create custom Request and use the Do function to execute it. On the other hand users could also just write a simple Do function them self and use the Do of the root client which is still needed.

So imo we could also remove it.

[Jakob3xD]

There is one more thing I am concerned about.

404 responses are treated as error which is okay but there is no simple way for users to accept 404 errors.
They would need to parse the error with errors.As() and then try to match on something in the error struct, as we no longer return the opensearch.Response which included the response code.
This would look like following for the security plugin:

	delResp, err := client.Users.Delete(ctx, &security.UsersDeleteReq{User: "test"})
	if err != nil {
		var errCheck security.Error
		if !errors.As(err, &errCheck) || errCheck.Status != "NOT_FOUND" {
			return err
		}
	}

Don´t know if this is okay for us or if we need to find a solution for it. Right now I can't think of a nice one. We could return the status code as an extra return value.
As we can't add the status code to the returned Response on a lower level. Another option would be that each client function sets the status code to the custom response struct, requiring all response types to have a status field that does not collide with any returned status field of the api.

[dblock]

So imo we could also remove it.

Let's do this until someone complains.

[dblock]

@Jakob3xD I don't know how to implement this, but conceptually I think I'd be great to have access to the raw HTTP request and the raw HTTP response, so that I can do both response.httpResponse.code and err.httpRequest.code or something like that? WDYT?

[Jakob3xD]

@Jakob3xD I don't know how to implement this, but conceptually I think I'd be great to have access to the raw HTTP request and the raw HTTP response, so that I can do both response.httpResponse.code and err.httpRequest.code or something like that? WDYT?

I checked back with my co-worker and we both think, that we should keep it as it is in my POC. Normally as a user you just want the parsed struct and the error and not the response as you don't want to process it your self. So I suggest writing an implementation like my POC with a plugins folder. Otherwise we would need to return the Response as a third return argument.

[henvic]

I'm not saying you should do it, but a clean way to do is to have an Inspect function that would receive it and expose an unimported field. This is awkward because it has hidden magic, but at least you keep a sane API for the average consumer and make it hard to abuse it.

I'd find it quite useful for debugging or in the very unlikely case where you need to access something that isn't exposed, though.

[Jakob3xD]

@henvic thank your for the reply. If i understand you correctly, then I like your idea. Just to verify. You meant that our response types should have an unexported response field with a Inspect() function that returns the opensearch.Response?

So for example:

type UsersGetResp struct {
	Users    map[string]UserGetResp
	response *opensearch.Response
}

type UserGetResp struct {
	Reserved                bool              `json:"reserved"`
	Hidden                  bool              `json:"hidden"`
	BackendRoles            []string          `json:"backend_roles"`
	Attributes              map[string]string `json:"attributes"`
	OpensearchSecurityRoles []string          `json:"opendistro_security_roles"`
	Static                  bool              `json:"static"`
}

func (r UsersGetResp) Inspect() *opensearch.Response {
	return r.response
}

If this is your suggestion, I have one question. Would you expect the UsersGetResp to be returned on errors or should it be nil? For error debugging it would make sense to return it?

func (c usersClient) Get(ctx context.Context, req *UsersGetReq) (*UsersGetResp, error) {
	var (
		users UsersGetResp
		err   error
	)
	users.response, err = c.securityClient.do(ctx, req, &users.Users)
	if err != nil {
		return &users, err
	}
	return &users, nil
}

[dblock]

I like Inspect(). Let's return a type though so that we can add more things to it. I think both request and response may be useful.

[Jakob3xD]

I added an Inspect struct to opensearch and used it for the security responses in my POC branches. I assume you meant the http request and not the lib requests which the user created? Not sure though if we need/want the request.
However, I would start rewriting the lib and add the Inspect later.

[dblock]

I am thinking at a high level: a user may find a workaround for features not present in the library if they get access to the original request and response.

[Jakob3xD]

If you want the request, then we could just replace all the Get functions from the custom request and replace them with one GetRequest() function, which returns the *http.Request. So we move the http request building from the client Do function into a separate one so everyone could re-use it to avoid code duplication and if needed the custom Request can implement its own http request building.

[Jakob3xD]

Took me some time to build the basic structure due to limited time but it is currently present under my feature branch. I appreciate if someone can take a look at the current state and tell me if I need to change something or something is unclear.
If everything looks fine I would push the feature branch to upstream so we can create a PR for every API endpoint that needs to be added, so that we can review it. Doing a review of the current feature branch against main would be really difficulty. Therefore my plane is to push everything to the feature branch and later merge the feature branch to main or replace main with the content of the feature branch.

[dblock]

@Jakob3xD This is great work. I think you should hesitate less and go for it.

[Jakob3xD]

Following is a list of functions that already exist in the current client. They are checked when I implemented them in my feature branch. The lists also helps me to keep track of what I am missing.

List

• bulk
• cat.aliases
• cat.allocation
• cat.cluster_manager
• cat.count
• cat.fielddata
• cat.health
• cat.help -> Returns no json, not sure if we want to implement this
• cat.indices
• cat.master
• cat.nodeattrs
• cat.nodes
• cat.pending_tasks
• cat.plugins
• cat.recovery
• cat.repositories
• cat.segments
• cat.shards
• cat.snapshots
• cat.tasks
• cat.templates
• cat.thread_pool
• clear_scroll
• cluster.allocation_explain
• cluster.delete_component_template
• cluster.delete_voting_config_exclusions
• cluster.exists_component_template
• cluster.get_component_template
• cluster.get_settings
• cluster.health
• cluster.pending_tasks
• cluster.post_voting_config_exclusions
• cluster.put_component_template
• cluster.put_settings
• cluster.remote_info
• cluster.reroute
• cluster.state
• cluster.stats
• count
• create
• dangling_indices.delete_dangling_index
• dangling_indices.import_dangling_index
• dangling_indices.list_dangling_indices
• delete_by_query
• delete_by_query_rethrottle
• delete
• delete_script
• exists
• exists_source
• explain
• field_caps
• get
• get_script_context
• get_script
• get_script_languages
• get_source
• index
• indices.add_block
• indices.analyze
• indices.clear_cache
• indices.clone
• indices.close
• indices.create_datastream
• indices.create
• indices.delete_alias
• indices.delete_datastream
• indices.delete
• indices.delete_index_template
• indices.delete_template
• indices.disk_usage -> Won't implement as it does not exists in OpenSearch 2.X
• indices.exists_alias
• indices.exists
• indices.exists_index_template
• indices.exists_template
• indices.field_usage_stats -> Won't implement as it does not exists in OpenSearch 2.X
• indices.flush
• indices.forcemerge
• indices.get_alias
• indices.get_datastream
• indices.get_datastream_stats
• indices.get_field_mapping
• indices.get
• indices.get_index_template
• indices.get_mapping
• indices.get_settings
• indices.get_template
• indices.get_upgrade -> Not sure if needed
• indices.open
• indices.put_alias
• indices.put_index_template
• indices.put_mapping
• indices.put_settings
• indices.put_template
• indices.recovery
• indices.refresh
• indices.resolve_index
• indices.rollover
• indices.segments
• indices.shard_stores
• indices.shrink
• indices.simulate_index_template
• indices.simulate_template
• indices.split
• indices.stats
• indices.update_aliases
• indices.upgrade -> Not sure if needed
• indices.validate_query
• info
• ingest.delete_pipeline
• ingest.get_pipeline
• ingest.processor_grok
• ingest.put_pipeline
• ingest.simulate
• mget
• msearch
• msearch_template
• mtermvectors
• nodes.hot_threads
• nodes.info
• nodes.reload_secure_settings
• nodes.stats
• nodes.usage
• ping
• pointintime.create
• pointintime.delete
• pointintime.get
• put_script
• rank_eval
• reindex
• reindex_rethrottle
• render_search_template
• scripts_painless_execute
• scroll
• search
• search_shards
• search_template
• snapshot.cleanup_repository
• snapshot.clone
• snapshot.create
• snapshot.create_repository
• snapshot.delete
• snapshot.delete_repository
• snapshot.get
• snapshot.get_repository
• snapshot.restore
• snapshot.status
• snapshot.verify_repository
• tasks.cancel
• tasks.get
• tasks.list
• terms_enum -> Won't implement as it does not exists in OpenSearch 2.X
• termvectors
• update_by_query
• update_by_query_rethrottle
• update

The diff is already huge: main...opensearch-project:opensearch-go:feature/idiomatic

[tannerjones4075]

@Jakob3xD do you need help with this?

[Jakob3xD]

@Jakob3xD do you need help with this?

If you want, you can use my feature branch as example/base for an ism or security client.

[Jakob3xD]

@dblock The features branch now contains nearly all functions. How should we proceed "merging" this?

There might be some comments that need to be updated due to copy paste and the guids also need an update.

[dblock]

@dblock The features branch now contains nearly all functions. How should we proceed "merging" this?

There might be some comments that need to be updated due to copy paste and the guids also need an update.

Make a passing PR with a major version increment?