diff --git a/internal/services/sast-metadata.go b/internal/services/sast-metadata.go index 0005790a..35e2be70 100644 --- a/internal/services/sast-metadata.go +++ b/internal/services/sast-metadata.go @@ -2,6 +2,7 @@ package services import ( "context" + "errors" "strings" "sync" @@ -11,7 +12,8 @@ import ( ) const ( - BatchSize = 200 + // BatchSize must not exceed the sast-metadata backend's per-request scan-ids cap (currently 50). + BatchSize = 50 ) func GetSastMetadataByIDs(sastMetaDataWrapper wrappers.SastMetadataWrapper, scanIDs []string) (*wrappers.SastMetadataModel, error) { @@ -21,7 +23,7 @@ func GetSastMetadataByIDs(sastMetaDataWrapper wrappers.SastMetadataWrapper, scan var wg sync.WaitGroup results := make(chan wrappers.SastMetadataModel, totalBatches) - errors := make(chan error, totalBatches) + errCh := make(chan error, totalBatches) ctx := context.Background() for i := 0; i < totalBatches; i++ { @@ -46,21 +48,23 @@ func GetSastMetadataByIDs(sastMetaDataWrapper wrappers.SastMetadataWrapper, scan result, err := sastMetaDataWrapper.GetSastMetadataByIDs(batchParams) if err != nil { - errors <- err + errCh <- err return } results <- *result }() } - go func() { - wg.Wait() - close(results) - close(errors) - }() + wg.Wait() + close(results) + close(errCh) - if len(errors) > 0 { - return nil, <-errors + if len(errCh) > 0 { + var batchErrs []error + for batchErr := range errCh { + batchErrs = append(batchErrs, batchErr) + } + return nil, errors.Join(batchErrs...) } var models []wrappers.SastMetadataModel diff --git a/internal/services/sast-metadata_test.go b/internal/services/sast-metadata_test.go index ac96bfb8..34b18aa3 100644 --- a/internal/services/sast-metadata_test.go +++ b/internal/services/sast-metadata_test.go @@ -48,6 +48,28 @@ func TestGetSastMetadataByIDs(t *testing.T) { } } +func TestGetSastMetadataByIDs_BatchFailureIsSurfacedNotPanicked(t *testing.T) { + // A failing batch must be reliably surfaced as an error, not raced past or dereferenced as nil. + scanIDs := append(createScanIDs(BatchSize), createErrorScanIDs(BatchSize)...) + scanIDs = append(scanIDs, createScanIDs(BatchSize)...) + + wrapper := &mock.SastMetadataMockWrapper{} + actual, err := GetSastMetadataByIDs(wrapper, scanIDs) + + assert.Nil(t, actual) + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "Too many ids in url") + } +} + +func createErrorScanIDs(count int) []string { + scanIDs := make([]string, count) + for i := 0; i < count; i++ { + scanIDs[i] = fmt.Sprintf("TriggerError%d", i) + } + return scanIDs +} + func createScanIDs(count int) []string { scanIDs := make([]string, count) for i := 0; i < count; i++ { diff --git a/internal/wrappers/mock/sast-metadata-mock.go b/internal/wrappers/mock/sast-metadata-mock.go index 091d36f9..4d4cb0fb 100644 --- a/internal/wrappers/mock/sast-metadata-mock.go +++ b/internal/wrappers/mock/sast-metadata-mock.go @@ -1,6 +1,7 @@ package mock import ( + "fmt" "strings" "github.com/checkmarx/ast-cli/internal/wrappers" @@ -10,8 +11,11 @@ import ( type SastMetadataMockWrapper struct{} const ( - scanIDParam = "scan-ids" - concurrent = "ConcurrentTest" + scanIDParam = "scan-ids" + concurrent = "ConcurrentTest" + errorTrigger = "TriggerError" + tooManyIDsErrorCode = 3102 + tooManyIDsMessage = "Too many ids in url" // FakeMetadataErrorID is used to trigger error responses in mock SAST metadata calls. FakeMetadataErrorID = "fake-sast-metadata-error-id" // FakeMetadataEmptyID is used to trigger empty responses in mock SAST metadata calls. @@ -22,6 +26,9 @@ func (s SastMetadataMockWrapper) GetSastMetadataByIDs(params map[string]string) *wrappers.SastMetadataModel, error, ) { + if strings.Contains(params[scanIDParam], errorTrigger) { + return nil, fmt.Errorf("%d - %s", tooManyIDsErrorCode, tooManyIDsMessage) + } if strings.Contains(params[scanIDParam], FakeMetadataErrorID) { return nil, errors.New("fake sast metadata fetch error") } diff --git a/internal/wrappers/sast-metadata-http.go b/internal/wrappers/sast-metadata-http.go index 7fa5139f..23204d51 100644 --- a/internal/wrappers/sast-metadata-http.go +++ b/internal/wrappers/sast-metadata-http.go @@ -27,13 +27,10 @@ func (s *SastIncrementalHTTPWrapper) GetSastMetadataByIDs(params map[string]stri if err != nil { return nil, err } - decoder := json.NewDecoder(resp.Body) - defer func() { - if err == nil { - _ = resp.Body.Close() - } + _ = resp.Body.Close() }() + decoder := json.NewDecoder(resp.Body) switch resp.StatusCode { case http.StatusBadRequest, http.StatusInternalServerError: @@ -42,7 +39,7 @@ func (s *SastIncrementalHTTPWrapper) GetSastMetadataByIDs(params map[string]stri if err != nil { return nil, fmt.Errorf("%v %s", err, failedToParseGetAll) } - return nil, err + return nil, fmt.Errorf("%d - %s", errorModel.Code, errorModel.Message) case http.StatusOK: model := SastMetadataModel{} err = decoder.Decode(&model)