Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return NotFound error when decks uri is empty (#642) #6228

Merged
merged 1 commit into from
Feb 7, 2025
Merged

Return NotFound error when decks uri is empty (#642) #6228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion flyteadmin/dataproxy/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ func (s Service) CreateDownloadLink(ctx context.Context, req *service.CreateDown
}

if len(nativeURL) == 0 {
return nil, errors.NewFlyteAdminErrorf(codes.Internal, "no deckUrl found for request [%+v]", req)
return nil, errors.NewFlyteAdminErrorf(codes.NotFound, "no deckUrl found for request [%+v]", req)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error code change may mask issues

Consider if changing error code from Internal to NotFound is appropriate here. The NotFound error code should only be used when we're certain the resource doesn't exist. In this case, an empty deckUrl could indicate an internal system issue rather than a missing resource.

Code suggestion
Check the AI-generated fix before applying
Suggested change
return nil, errors.NewFlyteAdminErrorf(codes.NotFound, "no deckUrl found for request [%+v]", req)
return nil, errors.NewFlyteAdminErrorf(codes.Internal, "no deckUrl found for request [%+v]", req)

Code Review Run #b1cb17

Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

}

ref := storage.DataReference(nativeURL)
Expand Down
86 changes: 66 additions & 20 deletions flyteadmin/dataproxy/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,34 +170,50 @@ func (t testMetadata) Exists() bool {
return t.exists
}

func TestCreateDownloadLink(t *testing.T) {
type CreateDownloadLinkDependencies struct {
dataStore *storage.DataStore
nodeExecutionManager *mocks.NodeExecutionInterface
taskExecutionManager *mocks.TaskExecutionInterface
}

func setupCreateDownloadLink(t *testing.T) (Service, CreateDownloadLinkDependencies) {
dataStore := commonMocks.GetMockStorageClient()
nodeExecutionManager := &mocks.NodeExecutionInterface{}
nodeExecutionManager.EXPECT().GetNodeExecution(mock.Anything, mock.Anything).RunAndReturn(func(ctx context.Context, request *admin.NodeExecutionGetRequest) (*admin.NodeExecution, error) {
taskExecutionManager := &mocks.TaskExecutionInterface{}
s, err := NewService(config.DataProxyConfig{Download: config.DataProxyDownloadConfig{MaxExpiresIn: stdlibConfig.Duration{Duration: time.Hour}}}, nodeExecutionManager, dataStore, taskExecutionManager)
assert.NoError(t, err)
return s, CreateDownloadLinkDependencies{
dataStore: dataStore,
nodeExecutionManager: nodeExecutionManager,
taskExecutionManager: taskExecutionManager,
}
}

func TestCreateDownloadLink(t *testing.T) {
nodeExecutionFunc := func(ctx context.Context, request *admin.NodeExecutionGetRequest) (*admin.NodeExecution, error) {
return &admin.NodeExecution{
Closure: &admin.NodeExecutionClosure{
DeckUri: "s3://something/something",
},
}, nil
})
taskExecutionManager := &mocks.TaskExecutionInterface{}

s, err := NewService(config.DataProxyConfig{Download: config.DataProxyDownloadConfig{MaxExpiresIn: stdlibConfig.Duration{Duration: time.Hour}}}, nodeExecutionManager, dataStore, taskExecutionManager)
assert.NoError(t, err)
}

t.Run("Invalid expiry", func(t *testing.T) {
_, err = s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{
s, dependencies := setupCreateDownloadLink(t)
dependencies.nodeExecutionManager.EXPECT().GetNodeExecution(mock.Anything, mock.Anything).RunAndReturn(nodeExecutionFunc)
_, err := s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{
Comment on lines +202 to +204
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider consolidating duplicate test setup code

Consider consolidating the duplicate setup code. The setupCreateDownloadLink() call appears multiple times in the test function which could be refactored to improve maintainability.

Code suggestion
Check the AI-generated fix before applying
 -	s, dependencies := setupCreateDownloadLink(t)
 -	dependencies.nodeExecutionManager.EXPECT().GetNodeExecution(mock.Anything, mock.Anything).RunAndReturn(nodeExecutionFunc)
 -	_, err := s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{
 -
 -	s, dependencies := setupCreateDownloadLink(t) 
 -	dependencies.dataStore.ComposedProtobufStore.(*commonMocks.TestDataStore).HeadCb = func(ctx context.Context, ref storage.DataReference) (storage.Metadata, error) {
 +	setupAndRunTest := func(t *testing.T, setupFn func(*CreateDownloadLinkDependencies)) error {
 +		s, dependencies := setupCreateDownloadLink(t)
 +		setupFn(dependencies)
 +		return s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{})
 +	}

Code Review Run #b1cb17

Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

ExpiresIn: durationpb.New(-time.Hour),
})
assert.Error(t, err)
})

t.Run("item not found", func(t *testing.T) {
dataStore.ComposedProtobufStore.(*commonMocks.TestDataStore).HeadCb = func(ctx context.Context, ref storage.DataReference) (storage.Metadata, error) {
s, dependencies := setupCreateDownloadLink(t)
dependencies.dataStore.ComposedProtobufStore.(*commonMocks.TestDataStore).HeadCb = func(ctx context.Context, ref storage.DataReference) (storage.Metadata, error) {
return testMetadata{exists: false}, nil
}

_, err = s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{
dependencies.nodeExecutionManager.EXPECT().GetNodeExecution(mock.Anything, mock.Anything).RunAndReturn(nodeExecutionFunc)
_, err := s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{
ArtifactType: service.ArtifactType_ARTIFACT_TYPE_DECK,
Source: &service.CreateDownloadLinkRequest_NodeExecutionId{
NodeExecutionId: &core.NodeExecutionIdentifier{},
Expand All @@ -212,11 +228,12 @@ func TestCreateDownloadLink(t *testing.T) {
})

t.Run("valid config", func(t *testing.T) {
dataStore.ComposedProtobufStore.(*commonMocks.TestDataStore).HeadCb = func(ctx context.Context, ref storage.DataReference) (storage.Metadata, error) {
s, dependencies := setupCreateDownloadLink(t)
dependencies.dataStore.ComposedProtobufStore.(*commonMocks.TestDataStore).HeadCb = func(ctx context.Context, ref storage.DataReference) (storage.Metadata, error) {
return testMetadata{exists: true}, nil
}

_, err = s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{
dependencies.nodeExecutionManager.EXPECT().GetNodeExecution(mock.Anything, mock.Anything).RunAndReturn(nodeExecutionFunc)
_, err := s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{
ArtifactType: service.ArtifactType_ARTIFACT_TYPE_DECK,
Source: &service.CreateDownloadLinkRequest_NodeExecutionId{
NodeExecutionId: &core.NodeExecutionIdentifier{},
Expand All @@ -227,12 +244,40 @@ func TestCreateDownloadLink(t *testing.T) {
assert.NoError(t, err)
})

t.Run("no deckUrl found", func(t *testing.T) {
s, dependencies := setupCreateDownloadLink(t)
dependencies.dataStore.ComposedProtobufStore.(*commonMocks.TestDataStore).HeadCb = func(ctx context.Context, ref storage.DataReference) (storage.Metadata, error) {
return testMetadata{exists: true}, nil
}
nodeExecutionFuncWithoutDecks := func(ctx context.Context, request *admin.NodeExecutionGetRequest) (*admin.NodeExecution, error) {
return &admin.NodeExecution{
Closure: &admin.NodeExecutionClosure{
DeckUri: "",
},
}, nil
}
dependencies.nodeExecutionManager.EXPECT().GetNodeExecution(mock.Anything, mock.Anything).RunAndReturn(nodeExecutionFuncWithoutDecks)
_, err := s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{
ArtifactType: service.ArtifactType_ARTIFACT_TYPE_DECK,
Source: &service.CreateDownloadLinkRequest_NodeExecutionId{
NodeExecutionId: &core.NodeExecutionIdentifier{},
},
ExpiresIn: durationpb.New(time.Hour),
})
assert.Error(t, err)
st, ok := status.FromError(err)
assert.True(t, ok)
assert.Equal(t, codes.NotFound, st.Code())
assert.Contains(t, st.Message(), "no deckUrl found for request")
})

t.Run("head failed", func(t *testing.T) {
dataStore.ComposedProtobufStore.(*commonMocks.TestDataStore).HeadCb = func(ctx context.Context, ref storage.DataReference) (storage.Metadata, error) {
s, dependencies := setupCreateDownloadLink(t)
dependencies.dataStore.ComposedProtobufStore.(*commonMocks.TestDataStore).HeadCb = func(ctx context.Context, ref storage.DataReference) (storage.Metadata, error) {
return testMetadata{}, fmt.Errorf("head fail")
}

_, err = s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{
dependencies.nodeExecutionManager.EXPECT().GetNodeExecution(mock.Anything, mock.Anything).RunAndReturn(nodeExecutionFunc)
_, err := s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{
ArtifactType: service.ArtifactType_ARTIFACT_TYPE_DECK,
Source: &service.CreateDownloadLinkRequest_NodeExecutionId{
NodeExecutionId: &core.NodeExecutionIdentifier{},
Expand All @@ -247,11 +292,12 @@ func TestCreateDownloadLink(t *testing.T) {
})

t.Run("use default ExpiresIn", func(t *testing.T) {
dataStore.ComposedProtobufStore.(*commonMocks.TestDataStore).HeadCb = func(ctx context.Context, ref storage.DataReference) (storage.Metadata, error) {
s, dependencies := setupCreateDownloadLink(t)
dependencies.dataStore.ComposedProtobufStore.(*commonMocks.TestDataStore).HeadCb = func(ctx context.Context, ref storage.DataReference) (storage.Metadata, error) {
return testMetadata{exists: true}, nil
}

_, err = s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{
dependencies.nodeExecutionManager.EXPECT().GetNodeExecution(mock.Anything, mock.Anything).RunAndReturn(nodeExecutionFunc)
_, err := s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable shadowing in CreateDownloadLink test

Consider declaring err in the first usage to avoid shadowing. The current declaration of err shadows the outer scope variable.

Code suggestion
Check the AI-generated fix before applying
Suggested change
_, err := s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{
_, err = s.CreateDownloadLink(context.Background(), &service.CreateDownloadLinkRequest{

Code Review Run #b1cb17

Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

ArtifactType: service.ArtifactType_ARTIFACT_TYPE_DECK,
Source: &service.CreateDownloadLinkRequest_NodeExecutionId{
NodeExecutionId: &core.NodeExecutionIdentifier{},
Expand Down
Loading