Skip to content

Commit 55bd3c2

Browse files
authored
Fix workflow successors behavior to allow excluding current bucket (#4734)
1 parent d219305 commit 55bd3c2

File tree

11 files changed

+290
-65
lines changed

11 files changed

+290
-65
lines changed

catalog/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ where verb is one of
1818

1919
## Changes
2020

21+
- [Changed] Package creation from S3 files now includes current bucket as default destination when no workflow config exists; cross-bucket push strictly respects workflow successors configuration ([#4734](https://github.com/quiltdata/quilt/pull/4734))
2122
- [Fixed] Add missing `deleteObject` property to GUI config editor to enable/disable delete buttons for files and directories ([#4692](https://github.com/quiltdata/quilt/pull/4692))
2223
- [Changed] Hide file and directory delete buttons in Bucket tab by default; enable with `ui.actions.deleteObject` ([#4689](https://github.com/quiltdata/quilt/pull/4689))
2324
- [Added] Display detailed permission error messages when bucket add/update fails due to insufficient S3 permissions ([#4670](https://github.com/quiltdata/quilt/pull/4670))
Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
import { renderHook } from '@testing-library/react-hooks'
2+
import { describe, it, expect, vi } from 'vitest'
3+
import dedent from 'dedent'
4+
5+
import { FileNotFound } from 'containers/Bucket/errors'
6+
import Log from 'utils/Logging'
7+
import * as Request from 'utils/useRequest'
8+
import type { Successor } from 'utils/workflows'
9+
10+
import useSuccessors from './useSuccessors'
11+
12+
vi.mock('constants/config', () => ({ default: {} }))
13+
14+
// Create a stable S3 client instance
15+
const s3 = {
16+
headObject: vi.fn(() => ({ promise: () => Promise.resolve({}) })),
17+
getObject: vi.fn(),
18+
}
19+
20+
vi.mock('utils/AWS', () => ({ S3: { use: () => s3 } }))
21+
22+
describe('useSuccessors integration tests', () => {
23+
const CURRENT_BUCKET = 'foo-bucket'
24+
25+
const SUCCESSOR_FOR_CURRENT_BUCKET = {
26+
copyData: true,
27+
name: CURRENT_BUCKET,
28+
slug: CURRENT_BUCKET,
29+
url: `s3://${CURRENT_BUCKET}`,
30+
}
31+
32+
// Helper function to mock S3 getObject to return YAML config
33+
const mockS3ConfigYaml = (yamlString: string) => {
34+
s3.getObject.mockReturnValue({
35+
promise: () =>
36+
Promise.resolve({
37+
Body: Buffer.from(yamlString, 'utf-8'),
38+
}),
39+
})
40+
}
41+
42+
describe('useRequest states', () => {
43+
it('should return Loading while request is in progress', async () => {
44+
s3.getObject.mockReturnValue({
45+
promise: () => new Promise((resolve) => setTimeout(() => resolve(null), 1000)),
46+
})
47+
48+
const { result } = renderHook(() => useSuccessors(CURRENT_BUCKET))
49+
50+
expect(result.current).toBe(Request.Loading)
51+
})
52+
53+
it('should return Error when request fails', async () => {
54+
const loglevel = Log.getLevel()
55+
Log.setLevel('silent')
56+
57+
const testError = new Error('Network error')
58+
s3.getObject.mockReturnValue({
59+
promise: () => Promise.reject(testError),
60+
})
61+
62+
const { result, waitForValueToChange } = renderHook(() =>
63+
useSuccessors(CURRENT_BUCKET),
64+
)
65+
66+
await waitForValueToChange(() => result.current, { timeout: 5000 })
67+
expect(result.current).toBe(testError)
68+
69+
Log.setLevel(loglevel)
70+
})
71+
})
72+
73+
describe('when no successors are defined in workflow config', () => {
74+
it('should return empty successors (respect explicit config)', async () => {
75+
mockS3ConfigYaml(dedent`
76+
version: "1"
77+
workflows:
78+
workflow_1:
79+
name: Test Workflow
80+
`)
81+
82+
const { result, waitForValueToChange } = renderHook(() =>
83+
useSuccessors(CURRENT_BUCKET),
84+
)
85+
await waitForValueToChange(() => result.current, { timeout: 5000 })
86+
87+
expect(result.current).toEqual([])
88+
})
89+
90+
it('should handle empty workflow config when file not found', async () => {
91+
s3.getObject.mockReturnValue({
92+
promise: () => Promise.reject(new FileNotFound('Object not found')),
93+
})
94+
95+
const { result, waitForValueToChange } = renderHook(() =>
96+
useSuccessors(CURRENT_BUCKET),
97+
)
98+
99+
await waitForValueToChange(() => result.current, { timeout: 5000 })
100+
expect(result.current).toEqual([SUCCESSOR_FOR_CURRENT_BUCKET])
101+
})
102+
103+
it('should handle empty workflow config when file exists but is empty', async () => {
104+
mockS3ConfigYaml('')
105+
106+
const { result, waitForValueToChange } = renderHook(() =>
107+
useSuccessors(CURRENT_BUCKET),
108+
)
109+
110+
await waitForValueToChange(() => result.current, { timeout: 5000 })
111+
expect(result.current).toEqual([SUCCESSOR_FOR_CURRENT_BUCKET])
112+
})
113+
})
114+
115+
describe('when successors are explicitly defined without current bucket', () => {
116+
it('should exclude current bucket and return only defined successors', async () => {
117+
mockS3ConfigYaml(dedent`
118+
version: "1"
119+
successors:
120+
s3://destination-bucket:
121+
title: Destination Bucket
122+
copy_data: true
123+
s3://another-bucket:
124+
title: Another Bucket
125+
copy_data: false
126+
workflows:
127+
workflow_1:
128+
name: Test Workflow
129+
`)
130+
131+
const { result, waitForValueToChange } = renderHook(() =>
132+
useSuccessors(CURRENT_BUCKET),
133+
)
134+
await waitForValueToChange(() => result.current, { timeout: 5000 })
135+
136+
expect(result.current).toEqual([
137+
{
138+
name: 'Destination Bucket',
139+
slug: 'destination-bucket',
140+
url: 's3://destination-bucket',
141+
copyData: true,
142+
},
143+
{
144+
name: 'Another Bucket',
145+
slug: 'another-bucket',
146+
url: 's3://another-bucket',
147+
copyData: false,
148+
},
149+
])
150+
151+
// Verify current bucket is not included
152+
expect(result.current).not.toContainEqual(
153+
expect.objectContaining({ slug: CURRENT_BUCKET }),
154+
)
155+
})
156+
})
157+
158+
describe('when successors are explicitly defined with current bucket', () => {
159+
it('should return all defined successors without duplicating current bucket', async () => {
160+
mockS3ConfigYaml(dedent`
161+
version: "1"
162+
successors:
163+
s3://other-bucket:
164+
title: Other Bucket
165+
s3://${CURRENT_BUCKET}:
166+
title: Test Bucket (Current)
167+
copy_data: true
168+
s3://third-bucket:
169+
title: Third Bucket
170+
copy_data: false
171+
workflows:
172+
workflow_1:
173+
name: Test Workflow
174+
`)
175+
176+
const { result, waitForValueToChange } = renderHook(() =>
177+
useSuccessors(CURRENT_BUCKET),
178+
)
179+
180+
await waitForValueToChange(() => result.current, { timeout: 5000 })
181+
expect(result.current).toEqual([
182+
{
183+
copyData: true,
184+
name: 'Other Bucket',
185+
slug: 'other-bucket',
186+
url: 's3://other-bucket',
187+
},
188+
{
189+
copyData: true,
190+
name: 'Test Bucket (Current)',
191+
slug: CURRENT_BUCKET,
192+
url: `s3://${CURRENT_BUCKET}`,
193+
},
194+
{
195+
copyData: false,
196+
name: 'Third Bucket',
197+
slug: 'third-bucket',
198+
url: 's3://third-bucket',
199+
},
200+
])
201+
202+
// Verify current bucket is not duplicated
203+
const currentBucketMatches = (result.current as Successor[]).filter(
204+
(s) => s.slug === CURRENT_BUCKET,
205+
)
206+
expect(currentBucketMatches).toHaveLength(1)
207+
})
208+
})
209+
})

catalog/app/containers/Bucket/Dir/Toolbar/CreatePackage/useSuccessors.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ import * as React from 'react'
33
import { workflowsConfig } from 'containers/Bucket/requests'
44
import * as AWS from 'utils/AWS'
55
import * as Request from 'utils/useRequest'
6-
import { bucketToSuccessor } from 'utils/workflows'
76
import type { Successor } from 'utils/workflows'
87

98
export default function useSuccessors(bucket: string): Request.Result<Successor[]> {
@@ -15,7 +14,5 @@ export default function useSuccessors(bucket: string): Request.Result<Successor[
1514
return data
1615
}
1716

18-
return data.successors.find(({ slug }) => slug === bucket)
19-
? data.successors
20-
: [bucketToSuccessor(bucket), ...data.successors]
17+
return data.successors
2118
}

catalog/app/containers/Bucket/Dir/Toolbar/Toolbar.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ function DirToolbar({ className, features, handle, onReload }: DirToolbarProps)
3838

3939
const dst = React.useMemo(() => ({ bucket: handle.bucket }), [handle.bucket])
4040
const packageDirectoryDialog = useCreateDialog({
41-
currentBucketCanBeSuccessor: true,
4241
delayHashing: true,
4342
disableStateDisplay: true,
4443
dst,

catalog/app/containers/Bucket/PackageDialog/Create.tsx

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,6 @@ const useStyles = M.makeStyles((t) => ({
132132

133133
interface PackageCreationFormProps {
134134
close: () => void
135-
currentBucketCanBeSuccessor: boolean
136135
disableStateDisplay: boolean
137136
delayHashing: boolean
138137
state: PDModel.State
@@ -145,7 +144,6 @@ interface PackageCreationFormProps {
145144

146145
function PackageCreationForm({
147146
close,
148-
currentBucketCanBeSuccessor,
149147
delayHashing,
150148
state: {
151149
create,
@@ -196,7 +194,6 @@ function PackageCreationForm({
196194
{ui.title || 'Create package'} in{' '}
197195
<Successors.Dropdown
198196
bucket={dst.bucket || ''}
199-
currentBucketCanBeSuccessor={currentBucketCanBeSuccessor}
200197
successor={successor}
201198
onChange={(s) => setDst((d) => ({ ...d, bucket: s.slug }))}
202199
/>{' '}
@@ -274,7 +271,6 @@ interface PackageCreationDialogUIOptions {
274271

275272
interface RenderDialogProps {
276273
close: () => void
277-
currentBucketCanBeSuccessor: boolean
278274
delayHashing: boolean
279275
disableStateDisplay: boolean
280276
dialogStatus: PDModel.DialogStatus
@@ -284,7 +280,6 @@ interface RenderDialogProps {
284280

285281
function RenderDialog({
286282
close,
287-
currentBucketCanBeSuccessor,
288283
delayHashing,
289284
disableStateDisplay,
290285
dialogStatus,
@@ -331,7 +326,6 @@ function RenderDialog({
331326
return (
332327
<PackageCreationForm
333328
close={close}
334-
currentBucketCanBeSuccessor={currentBucketCanBeSuccessor}
335329
delayHashing={delayHashing}
336330
disableStateDisplay={disableStateDisplay}
337331
state={formState}
@@ -344,7 +338,6 @@ function RenderDialog({
344338
}
345339

346340
interface UseCreateDialogOptions {
347-
currentBucketCanBeSuccessor?: boolean
348341
delayHashing?: boolean
349342
disableStateDisplay?: boolean
350343
dst: PDModel.PackageDst
@@ -361,7 +354,6 @@ interface UseCreateDialogOptions {
361354
export function useCreateDialog({
362355
disableStateDisplay = false,
363356
delayHashing = false,
364-
currentBucketCanBeSuccessor = false,
365357
dst: initialDst,
366358
src: initialSrc,
367359
onClose,
@@ -454,7 +446,6 @@ export function useCreateDialog({
454446
{!exited && (
455447
<RenderDialog
456448
close={close}
457-
currentBucketCanBeSuccessor={currentBucketCanBeSuccessor}
458449
disableStateDisplay={disableStateDisplay}
459450
delayHashing={delayHashing}
460451
dialogStatus={dialogStatus}

catalog/app/containers/Bucket/PackageDialog/State/workflow.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,11 @@ import { ManifestStatus } from './manifest'
1010

1111
export type WorkflowsConfigStatus =
1212
| { _tag: 'idle' }
13-
| { _tag: 'loading'; config: /*empty config as fallback*/ workflows.WorkflowsConfig }
13+
| { _tag: 'loading'; config: /* null config as fallback */ workflows.WorkflowsConfig }
1414
| {
1515
_tag: 'error'
1616
error: Error
17-
config: /*empty config as fallback*/ workflows.WorkflowsConfig
17+
config: /* null config as fallback */ workflows.WorkflowsConfig
1818
}
1919
| { _tag: 'ready'; config: workflows.WorkflowsConfig }
2020

@@ -49,10 +49,10 @@ export function useWorkflowsConfig(
4949
return { _tag: 'idle' }
5050
}
5151
if (result === Request.Loading) {
52-
return { _tag: 'loading', config: workflows.emptyConfig }
52+
return { _tag: 'loading', config: workflows.nullConfig }
5353
}
5454
if (result instanceof Error) {
55-
return { _tag: 'error', error: result, config: workflows.emptyConfig }
55+
return { _tag: 'error', error: result, config: workflows.nullConfig }
5656
}
5757

5858
return { _tag: 'ready', config: result }

0 commit comments

Comments
 (0)