From a74edfae52e930d3bf21f7d82a751426c9de2de7 Mon Sep 17 00:00:00 2001 From: dehort Date: Tue, 5 Mar 2024 07:26:36 -0600 Subject: [PATCH] Check for nil pointer in the sources client (#340) * Check for nil pointer in the sources client * Move more of the error handling logic up into the call that makes the rest request...trying to simplify the core logic of the sources connector --- internal/api/connectors/sources/impl.go | 54 +++++++++------- .../api/connectors/sources/sources_test.go | 61 +++++++++++++++++++ 2 files changed, 93 insertions(+), 22 deletions(-) diff --git a/internal/api/connectors/sources/impl.go b/internal/api/connectors/sources/impl.go index b3a62c9a..5edf0831 100644 --- a/internal/api/connectors/sources/impl.go +++ b/internal/api/connectors/sources/impl.go @@ -69,7 +69,7 @@ func NewSourcesClient(cfg *viper.Viper) SourcesConnector { return NewSourcesClientWithHttpRequestDoer(cfg, &doer) } -func (this *sourcesClientImpl) getRHCConnectionStatus(ctx context.Context, sourceId string) (*RhcConnectionCollection, error) { +func (this *sourcesClientImpl) getRHCConnectionStatus(ctx context.Context, sourceId string) (*string, *string, error) { utils.GetLogFromContext(ctx).Debugw("Sending Sources RHC Connection Request") @@ -80,28 +80,32 @@ func (this *sourcesClientImpl) getRHCConnectionStatus(ctx context.Context, sourc res, err := this.client.GetSourcesRhcConnectionWithResponse(ctx, ID, ¶ms) if err != nil { - return nil, err + return nil, nil, err } if res.HTTPResponse.StatusCode == 404 { - return nil, fmt.Errorf("RHCStatus Not Found") + return nil, nil, fmt.Errorf("RHCStatus Not Found") } if res.HTTPResponse.StatusCode == 400 { - return nil, fmt.Errorf("RHCStatus Bad Request") + return nil, nil, fmt.Errorf("RHCStatus Bad Request") } if res.JSON200 == nil { - return nil, fmt.Errorf(`GetRhcConnectionStatus unexpected status code "%d" or content type "%s"`, res.HTTPResponse.StatusCode, res.HTTPResponse.Header.Get("content-type")) + return nil, nil, fmt.Errorf(`GetRhcConnectionStatus unexpected status code "%d" or content type "%s"`, res.HTTPResponse.StatusCode, res.HTTPResponse.Header.Get("content-type")) } - return res.JSON200, err + if res.JSON200.Data == nil || len(*res.JSON200.Data) == 0 { + return nil, nil, fmt.Errorf("GetRHCConnectionStatus returned an empty response") + } + + return (*res.JSON200.Data)[0].RhcId, (*res.JSON200.Data)[0].AvailabilityStatus, err } -func (this *sourcesClientImpl) getSources(ctx context.Context, sourceId string) (sources *[]Source, err error) { +func (this *sourcesClientImpl) getSourceIdBySatelliteId(ctx context.Context, satelliteId string) (sourceId string, sourceName string, err error) { utils.GetLogFromContext(ctx).Debugw("Sending Sources Request") - ID := ID(sourceId) + ID := ID(satelliteId) queryFilter := filterPath + QueryFilter(ID) params := &ListSourcesParams{ @@ -111,43 +115,49 @@ func (this *sourcesClientImpl) getSources(ctx context.Context, sourceId string) res, err := this.client.ListSourcesWithResponse(ctx, params) if err != nil { - return nil, err + return "", "", err } if res.JSON400 != nil { - return nil, fmt.Errorf("Source Bad Request") + return "", "", fmt.Errorf("Source Bad Request") } if res.JSON200 == nil { - return nil, fmt.Errorf(`GetSources unexpected status code "%d" or content type "%s"`, res.HTTPResponse.StatusCode, res.HTTPResponse.Header.Get("content-type")) + return "", "", fmt.Errorf(`GetSources unexpected status code "%d" or content type "%s"`, res.HTTPResponse.StatusCode, res.HTTPResponse.Header.Get("content-type")) + } + + if res.JSON200.Data == nil || len(*res.JSON200.Data) == 0 { + return "", "", fmt.Errorf("GetSources returned an empty response") } - return res.JSON200.Data, err + source := (*res.JSON200.Data)[0] + + if source.Id == nil { + return "", "", fmt.Errorf("GetSources did not return a valid sources id") + } + + return string(*source.Id), *source.Name, nil } func (this *sourcesClientImpl) GetSourceConnectionDetails(ctx context.Context, sourceID string) (details SourceConnectionStatus, err error) { utils.GetLogFromContext(ctx).Debugw("Gathering Source Connection Details") - sourcesResponse, err := this.getSources(ctx, sourceID) + sourceId, sourceName, err := this.getSourceIdBySatelliteId(ctx, sourceID) if err != nil { return SourceConnectionStatus{}, err } - source := (*sourcesResponse)[0] - - rhcConnectionResponse, err := this.getRHCConnectionStatus(ctx, string(*source.Id)) + rhcId, availabilityStatus, err := this.getRHCConnectionStatus(ctx, sourceId) if err != nil { return SourceConnectionStatus{}, err } - rhcInfo := (*rhcConnectionResponse).Data - return SourceConnectionStatus{ - ID: string(*source.Id), - SourceName: source.Name, - RhcID: (*rhcInfo)[0].RhcId, - AvailabilityStatus: (*rhcInfo)[0].AvailabilityStatus, + ID: sourceId, + SourceName: &sourceName, + RhcID: rhcId, + AvailabilityStatus: availabilityStatus, }, err } diff --git a/internal/api/connectors/sources/sources_test.go b/internal/api/connectors/sources/sources_test.go index 148b61e2..94f733a9 100644 --- a/internal/api/connectors/sources/sources_test.go +++ b/internal/api/connectors/sources/sources_test.go @@ -71,6 +71,51 @@ var _ = Describe("Sources", func() { Expect(err.Error()).To(ContainSubstring("Source Bad Request")) }) + It("interperates response correctly if getSources returns an empty response", func() { + responses := []test.MockHttpResponse{ + {StatusCode: 200, Body: `{}`}, + {StatusCode: 200, Body: `{}`}, + } + + doer := test.MockMultiResponseHttpClient(responses...) + client := NewSourcesClientWithHttpRequestDoer(config.Get(), doer) + ctx := test.TestContext() + + _, err := client.GetSourceConnectionDetails(ctx, "4f37c752-ba1c-48b1-bcf7-4ef8f585d9ee") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("GetSources returned an empty response")) + }) + + It("interperates response correctly if getSources returns a response where an entry does not have an id", func() { + responses := []test.MockHttpResponse{ + {StatusCode: 200, Body: `{"data": [{"name": "test", "availability_status": "connected"}]}`}, + {StatusCode: 200, Body: `{}`}, + } + + doer := test.MockMultiResponseHttpClient(responses...) + client := NewSourcesClientWithHttpRequestDoer(config.Get(), doer) + ctx := test.TestContext() + + _, err := client.GetSourceConnectionDetails(ctx, "4f37c752-ba1c-48b1-bcf7-4ef8f585d9ee") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("GetSources did not return a valid sources id")) + }) + + It("interperates response correctly if getSources returns a response where the data array is empty", func() { + responses := []test.MockHttpResponse{ + {StatusCode: 200, Body: `{"data": []}`}, + {StatusCode: 200, Body: `{}`}, + } + + doer := test.MockMultiResponseHttpClient(responses...) + client := NewSourcesClientWithHttpRequestDoer(config.Get(), doer) + ctx := test.TestContext() + + _, err := client.GetSourceConnectionDetails(ctx, "4f37c752-ba1c-48b1-bcf7-4ef8f585d9ee") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("GetSources returned an empty response")) + }) + It("interperates response correctly if getRhcConnectionStatus returns a 404", func() { responses := []test.MockHttpResponse{ {StatusCode: 200, Body: `{"data": [{"id": "1", "name": "test", "availability_status": "connected"}]}`}, @@ -100,5 +145,21 @@ var _ = Describe("Sources", func() { Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("RHCStatus Bad Request")) }) + + It("interperates response correctly if getRhcConnectionStatus returns an empty response", func() { + responses := []test.MockHttpResponse{ + {StatusCode: 200, Body: `{"data": [{"id": "1", "name": "test", "availability_status": "connected"}]}`}, + {StatusCode: 200, Body: `{}`}, + } + + doer := test.MockMultiResponseHttpClient(responses...) + client := NewSourcesClientWithHttpRequestDoer(config.Get(), doer) + ctx := test.TestContext() + + _, err := client.GetSourceConnectionDetails(ctx, "4f37c752-ba1c-48b1-bcf7-4ef8f585d9ee") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("GetRHCConnectionStatus returned an empty response")) + }) + }) })