Skip to content

Commit

Permalink
Fix reversed ordering of arguments (#1648)
Browse files Browse the repository at this point in the history
The source manager initialization function was defined as `sourceID`
followed by `jobID`, while the source initialization function is the
reverse. This is confusing and easy to mix up since the parameters are
the same type.

This commit adds a test to make sure the source manager initializes in
the correct order, but it doesn't prevent the library user to make the
same mistake. We may want to consider using different types.
  • Loading branch information
mcastorina authored Aug 22, 2023
1 parent 9a13c74 commit 5cfbde7
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 2 deletions.
4 changes: 2 additions & 2 deletions pkg/sources/source_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type handle int64

// SourceInitFunc is a function that takes a source and job ID and returns an
// initialized Source.
type SourceInitFunc func(ctx context.Context, sourceID int64, jobID int64) (Source, error)
type SourceInitFunc func(ctx context.Context, jobID, sourceID int64) (Source, error)

// sourceInfo is an aggregate struct to store source information provided on
// initialization.
Expand Down Expand Up @@ -228,7 +228,7 @@ func (s *SourceManager) run(ctx context.Context, handle handle, jobID int64, rep
report.ReportError(Fatal{err})
return Fatal{err}
}
source, err := sourceInfo.initFunc(ctx, int64(handle), jobID)
source, err := sourceInfo.initFunc(ctx, jobID, int64(handle))
if err != nil {
report.ReportError(Fatal{err})
return Fatal{err}
Expand Down
39 changes: 39 additions & 0 deletions pkg/sources/source_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,3 +245,42 @@ func TestSourceManagerContextCancelled(t *testing.T) {
report := ref.Snapshot()
assert.Error(t, report.FatalError())
}

type DummyAPI struct {
registerSource func(context.Context, string, sourcespb.SourceType) (int64, error)
getJobID func(context.Context, int64) (int64, error)
}

func (api DummyAPI) RegisterSource(ctx context.Context, name string, kind sourcespb.SourceType) (int64, error) {
return api.registerSource(ctx, name, kind)
}

func (api DummyAPI) GetJobID(ctx context.Context, id int64) (int64, error) {
return api.getJobID(ctx, id)
}

func TestSourceManagerJobAndSourceIDs(t *testing.T) {
mgr := NewManager(WithAPI(DummyAPI{
registerSource: func(context.Context, string, sourcespb.SourceType) (int64, error) {
return 1337, nil
},
getJobID: func(context.Context, int64) (int64, error) {
return 9001, nil
},
}))
var (
initializedJobID int64
initializedSourceID int64
)
handle, err := mgr.Enroll(context.Background(), "dummy", 1337,
func(ctx context.Context, jobID, sourceID int64) (Source, error) {
initializedJobID = jobID
initializedSourceID = sourceID
return nil, fmt.Errorf("ignore")
})
assert.NoError(t, err)

_, _ = mgr.Run(context.Background(), handle)
assert.Equal(t, int64(1337), initializedSourceID)
assert.Equal(t, int64(9001), initializedJobID)
}

0 comments on commit 5cfbde7

Please sign in to comment.