diff --git a/.github/workflows/test-integration.yml b/.github/workflows/test-integration.yml index 7196ae3ae..2460c1c8d 100644 --- a/.github/workflows/test-integration.yml +++ b/.github/workflows/test-integration.yml @@ -27,7 +27,7 @@ jobs: if curl -s localhost:9200; \ then echo '=====> ready'; break; fi; if [ $attempt == 25 ]; then exit 1; fi; echo '=====> waiting...'; done - run: make test-integ race=true coverage=true - - uses: codecov/codecov-action@v3 + - uses: codecov/codecov-action@v4 with: file: tmp/integ.cov flags: integration diff --git a/.github/workflows/test-unit.yml b/.github/workflows/test-unit.yml index b5717ed2f..38f21eeff 100644 --- a/.github/workflows/test-unit.yml +++ b/.github/workflows/test-unit.yml @@ -22,7 +22,7 @@ jobs: if: matrix.os != 'ubuntu-latest' - run: make test-unit race=true coverage=true if: matrix.os == 'ubuntu-latest' - - uses: codecov/codecov-action@v3 + - uses: codecov/codecov-action@v4 with: file: tmp/unit.cov flags: unit diff --git a/CHANGELOG.md b/CHANGELOG.md index 765f8e730..7786df4e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,9 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Moved Error structs from opensearchapi to opensearch package ([#512](https://github.com/opensearch-project/opensearch-go/pull/506)) - Move parseError function from opensearchapi to opensearch package as ParseError ([#512](https://github.com/opensearch-project/opensearch-go/pull/506)) - Change ParseError function to do type assertion to determine error type ([#512](https://github.com/opensearch-project/opensearch-go/pull/506)) +- Removed unused structs and functions from opensearch ([#517](https://github.com/opensearch-project/opensearch-go/pull/517)) +- Adjust and extent opensearch tests for better coverage ([#517](https://github.com/opensearch-project/opensearch-go/pull/517)) +- Bump codecov action version to v4 ([#517](https://github.com/opensearch-project/opensearch-go/pull/517)) ### Deprecated @@ -24,6 +27,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ### Fixed - Fix search request missing a slash when no indices are given ([#470](https://github.com/opensearch-project/opensearch-go/pull/469)) +- Fix opensearchtransport check for nil response body ([#517](https://github.com/opensearch-project/opensearch-go/pull/517)) ### Security @@ -185,4 +189,4 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) [2.1.0]: https://github.com/opensearch-project/opensearch-go/compare/v2.0.1...v2.1.0 [2.0.1]: https://github.com/opensearch-project/opensearch-go/compare/v2.0.0...v2.0.1 [2.0.0]: https://github.com/opensearch-project/opensearch-go/compare/v1.1.0...v2.0.0 -[1.0.0]: https://github.com/opensearch-project/opensearch-go/compare/v1.0.0...v1.1.0 \ No newline at end of file +[1.0.0]: https://github.com/opensearch-project/opensearch-go/compare/v1.0.0...v1.1.0 diff --git a/opensearch.go b/opensearch.go index c99052530..85bce04c3 100644 --- a/opensearch.go +++ b/opensearch.go @@ -56,9 +56,6 @@ const ( // Version returns the package version as a string. const Version = version.Client -// SupportedElasticVersion defines the supported major elasticsearch version -const SupportedElasticVersion = 7 - // Error vars var ( ErrCreateClient = errors.New("cannot create client") @@ -112,17 +109,6 @@ type Client struct { Transport opensearchtransport.Interface } -type esVersion struct { - Number string `json:"number"` - BuildFlavor string `json:"build_flavor"` - Distribution string `json:"distribution"` -} - -type info struct { - Version esVersion `json:"version"` - Tagline string `json:"tagline"` -} - // NewDefaultClient creates a new client with default options. // // It will use http://localhost:9200 as the default address. @@ -217,24 +203,6 @@ func getAddressFromEnvironment() []string { return addrsFromEnvironment(envOpenSearchURL) } -// checkCompatibleInfo validates the information given by OpenSearch -func checkCompatibleInfo(info info) error { - major, _, _, err := ParseVersion(info.Version.Number) - if err != nil { - return err - } - - if info.Version.Distribution == openSearch { - return nil - } - - if major != SupportedElasticVersion { - return errors.New(unsupportedProduct) - } - - return nil -} - // ParseVersion returns an int64 representation of version. func ParseVersion(version string) (int64, int64, int64, error) { reVersion := regexp.MustCompile(`^([0-9]+)\.([0-9]+)\.([0-9]+)`) @@ -294,13 +262,13 @@ func (c *Client) Do(ctx context.Context, req Request, dataPointer interface{}) ( if dataPointer != nil && resp.Body != nil && !response.IsError() { data, err := io.ReadAll(resp.Body) if err != nil { - return response, fmt.Errorf("failed to read the response body, status: %d, err: %w", resp.StatusCode, err) + return response, fmt.Errorf("%w, status: %d, err: %w", ErrReadBody, resp.StatusCode, err) } response.Body = io.NopCloser(bytes.NewReader(data)) if err := json.Unmarshal(data, dataPointer); err != nil { - return response, fmt.Errorf("failed to parse body into the pointer, status: %d, body: %s, err: %w", resp.StatusCode, data, err) + return response, fmt.Errorf("%w, status: %d, body: %s, err: %w", ErrJSONUnmarshalBody, resp.StatusCode, data, err) } } diff --git a/opensearch_internal_test.go b/opensearch_internal_test.go index 637dfa8d4..1a1d49789 100644 --- a/opensearch_internal_test.go +++ b/opensearch_internal_test.go @@ -29,19 +29,24 @@ package opensearch import ( + "context" "errors" + "fmt" "io" "net/http" "net/url" "os" - "regexp" "strings" "testing" + "testing/iotest" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/opensearch-project/opensearch-go/v3/opensearchtransport" ) -var called bool +var called int type mockTransp struct { RoundTripFunc func(*http.Request) (*http.Response, error) @@ -58,8 +63,8 @@ var defaultRoundTripFunc = func(req *http.Request) (*http.Response, error) { } }`)) response.Header.Add("Content-Type", "application/json") - } else { - called = true + } else if req.URL.Path == "/test" { + called++ } return response, nil @@ -72,31 +77,44 @@ func (t *mockTransp) RoundTrip(req *http.Request) (*http.Response, error) { return t.RoundTripFunc(req) } +type testReq struct { + Method string + Path string + Body io.Reader + Params map[string]string + Error bool + Headers http.Header +} + +func (r testReq) GetRequest() (*http.Request, error) { + if r.Error { + return nil, fmt.Errorf("test error") + } + if r.Method == "" { + r.Method = http.MethodGet + } + return BuildRequest( + r.Method, + r.Path, + r.Body, + r.Params, + r.Headers, + ) +} + func TestClientConfiguration(t *testing.T) { t.Run("With empty", func(t *testing.T) { c, err := NewDefaultClient() - if err != nil { - t.Errorf("Unexpected error: %s", err) - } - + require.NoError(t, err) u := c.Transport.(*opensearchtransport.Client).URLs()[0].String() - - if u != defaultURL { - t.Errorf("Unexpected URL, want=%s, got=%s", defaultURL, u) - } + assert.Equal(t, u, defaultURL) }) t.Run("With URL from Addresses", func(t *testing.T) { c, err := NewClient(Config{Addresses: []string{"http://localhost:8080//"}, Transport: &mockTransp{}}) - if err != nil { - t.Fatalf("Unexpected error: %s", err) - } - + require.NoError(t, err) u := c.Transport.(*opensearchtransport.Client).URLs()[0].String() - - if u != "http://localhost:8080" { - t.Errorf("Unexpected URL, want=http://localhost:8080, got=%s", u) - } + assert.Equal(t, u, "http://localhost:8080") }) t.Run("With URL from OPENSEARCH_URL", func(t *testing.T) { @@ -104,15 +122,9 @@ func TestClientConfiguration(t *testing.T) { defer func() { os.Setenv(envOpenSearchURL, "") }() c, err := NewClient(Config{Transport: &mockTransp{}}) - if err != nil { - t.Errorf("Unexpected error: %s", err) - } - + require.NoError(t, err) u := c.Transport.(*opensearchtransport.Client).URLs()[0].String() - - if u != "http://opensearch.com" { - t.Errorf("Unexpected URL, want=http://opensearch.com, got=%s", u) - } + assert.Equal(t, u, "http://opensearch.com") }) t.Run("With URL from environment and cfg.Addresses", func(t *testing.T) { @@ -120,34 +132,24 @@ func TestClientConfiguration(t *testing.T) { defer func() { os.Setenv(envOpenSearchURL, "") }() c, err := NewClient(Config{Addresses: []string{"http://localhost:8080//"}, Transport: &mockTransp{}}) - if err != nil { - t.Fatalf("Unexpected error: %s", err) - } - + require.NoError(t, err) u := c.Transport.(*opensearchtransport.Client).URLs()[0].String() - - if u != "http://localhost:8080" { - t.Errorf("Unexpected URL, want=http://localhost:8080, got=%s", u) - } + assert.Equal(t, u, "http://localhost:8080") }) t.Run("With invalid URL", func(t *testing.T) { u := ":foo" _, err := NewClient(Config{Addresses: []string{u}}) - if err == nil { - t.Errorf("Expected error for URL %q, got %v", u, err) - } + assert.Error(t, err) }) t.Run("With invalid URL from environment", func(t *testing.T) { os.Setenv(envOpenSearchURL, ":foobar") defer func() { os.Setenv(envOpenSearchURL, "") }() - c, err := NewDefaultClient() - if err == nil { - t.Errorf("Expected error, got: %+v", c) - } + _, err := NewDefaultClient() + assert.Error(t, err) }) t.Run("With skip check", func(t *testing.T) { @@ -162,31 +164,119 @@ func TestClientConfiguration(t *testing.T) { }, }, }) - if err != nil { - t.Errorf("Unexpected error, got: %+v", err) - } + assert.NoError(t, err) + }) + + t.Run("With User:Password", func(t *testing.T) { + c, err := NewClient( + Config{ + Addresses: []string{"http://admin:admin@localhost:8080//"}, + Transport: &mockTransp{}, + }, + ) + require.NoError(t, err) + u := c.Transport.(*opensearchtransport.Client).URLs()[0].String() + assert.Equal(t, u, "http://admin:admin@localhost:8080") + }) + + t.Run("With DiscoverNodes on start", func(t *testing.T) { + c, err := NewClient( + Config{ + Addresses: []string{"http://localhost:8080//"}, + Transport: &mockTransp{}, + DiscoverNodesOnStart: true, + }, + ) + require.NoError(t, err) + u := c.Transport.(*opensearchtransport.Client).URLs()[0].String() + assert.Equal(t, u, "http://localhost:8080") + }) + + t.Run("With failing creation", func(t *testing.T) { + _, err := NewClient( + Config{ + Addresses: []string{"http://admin:admin@localhost:8080//"}, + Transport: &mockTransp{}, + CACert: []byte{1}, + }, + ) + assert.ErrorIs(t, err, ErrCreateTransport) }) } -func TestClientInterface(t *testing.T) { - t.Run("Transport", func(t *testing.T) { +func TestClientInterfe(t *testing.T) { + t.Run("Perform()", func(t *testing.T) { c, err := NewClient(Config{Transport: &mockTransp{}}) - if err != nil { - t.Fatalf("Unexpected error: %s", err) - } + require.NoError(t, err) - if called != false { - t.Errorf("Unexpected call to transport by client") - } + call := called - res, err := c.Perform(&http.Request{URL: &url.URL{}, Header: make(http.Header)}) // errcheck ignore + res, err := c.Perform(&http.Request{URL: &url.URL{Path: "/test"}, Header: make(http.Header)}) if err == nil && res != nil && res.Body != nil { res.Body.Close() } - if called != true { - t.Errorf("Expected client to call transport") + assert.True(t, called-1 == call, "Expected client to call transport") + }) + + t.Run("Do()", func(t *testing.T) { + c, err := NewClient(Config{Transport: &mockTransp{}}) + require.NoError(t, err) + + req := testReq{} + resp, err := c.Do(context.TODO(), req, nil) + assert.NoError(t, err) + assert.NotNil(t, resp) + }) + + t.Run("Do() GetRequest error", func(t *testing.T) { + c, err := NewClient(Config{Transport: &mockTransp{}}) + require.NoError(t, err) + + req := testReq{Error: true} + resp, err := c.Do(context.TODO(), req, nil) + assert.Error(t, err) + assert.Nil(t, resp) + }) + + t.Run("Do() Unmarshal error", func(t *testing.T) { + c, err := NewClient(Config{Transport: &mockTransp{}}) + require.NoError(t, err) + + type failStr struct { + Version int `json:"version"` + } + req := testReq{Path: "/"} + resp, err := c.Do(context.TODO(), req, &failStr{}) + require.Error(t, err) + assert.ErrorIs(t, err, ErrJSONUnmarshalBody) + assert.NotNil(t, resp) + }) + + t.Run("Do() io read error", func(t *testing.T) { + c, err := NewClient( + Config{ + Transport: &mockTransp{ + RoundTripFunc: func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(iotest.ErrReader(errors.New("io reader test"))), + }, + nil + }, + }, + }, + ) + require.NoError(t, err) + + type failStr struct { + Version int `json:"version"` } + req := testReq{} + resp, err := c.Do(context.TODO(), req, &failStr{}) + require.Error(t, err) + assert.ErrorIs(t, err, ErrReadBody) + assert.NotNil(t, resp) }) } @@ -238,51 +328,34 @@ func TestAddrsToURLs(t *testing.T) { res, err := addrsToURLs(tc.addrs) if tc.err != nil { - if err == nil { - t.Errorf("Expected error, got: %v", err) - } - match, _ := regexp.MatchString(tc.err.Error(), err.Error()) - if !match { - t.Errorf("Expected err [%s] to match: %s", err.Error(), tc.err.Error()) - } + require.Error(t, err) + assert.Contains(t, err.Error(), tc.err.Error()) } for i := range tc.urls { - if res[i].Scheme != tc.urls[i].Scheme { - t.Errorf("%s: Unexpected scheme, want=%s, got=%s", tc.name, tc.urls[i].Scheme, res[i].Scheme) - } + assert.Equal(t, tc.urls[i].Scheme, res[i].Scheme, tc.name) } for i := range tc.urls { - if res[i].Host != tc.urls[i].Host { - t.Errorf("%s: Unexpected host, want=%s, got=%s", tc.name, tc.urls[i].Host, res[i].Host) - } + assert.Equal(t, tc.urls[i].Host, res[i].Host, tc.name) } for i := range tc.urls { - if res[i].Path != tc.urls[i].Path { - t.Errorf("%s: Unexpected path, want=%s, got=%s", tc.name, tc.urls[i].Path, res[i].Path) - } + assert.Equal(t, tc.urls[i].Path, res[i].Path, tc.name) } }) } } func TestVersion(t *testing.T) { - if Version == "" { - t.Error("Version is empty") - } + require.NotEmpty(t, Version) } func TestClientMetrics(t *testing.T) { c, _ := NewClient(Config{EnableMetrics: true, Transport: &mockTransp{}}) m, err := c.Metrics() - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } + require.Nil(t, err) - if m.Requests > 1 { - t.Errorf("Unexpected output: %s", m) - } + assert.LessOrEqual(t, m.Requests, 1, m) } func TestParseElasticsearchVersion(t *testing.T) { @@ -329,83 +402,15 @@ func TestParseElasticsearchVersion(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, got1, got2, err := ParseVersion(tt.version) - if (err != nil) != tt.wantErr { - t.Errorf("ParseVersion() error = %v, wantErr %v", err, tt.wantErr) - return - } - if got != tt.major { - t.Errorf("ParseVersion() got = %v, want %v", got, tt.major) - } - if got1 != tt.minor { - t.Errorf("ParseVersion() got1 = %v, want %v", got1, tt.minor) - } - if got2 != tt.patch { - t.Errorf("ParseVersion() got2 = %v, want %v", got2, tt.patch) - } - }) - } -} - -func TestGenuineCheckInfo(t *testing.T) { - tests := []struct { - name string - info info - wantErr bool - err error - }{ - { - name: "Supported OpenSearch 1.0.0", - info: info{ - Version: esVersion{ - Number: "1.0.0", - Distribution: openSearch, - }, - }, - wantErr: false, - err: nil, - }, - { - name: "Supported Elasticsearch 7.10.0", - info: info{ - Version: esVersion{ - Number: "7.10.0", - BuildFlavor: "anything", - }, - }, - wantErr: false, - err: nil, - }, - { - name: "Unsupported Elasticsearch Version 6.15.1", - info: info{ - Version: esVersion{ - Number: "6.15.1", - BuildFlavor: "default", - }, - Tagline: "You Know, for Search", - }, - wantErr: true, - err: errors.New(unsupportedProduct), - }, - { - name: "Elasticsearch oss", - info: info{ - Version: esVersion{ - Number: "7.10.0", - BuildFlavor: "oss", - }, - Tagline: "You Know, for Search", - }, - wantErr: false, - err: nil, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if err := checkCompatibleInfo(tt.info); (err != nil) != tt.wantErr && !errors.Is(err, tt.err) { - t.Errorf("checkCompatibleInfo() error = %v, wantErr %v", err, tt.wantErr) + major, minor, patch, err := ParseVersion(tt.version) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) } + assert.Equal(t, major, tt.major) + assert.Equal(t, minor, tt.minor) + assert.Equal(t, patch, tt.patch) }) } } diff --git a/opensearchtransport/discovery.go b/opensearchtransport/discovery.go index 93ed61e9d..2e0bbec85 100644 --- a/opensearchtransport/discovery.go +++ b/opensearchtransport/discovery.go @@ -142,6 +142,10 @@ func (c *Client) getNodesInfo() ([]nodeInfo, error) { if err != nil { return nil, err } + + if res.Body == nil { + return nil, fmt.Errorf("unexpected empty body") + } defer res.Body.Close() if res.StatusCode > http.StatusOK { diff --git a/opensearchtransport/discovery_internal_test.go b/opensearchtransport/discovery_internal_test.go index 0a27d5f21..79373c33a 100644 --- a/opensearchtransport/discovery_internal_test.go +++ b/opensearchtransport/discovery_internal_test.go @@ -40,6 +40,9 @@ import ( "reflect" "testing" "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestDiscovery(t *testing.T) { @@ -80,7 +83,6 @@ func TestDiscovery(t *testing.T) { if err != nil { t.Fatalf("ERROR: %s", err) } - fmt.Printf("NodesInfo: %+v\n", nodes) if len(nodes) != 4 { t.Errorf("Unexpected number of nodes, want=4, got=%d", len(nodes)) @@ -108,6 +110,24 @@ func TestDiscovery(t *testing.T) { } }) + t.Run("getNodesInfo() empty Body", func(t *testing.T) { + newRoundTripper := func() http.RoundTripper { + return &mockTransp{ + RoundTripFunc: func(req *http.Request) (*http.Response, error) { + return &http.Response{Header: http.Header{}}, nil + }, + } + } + + u, _ := url.Parse("http://localhost:8080") + tp, err := New(Config{URLs: []*url.URL{u}, Transport: newRoundTripper()}) + require.NoError(t, err) + + _, err = tp.getNodesInfo() + assert.Error(t, err) + assert.Contains(t, err.Error(), "unexpected empty body") + }) + t.Run("DiscoverNodes()", func(t *testing.T) { u, _ := url.Parse("http://" + srv.Addr) tp, _ := New(Config{URLs: []*url.URL{u}})