diff --git a/activities/vidispine/meta.go b/activities/vidispine/meta.go index dcaa062e..c0e24e16 100644 --- a/activities/vidispine/meta.go +++ b/activities/vidispine/meta.go @@ -90,6 +90,26 @@ func (a Activities) AddToVXMetadataFieldActivity(ctx context.Context, params vsa return nil, err } +type DeleteMetadataGroupParams struct { + VXID string + Group string +} + +type DeleteMetadataGroupResult struct { + DeletedInstances int +} + +func (a Activities) DeleteMetadataGroupInstancesActivity(ctx context.Context, params DeleteMetadataGroupParams) (*DeleteMetadataGroupResult, error) { + log := activity.GetLogger(ctx) + log.Info("Starting DeleteMetadataGroupInstancesActivity", "vxid", params.VXID, "group", params.Group) + + count, err := a.Client.DeleteMetadataGroupInstances(params.VXID, params.Group) + if err != nil { + return nil, err + } + return &DeleteMetadataGroupResult{DeletedInstances: count}, nil +} + type GetResolutionsParams struct { VXID string } diff --git a/potential_improvements.md b/potential_improvements.md index 9df5da85..abf3730d 100644 --- a/potential_improvements.md +++ b/potential_improvements.md @@ -4,6 +4,9 @@ Condensed 2026-08-21. Items confirmed fixed were removed. Bugs section validated ## Bugs +- `workflows/misc/merge_import_subs.go:145` — `langs = append(langs, lang)` inside `for _, lang := range langs`; copy-paste from import_subs.go where `langs` is a separate accumulator. Here it just grows the slice being ranged over with duplicates. Remove the append. +- `workflows/misc/merge_import_subs.go:147` — `_ = wfutils.Execute(...WaitForJobCompletion...).Wait(ctx)` discards the job result, so a failed shape-import job doesn't fail the workflow (same pattern was fixed in import_subs.go). + ## Security diff --git a/services/vidispine/service.go b/services/vidispine/service.go index a09ca192..5a092721 100644 --- a/services/vidispine/service.go +++ b/services/vidispine/service.go @@ -16,6 +16,7 @@ type Client interface { CreateThumbnails(assetID string, width, height int) (string, error) DeleteItems(ctx context.Context, itemVXIDs []string, deleteFiles bool) error + DeleteMetadataGroupInstances(itemID, groupName string) (int, error) DeleteShape(assetID, shapeID string) error FindJob(itemID string, jobType string) (*vsapi.JobDocument, error) @@ -30,6 +31,7 @@ type Client interface { GetJob(jobID string) (*vsapi.JobDocument, error) GetMetadata(vsID string) (*vsapi.MetadataResult, error) GetMetadataFields(vsID string, fields []string) (*vsapi.MetadataResult, error) + GetMetadataGroupInstances(itemID, groupName string) ([]vsapi.MetadataGroupInstance, error) GetRelations(assetID string) ([]vsapi.Relation, error) GetResolutions(itemVXID string) ([]vsapi.Resolution, error) GetSequence(itemVXID string) (*vsapi.SequenceDocument, error) diff --git a/services/vidispine/vsapi/metadata.go b/services/vidispine/vsapi/metadata.go index a0dc2663..c4c28b49 100644 --- a/services/vidispine/vsapi/metadata.go +++ b/services/vidispine/vsapi/metadata.go @@ -101,6 +101,110 @@ func (c *Client) GetMetadataAdvanced(params GetMetadataAdvancedParams) (*Metadat return resp.Result().(*MetadataResult), nil } +// MetadataGroupInstance identifies one occurrence of a named metadata group on an +// item: the group's uuid and the timespan it lives in. +type MetadataGroupInstance struct { + UUID string + Start string + End string +} + +// The non-terse metadata endpoint answers with a MetadataListDocument +// ({"item":[{"metadata":{"timespan":[...]}}]}); a bare MetadataDocument carries +// the timespans at the top level. metadataDocumentJSON accepts both. +type metadataDocumentJSON struct { + Item []struct { + Metadata struct { + Timespan []metadataTimespanJSON `json:"timespan"` + } `json:"metadata"` + } `json:"item"` + Timespan []metadataTimespanJSON `json:"timespan"` +} + +type metadataTimespanJSON struct { + Start string `json:"start"` + End string `json:"end"` + Group []metadataGroupJSON `json:"group"` +} + +type metadataGroupJSON struct { + UUID string `json:"uuid"` + Name string `json:"name"` + Group []metadataGroupJSON `json:"group"` +} + +func collectGroupInstances(groups []metadataGroupJSON, name, start, end string, out []MetadataGroupInstance) []MetadataGroupInstance { + for _, g := range groups { + if g.Name == name && g.UUID != "" { + out = append(out, MetadataGroupInstance{UUID: g.UUID, Start: start, End: end}) + } + out = collectGroupInstances(g.Group, name, start, end, out) + } + return out +} + +// GetMetadataGroupInstances lists every occurrence of the named metadata group on +// the item, across all timespans (nested groups included). +func (c *Client) GetMetadataGroupInstances(itemID, groupName string) ([]MetadataGroupInstance, error) { + requestURL, _ := url.Parse(c.baseURL) + requestURL.Path += fmt.Sprintf("/item/%s/metadata", url.PathEscape(itemID)) + q := requestURL.Query() + q.Set("group", groupName) + requestURL.RawQuery = q.Encode() + + // An item with no instances of the group can come back as 404. + resp, err := tolerating404(c.restyClient.R()). + SetResult(&metadataDocumentJSON{}). + Get(requestURL.String()) + if err != nil { + return nil, err + } + + doc := resp.Result().(*metadataDocumentJSON) + timespans := doc.Timespan + for _, item := range doc.Item { + timespans = append(timespans, item.Metadata.Timespan...) + } + + var out []MetadataGroupInstance + for _, ts := range timespans { + out = collectGroupInstances(ts.Group, groupName, ts.Start, ts.End, out) + } + return out, nil +} + +// DeleteMetadataGroupInstances removes every occurrence of the named metadata group +// from the item and returns how many were removed. Removal is addressed by group +// uuid per timespan — addressing by name is what Vidispine rejects as "ambiguous +// path to group" when the name resolves to more than one path. +func (c *Client) DeleteMetadataGroupInstances(itemID, groupName string) (int, error) { + instances, err := c.GetMetadataGroupInstances(itemID, groupName) + if err != nil { + return 0, err + } + if len(instances) == 0 { + return 0, nil + } + + body, err := createRemoveMetadataGroupsXml(instances) + if err != nil { + return 0, err + } + + requestURL, _ := url.Parse(c.baseURL) + requestURL.Path += fmt.Sprintf("/item/%s/metadata", url.PathEscape(itemID)) + + _, err = c.restyClient.R(). + SetHeader("content-type", "application/xml"). + SetBody(body.String()). + Put(requestURL.String()) + if err != nil { + return 0, err + } + + return len(instances), nil +} + type ItemMetadataFieldParams struct { ItemID string GroupID string diff --git a/services/vidispine/vsapi/metadata_test.go b/services/vidispine/vsapi/metadata_test.go index dc41c6ed..86a85464 100644 --- a/services/vidispine/vsapi/metadata_test.go +++ b/services/vidispine/vsapi/metadata_test.go @@ -136,3 +136,47 @@ func Test_GenerateMetUpdateWithTCXML(t *testing.T) { ` assert.Equal(t, expected, buf.String()) } + +func Test_MetadataDocumentJSON_GroupInstances(t *testing.T) { + // MetadataListDocument envelope with nested groups. + listDoc := `{"item":[{"id":"VX-1","metadata":{"timespan":[ + {"start":"-INF","end":"+INF","group":[ + {"uuid":"uuid-1","name":"stl_subtitle"}, + {"uuid":"uuid-2","name":"Subclips","group":[{"uuid":"uuid-3","name":"stl_subtitle"}]} + ]}, + {"start":"0@PAL","end":"250@PAL","group":[{"uuid":"uuid-4","name":"stl_subtitle"}]} + ]}}]}` + + doc := metadataDocumentJSON{} + assert.NoError(t, json.Unmarshal([]byte(listDoc), &doc)) + + timespans := doc.Timespan + for _, item := range doc.Item { + timespans = append(timespans, item.Metadata.Timespan...) + } + + var out []MetadataGroupInstance + for _, ts := range timespans { + out = collectGroupInstances(ts.Group, "stl_subtitle", ts.Start, ts.End, out) + } + + assert.Equal(t, []MetadataGroupInstance{ + {UUID: "uuid-1", Start: "-INF", End: "+INF"}, + {UUID: "uuid-3", Start: "-INF", End: "+INF"}, + {UUID: "uuid-4", Start: "0@PAL", End: "250@PAL"}, + }, out) +} + +func Test_MetadataDocumentJSON_BareDocument(t *testing.T) { + bareDoc := `{"timespan":[{"start":"-INF","end":"+INF","group":[{"uuid":"uuid-9","name":"stl_subtitle"}]}]}` + + doc := metadataDocumentJSON{} + assert.NoError(t, json.Unmarshal([]byte(bareDoc), &doc)) + + var out []MetadataGroupInstance + for _, ts := range doc.Timespan { + out = collectGroupInstances(ts.Group, "stl_subtitle", ts.Start, ts.End, out) + } + + assert.Equal(t, []MetadataGroupInstance{{UUID: "uuid-9", Start: "-INF", End: "+INF"}}, out) +} diff --git a/services/vidispine/vsapi/xml_templates.go b/services/vidispine/vsapi/xml_templates.go index 89c1287c..d13bd61b 100644 --- a/services/vidispine/vsapi/xml_templates.go +++ b/services/vidispine/vsapi/xml_templates.go @@ -9,6 +9,7 @@ var ( xmlMasterPlaceholderTmpl = template.Must(template.New("master").Parse(xmlMasterPlaceholder)) xmlRawMaterialPlaceholderTmpl = template.Must(template.New("raw").Parse(xmlRawMaterialPlaceholder)) xmlSetMetadataPlaceholderTmpl = template.Must(template.New("metadata").Parse(xmlSetItemMetadataFieldPlaceholder)) + xmlRemoveMetadataGroupsTmpl = template.Must(template.New("removeGroups").Parse(xmlRemoveMetadataGroupsPlaceholder)) ) const ( @@ -84,6 +85,15 @@ const ( {{end}} +` + + xmlRemoveMetadataGroupsPlaceholder = ` + +{{- range . }} + + + +{{- end }} ` ) @@ -96,6 +106,12 @@ type xmlSetItemMetadataFieldParams struct { Add bool } +func createRemoveMetadataGroupsXml(instances []MetadataGroupInstance) (*bytes.Buffer, error) { + buf := new(bytes.Buffer) + err := xmlRemoveMetadataGroupsTmpl.Execute(buf, instances) + return buf, err +} + func createSetItemMetadataFieldXml(params xmlSetItemMetadataFieldParams) (*bytes.Buffer, error) { if params.StartTC == "" { params.StartTC = MinusInf diff --git a/services/vidispine/vsapi/xml_templates_test.go b/services/vidispine/vsapi/xml_templates_test.go new file mode 100644 index 00000000..d56dbc56 --- /dev/null +++ b/services/vidispine/vsapi/xml_templates_test.go @@ -0,0 +1,36 @@ +package vsapi + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCreateRemoveMetadataGroupsXml_Empty(t *testing.T) { + buf, err := createRemoveMetadataGroupsXml(nil) + assert.NoError(t, err) + assert.NotContains(t, buf.String(), "`) + assert.Contains(t, out, ``) +} + +func TestCreateRemoveMetadataGroupsXml_Multiple(t *testing.T) { + buf, err := createRemoveMetadataGroupsXml([]MetadataGroupInstance{ + {UUID: "uuid-1", Start: "0@PAL", End: "250@PAL"}, + {UUID: "uuid-2", Start: "250@PAL", End: "500@PAL"}, + }) + assert.NoError(t, err) + out := buf.String() + assert.Contains(t, out, ``) + assert.Contains(t, out, ``) + assert.Contains(t, out, ``) + assert.Contains(t, out, ``) +} diff --git a/services/vidispine/vscommon/fields.go b/services/vidispine/vscommon/fields.go index 4ffe6dfc..48362247 100644 --- a/services/vidispine/vscommon/fields.go +++ b/services/vidispine/vscommon/fields.go @@ -4,6 +4,10 @@ import "github.com/orsinium-labs/enum" type FieldType enum.Member[string] +// GroupStlSubtitle is the metadata group Vidispine writes subtitle cues +// (FieldStlText) into during sidecar import. +const GroupStlSubtitle = "stl_subtitle" + var ( FieldDurationSeconds = FieldType{"durationSeconds"} FieldDescription = FieldType{"portal_mf982016"} diff --git a/services/vidispine/vsmock/mock_Client.go b/services/vidispine/vsmock/mock_Client.go index b992c817..a61e0e34 100644 --- a/services/vidispine/vsmock/mock_Client.go +++ b/services/vidispine/vsmock/mock_Client.go @@ -144,6 +144,21 @@ func (mr *MockClientMockRecorder) DeleteItems(ctx, itemVXIDs, deleteFiles any) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteItems", reflect.TypeOf((*MockClient)(nil).DeleteItems), ctx, itemVXIDs, deleteFiles) } +// DeleteMetadataGroupInstances mocks base method. +func (m *MockClient) DeleteMetadataGroupInstances(itemID, groupName string) (int, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteMetadataGroupInstances", itemID, groupName) + ret0, _ := ret[0].(int) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// DeleteMetadataGroupInstances indicates an expected call of DeleteMetadataGroupInstances. +func (mr *MockClientMockRecorder) DeleteMetadataGroupInstances(itemID, groupName any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteMetadataGroupInstances", reflect.TypeOf((*MockClient)(nil).DeleteMetadataGroupInstances), itemID, groupName) +} + // DeleteShape mocks base method. func (m *MockClient) DeleteShape(assetID, shapeID string) error { m.ctrl.T.Helper() @@ -263,6 +278,21 @@ func (mr *MockClientMockRecorder) GetMetadataFields(vsID, fields any) *gomock.Ca return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetMetadataFields", reflect.TypeOf((*MockClient)(nil).GetMetadataFields), vsID, fields) } +// GetMetadataGroupInstances mocks base method. +func (m *MockClient) GetMetadataGroupInstances(itemID, groupName string) ([]vsapi.MetadataGroupInstance, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetMetadataGroupInstances", itemID, groupName) + ret0, _ := ret[0].([]vsapi.MetadataGroupInstance) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetMetadataGroupInstances indicates an expected call of GetMetadataGroupInstances. +func (mr *MockClientMockRecorder) GetMetadataGroupInstances(itemID, groupName any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetMetadataGroupInstances", reflect.TypeOf((*MockClient)(nil).GetMetadataGroupInstances), itemID, groupName) +} + // GetRelations mocks base method. func (m *MockClient) GetRelations(assetID string) ([]vsapi.Relation, error) { m.ctrl.T.Helper() diff --git a/workflows/ingest/import_subtitles.go b/workflows/ingest/import_subtitles.go index af9175ef..46916a5c 100644 --- a/workflows/ingest/import_subtitles.go +++ b/workflows/ingest/import_subtitles.go @@ -7,7 +7,9 @@ import ( "strings" vsactivity "github.com/bcc-code/bcc-media-flows/activities/vidispine" + "github.com/bcc-code/bcc-media-flows/languages" "github.com/bcc-code/bcc-media-flows/paths" + "github.com/bcc-code/bcc-media-flows/services/vidispine/vscommon" wfutils "github.com/bcc-code/bcc-media-flows/utils/workflows" "go.temporal.io/sdk/workflow" ) @@ -105,6 +107,10 @@ func ImportSubtitles(ctx workflow.Context, input ImportSubtitlesInput) error { if input.Language == "" { return errors.New("missing language") } + language, err := languages.ParseLanguageCode(input.Language) + if err != nil { + return fmt.Errorf("unknown language %q: %w", input.Language, err) + } subtitles, err := resolveSubtitles(ctx, input) if err != nil { @@ -155,6 +161,16 @@ func ImportSubtitles(ctx workflow.Context, input ImportSubtitlesInput) error { Replace: true, }) + // Import SRT as a language-coded shape so exports treat it as a real + // subtitle for that language, not the AI-generated "und" fallback. + importLangSRTJob := wfutils.Execute(ctx, vsactivity.Vidispine.ImportFileAsShapeActivity, + vsactivity.ImportFileAsShapeParams{ + AssetID: input.VXID, + FilePath: srtFilePath, + ShapeTag: fmt.Sprintf("sub_%s_srt", language.ISO6391), + Replace: true, + }) + var errs []error importSRTResult, err := importSRTJob.Result(ctx) if err != nil { @@ -166,6 +182,11 @@ func ImportSubtitles(ctx workflow.Context, input ImportSubtitlesInput) error { errs = append(errs, err) } + importLangSRTResult, err := importLangSRTJob.Result(ctx) + if err != nil { + errs = append(errs, err) + } + if len(errs) > 0 { return fmt.Errorf("failed to import subtitle shapes: %w", errors.Join(errs...)) } @@ -178,19 +199,38 @@ func ImportSubtitles(ctx workflow.Context, input ImportSubtitlesInput) error { if err != nil { return fmt.Errorf("importing of JSON file into Mediabanken failed: %w", err) } + err = wfutils.WaitForVidispineJob(ctx, importLangSRTResult.JobID) + if err != nil { + return fmt.Errorf("importing of language SRT shape into Mediabanken failed: %w", err) + } - // Import SRT as sidecar independently (non-blocking, fire-and-forget) - err = wfutils.Execute(ctx, vsactivity.Vidispine.ImportFileAsSidecarActivity, vsactivity.ImportSubtitleAsSidecarParams{ + // Vidispine resolves the subtitle group by name during sidecar import, and + // stale instances from earlier imports make that fail with "ambiguous path + // to group: stl_subtitle" — so remove them first. + err = wfutils.Execute(ctx, vsactivity.Vidispine.DeleteMetadataGroupInstancesActivity, vsactivity.DeleteMetadataGroupParams{ + VXID: input.VXID, + Group: vscommon.GroupStlSubtitle, + }).Wait(ctx) + if err != nil { + return fmt.Errorf("removing existing %s metadata failed: %w", vscommon.GroupStlSubtitle, err) + } + + sidecarResult, err := wfutils.Execute(ctx, vsactivity.Vidispine.ImportFileAsSidecarActivity, vsactivity.ImportSubtitleAsSidecarParams{ FilePath: srtFilePath, Language: input.Language, AssetID: input.VXID, - }).Wait(ctx) - + }).Result(ctx) if err != nil { return fmt.Errorf("importing of SRT file as sidecar failed: %w", err) } + if sidecarResult != nil && sidecarResult.JobID != "" { + err = wfutils.WaitForVidispineJob(ctx, sidecarResult.JobID) + if err != nil { + return fmt.Errorf("sidecar import job failed: %w", err) + } + } - logger.Info("Subtitle SRT and JSON imported as shapes; SRT as sidecar (async)", "vxid", input.VXID) + logger.Info("Subtitle SRT and JSON imported as shapes and sidecar", "vxid", input.VXID) return nil } diff --git a/workflows/ingest/import_subtitles_test.go b/workflows/ingest/import_subtitles_test.go index ec5dbaba..37689982 100644 --- a/workflows/ingest/import_subtitles_test.go +++ b/workflows/ingest/import_subtitles_test.go @@ -111,11 +111,11 @@ func (s *ImportSubtitlesTestSuite) Test_ImportSubtitlesWorkflow() { Subtitles: Transcription{Segments: segments}, } - // Use a valid Path string for your environment - //mockOutputPath := paths.MustParse("./testdata/output") - - s.env.OnActivity(activities.Vidispine.ImportFileAsShapeActivity, mock.Anything, mock.Anything).Return(&vsactivity.ImportFileResult{JobID: "job-srt"}, nil) - s.env.OnActivity(activities.Vidispine.ImportFileAsShapeActivity, mock.Anything, mock.Anything).Return(&vsactivity.ImportFileResult{JobID: "job-json"}, nil) + shapeTags := map[string]bool{} + s.env.OnActivity(activities.Vidispine.ImportFileAsShapeActivity, mock.Anything, mock.MatchedBy(func(p vsactivity.ImportFileAsShapeParams) bool { + shapeTags[p.ShapeTag] = true + return true + })).Return(&vsactivity.ImportFileResult{JobID: "job-shape"}, nil) s.env.OnActivity(activities.Util.CreateFolder, mock.Anything, mock.Anything).Return("", nil) @@ -139,13 +139,46 @@ func (s *ImportSubtitlesTestSuite) Test_ImportSubtitlesWorkflow() { Data: []byte("1\n00:00:00,000 --> 00:00:01,000\nHello\n\n2\n00:00:01,500 --> 00:00:02,500\nWorld\n\n"), }).Return("", nil).Once() - s.env.OnActivity(activities.Vidispine.ImportFileAsSidecarActivity, mock.Anything, mock.Anything).Return(nil, nil) + s.env.OnActivity(activities.Vidispine.DeleteMetadataGroupInstancesActivity, mock.Anything, vsactivity.DeleteMetadataGroupParams{ + VXID: vxid, + Group: "stl_subtitle", + }).Return(&vsactivity.DeleteMetadataGroupResult{DeletedInstances: 2}, nil).Once() + s.env.OnActivity(activities.Vidispine.ImportFileAsSidecarActivity, mock.Anything, mock.Anything).Return(&vsactivity.ImportFileAsSidecarResult{JobID: "job-sidecar"}, nil).Once() s.env.OnActivity(activities.Vidispine.JobCompleteOrErr, mock.Anything, mock.Anything).Return(true, nil) s.env.ExecuteWorkflow(ImportSubtitles, input) s.True(s.env.IsWorkflowCompleted()) err = s.env.GetWorkflowError() s.NoError(err) + + s.True(shapeTags["Transcribed_Subtitle_SRT"]) + s.True(shapeTags["transcription_json"]) + s.True(shapeTags["sub_eng_srt"]) +} + +// A failing sidecar import job fails the workflow instead of being swallowed. +func (s *ImportSubtitlesTestSuite) Test_ImportSubtitlesSidecarJobFails() { + input := ImportSubtitlesInput{ + VXID: "VX-123", + Language: "no", + Subtitles: Transcription{Segments: []Segment{ + {Start: 0.0, End: 1.0, Text: "Hei"}, + }}, + } + + s.env.OnActivity(activities.Util.CreateFolder, mock.Anything, mock.Anything).Return("", nil) + s.env.OnActivity(activities.Util.WriteFile, mock.Anything, mock.Anything).Return("", nil) + s.env.OnActivity(activities.Vidispine.ImportFileAsShapeActivity, mock.Anything, mock.Anything).Return(&vsactivity.ImportFileResult{JobID: "job-shape"}, nil) + s.env.OnActivity(activities.Vidispine.DeleteMetadataGroupInstancesActivity, mock.Anything, mock.Anything).Return(&vsactivity.DeleteMetadataGroupResult{}, nil) + s.env.OnActivity(activities.Vidispine.ImportFileAsSidecarActivity, mock.Anything, mock.Anything).Return(&vsactivity.ImportFileAsSidecarResult{JobID: "job-sidecar"}, nil) + s.env.OnActivity(activities.Vidispine.JobCompleteOrErr, mock.Anything, vsactivity.WaitForJobCompletionParams{JobID: "job-shape"}).Return(true, nil) + s.env.OnActivity(activities.Vidispine.JobCompleteOrErr, mock.Anything, vsactivity.WaitForJobCompletionParams{JobID: "job-sidecar"}).Return(false, assert.AnError) + + s.env.ExecuteWorkflow(ImportSubtitles, input) + s.True(s.env.IsWorkflowCompleted()) + err := s.env.GetWorkflowError() + s.Error(err) + s.Contains(err.Error(), "sidecar import job failed") } // Given a path instead of the transcription, the workflow reads it and behaves @@ -190,6 +223,10 @@ func (s *ImportSubtitlesTestSuite) Test_ImportSubtitlesFromFile() { Data: []byte("1\n00:00:00,000 --> 00:00:01,000\nHello\n\n2\n00:00:01,500 --> 00:00:02,500\nWorld\n\n"), }).Return("", nil).Once() + s.env.OnActivity(activities.Vidispine.DeleteMetadataGroupInstancesActivity, mock.Anything, vsactivity.DeleteMetadataGroupParams{ + VXID: vxid, + Group: "stl_subtitle", + }).Return(&vsactivity.DeleteMetadataGroupResult{}, nil).Once() s.env.OnActivity(activities.Vidispine.ImportFileAsSidecarActivity, mock.Anything, mock.Anything).Return(nil, nil) s.env.OnActivity(activities.Vidispine.JobCompleteOrErr, mock.Anything, mock.Anything).Return(true, nil) diff --git a/workflows/misc/import_sidecar_subtitle.go b/workflows/misc/import_sidecar_subtitle.go index dfa04a97..66500fa8 100644 --- a/workflows/misc/import_sidecar_subtitle.go +++ b/workflows/misc/import_sidecar_subtitle.go @@ -1,9 +1,12 @@ package miscworkflows import ( + "fmt" + "github.com/bcc-code/bcc-media-flows/activities" vsactivity "github.com/bcc-code/bcc-media-flows/activities/vidispine" "github.com/bcc-code/bcc-media-flows/paths" + "github.com/bcc-code/bcc-media-flows/services/vidispine/vscommon" wfutils "github.com/bcc-code/bcc-media-flows/utils/workflows" "go.temporal.io/sdk/workflow" ) @@ -27,6 +30,17 @@ func ImportSidecarSubtitle(ctx workflow.Context, params ImportSidecarSubtitleInp ctx = workflow.WithActivityOptions(ctx, wfutils.GetDefaultActivityOptions()) + // Vidispine resolves the subtitle group by name during sidecar import, and + // stale instances from earlier imports make that fail with "ambiguous path + // to group: stl_subtitle" — so remove them first. + err := wfutils.Execute(ctx, activities.Vidispine.DeleteMetadataGroupInstancesActivity, vsactivity.DeleteMetadataGroupParams{ + VXID: params.VXID, + Group: vscommon.GroupStlSubtitle, + }).Wait(ctx) + if err != nil { + return fmt.Errorf("removing existing %s metadata failed: %w", vscommon.GroupStlSubtitle, err) + } + return wfutils.Execute(ctx, activities.Vidispine.ImportFileAsSidecarActivity, vsactivity.ImportSubtitleAsSidecarParams{ AssetID: params.VXID, FilePath: params.FilePath, diff --git a/workflows/misc/import_sidecar_subtitle_test.go b/workflows/misc/import_sidecar_subtitle_test.go index 905d50b2..4099f139 100644 --- a/workflows/misc/import_sidecar_subtitle_test.go +++ b/workflows/misc/import_sidecar_subtitle_test.go @@ -24,6 +24,11 @@ func (s *ImportSidecarSubtitleTestSuite) Test_ImportsTheSubtitle() { srtPath := paths.MustParse("/mnt/temp/workflows/transcript.srt") + env.OnActivity(activities.Vidispine.DeleteMetadataGroupInstancesActivity, mock.Anything, vsactivity.DeleteMetadataGroupParams{ + VXID: "VX-1", + Group: "stl_subtitle", + }).Once().Return(&vsactivity.DeleteMetadataGroupResult{}, nil) + var got vsactivity.ImportSubtitleAsSidecarParams env.OnActivity(activities.Vidispine.ImportFileAsSidecarActivity, mock.Anything, mock.MatchedBy( func(input vsactivity.ImportSubtitleAsSidecarParams) bool { @@ -49,6 +54,8 @@ func (s *ImportSidecarSubtitleTestSuite) Test_ImportsTheSubtitle() { func (s *ImportSidecarSubtitleTestSuite) Test_ReportsActivityFailure() { env := s.NewTestWorkflowEnvironment() + env.OnActivity(activities.Vidispine.DeleteMetadataGroupInstancesActivity, mock.Anything, mock.Anything). + Return(&vsactivity.DeleteMetadataGroupResult{}, nil) env.OnActivity(activities.Vidispine.ImportFileAsSidecarActivity, mock.Anything, mock.Anything). Return(nil, errors.New("vidispine rejected the sidecar")) diff --git a/workflows/misc/import_subs.go b/workflows/misc/import_subs.go index 0fd6976b..95e30690 100644 --- a/workflows/misc/import_subs.go +++ b/workflows/misc/import_subs.go @@ -7,6 +7,7 @@ import ( "github.com/bcc-code/bcc-media-flows/services/telegram" vsactivity "github.com/bcc-code/bcc-media-flows/activities/vidispine" + "github.com/bcc-code/bcc-media-flows/services/vidispine/vscommon" wfutils "github.com/bcc-code/bcc-media-flows/utils/workflows" "github.com/bcc-code/bcc-media-flows/activities" @@ -92,10 +93,13 @@ func doImportSubtitlesFromSubtrans(ctx workflow.Context, params ImportSubtitlesF langs = append(langs, lang) - _ = wfutils.Execute(ctx, activities.Vidispine.WaitForJobCompletion, vsactivity.WaitForJobCompletionParams{ + err = wfutils.Execute(ctx, activities.Vidispine.WaitForJobCompletion, vsactivity.WaitForJobCompletionParams{ JobID: jobRes.JobID, SleepTime: 10, }).Wait(ctx) + if err != nil { + return fmt.Errorf("waiting for subtitle shape import job for %s failed: %w", lang, err) + } } wfutils.SendTelegramText( @@ -104,6 +108,18 @@ func doImportSubtitlesFromSubtrans(ctx workflow.Context, params ImportSubtitlesF fmt.Sprintf("Sub import for VXID: %s finished (%s). Starting preview import.", params.VXID, strings.Join(langs, ", ")), ) + // Vidispine resolves the subtitle group by name during sidecar import, and + // stale instances from earlier imports make that fail with "ambiguous path + // to group: stl_subtitle". Clean once before the loop — cleaning per + // language would wipe the cues the previous iteration just imported. + err = wfutils.Execute(ctx, activities.Vidispine.DeleteMetadataGroupInstancesActivity, vsactivity.DeleteMetadataGroupParams{ + VXID: params.VXID, + Group: vscommon.GroupStlSubtitle, + }).Wait(ctx) + if err != nil { + return fmt.Errorf("removing existing %s metadata failed: %w", vscommon.GroupStlSubtitle, err) + } + for _, lang := range subsKeys { sub := subsList[lang] lang := strings.ToLower(lang) @@ -123,10 +139,13 @@ func doImportSubtitlesFromSubtrans(ctx workflow.Context, params ImportSubtitlesF continue } - _ = wfutils.Execute(ctx, activities.Vidispine.WaitForJobCompletion, vsactivity.WaitForJobCompletionParams{ + err = wfutils.Execute(ctx, activities.Vidispine.WaitForJobCompletion, vsactivity.WaitForJobCompletionParams{ JobID: jobRes.JobID, SleepTime: 10, }).Wait(ctx) + if err != nil { + return fmt.Errorf("waiting for subtitle sidecar import job for %s failed: %w", lang, err) + } } return nil diff --git a/workflows/misc/import_subs_test.go b/workflows/misc/import_subs_test.go new file mode 100644 index 00000000..82aee4d3 --- /dev/null +++ b/workflows/misc/import_subs_test.go @@ -0,0 +1,90 @@ +package miscworkflows + +import ( + "testing" + + "github.com/bcc-code/bcc-media-flows/activities" + vsactivity "github.com/bcc-code/bcc-media-flows/activities/vidispine" + "github.com/bcc-code/bcc-media-flows/paths" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/suite" + "go.temporal.io/sdk/testsuite" +) + +type ImportSubsTestSuite struct { + suite.Suite + testsuite.WorkflowTestSuite +} + +func (s *ImportSubsTestSuite) mockSubtrans(env *testsuite.TestWorkflowEnvironment, vxid string, subs map[string]paths.Path) { + env.OnActivity(activities.Util.GetSubtransIDActivity, mock.Anything, activities.GetSubtransIDInput{ + VXID: vxid, + NoSubsOK: true, + }).Return(&activities.GetSubtransIDOutput{SubtransID: "story-1"}, nil) + env.OnActivity(activities.Util.GetSubtitlesActivity, mock.Anything, mock.Anything).Return(subs, nil) + env.OnActivity(activities.Util.SendTelegramMessage, mock.Anything, mock.Anything).Return(nil, nil).Maybe() +} + +// Cleanup of stl_subtitle runs exactly once even with multiple languages, and +// every language gets both a shape and a sidecar import. +func (s *ImportSubsTestSuite) Test_CleansOnceForMultipleLanguages() { + env := s.NewTestWorkflowEnvironment() + vxid := "VX-42" + + subs := map[string]paths.Path{ + "nor": paths.MustParse("/mnt/temp/subs/nor.srt"), + "deu": paths.MustParse("/mnt/temp/subs/deu.srt"), + } + s.mockSubtrans(env, vxid, subs) + + env.OnActivity(activities.Vidispine.ImportFileAsShapeActivity, mock.Anything, mock.Anything). + Return(&vsactivity.ImportFileResult{JobID: "job-shape"}, nil).Times(2) + env.OnActivity(activities.Vidispine.DeleteMetadataGroupInstancesActivity, mock.Anything, vsactivity.DeleteMetadataGroupParams{ + VXID: vxid, + Group: "stl_subtitle", + }).Return(&vsactivity.DeleteMetadataGroupResult{DeletedInstances: 3}, nil).Once() + env.OnActivity(activities.Vidispine.ImportFileAsSidecarActivity, mock.Anything, mock.Anything). + Return(&vsactivity.ImportFileAsSidecarResult{JobID: "job-sidecar"}, nil).Times(2) + env.OnActivity(activities.Vidispine.WaitForJobCompletion, mock.Anything, mock.Anything). + Return(nil, nil).Times(4) + + env.ExecuteWorkflow(ImportSubtitlesFromSubtrans, ImportSubtitlesFromSubtransInput{VXID: vxid}) + + s.True(env.IsWorkflowCompleted()) + s.NoError(env.GetWorkflowError()) + env.AssertExpectations(s.T()) +} + +// A failing sidecar job now fails the workflow instead of being discarded. +func (s *ImportSubsTestSuite) Test_SidecarJobFailureFailsWorkflow() { + env := s.NewTestWorkflowEnvironment() + vxid := "VX-42" + + subs := map[string]paths.Path{ + "nor": paths.MustParse("/mnt/temp/subs/nor.srt"), + } + s.mockSubtrans(env, vxid, subs) + + env.OnActivity(activities.Vidispine.ImportFileAsShapeActivity, mock.Anything, mock.Anything). + Return(&vsactivity.ImportFileResult{JobID: "job-shape"}, nil) + env.OnActivity(activities.Vidispine.DeleteMetadataGroupInstancesActivity, mock.Anything, mock.Anything). + Return(&vsactivity.DeleteMetadataGroupResult{}, nil) + env.OnActivity(activities.Vidispine.ImportFileAsSidecarActivity, mock.Anything, mock.Anything). + Return(&vsactivity.ImportFileAsSidecarResult{JobID: "job-sidecar"}, nil) + env.OnActivity(activities.Vidispine.WaitForJobCompletion, mock.Anything, vsactivity.WaitForJobCompletionParams{JobID: "job-shape", SleepTime: 10}). + Return(nil, nil) + env.OnActivity(activities.Vidispine.WaitForJobCompletion, mock.Anything, vsactivity.WaitForJobCompletionParams{JobID: "job-sidecar", SleepTime: 10}). + Return(nil, assert.AnError) + + env.ExecuteWorkflow(ImportSubtitlesFromSubtrans, ImportSubtitlesFromSubtransInput{VXID: vxid}) + + s.True(env.IsWorkflowCompleted()) + err := env.GetWorkflowError() + s.Error(err) + s.Contains(err.Error(), "sidecar import job") +} + +func TestImportSubsTestSuite(t *testing.T) { + suite.Run(t, new(ImportSubsTestSuite)) +}