Skip to content

Commit b5f295f

Browse files
andrewrobinsonhodges-snykandrewrobinsonhodges-snyk
authored
refactor: extract bundle upload logic [IDE-1085] (#86)
1 parent 428b642 commit b5f295f

File tree

3 files changed

+117
-73
lines changed

3 files changed

+117
-73
lines changed

‎internal/util/testutil/test_config.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,19 +24,19 @@ type localConfig struct {
2424
}
2525

2626
func (l localConfig) Organization() string {
27-
return "b29cf58e-6684-481d-aca4-b24b58821b85"
27+
return "3964634d-2142-4ae5-ba98-c414620609b4"
2828
}
2929

3030
func (l localConfig) IsFedramp() bool {
3131
return false
3232
}
3333

3434
func (l localConfig) SnykCodeApi() string {
35-
return "https://deeproxy.dev.snyk.io"
35+
return "https://deeproxy.snyk.io"
3636
}
3737

3838
func (l localConfig) SnykApi() string {
39-
return "https://app.dev.snyk.io/api"
39+
return "https://api.snyk.io"
4040
}
4141

4242
func (l localConfig) SnykCodeAnalysisTimeout() time.Duration {

‎scan.go

Lines changed: 66 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -182,18 +182,64 @@ func (c *codeScanner) WithAnalysisOrchestrator(analysisOrchestrator analysis.Ana
182182
}
183183
}
184184

185+
// Upload creates a bundle from changed files and uploads it, returning the uploaded Bundle.
186+
func (c *codeScanner) Upload(
187+
ctx context.Context,
188+
requestId string,
189+
target scan.Target,
190+
files <-chan string,
191+
changedFiles map[string]bool,
192+
) (bundle.Bundle, error) {
193+
err := c.checkCancellationOrLogError(ctx, target.GetPath(), nil, "")
194+
if err != nil {
195+
return nil, err
196+
}
197+
198+
originalBundle, err := c.bundleManager.Create(ctx, requestId, target.GetPath(), files, changedFiles)
199+
err = c.checkCancellationOrLogError(ctx, target.GetPath(), err, "error creating bundle...")
200+
if err != nil {
201+
return nil, err
202+
}
203+
204+
filesToUpload := originalBundle.GetFiles()
205+
uploadedBundle, err := c.bundleManager.Upload(ctx, requestId, originalBundle, filesToUpload)
206+
err = c.checkCancellationOrLogError(ctx, target.GetPath(), err, "error uploading bundle...")
207+
if err != nil {
208+
return uploadedBundle, err
209+
}
210+
211+
return uploadedBundle, nil
212+
}
213+
214+
// Utility function to check for cancellations before optionally logging an error (if one is provided). Cancellations
215+
// always take precedence. Returns any error or cancellation that was handled, nil otherwise.
216+
func (c *codeScanner) checkCancellationOrLogError(ctx context.Context, targetPath string, err error, message string) error {
217+
returnError := ctx.Err()
218+
if returnError != nil {
219+
c.logger.Info().Msg("Canceling Code scan - Code scanner received cancellation signal")
220+
} else if err != nil {
221+
if message != "" {
222+
err = errors.Wrap(err, message)
223+
}
224+
c.errorReporter.CaptureError(err, observability.ErrorReporterOptions{ErrorDiagnosticPath: targetPath})
225+
returnError = err
226+
}
227+
return returnError
228+
}
229+
230+
// UploadAndAnalyze returns a fake SARIF response for testing. Use target-service to run analysis on.
185231
func (c *codeScanner) UploadAndAnalyze(
186232
ctx context.Context,
187233
requestId string,
188234
target scan.Target,
189235
files <-chan string,
190236
changedFiles map[string]bool,
191237
) (*sarif.SarifResponse, string, error) {
192-
sarif, bundleHash, _, err := c.UploadAndAnalyzeWithOptions(ctx, requestId, target, files, changedFiles)
193-
return sarif, bundleHash, err
238+
response, bundleHash, _, err := c.UploadAndAnalyzeWithOptions(ctx, requestId, target, files, changedFiles)
239+
return response, bundleHash, err
194240
}
195241

196-
// UploadAndAnalyze returns a fake SARIF response for testing. Use target-service to run analysis on.
242+
// UploadAndAnalyzeWithOptions returns a fake SARIF response for testing. Use target-service to run analysis on.
197243
func (c *codeScanner) UploadAndAnalyzeWithOptions(
198244
ctx context.Context,
199245
requestId string,
@@ -202,58 +248,25 @@ func (c *codeScanner) UploadAndAnalyzeWithOptions(
202248
changedFiles map[string]bool,
203249
options ...AnalysisOption,
204250
) (*sarif.SarifResponse, string, *scan.ResultMetaData, error) {
251+
uploadedBundle, err := c.Upload(ctx, requestId, target, files, changedFiles)
252+
253+
if err != nil || uploadedBundle == nil || uploadedBundle.GetBundleHash() == "" {
254+
c.logger.Debug().Msg("empty bundle, no Snyk Code analysis")
255+
return nil, "", nil, err
256+
}
257+
205258
cfg := analysis.AnalysisConfig{}
206259
for _, opt := range options {
207260
opt(&cfg)
208261
}
209262

210-
if ctx.Err() != nil {
211-
c.logger.Info().Msg("Canceling Code scan - Code scanner received cancellation signal")
212-
return nil, "", nil, nil
213-
}
214-
b, err := c.bundleManager.Create(ctx, requestId, target.GetPath(), files, changedFiles)
215-
if err != nil {
216-
if bundle.IsNoFilesError(err) {
217-
return nil, "", nil, nil
218-
}
219-
if ctx.Err() == nil { // Only report errors that are not intentional cancellations
220-
msg := "error creating bundle..."
221-
c.errorReporter.CaptureError(errors.Wrap(err, msg), observability.ErrorReporterOptions{ErrorDiagnosticPath: target.GetPath()})
222-
return nil, "", nil, err
223-
} else {
224-
c.logger.Info().Msg("Canceling Code scan - Code scanner received cancellation signal")
225-
return nil, "", nil, nil
226-
}
227-
}
228-
229-
uploadedFiles := b.GetFiles()
230-
231-
b, err = c.bundleManager.Upload(ctx, requestId, b, uploadedFiles)
232-
bundleHash := b.GetBundleHash()
263+
response, metadata, err := c.analysisOrchestrator.RunTest(ctx, c.config.Organization(), uploadedBundle, target, cfg)
264+
err = c.checkCancellationOrLogError(ctx, target.GetPath(), err, "error running analysis...")
233265
if err != nil {
234-
if ctx.Err() != nil { // Only handle errors that are not intentional cancellations
235-
msg := "error uploading files..."
236-
c.errorReporter.CaptureError(errors.Wrap(err, msg), observability.ErrorReporterOptions{ErrorDiagnosticPath: target.GetPath()})
237-
return nil, bundleHash, nil, err
238-
} else {
239-
c.logger.Info().Msg("Canceling Code scan - Code scanner received cancellation signal")
240-
return nil, bundleHash, nil, nil
241-
}
266+
return nil, "", nil, err
242267
}
243268

244-
if bundleHash == "" {
245-
c.logger.Debug().Msg("empty bundle, no Snyk Code analysis")
246-
return nil, bundleHash, nil, nil
247-
}
248-
249-
response, metadata, err := c.analysisOrchestrator.RunTest(ctx, c.config.Organization(), b, target, cfg)
250-
251-
if ctx.Err() != nil {
252-
c.logger.Info().Msg("Canceling Code scan - Code scanner received cancellation signal")
253-
return nil, bundleHash, nil, nil
254-
}
255-
256-
return response, bundleHash, metadata, err
269+
return response, uploadedBundle.GetBundleHash(), metadata, err
257270
}
258271

259272
func (c *codeScanner) AnalyzeRemote(ctx context.Context, options ...AnalysisOption) (*sarif.SarifResponse, *scan.ResultMetaData, error) {
@@ -262,15 +275,15 @@ func (c *codeScanner) AnalyzeRemote(ctx context.Context, options ...AnalysisOpti
262275
opt(&cfg)
263276
}
264277

265-
if ctx.Err() != nil {
266-
c.logger.Info().Msg("Canceling Code scan - Code scanner received cancellation signal")
267-
return nil, nil, nil
278+
err := c.checkCancellationOrLogError(ctx, "", nil, "")
279+
if err != nil {
280+
return nil, nil, err
268281
}
269282
response, metadata, err := c.analysisOrchestrator.RunTestRemote(ctx, c.config.Organization(), cfg)
270283

271-
if ctx.Err() != nil {
272-
c.logger.Info().Msg("Canceling Code scan - Code scanner received cancellation signal")
273-
return nil, nil, nil
284+
err = c.checkCancellationOrLogError(ctx, "", err, "")
285+
if err != nil {
286+
return nil, nil, err
274287
}
275288

276289
return response, metadata, err

‎scan_test.go

Lines changed: 48 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package codeclient_test
1717

1818
import (
1919
"context"
20+
"github.com/google/uuid"
2021
"os"
2122
"path/filepath"
2223
"testing"
@@ -50,15 +51,16 @@ func Test_UploadAndAnalyze(t *testing.T) {
5051

5152
logger := zerolog.Nop()
5253

54+
testOrgId := uuid.NewString()
55+
5356
ctrl := gomock.NewController(t)
5457
mockHTTPClient := httpmocks.NewMockHTTPClient(ctrl)
5558
mockConfig := confMocks.NewMockConfig(ctrl)
5659
mockConfig.EXPECT().SnykCodeApi().AnyTimes().Return("")
5760
mockConfig.EXPECT().IsFedramp().AnyTimes().Return(false)
58-
mockConfig.EXPECT().Organization().AnyTimes().Return("4a72d1db-b465-4764-99e1-ecedad03b06a")
61+
mockConfig.EXPECT().Organization().AnyTimes().Return(testOrgId)
5962
mockConfig.EXPECT().SnykApi().AnyTimes().Return("")
6063
mockSpan := mocks.NewMockSpan(ctrl)
61-
mockSpan.EXPECT().GetTraceId().Return("testRequestId").AnyTimes()
6264
mockSpan.EXPECT().Context().Return(context.Background()).AnyTimes()
6365
mockInstrumentor := mocks.NewMockInstrumentor(ctrl)
6466
mockInstrumentor.EXPECT().StartSpan(gomock.Any(), gomock.Any()).Return(mockSpan).AnyTimes()
@@ -70,10 +72,11 @@ func Test_UploadAndAnalyze(t *testing.T) {
7072

7173
t.Run(
7274
"should just create bundle when hash empty", func(t *testing.T) {
75+
requestId := uuid.NewString()
7376
mockBundle := bundle.NewBundle(deepcodeMocks.NewMockDeepcodeClient(ctrl), mockInstrumentor, mockErrorReporter, &logger, "testRootPath", "", files, []string{}, []string{})
7477
mockBundleManager := bundleMocks.NewMockBundleManager(ctrl)
75-
mockBundleManager.EXPECT().Create(gomock.Any(), "testRequestId", baseDir, gomock.Any(), map[string]bool{}).Return(mockBundle, nil)
76-
mockBundleManager.EXPECT().Upload(gomock.Any(), "testRequestId", mockBundle, files).Return(mockBundle, nil)
78+
mockBundleManager.EXPECT().Create(gomock.Any(), requestId, baseDir, gomock.Any(), map[string]bool{}).Return(mockBundle, nil)
79+
mockBundleManager.EXPECT().Upload(gomock.Any(), requestId, mockBundle, files).Return(mockBundle, nil)
7780

7881
codeScanner := codeclient.NewCodeScanner(
7982
mockConfig,
@@ -84,24 +87,50 @@ func Test_UploadAndAnalyze(t *testing.T) {
8487
codeclient.WithLogger(&logger),
8588
)
8689

87-
response, bundleHash, err := codeScanner.WithBundleManager(mockBundleManager).UploadAndAnalyze(context.Background(), "testRequestId", target, docs, map[string]bool{})
90+
response, bundleHash, err := codeScanner.WithBundleManager(mockBundleManager).UploadAndAnalyze(context.Background(), requestId, target, docs, map[string]bool{})
8891
require.NoError(t, err)
8992
assert.Equal(t, "", bundleHash)
9093
assert.Nil(t, response)
9194
},
9295
)
9396

97+
t.Run(
98+
"should be able to upload without analysis", func(t *testing.T) {
99+
requestId := uuid.NewString()
100+
mockBundle := bundle.NewBundle(deepcodeMocks.NewMockDeepcodeClient(ctrl), mockInstrumentor, mockErrorReporter, &logger, "testRootPath", uuid.NewString(), files, []string{}, []string{})
101+
mockBundleManager := bundleMocks.NewMockBundleManager(ctrl)
102+
mockBundleManager.EXPECT().Create(gomock.Any(), requestId, baseDir, gomock.Any(), map[string]bool{}).Return(mockBundle, nil)
103+
mockBundleManager.EXPECT().Upload(gomock.Any(), requestId, mockBundle, files).Return(mockBundle, nil)
104+
105+
codeScanner := codeclient.NewCodeScanner(
106+
mockConfig,
107+
mockHTTPClient,
108+
codeclient.WithTrackerFactory(mockTrackerFactory),
109+
codeclient.WithInstrumentor(mockInstrumentor),
110+
codeclient.WithErrorReporter(mockErrorReporter),
111+
codeclient.WithLogger(&logger),
112+
)
113+
114+
uploadedBundle, err := codeScanner.
115+
WithBundleManager(mockBundleManager).
116+
Upload(context.Background(), requestId, target, docs, map[string]bool{})
117+
require.NoError(t, err)
118+
assert.Equal(t, mockBundle.GetBundleHash(), uploadedBundle.GetBundleHash())
119+
},
120+
)
121+
94122
t.Run(
95123
"should retrieve from backend", func(t *testing.T) {
96-
mockBundle := bundle.NewBundle(deepcodeMocks.NewMockDeepcodeClient(ctrl), mockInstrumentor, mockErrorReporter, &logger, "testRootPath", "testBundleHash", files, []string{}, []string{})
124+
requestId := uuid.NewString()
125+
mockBundle := bundle.NewBundle(deepcodeMocks.NewMockDeepcodeClient(ctrl), mockInstrumentor, mockErrorReporter, &logger, "testRootPath", uuid.NewString(), files, []string{}, []string{})
97126
mockBundleManager := bundleMocks.NewMockBundleManager(ctrl)
98-
mockBundleManager.EXPECT().Create(gomock.Any(), "b372d1db-b465-4764-99e1-ecedad03b06a", baseDir, gomock.Any(), map[string]bool{}).Return(mockBundle, nil)
99-
mockBundleManager.EXPECT().Upload(gomock.Any(), "b372d1db-b465-4764-99e1-ecedad03b06a", mockBundle, files).Return(mockBundle, nil)
127+
mockBundleManager.EXPECT().Create(gomock.Any(), requestId, baseDir, gomock.Any(), map[string]bool{}).Return(mockBundle, nil)
128+
mockBundleManager.EXPECT().Upload(gomock.Any(), requestId, mockBundle, files).Return(mockBundle, nil)
100129

101130
mockAnalysisOrchestrator := mockAnalysis.NewMockAnalysisOrchestrator(ctrl)
102131
mockAnalysisOrchestrator.EXPECT().RunTest(
103132
gomock.Any(),
104-
"4a72d1db-b465-4764-99e1-ecedad03b06a",
133+
testOrgId,
105134
gomock.Any(),
106135
gomock.Any(),
107136
gomock.Any(),
@@ -119,26 +148,26 @@ func Test_UploadAndAnalyze(t *testing.T) {
119148
response, bundleHash, err := codeScanner.
120149
WithBundleManager(mockBundleManager).
121150
WithAnalysisOrchestrator(mockAnalysisOrchestrator).
122-
UploadAndAnalyze(context.Background(), "b372d1db-b465-4764-99e1-ecedad03b06a", target, docs, map[string]bool{})
151+
UploadAndAnalyze(context.Background(), requestId, target, docs, map[string]bool{})
123152
require.NoError(t, err)
124153
assert.Equal(t, "COMPLETE", response.Status)
125-
assert.Equal(t, "testBundleHash", bundleHash)
154+
assert.Equal(t, mockBundle.GetBundleHash(), bundleHash)
126155
},
127156
)
128157

129158
t.Run(
130159
"should send the changed files to the analysis", func(t *testing.T) {
131160
relativeChangedFile := "./nested/folder/nested/file.ts"
132-
133-
mockBundle := bundle.NewBundle(deepcodeMocks.NewMockDeepcodeClient(ctrl), mockInstrumentor, mockErrorReporter, &logger, "testRootPath", "testBundleHash", files, []string{relativeChangedFile}, []string{})
161+
requestId := uuid.NewString()
162+
mockBundle := bundle.NewBundle(deepcodeMocks.NewMockDeepcodeClient(ctrl), mockInstrumentor, mockErrorReporter, &logger, "testRootPath", uuid.NewString(), files, []string{relativeChangedFile}, []string{})
134163
mockBundleManager := bundleMocks.NewMockBundleManager(ctrl)
135-
mockBundleManager.EXPECT().Create(gomock.Any(), "b372d1db-b465-4764-99e1-ecedad03b06a", baseDir, gomock.Any(), map[string]bool{}).Return(mockBundle, nil)
136-
mockBundleManager.EXPECT().Upload(gomock.Any(), "b372d1db-b465-4764-99e1-ecedad03b06a", mockBundle, files).Return(mockBundle, nil)
164+
mockBundleManager.EXPECT().Create(gomock.Any(), requestId, baseDir, gomock.Any(), map[string]bool{}).Return(mockBundle, nil)
165+
mockBundleManager.EXPECT().Upload(gomock.Any(), requestId, mockBundle, files).Return(mockBundle, nil)
137166

138167
mockAnalysisOrchestrator := mockAnalysis.NewMockAnalysisOrchestrator(ctrl)
139168
mockAnalysisOrchestrator.EXPECT().RunTest(
140169
gomock.Any(),
141-
"4a72d1db-b465-4764-99e1-ecedad03b06a",
170+
testOrgId,
142171
gomock.Any(),
143172
gomock.Any(),
144173
gomock.Any(),
@@ -156,7 +185,7 @@ func Test_UploadAndAnalyze(t *testing.T) {
156185
response, _, err := codeScanner.
157186
WithBundleManager(mockBundleManager).
158187
WithAnalysisOrchestrator(mockAnalysisOrchestrator).
159-
UploadAndAnalyze(context.Background(), "b372d1db-b465-4764-99e1-ecedad03b06a", target, docs, map[string]bool{})
188+
UploadAndAnalyze(context.Background(), requestId, target, docs, map[string]bool{})
160189
require.NoError(t, err)
161190
assert.Equal(t, "COMPLETE", response.Status)
162191
},
@@ -212,6 +241,8 @@ func TestAnalyzeRemote(t *testing.T) {
212241
gomock.Any(),
213242
).Return(nil, nil, assert.AnError)
214243

244+
mockErrorReporter.EXPECT().CaptureError(gomock.Any(), gomock.Any())
245+
215246
response, _, err := codeScanner.AnalyzeRemote(context.Background())
216247
assert.Nil(t, response)
217248
assert.Error(t, err)

0 commit comments

Comments
 (0)