feat(feeds): remove stream ID lookup when approving data streams jobs#22508
Open
jkongie wants to merge 1 commit into
Open
feat(feeds): remove stream ID lookup when approving data streams jobs#22508jkongie wants to merge 1 commit into
jkongie wants to merge 1 commit into
Conversation
Contributor
|
I see you updated files related to
|
Contributor
|
✅ No conflicts with other open PRs targeting |
product-security-plaid-production
Bot
requested review from
ChrisAmora,
DylanTinianov and
patrickhuie19
DimitriosNaikopoulos
previously approved these changes
May 18, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Risk Rating: MEDIUM
Adjusts feeds job-proposal approval behavior for data streams so stream jobs are matched solely by externalJobID (removing the prior stream-ID-based “force replace” path), aligning streams with other job types that are only matchable by external job ID.
Changes:
- Removed
FindJobIDByStreamIDfrom the job ORM interface/implementation and its generated mock. - Updated
ApproveSpecto no longer look up existing stream jobs bystreamID(external job ID only). - Updated stream approval tests to stop expecting stream-ID lookups and to cover the “same streamID, different external job ID” approval scenario.
Targeted areas needing scrupulous human review:
core/services/feeds/service.goapproval/force-replace logic forjob.Stream, especially compatibility with existing DB constraints onjobs.stream_idand the resulting runtime behavior on collisions.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/services/job/orm.go | Removes stream-ID lookup API from job ORM. |
| core/services/job/mocks/orm.go | Updates generated job ORM mock to match interface changes. |
| core/services/feeds/service.go | Stops stream job collision checks by stream ID during approval. |
| core/services/feeds/service_test.go | Updates/reshapes stream approval tests to reflect new matching behavior. |
Files not reviewed (1)
- core/services/job/mocks/orm.go: Language not supported
Comments suppressed due to low confidence (2)
core/services/feeds/service_test.go:4046
- Same issue here:
GetSpecis called with only (ctx, id) in production code, but this expectation includes an extra argument and won't match at runtime. Update the mock expectation to only include ctx and the spec ID.
svc.orm.On("GetSpec", mock.Anything, cancelledSpec.ID, mock.Anything).Return(cancelledSpec, nil)
svc.orm.On("GetJobProposal", mock.Anything, jp.ID).Return(jp, nil)
svc.orm.On("ListSpecsByJobProposalIDs", mock.Anything, []int64{cancelledSpec.JobProposalID}).
core/services/feeds/service_test.go:4057
- Same mismatch again: this
GetSpecexpectation supplies 3 arguments, but the realGetSpec(ctx, id)call supplies 2. This will cause a testify/mock unexpected-call failure when the test runs.
before: func(svc *TestService) {
svc.orm.On("GetSpec", mock.Anything, cancelledSpec.ID, mock.Anything).Return(rejectedSpec, nil)
svc.orm.On("GetJobProposal", mock.Anything, jp.ID).Return(jp, nil)
Comment on lines
+1063
to
+1064
| case job.CRESettings, | ||
| job.Stream, |
Contributor
Author
There was a problem hiding this comment.
This is what we want to happen though.
The logic is determined by the Data Streams team on what they allow the node to run.
Am I missing something with this?
Match data streams (job.Stream) specs only by external job ID when approving job proposals, the same as CRESettings and CCV job types. Previously, a stream spec with a different external job ID but the same stream ID as a running job would trigger the force-replace path (delete + recreate). That auto-delete behavior is now removed; stream job uniqueness is determined exclusively by external job ID. OPDATA-5937
jkongie
force-pushed
the
OPDATA-5937/remove-data-streams-auto-delete
branch
from
4ae592d to
12e3a53
Compare
|
ro-tex
approved these changes
May 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Match data streams (job.Stream) specs only by external job ID when approving job proposals, the same as CRESettings and CCV job types. Previously, a stream spec with a different external job ID but the same stream ID as a running job would trigger the force-replace path (delete + recreate). That auto-delete behavior is now removed; stream job uniqueness is determined exclusively by external job ID.
OPDATA-5937