add asset package #48
Conversation
WalkthroughThis pull request introduces a comprehensive set of domain and infrastructure components for managing assets in a software system. The changes span multiple packages within the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 57
🧹 Nitpick comments (114)
asset/assetdomain/integration/integration_type_test.go (1)
10-30: Consider adding more test cases for edge scenarios.To improve test coverage, consider adding test cases for:
- Empty string input
- Input with special characters
- Case sensitivity verification (e.g., "PUBLIC", "Private")
Here's a suggested addition to the test cases:
tests := []struct { name string input string want Type }{ // ... existing test cases ... + { + name: "empty string", + input: "", + want: TypePrivate, + }, + { + name: "uppercase", + input: "PUBLIC", + want: TypePrivate, // or TypePublic depending on intended behavior + }, + { + name: "special characters", + input: "public!@#", + want: TypePrivate, + }, }asset/assetdomain/project/publication_test.go (2)
9-26: Consider using table-driven tests for better maintainability.While the test cases are comprehensive, converting to a table-driven test would make it easier to add new test cases and improve readability.
Here's a suggested refactoring:
func TestNewPublication(t *testing.T) { - assert.Equal(t, &Publication{ - scope: PublicationScopePrivate, - assetPublic: false, - }, NewPublication(PublicationScopePrivate, false)) - assert.Equal(t, &Publication{ - scope: PublicationScopeLimited, - assetPublic: true, - }, NewPublication(PublicationScopeLimited, true)) - assert.Equal(t, &Publication{ - scope: PublicationScopePublic, - assetPublic: false, - }, NewPublication(PublicationScopePublic, false)) - assert.Equal(t, &Publication{ - scope: PublicationScopePrivate, - assetPublic: true, - }, NewPublication("", true)) + tests := []struct { + name string + scope string + assetPublic bool + want *Publication + }{ + { + name: "private scope with private assets", + scope: PublicationScopePrivate, + assetPublic: false, + want: &Publication{scope: PublicationScopePrivate, assetPublic: false}, + }, + { + name: "limited scope with public assets", + scope: PublicationScopeLimited, + assetPublic: true, + want: &Publication{scope: PublicationScopeLimited, assetPublic: true}, + }, + { + name: "public scope with private assets", + scope: PublicationScopePublic, + assetPublic: false, + want: &Publication{scope: PublicationScopePublic, assetPublic: false}, + }, + { + name: "empty scope defaults to private with public assets", + scope: "", + assetPublic: true, + want: &Publication{scope: PublicationScopePrivate, assetPublic: true}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := NewPublication(tt.scope, tt.assetPublic) + assert.Equal(t, tt.want, got) + }) + } }
28-31: Consider adding test cases for all scope values.While the test covers the default and public scopes, it would be beneficial to test the limited scope as well for complete coverage.
Add this test case:
func TestPublication_Scope(t *testing.T) { assert.Equal(t, PublicationScopePrivate, (&Publication{}).Scope()) assert.Equal(t, PublicationScopePublic, (&Publication{scope: PublicationScopePublic}).Scope()) + assert.Equal(t, PublicationScopeLimited, (&Publication{scope: PublicationScopeLimited}).Scope()) }asset/assetdomain/thread/thread_test.go (3)
12-26: Improve test readability with better variable namingConsider renaming the
gotvariable tothreadto better represent what it is, and make the test setup more explicit.- got := Thread{ + thread := Thread{ id: thid, workspace: wid, comments: c, } - assert.Equal(t, thid, got.ID()) - assert.Equal(t, wid, got.Workspace()) - assert.Equal(t, c, got.Comments()) + assert.Equal(t, thid, thread.ID()) + assert.Equal(t, wid, thread.Workspace()) + assert.Equal(t, c, thread.Comments())
39-72: Consider using table-driven tests for better coverageThe current tests cover basic scenarios, but table-driven tests would make it easier to test more edge cases and maintain the tests.
Example refactor for HasComment:
func TestThread_HasComment(t *testing.T) { tests := []struct { name string thread *Thread comment *Comment want bool }{ { name: "nil thread", thread: nil, comment: NewComment(NewCommentID(), operator.OperatorFromUser(NewUserID()), "test"), want: false, }, // Add more test cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := tt.thread.HasComment(tt.comment.id) assert.Equal(t, tt.want, got) }) } }
1-137: Consider adding benchmarks and documentationThe test suite is comprehensive but could be enhanced with:
- Benchmark tests for performance-critical operations
- Doc comments explaining the purpose of each test function
- Test coverage for concurrent operations if the Thread type is meant to be thread-safe
Example benchmark:
func BenchmarkThread_AddComment(b *testing.B) { thread := &Thread{ id: NewID(), workspace: accountdomain.NewWorkspaceID(), } b.ResetTimer() for i := 0; i < b.N; i++ { comment := NewComment(NewCommentID(), operator.OperatorFromUser(NewUserID()), "test") _ = thread.AddComment(comment) } }asset/assetinfrastructure/assetmemory/event_test.go (2)
18-42: Consider enhancing test coverage and maintainability.While the current test cases cover basic functionality, consider the following improvements:
- Extract test data setup into helper functions to reduce duplication with TestEvent_Save
- Add test cases for edge cases:
- nil context
- nil event ID
- Convert to table-driven tests for better maintainability
Here's a suggested refactor:
+func setupTestData(t *testing.T) (*user.User, *asset.Asset, *event.Event[any]) { + now := time.Now() + u := user.New().NewID().Email("[email protected]").Name("John").MustBuild() + a := asset.New().NewID().Project(project.NewID()).Size(100).NewUUID(). + CreatedByUser(u.ID()).Thread(id.NewThreadID()).MustBuild() + ev := event.New[any]().ID(event.NewID()).Timestamp(now).Type(event.AssetCreate). + Operator(operator.OperatorFromUser(u.ID())).Object(a).MustBuild() + return u, a, ev +} func TestEvent_FindByID(t *testing.T) { - now := time.Now() - u := user.New().NewID().Email("[email protected]").Name("John").MustBuild() - a := asset.New().NewID().Project(project.NewID()).Size(100).NewUUID(). - CreatedByUser(u.ID()).Thread(id.NewThreadID()).MustBuild() - eID1 := event.NewID() - ev := event.New[any]().ID(eID1).Timestamp(now).Type(event.AssetCreate). - Operator(operator.OperatorFromUser(u.ID())).Object(a).MustBuild() + tests := []struct { + name string + ctx context.Context + eventID event.ID + setup func(*Event) + want *event.Event[any] + wantErr error + }{ + { + name: "success", + ctx: context.Background(), + setup: func(r *Event) { + _, _, ev := setupTestData(t) + r.Save(context.Background(), ev) + }, + want: ev, + wantErr: nil, + }, + { + name: "not found", + ctx: context.Background(), + eventID: event.NewID(), + setup: func(r *Event) {}, + want: nil, + wantErr: rerror.ErrNotFound, + }, + { + name: "nil context", + ctx: nil, + eventID: event.NewID(), + setup: func(r *Event) {}, + want: nil, + wantErr: rerror.ErrInvalidContext, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := NewEvent() + tt.setup(r) + got, err := r.FindByID(tt.ctx, tt.eventID) + assert.Equal(t, tt.wantErr, err) + assert.Equal(t, tt.want, got) + }) + } }
1-61: Consider implementing a test helper package.Given the complexity of creating test data (users, assets, events) and the likelihood of needing these helpers across multiple test files, consider creating a dedicated test helper package. This would:
- Reduce duplication across test files
- Ensure consistent test data creation
- Make tests more maintainable
- Provide reusable assertions for common checks
Consider creating
assetmemory/testing/factory.gowith helper functions:package testing // Factory provides methods to create test entities type Factory struct { t *testing.T } func NewFactory(t *testing.T) *Factory { return &Factory{t: t} } func (f *Factory) NewUser() *user.User { return user.New().NewID().Email("[email protected]").Name("Test User").MustBuild() } func (f *Factory) NewAsset(u *user.User) *asset.Asset { return asset.New().NewID().Project(project.NewID()).Size(100).NewUUID(). CreatedByUser(u.ID()).Thread(id.NewThreadID()).MustBuild() } func (f *Factory) NewEvent(u *user.User, a *asset.Asset) *event.Event[any] { return event.New[any]().ID(event.NewID()).Timestamp(time.Now()). Type(event.AssetCreate).Operator(operator.OperatorFromUser(u.ID())). Object(a).MustBuild() }asset/assetdomain/file/file.go (3)
17-22: Consider a size check initialization.
Currently, the File struct doesn't compute or store file size except when created from URL. If there's a potential need to handle size constraints (e.g., max file size limits on uploads), consider adding logic to get the size from the multipart part to unify size handling.
56-69: Handle redirect or advanced HTTP statuses.
The code currently does a simple GET and checks if StatusCode > 300. It might be worth following redirects or differentiating 4xx vs. 5xx errors for more targeted error messages. This can clarify errors for users (e.g., “404 Not Found” vs “500 Internal Server Error”), rather than a generic error.
74-79: Refine file name inference.
When inferring the file name from the Content-Disposition header, consider fallback strategies for unknown or maliciously crafted headers.asset/assetdomain/thread/thread.go (2)
51-58: Prevent in-place content overwrite.
When updating a comment, content is overwritten without versioning or undo support. If audit trails are required, consider storing older revisions or timestamps in order to track changes historically.
60-69: Optimize the delete operation.
You call th.Comments() multiple times, resulting in repeated array clones. Although not a big performance issue for small collections, repeated clones may add overhead. Consider storing the index once and operating directly on th.comments.asset/assetinfrastructure/assetmemory/asset.go (3)
17-21: Consider lock granularity.
All assets are stored in a SyncMap, which is good for concurrency. However, if frequent writes or large sets of data are expected, you might explore finer-grained locking or a sharded approach to avoid potential bottlenecks.
57-83: Check pagination queries with filters.
While the code returns everything in one pass before constructing pagination info, for very large result sets, consider partial retrieval or chunking. This could be beneficial if you plan to scale.
86-97: Log persist events.
When saving an asset, it might be helpful to log the event, including the ID or relevant project info, to assist in debugging or auditing.asset/assetdomain/asset/preview_type.go (2)
73-81: Log or handle unknown content types.
When a file content type is unrecognized, the code simply returns PreviewTypeUnknown. You might want to log or track these instances for debugging, especially if you expect certain content types that might occasionally be missed.
83-94: Extend recognized MIME types.
“text/csv” is recognized, but other plain text variants or CSV-like types (e.g., “text/tab-separated-values”) might exist. Consider expanding coverage or letting users map custom content types.asset/assetdomain/project/project.go (2)
16-19: Validate alias string length more precisely.
While the current regular expression enforces the length range (5-32), consider documenting the rationale for these limits (5 and 32) directly in the code or a comment for better maintainability.
108-115: Ensure alias validation errors are handled consistently.
Currently, a simple boolean is used to validate the alias, returning an error if the pattern check fails. You might consider logging or providing more context in the error message to help users identify the invalid alias reason.asset/asset.graphql (1)
3-16: Redundancy of both 'project' and 'projectId' fields.
Including both fields may be redundant. Consider if the 'project' object can be resolved via 'projectId' to reduce potential schema clutter.asset/assetdomain/asset/file.go (2)
105-117: Potential infinite recursion.
While FlattenChildren() recursively collates children, ensure there's no circular reference scenario, or it could cause infinite recursion. If this is guaranteed not to happen by design, consider adding a protective check or an explicit comment stating the assumption.
126-164: Memory usage caution in FoldFiles.
For large file sets, the approach of sorting and building a multi-level directory structure in memory might consume significant resources. If the expected usage is large, consider streaming or chunked approaches to folder construction.asset/assetdomain/integration/integration.go (2)
86-92: Return cloned or safely handled slices.
The Webhooks() getter returns the original slice, allowing direct modification by the caller. Consider returning a copy to maintain immutability.-func (i *Integration) Webhooks() []*Webhook { - return i.webhooks +func (i *Integration) Webhooks() []*Webhook { + return slices.Clone(i.webhooks) }
163-175: Add test coverage for randomString edge cases.
The randomString function is a key mechanism for security tokens. Consider adding tests for zero or negative input, or for verifying the distribution of generated characters.asset/assetinfrastructure/assetmemory/project.go (5)
46-48: Validate workspace read access more explicitly.
Although the code checks r.f.CanRead(v.Workspace()), consider returning an explicit error such as repo.ErrOperationDenied if the user doesn’t have read access, instead of silently filtering matching entries.
92-111: Clarify project ID vs alias usage.
When both pid and alias are empty (lines 99-101), you immediately return rerror.ErrNotFound. Consider returning an error that clarifies invalid input vs. resource not found, since no valid query was provided. This can help distinguish between a true “not found” scenario and invalid parameters.
137-144: Centralize error handling for CountByWorkspace.
The function returns 0 if the workspace is unreadable and 0 if the project is not found, which may merge two different cases (no projects vs. no read access). Returning a permission-denied error or differentiating these scenarios could improve observability.
165-169: Unify error messages for remove operation.
The Remove method returns rerror.ErrNotFound when the user lacks write access, making it ambiguous whether the resource truly doesn’t exist or the user just can’t modify it. Consider returning a clearer permission error instead.
172-178: Potential concurrency testing suggestion.
You provide MockProjectNow and SetProjectError for testing. You could also consider concurrency tests to ensure the underlying SyncMap usage remains safe under parallel read/writes.asset/assetinfrastructure/assetfs/file.go (2)
45-53: Add logging or metrics for file read failures.
When fileUUID or filename is empty (line 47), you return an error. Logging could help diagnose usage issues.
127-133: Unsupported operations as part of the interface.
IssueUploadAssetLink and UploadedAsset return ErrUnsupportedOperation. Confirm if these are guaranteed by the domain or if stubs are expected to be eventually implemented.asset/assetdomain/integration/integration_test.go (3)
84-127: Adequate coverage of constructor-like logic.
Testing that retrieved timestamps mirror the Integration ID timestamp is a neat approach, but consider also verifying edge cases, such as future-dated IDs (invalid) or explicit checks for zero/uninitialized timestamps.
703-773: Comprehensive webhook set capability.
The tests verify multiple scenarios (empty slice, non-empty slice). Looks good. Consider adding boundary checks on ID validity if required in production code (e.g., ensure no duplicates in the slice).
1310-1316: Token generation approach is straightforward.
Users can guess that "secret_" is the prefix. If security is a concern, consider a more random prefix or hashing approach. Alternatively, if secrecy is not a priority, this is fine.asset/assetdomain/integration/integration_type.go (1)
13-22: Potential edge case in TypeFrom.
When the input is unrecognized, it defaults to TypePrivate. This is acceptable but be aware that invalid inputs might lead to an unintended private type. Consider adding logging or an error if unexpected values occur.asset/assetdomain/asset/map.go (1)
13-20: Potential performance note for large maps.
When iterating over a large IDList, repeated map lookups are O(1) each, but can add up. This is likely fine for moderate data sizes.asset/assetdomain/operator/id.go (1)
13-14: Consider documentation updates.
Providing doc comments for ErrInvalidID and NewIntegrationID can help future maintainers understand their usage.asset/assetdomain/thread/common.go (1)
8-12: Consider enhancing error messages with more contextWhile the error messages are clear, they could be more specific to help with debugging and user experience.
Consider this enhancement:
- ErrNoWorkspaceID = rerror.NewE(i18n.T("workspace id is required")) + ErrNoWorkspaceID = rerror.NewE(i18n.T("workspace id is required for thread operations")) - ErrCommentAlreadyExist = rerror.NewE(i18n.T("comment already exist in this thread")) + ErrCommentAlreadyExist = rerror.NewE(i18n.T("comment with this ID already exists in the thread")) - ErrCommentDoesNotExist = rerror.NewE(i18n.T("comment does not exist in this thread")) + ErrCommentDoesNotExist = rerror.NewE(i18n.T("comment with the specified ID does not exist in this thread"))asset/assetdomain/asset/common.go (1)
8-14: Consider enhancing error messages for better debuggingWhile the error messages are clear, they could be more descriptive to aid in debugging. Consider adding more context about where and why these errors occur.
var ( - ErrNoProjectID = rerror.NewE(i18n.T("projectID is required")) - ErrZeroSize = rerror.NewE(i18n.T("file size cannot be zero")) - ErrNoUser = rerror.NewE(i18n.T("createdBy is required")) - ErrNoThread = rerror.NewE(i18n.T("thread is required")) - ErrNoUUID = rerror.NewE(i18n.T("uuid is required")) + ErrNoProjectID = rerror.NewE(i18n.T("asset creation failed: projectID is required")) + ErrZeroSize = rerror.NewE(i18n.T("asset validation failed: file size cannot be zero")) + ErrNoUser = rerror.NewE(i18n.T("asset creation failed: createdBy user is required")) + ErrNoThread = rerror.NewE(i18n.T("asset operation failed: thread reference is required")) + ErrNoUUID = rerror.NewE(i18n.T("asset identification failed: uuid is required")) )asset/assetdomain/thread/list.go (1)
8-20: Add Map method for consistencyThe Map method mentioned in the AI summary is missing from the implementation. This method would be useful for converting the list to a map keyed by thread IDs.
Would you like me to provide an implementation for the Map method to maintain consistency with other domain models?
asset/assetdomain/asset/list.go (1)
1-29: Well-structured implementation with good immutability practices.The implementation demonstrates good practices:
- Immutable operations returning new instances
- Efficient use of modern Go libraries
- Type-safe operations with proper error handling
Consider adding these methods for enhanced functionality:
Filter(predicate func(*Asset) bool) Listfor filtered viewsReduce(fn func(*Asset, *Asset) *Asset) *Assetfor aggregationsasset/assetdomain/integration/id.go (1)
9-12: Consider generating ID types to reduce boilerplate.The ID type definitions follow a consistent pattern across domains. Consider using code generation to maintain consistency and reduce duplication.
asset/assetdomain/asset/upload.go (1)
31-33: Improve time handling in Expired methodThe current implementation could be enhanced to:
- Handle nil time.Time values
- Use time.Now() as default if no time is provided
- Add documentation explaining the behavior
Here's a suggested improvement:
+// Expired checks if the upload has expired. +// If t is zero value, time.Now() is used. func (u *Upload) Expired(t time.Time) bool { - return t.After(u.expiresAt) + checkTime := t + if checkTime.IsZero() { + checkTime = time.Now() + } + return checkTime.After(u.expiresAt) }asset/assetdomain/project/list_test.go (1)
9-27: Enhance test coverage for List_SortByIDThe current test could be improved by:
- Adding test case for nil List
- Testing empty List
- Testing List with duplicate IDs
Here's a suggested addition:
func TestList_SortByID(t *testing.T) { + tests := []struct { + name string + list List + want List + }{ + { + name: "nil list", + list: nil, + want: nil, + }, + { + name: "empty list", + list: List{}, + want: List{}, + }, // existing test case... + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.list.SortByID() + assert.Equal(t, tt.want, got) + }) + } }asset/assetdomain/integration/list.go (2)
10-10: Add documentation for public List typeThe List type is exported but lacks documentation explaining its purpose and usage.
Add documentation:
+// List represents a collection of Integration pointers that can be sorted and filtered. +// It provides methods for sorting by ID, cloning, and filtering active webhooks. type List []*Integration
3-8: Consider package organization and dependenciesThe integration package depends on multiple external packages. Consider:
- Documenting the rationale for using
lovs standard library- Moving common slice operations to a shared utility package
asset/assetdomain/thread/list_test.go (2)
10-28: Enhance test coverage for SortByIDWhile the basic sorting test is good, consider adding edge cases:
- Empty list
- Single item list
- List with duplicate IDs
- Nil list
func TestList_SortByID(t *testing.T) { + t.Run("normal case", func(t *testing.T) { id1 := NewID() id2 := NewID() list := List{ &Thread{id: id2}, &Thread{id: id1}, } res := list.SortByID() assert.Equal(t, List{ &Thread{id: id1}, &Thread{id: id2}, }, res) assert.Equal(t, List{ &Thread{id: id2}, &Thread{id: id1}, }, list) + }) + + t.Run("edge cases", func(t *testing.T) { + assert.Empty(t, List{}.SortByID()) + assert.Empty(t, List(nil).SortByID()) + + id := NewID() + singleList := List{&Thread{id: id}} + assert.Equal(t, singleList, singleList.SortByID()) + }) }
30-37: Add edge cases to Clone testConsider adding tests for edge cases in the Clone method:
- Empty list
- Nil list
- List with nil elements
func TestList_Clone(t *testing.T) { + t.Run("normal case", func(t *testing.T) { th := New().NewID().Workspace(accountdomain.NewWorkspaceID()).MustBuild() list := List{th} got := list.Clone() assert.Equal(t, list, got) assert.NotSame(t, list[0], got[0]) + }) + + t.Run("edge cases", func(t *testing.T) { + assert.Empty(t, List{}.Clone()) + assert.Empty(t, List(nil).Clone()) + + list := List{nil} + assert.Equal(t, list, list.Clone()) + }) }asset/assetdomain/asset/map_test.go (2)
14-14: Extract test asset creation into helper functionThe asset creation chain is duplicated and quite long. Consider extracting it into a helper function for better maintainability and readability.
+func createTestAsset(pid ProjectID, uid accountdomain.UserID) *Asset { + return New(). + NewID(). + Project(pid). + CreatedByUser(uid). + Size(1000). + Thread(NewThreadID()). + NewUUID(). + MustBuild() +} + func TestMap_List(t *testing.T) { pid := NewProjectID() uid := accountdomain.NewUserID() - a := New().NewID().Project(pid).CreatedByUser(uid).Size(1000).Thread(NewThreadID()).NewUUID().MustBuild() + a := createTestAsset(pid, uid) // ... rest of the test }Also applies to: 24-24
10-18: Enhance test coverage with edge casesBoth test functions would benefit from additional test cases:
- Map with multiple assets
- Invalid/non-existent IDs in ListFrom
- Empty ID list in ListFrom
- Map with nil asset values
func TestMap_List(t *testing.T) { + t.Run("test cases", func(t *testing.T) { pid := NewProjectID() uid := accountdomain.NewUserID() a := createTestAsset(pid, uid) + b := createTestAsset(pid, uid) - assert.Equal(t, List{a}, Map{a.ID(): a}.List()) - assert.Equal(t, List{}, Map(nil).List()) + tests := []struct { + name string + m Map + want List + }{ + {"single asset", Map{a.ID(): a}, List{a}}, + {"multiple assets", Map{a.ID(): a, b.ID(): b}, List{a, b}}, + {"nil map", nil, List{}}, + {"map with nil value", Map{a.ID(): nil}, List{}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, tt.m.List()) + }) + } + }) }Also applies to: 20-28
asset/assetdomain/asset/upload_builder.go (1)
7-9: Add documentation for the UploadBuilder typeConsider adding documentation comments to describe the purpose of the UploadBuilder and its usage pattern. This will improve code maintainability and help other developers understand how to use this builder correctly.
asset/assetdomain/operator/operator_test.go (1)
10-37: Consider adding more test cases for comprehensive coverageThe current test covers basic functionality but could benefit from additional test cases:
- Invalid user/integration IDs
- Empty/nil operators
- Invalid state combinations
Would you like me to provide examples of additional test cases?
asset/assetdomain/operator/operator.go (1)
5-9: Consider adding JSON/GORM tags for persistenceIf this Operator struct needs to be serialized or persisted, consider adding appropriate tags.
Example implementation:
type Operator struct { - user *accountdomain.UserID - integration *IntegrationID - isMachine bool + user *accountdomain.UserID `json:"user,omitempty" gorm:"column:user_id"` + integration *IntegrationID `json:"integration,omitempty" gorm:"column:integration_id"` + isMachine bool `json:"is_machine" gorm:"column:is_machine"` }asset/assetdomain/project/id.go (2)
35-40: Improve error handling in Alias methodThe Alias method silently returns nil for empty strings. Consider making this behavior more explicit.
Consider this improvement:
func (i IDOrAlias) Alias() *string { + if string(i) == "" { + return nil + } if string(i) != "" && i.ID() == nil { return lo.ToPtr(string(i)) } return nil }
31-33: Consider caching ID conversion resultThe ID() method performs conversion on every call, which could be inefficient if called frequently.
Consider implementing caching:
type IDOrAlias string +type idOrAliasCache struct { + id *ID + valid bool +} + +var idCache sync.Map // map[IDOrAlias]idOrAliasCache func (i IDOrAlias) ID() *ID { + if cache, ok := idCache.Load(i); ok { + return cache.(idOrAliasCache).id + } + id := IDFromRef(lo.ToPtr(string(i))) + idCache.Store(i, idOrAliasCache{id: id, valid: true}) return id }asset/assetdomain/thread/builder.go (1)
16-26: Consider enhancing validation in Build method.While the current validation checks for nil ID and workspace, consider adding validation for the comments slice to ensure it's not nil and that all comments are valid.
func (b *Builder) Build() (*Thread, error) { if b.th.id.IsNil() { return nil, ErrInvalidID } if b.th.workspace.IsNil() { return nil, ErrNoWorkspaceID } + if b.th.comments == nil { + b.th.comments = []*Comment{} + } + + for _, c := range b.th.comments { + if c == nil || c.ID().IsNil() { + return nil, ErrInvalidComment + } + } + return b.th, nil }asset/assetdomain/thread/id.go (2)
9-12: Add documentation for exported type aliases.These type aliases are part of the public API. Consider adding documentation comments to explain their purpose and usage.
+// ID represents a unique identifier for a Thread type ID = id.ThreadID +// CommentID represents a unique identifier for a Comment within a Thread type CommentID = id.CommentID +// UserID represents a unique identifier for a User type UserID = accountdomain.UserID +// WorkspaceID represents a unique identifier for a Workspace type WorkspaceID = id.WorkspaceID
14-32: Consider grouping related declarations.The ID-related functions could be organized better by grouping them based on their purpose (creation, validation, conversion).
+// Creation functions var NewID = id.NewThreadID var NewCommentID = id.NewCommentID var NewUserID = accountdomain.NewUserID var NewWorkspaceID = accountdomain.NewWorkspaceID +// Validation functions var MustID = id.MustThreadID var MustCommentID = id.MustCommentID var MustUserID = id.MustUserID var MustWorkspaceID = id.MustWorkspaceID +// Conversion functions var IDFrom = id.ThreadIDFrom var CommentIDFrom = id.CommentIDFrom var UserIDFrom = accountdomain.UserIDFrom var WorkspaceIDFrom = id.WorkspaceIDFrom var IDFromRef = id.ThreadIDFromRef var CommentIDFromRef = id.CommentIDFromRef var UserIDFromRef = accountdomain.UserIDFromRef var WorkspaceIDFromRef = id.WorkspaceIDFromRefasset/assetinfrastructure/assetmemory/event.go (1)
13-16: Consider adding methods for event querying and filtering.The current implementation only supports basic CRUD operations. Consider adding methods for:
- Listing all events
- Filtering events by type or timestamp
- Pagination support
asset/assetdomain/thread/comment_test.go (2)
29-33: Use public methods in testsThe test is directly accessing the private
contentfield. It's better to use public methods to maintain encapsulation.func TestComment_SetContent(t *testing.T) { comment := Comment{} comment.SetContent("xxx") - assert.Equal(t, "xxx", comment.content) + assert.Equal(t, "xxx", comment.Content()) }
35-44: Consider using table-driven tests or sub-testsThe test covers multiple scenarios but could be more organized using Go's sub-tests.
func TestComment_Clone(t *testing.T) { - comment := (&Comment{ - id: NewCommentID(), - author: operator.OperatorFromUser(NewUserID()), - content: "test", - }) - assert.Nil(t, (*Comment)(nil).Clone()) - assert.Equal(t, comment, comment.Clone()) - assert.NotSame(t, comment, comment.Clone()) + t.Run("nil comment", func(t *testing.T) { + assert.Nil(t, (*Comment)(nil).Clone()) + }) + + t.Run("valid comment", func(t *testing.T) { + comment := (&Comment{ + id: NewCommentID(), + author: operator.OperatorFromUser(NewUserID()), + content: "test", + }) + clone := comment.Clone() + assert.Equal(t, comment, clone) + assert.NotSame(t, comment, clone) + }) }asset/assetdomain/asset/list_test.go (2)
10-29: Add more test cases for sortingWhile the test covers basic sorting, consider adding more cases:
- Empty list
- Single item list
- List with duplicate IDs
- Larger list to verify stable sort
func TestList_SortByID(t *testing.T) { - id1 := NewID() - id2 := NewID() - - list := List{ - &Asset{id: id2}, - &Asset{id: id1}, - } - res := list.SortByID() - - assert.Equal(t, List{ - &Asset{id: id1}, - &Asset{id: id2}, - }, res) - - assert.Equal(t, List{ - &Asset{id: id2}, - &Asset{id: id1}, - }, list) + tests := []struct { + name string + list List + want List + }{ + { + name: "empty list", + list: List{}, + want: List{}, + }, + { + name: "single item", + list: List{&Asset{id: NewID()}}, + want: List{&Asset{id: NewID()}}, + }, + { + name: "two items", + list: List{ + &Asset{id: NewID()}, + &Asset{id: NewID()}, + }, + want: List{ + &Asset{id: NewID()}, + &Asset{id: NewID()}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + original := tt.list.Clone() + got := tt.list.SortByID() + assert.Equal(t, tt.want, got) + assert.Equal(t, original, tt.list, "original list should not be modified") + }) + } }
31-53: Reduce duplication and improve test structureThe tests have duplicate setup code and could benefit from table-driven tests for better organization.
Consider extracting common setup and using table-driven tests:
+func setupTestAsset(t *testing.T) *Asset { + t.Helper() + pid := NewProjectID() + uid := accountdomain.NewUserID() + return New().NewID().Project(pid).CreatedByUser(uid). + Size(1000).Thread(NewThreadID()).NewUUID().MustBuild() +} + func TestList_Clone(t *testing.T) { - pid := NewProjectID() - uid := accountdomain.NewUserID() - a := New().NewID().Project(pid).CreatedByUser(uid).Size(1000).Thread(NewThreadID()).NewUUID().MustBuild() + a := setupTestAsset(t) list := List{a} got := list.Clone() assert.Equal(t, list, got) assert.NotSame(t, list[0], got[0]) } func TestList_Map(t *testing.T) { - pid := NewProjectID() - uid := accountdomain.NewUserID() - a := New().NewID().Project(pid).CreatedByUser(uid).Size(1000).Thread(NewThreadID()).NewUUID().MustBuild() - - assert.Equal(t, Map{ - a.ID(): a, - }, List{a, nil}.Map()) - assert.Equal(t, Map{}, List(nil).Map()) + tests := []struct { + name string + list List + want Map + }{ + { + name: "nil list", + list: nil, + want: Map{}, + }, + { + name: "list with nil element", + list: List{nil}, + want: Map{}, + }, + { + name: "valid list", + list: List{setupTestAsset(t)}, + want: func() Map { + a := setupTestAsset(t) + return Map{a.ID(): a} + }(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.list.Map() + assert.Equal(t, tt.want, got) + }) + } }asset/assetdomain/event/builder.go (1)
18-23: Consider enhancing validation in Build methodThe current validation only checks for nil ID and operator validation. Consider adding:
- Timestamp validation (non-zero check)
- Type validation (non-empty check)
- Project validation (nil check)
func (b *Builder[T]) Build() (*Event[T], error) { - if b.i.id.IsNil() || !b.i.operator.Validate() { + if b.i.id.IsNil() || !b.i.operator.Validate() || + b.i.timestamp.IsZero() || b.i.ty == "" || b.i.prj == nil { return nil, ErrInvalidID } return b.i, nil }asset/assetdomain/asset/id.go (2)
9-14: Add documentation for ID typesConsider adding documentation comments for each type alias to explain their purpose and usage context.
+// ID represents a unique identifier for an asset type ID = assetdomain.AssetID +// IDList represents a collection of asset IDs type IDList = assetdomain.AssetIDList +// ProjectID represents a unique identifier for a project type ProjectID = assetdomain.ProjectID
16-36: Consider grouping related functionsThe ID-related functions could be better organized by grouping them based on their functionality (e.g., constructors, validators, converters).
+// Constructors var NewID = assetdomain.NewAssetID var NewProjectID = assetdomain.NewProjectID var NewUserID = accountdomain.NewUserID var NewThreadID = assetdomain.NewThreadID var NewIntegrationID = assetdomain.NewIntegrationID +// Must constructors - panic on error var MustID = assetdomain.MustAssetID var MustProjectID = assetdomain.MustProjectID var MustUserID = accountdomain.MustUserID var MustThreadID = assetdomain.MustThreadID +// Conversion functions var IDFrom = assetdomain.AssetIDFrom var ProjectIDFrom = assetdomain.ProjectIDFromasset/assetdomain/project/publication.go (2)
3-10: Enhance type safety for PublicationScopeConsider using iota with string mapping for better type safety and performance.
+type PublicationScope int + const ( - PublicationScopePrivate PublicationScope = "private" - PublicationScopeLimited PublicationScope = "limited" - PublicationScopePublic PublicationScope = "public" + PublicationScopePrivate PublicationScope = iota + PublicationScopeLimited + PublicationScopePublic ) + +var publicationScopeNames = map[PublicationScope]string{ + PublicationScopePrivate: "private", + PublicationScopeLimited: "limited", + PublicationScopePublic: "public", +} + +func (p PublicationScope) String() string { + return publicationScopeNames[p] +}
46-55: Add documentation for Clone methodThe Clone method implements defensive copying but lacks documentation explaining its behavior with nil receivers.
+// Clone creates a deep copy of the Publication. +// Returns nil if the receiver is nil. func (p *Publication) Clone() *Publication {asset/assetdomain/event/builder_test.go (2)
22-43: Consider adding more test cases for comprehensive coverage.While the current test cases cover basic functionality, consider adding tests for:
- Events with nil operator
- Events with nil object
- Events with invalid timestamps
- Other event types defined in the system
26-29: Consider extracting test helper functions for event creation.The event creation code is duplicated between
evandev2. Consider extracting a helper function to reduce duplication and improve test maintainability.+func createTestEvent[T any](id ID, timestamp time.Time, eventType Type, op operator.Operator, obj T) *Event[T] { + return New[T](). + ID(id). + Timestamp(timestamp). + Type(eventType). + Operator(op). + Object(obj). + MustBuild() +}asset/assetdomain/asset/file_builder.go (1)
68-74: Enhance content type detection.The current implementation relies solely on file extension for content type detection. Consider:
- Using a more robust content type detection library
- Adding fallback mechanisms when extension is missing
asset/assetdomain/integration/webhook_builder.go (2)
18-26: Consider adding more validation checks in Build()While the ID validation is good, consider adding additional validations for:
- URL (non-nil and valid format)
- Name (non-empty)
- Trigger (valid value)
func (b *WebhookBuilder) Build() (*Webhook, error) { if b.w.id.IsNil() { return nil, ErrInvalidID } + if b.w.url == nil { + return nil, fmt.Errorf("url is required") + } + if b.w.name == "" { + return nil, fmt.Errorf("name is required") + } if b.w.updatedAt.IsZero() { b.w.updatedAt = b.w.CreatedAt() } return b.w, nil }
28-34: Consider returning error instead of panic in MustBuildUsing panic for control flow is generally discouraged in Go. Consider making this a regular Build call or documenting clearly that this should only be used in tests.
asset/assetdomain/event/event.go (1)
55-67: Improve Clone() implementation for deep copyingThe current Clone() implementation performs a shallow copy of the
objectfield. Consider:
- Adding a constraint on T to ensure it's cloneable
- Implementing deep copy for the object if needed
asset/assetdomain/asset/status.go (2)
17-33: Consider using a map for status conversionThe switch statement could be replaced with a map lookup for better maintainability and performance.
+var statusMap = map[string]ArchiveExtractionStatus{ + "skipped": ArchiveExtractionStatusSkipped, + "pending": ArchiveExtractionStatusPending, + "in_progress": ArchiveExtractionStatusInProgress, + "done": ArchiveExtractionStatusDone, + "failed": ArchiveExtractionStatusFailed, +} + func ArchiveExtractionStatusFrom(s string) (ArchiveExtractionStatus, bool) { ss := strings.ToLower(s) - switch ArchiveExtractionStatus(ss) { - case ArchiveExtractionStatusSkipped: - return ArchiveExtractionStatusSkipped, true - case ArchiveExtractionStatusPending: - return ArchiveExtractionStatusPending, true - case ArchiveExtractionStatusInProgress: - return ArchiveExtractionStatusInProgress, true - case ArchiveExtractionStatusDone: - return ArchiveExtractionStatusDone, true - case ArchiveExtractionStatusFailed: - return ArchiveExtractionStatusFailed, true - default: - return ArchiveExtractionStatus(""), false - } + status, ok := statusMap[ss] + if !ok { + return ArchiveExtractionStatus(""), false + } + return status, true }
51-57: Consider adding validation in StringRef()The method assumes the status value is valid. Consider validating it before converting to string.
func (s *ArchiveExtractionStatus) StringRef() *string { if s == nil { return nil } + if _, ok := ArchiveExtractionStatusFrom(string(*s)); !ok { + return nil + } s2 := string(*s) return &s2 }asset/assetinfrastructure/assetmemory/asset_file.go (3)
16-20: Consider adding documentation for AssetFileImpl structThe struct implements assetrepo.AssetFile interface but lacks documentation explaining its purpose and thread-safety guarantees provided by SyncMap.
Add documentation:
+// AssetFileImpl provides a thread-safe in-memory implementation of assetrepo.AssetFile interface. +// It uses sync.Map internally to ensure concurrent access safety. type AssetFileImpl struct {
41-42: Commented code should be removedThere's a commented line that appears to be old implementation. Either remove it or document why it's kept.
- // f = asset.FoldFiles(fs, f) f.SetFiles(fs)
56-63: Consider adding validation for nil parametersThe SaveFlat method should validate that parent and files parameters are not nil before proceeding.
func (r *AssetFileImpl) SaveFlat(ctx context.Context, id id.AssetID, parent *asset.File, files []*asset.File) error { if r.err != nil { return r.err } + if parent == nil { + return rerror.ErrInvalid.WithMessage("parent file cannot be nil") + } + if files == nil { + files = []*asset.File{} // or return error based on requirements + } r.data.Store(id, parent.Clone()) r.files.Store(id, slices.Clone(files)) return nil }asset/assetdomain/integration/builder.go (1)
26-32: Reconsider using panic in MustBuildWhile the pattern is common, consider if panic is appropriate for your use case. If kept, document the panic condition.
+// MustBuild is like Build but panics if the Integration cannot be built. +// It should only be used in initialization code where Integration creation cannot fail. func (b *Builder) MustBuild() *Integration {asset/assetdomain/asset/file_builder_test.go (2)
41-61: Enhance TestFileBuilder_Build test coverageThe test should include error cases and edge cases. Consider testing:
- Empty paths
- Invalid file extensions
- Maximum file sizes
- Empty/nil children arrays
Add test cases:
// Test error cases f3 := NewFile().Name("").Build() assert.Empty(t, f3.Name()) // Test edge cases f4 := NewFile().Name("file").Path("/.").Size(0).Build() assert.Equal(t, "/.", f4.Path()) assert.Zero(t, f4.Size()) // Test nil children f5 := NewFile().Children(nil).Build() assert.Nil(t, f5.Children())
21-29: Add test table for path handlingConsider using a test table to cover more path scenarios systematically.
func TestFileBuilder_Path(t *testing.T) { tests := []struct { name string input string expected string }{ {"already_slashed", "/hoge", "/hoge"}, {"needs_slash", "fuga", "/fuga"}, {"empty", "", "/"}, {"multiple_slashes", "//path", "/path"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { f := NewFile().Path(tt.input).Build() assert.Equal(t, tt.expected, f.Path()) }) } }asset/assetdomain/project/builder.go (2)
72-81: Consider defensive copying in ImageURL methodThe current implementation creates a copy of the URL to prevent modification of the original, which is good. However, the comment references a Go issue that's worth documenting more clearly.
Consider updating the comment to explain why copying is necessary:
- // https://github.com/golang/go/issues/38351 + // Create defensive copy of URL to prevent external modifications + // See: https://github.com/golang/go/issues/38351 for details on URL copying
93-96: Consider nil check for RequestRolesWhile
slices.Clonehandles nil slices correctly, it's good practice to be explicit about nil handling for clarity and maintainability.func (b *Builder) RequestRoles(requestRoles []workspace.Role) *Builder { + if requestRoles == nil { + b.p.requestRoles = nil + return b + } b.p.requestRoles = slices.Clone(requestRoles) return b }asset/assetdomain/asset/builder.go (1)
18-41: Consider adding size validation upper limitThe Build method validates for zero size but doesn't check for unreasonably large sizes.
const ( + MaxAssetSize = 1024 * 1024 * 1024 // 1GB or adjust as needed ) func (b *Builder) Build() (*Asset, error) { if b.a.id.IsNil() { return nil, ErrInvalidID } if b.a.project.IsNil() { return nil, ErrNoProjectID } if b.a.user.IsNil() && b.a.integration.IsNil() { return nil, ErrNoUser } if b.a.thread.IsNil() { return nil, ErrNoThread } if b.a.size == 0 { return nil, ErrZeroSize } + if b.a.size > MaxAssetSize { + return nil, ErrAssetTooLarge + } if b.a.uuid == "" { return nil, ErrNoUUID }asset/assetdomain/asset/asset.go (1)
10-23: Consider adding validation for required fieldsThe Asset struct contains fields that should be required (e.g., id, project, fileName), but there's no validation mechanism.
Consider adding a Validate method to ensure required fields are set:
func (a *Asset) Validate() error { if a == nil { return errors.New("nil asset") } if a.id.IsNil() { return errors.New("missing asset id") } if a.project.IsNil() { return errors.New("missing project id") } if a.fileName == "" { return errors.New("missing file name") } return nil }asset/assetinfrastructure/assetmemory/integration.go (2)
28-56: Consider extracting common error handling patternBoth methods share the same error handling pattern. Consider extracting it into a helper method to reduce duplication.
+func (r *Integration) checkError() error { + if r.err != nil { + return r.err + } + return nil +} func (r *Integration) FindByID(_ context.Context, iId id.IntegrationID) (*integration.Integration, error) { - if r.err != nil { - return nil, r.err - } + if err := r.checkError(); err != nil { + return nil, err + }
82-101: Consider adding validation in Save methodWhile the implementation is correct, consider adding validation before storing the integration:
- Check if integration is nil
- Validate required fields
func (r *Integration) Save(_ context.Context, i *integration.Integration) error { if r.err != nil { return r.err } + if i == nil { + return rerror.ErrInvalidParameter + } r.data.Store(i.ID(), i) return nil }asset/assetdomain/project/project_test.go (3)
13-37: Fix typo and enhance test coverage
- Fix the typo in "expexted" to "expected"
- Consider adding more test cases for edge cases:
- Empty string
- Special characters
- Maximum length
testCase := []struct { name, alias string - expexted bool + expected bool }{
39-84: Well-structured tests with good practicesGood use of:
- Table-driven tests
- Parallel test execution
- Clear assertions
Consider adding error cases for SetImageURL with invalid URLs.
86-113: Consider adding more test casesThe tests cover basic functionality but could benefit from additional cases:
- Nil checks for Publication
- Empty/invalid descriptions
- Invalid team IDs
- Empty/nil request roles
asset/assetdomain/asset/asset_test.go (2)
12-56: Improve test readability and maintainabilityConsider:
- Extracting test constants for magic values
- Using helper functions for common setup
- Breaking down the large test into smaller, focused tests
+const ( + testUUID = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" + testTime = "2021-03-16T04:19:57.592Z" + testFileName = "hoge" +) + +func setupTestAsset() *Asset { + return &Asset{ + id: NewID(), + project: NewProjectID(), + // ... other common setup + } +}
58-96: Reduce test setup duplicationConsider converting these tests to table-driven tests to reduce setup duplication and make it easier to add new test cases.
+func TestAsset_Getters(t *testing.T) { + tests := []struct { + name string + setup func(*Asset) + check func(*testing.T, *Asset) + }{ + { + name: "PreviewType nil", + setup: func(a *Asset) {}, + check: func(t *testing.T, a *Asset) { + assert.Nil(t, a.PreviewType()) + }, + }, + // Add more cases + } + // ... test execution +}asset/assetdomain/integration/list_test.go (4)
14-32: Consider adding more test cases for SortByID.The test covers basic sorting functionality and immutability, but could be enhanced with additional cases:
- Empty list
- Single item list
- List with duplicate IDs
34-44: Enhance clone test coverage.Consider adding test cases for:
- Empty list cloning
- List with multiple items
- Nested object modifications
153-157: Remove commented-out code.The commented-out test struct definition should be removed as it's no longer needed.
57-151: Consider refactoring test data setup.The test data setup is quite verbose. Consider creating helper functions to build test data, such as:
func createTestWebhook(id WebhookID, name string, active bool, triggers WebhookTrigger) *Webhook func createTestIntegration(id ID, name string, webhooks []*Webhook) *Integrationasset/assetinfrastructure/assetfs/file_test.go (1)
33-55: Add content type and size assertions.Consider adding assertions for:
- File content type verification
- File size checks
- Buffer handling for large files
asset/assetdomain/asset/preview_type_test.go (2)
11-130: Consider using test case struct as a named type.The test case struct is reused across multiple tests. Consider defining it as a named type to improve code reusability:
type previewTypeTestCase struct { Name string Expected struct { TA PreviewType Bool bool } }
211-228: Add more edge cases to DetectPreviewType test.Consider adding test cases for:
- Files without extensions
- Mixed case extensions
- Multiple extensions (e.g., file.tar.gz)
asset/assetdomain/asset/file_test.go (4)
9-30: Rename test function to better reflect its purposeThe function name
TestFile_FileTypedoesn't accurately represent what it's testing. The test actually verifies multiple file properties and methods, not just the file type.Consider renaming to
TestFile_BasicPropertiesorTestFile_CoreFunctionalityto better reflect its purpose.Additionally, consider adding edge cases for content type guessing:
- Empty files
- Files with unusual extensions
- Files without extensions
32-43: Add more test cases for children handlingWhile the current test covers basic scenarios, consider adding tests for:
- Maximum depth of nested children
- Circular references
- Empty children array vs nil children
45-71: Convert to table-driven tests for better maintainabilityConsider restructuring this test as a table-driven test to make it easier to add more test cases and improve readability. This would also make it easier to test edge cases like:
- Empty directories
- Files with same names in different directories
- Very deep directory structures
73-84: Add validation for file relationshipsConsider adding assertions to verify:
- Parent-child relationships are maintained
- File paths are consistent with the hierarchy
- Duplicate files are handled correctly
asset/assetdomain/project/builder_test.go (2)
14-34: Add more test cases for builder initializationWhile the basic initialization is tested, consider adding test cases for:
- Builder with pre-initialized values
- Builder with invalid initial state
113-202: Consider testing concurrent builder usageThe builder pattern implementation should be tested for concurrent usage safety. Consider adding test cases that verify the builder's behavior when used concurrently.
Example test:
func TestBuilder_ConcurrentUsage(t *testing.T) { builder := New() var wg sync.WaitGroup for i := 0; i < 10; i++ { wg.Add(1) go func() { defer wg.Done() // Perform concurrent builder operations }() } wg.Wait() }asset/assetdomain/asset/builder_test.go (2)
12-31: Consider adding validation for input fields.The
Inputstruct is used to test the builder but lacks field validation. Consider adding validation tags or documentation to clarify field requirements.type Input struct { - id ID - project ProjectID - createdAt time.Time - createdByUser accountdomain.UserID - createdByIntegration IntegrationID - fileName string - size uint64 - previewType *PreviewType - uuid string - thread ThreadID - archiveExtractionStatus *ArchiveExtractionStatus + id ID `validate:"required"` + project ProjectID `validate:"required"` + createdAt time.Time + createdByUser accountdomain.UserID + createdByIntegration IntegrationID + fileName string `validate:"required"` + size uint64 `validate:"required,gt=0"` + previewType *PreviewType + uuid string `validate:"required,uuid"` + thread ThreadID `validate:"required"` + archiveExtractionStatus *ArchiveExtractionStatus }
316-322: Consider testing NewID with multiple goroutines.The
TestBuilder_NewIDfunction should verify that IDs are unique when generated concurrently.func TestBuilder_NewID(t *testing.T) { + const numGoroutines = 100 + ids := make(chan ID, numGoroutines) + var wg sync.WaitGroup + wg.Add(numGoroutines) + + for i := 0; i < numGoroutines; i++ { + go func() { + defer wg.Done() + a := New().NewID().Project(pid).CreatedByUser(uid).Size(size).Thread(NewThreadID()).NewUUID().MustBuild() + ids <- a.id + }() + } + + wg.Wait() + close(ids) + + // Verify uniqueness + seen := make(map[ID]bool) + for id := range ids { + assert.False(t, seen[id], "Duplicate ID generated") + seen[id] = true + } }asset/assetdomain/integration/webhook_test.go (1)
493-501: Add comprehensive trigger combination tests.The webhook trigger tests should verify all possible combinations of triggers and their interactions.
func TestWebhookTrigger_Enable(t *testing.T) { - wt := WebhookTrigger{} - - assert.False(t, wt.IsActive(event.ItemCreate)) - wt.Enable(event.ItemCreate) - assert.True(t, wt.IsActive(event.ItemCreate)) - wt.Disable(event.ItemCreate) - assert.False(t, wt.IsActive(event.ItemCreate)) + tests := []struct { + name string + setup func(*WebhookTrigger) + check func(*testing.T, *WebhookTrigger) + }{ + { + name: "single_trigger", + setup: func(wt *WebhookTrigger) { + wt.Enable(event.ItemCreate) + }, + check: func(t *testing.T, wt *WebhookTrigger) { + assert.True(t, wt.IsActive(event.ItemCreate)) + assert.False(t, wt.IsActive(event.ItemUpdate)) + }, + }, + { + name: "multiple_triggers", + setup: func(wt *WebhookTrigger) { + wt.Enable(event.ItemCreate) + wt.Enable(event.ItemUpdate) + wt.Enable(event.ItemDelete) + }, + check: func(t *testing.T, wt *WebhookTrigger) { + assert.True(t, wt.IsActive(event.ItemCreate)) + assert.True(t, wt.IsActive(event.ItemUpdate)) + assert.True(t, wt.IsActive(event.ItemDelete)) + }, + }, + { + name: "toggle_triggers", + setup: func(wt *WebhookTrigger) { + wt.Enable(event.ItemCreate) + wt.Disable(event.ItemCreate) + wt.Enable(event.ItemCreate) + }, + check: func(t *testing.T, wt *WebhookTrigger) { + assert.True(t, wt.IsActive(event.ItemCreate)) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + wt := WebhookTrigger{} + tt.setup(&wt) + tt.check(t, &wt) + }) + } }asset/assetinfrastructure/assetmemory/asset_test.go (4)
17-73: Consider adding more test cases for comprehensive permission testing.While the current test cases cover basic scenarios, consider adding these cases for better coverage:
- Partial permissions (readable but not writable)
- Empty project list vs nil project list
- Mixed permissions across multiple projects
Example additional test case:
tests := []struct { name string seeds asset.List arg repo.ProjectFilter wantErr error mockErr bool }{ + { + name: "project filter with read-only permission", + seeds: asset.List{ + p1, + p2, + }, + arg: repo.ProjectFilter{ + Readable: []id.ProjectID{tid1}, + Writable: []id.ProjectID{}, + }, + wantErr: nil, + },
75-156: Consider adding test case for invalid ID format.The test suite is comprehensive but could be enhanced by adding a test case for invalid ID format, although this might be handled at the domain level.
Example additional test case:
tests := []struct { name string seeds []*asset.Asset arg id.AssetID want *asset.Asset wantErr error }{ + { + name: "Invalid ID format", + seeds: []*asset.Asset{a1}, + arg: id.AssetID{}, // or invalid format if supported + want: nil, + wantErr: rerror.ErrInvalidID, + },
158-252: Verify order of returned assets matches input order.The test cases should verify that the order of returned assets matches the order of requested IDs, as this is often important for maintaining consistency in the UI.
Example modification:
{ name: "2 count with multi assets", seeds: []*asset.Asset{ a1, a2, }, arg: id.AssetIDList{id2, id1}, // Reversed order - want: []*asset.Asset{a1, a2}, + want: []*asset.Asset{a2, a1}, // Should match input order wantErr: nil, },
1-456: Well-structured test suite with room for enhancement.The test suite follows Go best practices and provides good coverage. Consider these improvements:
- Add package documentation explaining the purpose of these tests
- Consider using test helper functions for common setup code
- Add more edge cases across all test functions
Add package documentation:
+// Package assetmemory provides an in-memory implementation of the asset repository. +// This test file verifies the implementation meets the repository interface contract. package assetmemory
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (82)
.gitignore(1 hunks)asset/asset.graphql(1 hunks)asset/assetdomain/asset/asset.go(1 hunks)asset/assetdomain/asset/asset_test.go(1 hunks)asset/assetdomain/asset/builder.go(1 hunks)asset/assetdomain/asset/builder_test.go(1 hunks)asset/assetdomain/asset/common.go(1 hunks)asset/assetdomain/asset/file.go(1 hunks)asset/assetdomain/asset/file_builder.go(1 hunks)asset/assetdomain/asset/file_builder_test.go(1 hunks)asset/assetdomain/asset/file_test.go(1 hunks)asset/assetdomain/asset/id.go(1 hunks)asset/assetdomain/asset/list.go(1 hunks)asset/assetdomain/asset/list_test.go(1 hunks)asset/assetdomain/asset/map.go(1 hunks)asset/assetdomain/asset/map_test.go(1 hunks)asset/assetdomain/asset/preview_type.go(1 hunks)asset/assetdomain/asset/preview_type_test.go(1 hunks)asset/assetdomain/asset/status.go(1 hunks)asset/assetdomain/asset/status_test.go(1 hunks)asset/assetdomain/asset/upload.go(1 hunks)asset/assetdomain/asset/upload_builder.go(1 hunks)asset/assetdomain/event/builder.go(1 hunks)asset/assetdomain/event/builder_test.go(1 hunks)asset/assetdomain/event/event.go(1 hunks)asset/assetdomain/event/event_test.go(1 hunks)asset/assetdomain/event/id.go(1 hunks)asset/assetdomain/file/file.go(1 hunks)asset/assetdomain/file/file_test.go(1 hunks)asset/assetdomain/file/testdata/test.txt(1 hunks)asset/assetdomain/id.go(1 hunks)asset/assetdomain/integration/builder.go(1 hunks)asset/assetdomain/integration/builder_test.go(1 hunks)asset/assetdomain/integration/id.go(1 hunks)asset/assetdomain/integration/integration.go(1 hunks)asset/assetdomain/integration/integration_test.go(1 hunks)asset/assetdomain/integration/integration_type.go(1 hunks)asset/assetdomain/integration/integration_type_test.go(1 hunks)asset/assetdomain/integration/list.go(1 hunks)asset/assetdomain/integration/list_test.go(1 hunks)asset/assetdomain/integration/webhook.go(1 hunks)asset/assetdomain/integration/webhook_builder.go(1 hunks)asset/assetdomain/integration/webhook_builder_test.go(1 hunks)asset/assetdomain/integration/webhook_test.go(1 hunks)asset/assetdomain/operator/id.go(1 hunks)asset/assetdomain/operator/operator.go(1 hunks)asset/assetdomain/operator/operator_test.go(1 hunks)asset/assetdomain/project/builder.go(1 hunks)asset/assetdomain/project/builder_test.go(1 hunks)asset/assetdomain/project/id.go(1 hunks)asset/assetdomain/project/id_test.go(1 hunks)asset/assetdomain/project/list.go(1 hunks)asset/assetdomain/project/list_test.go(1 hunks)asset/assetdomain/project/project.go(1 hunks)asset/assetdomain/project/project_test.go(1 hunks)asset/assetdomain/project/publication.go(1 hunks)asset/assetdomain/project/publication_test.go(1 hunks)asset/assetdomain/task/task.go(1 hunks)asset/assetdomain/thread/builder.go(1 hunks)asset/assetdomain/thread/builder_test.go(1 hunks)asset/assetdomain/thread/comment.go(1 hunks)asset/assetdomain/thread/comment_test.go(1 hunks)asset/assetdomain/thread/common.go(1 hunks)asset/assetdomain/thread/id.go(1 hunks)asset/assetdomain/thread/list.go(1 hunks)asset/assetdomain/thread/list_test.go(1 hunks)asset/assetdomain/thread/thread.go(1 hunks)asset/assetdomain/thread/thread_test.go(1 hunks)asset/assetinfrastructure/assetfs/common.go(1 hunks)asset/assetinfrastructure/assetfs/file.go(1 hunks)asset/assetinfrastructure/assetfs/file_test.go(1 hunks)asset/assetinfrastructure/assetmemory/asset.go(1 hunks)asset/assetinfrastructure/assetmemory/asset_file.go(1 hunks)asset/assetinfrastructure/assetmemory/asset_test.go(1 hunks)asset/assetinfrastructure/assetmemory/container.go(1 hunks)asset/assetinfrastructure/assetmemory/container_test.go(1 hunks)asset/assetinfrastructure/assetmemory/event.go(1 hunks)asset/assetinfrastructure/assetmemory/event_test.go(1 hunks)asset/assetinfrastructure/assetmemory/integration.go(1 hunks)asset/assetinfrastructure/assetmemory/integration_test.go(1 hunks)asset/assetinfrastructure/assetmemory/project.go(1 hunks)asset/assetinfrastructure/assetmemory/project_test.go(1 hunks)
⛔ Files not processed due to max files limit (23)
- asset/assetinfrastructure/assetmemory/thread.go
- asset/assetinfrastructure/assetmemory/thread_test.go
- asset/assetusecase/assetgateway/container.go
- asset/assetusecase/assetgateway/file.go
- asset/assetusecase/assetgateway/gatewaymock/task.go
- asset/assetusecase/assetgateway/genmock.go
- asset/assetusecase/assetgateway/task.go
- asset/assetusecase/assetinteractor/asset.go
- asset/assetusecase/assetinteractor/asset_test.go
- asset/assetusecase/assetinteractor/common.go
- asset/assetusecase/assetinteractor/common_test.go
- asset/assetusecase/assetinterfaces/asset.go
- asset/assetusecase/assetinterfaces/common.go
- asset/assetusecase/assetrepo/asset.go
- asset/assetusecase/assetrepo/asset_upload.go
- asset/assetusecase/assetrepo/container.go
- asset/assetusecase/assetrepo/event.go
- asset/assetusecase/assetrepo/intgration.go
- asset/assetusecase/assetrepo/project.go
- asset/assetusecase/assetrepo/thread.go
- asset/assetusecase/operator.go
- asset/assetusecase/operator_test.go
- go.mod
✅ Files skipped from review due to trivial changes (3)
- asset/assetdomain/file/testdata/test.txt
- .gitignore
- asset/assetinfrastructure/assetfs/common.go
🔇 Additional comments (50)
asset/assetdomain/integration/integration_type_test.go (1)
9-39: Well-structured test implementation!
The test implementation follows several good practices:
- Table-driven tests with clear structure
- Parallel test execution with proper loop variable handling
- Clear test case naming and organization
- Secure default behavior for unknown values
asset/assetinfrastructure/assetmemory/integration_test.go (1)
16-94: Solid test structure and mocking approach.
The test function “TestIntegrationRepo_FindByID” employs a clear table-driven testing pattern, thorough seeding logic, and mock error injection. Parallel sub-tests ensure faster feedback. This structure is consistent and covers both positive and negative cases.
asset/assetdomain/project/publication_test.go (4)
1-8: LGTM! Clean and minimal imports.
The package declaration and imports are appropriate for a test file, using the standard testing package and testify/assert for assertions.
37-65: LGTM! Comprehensive scope transition testing.
The test covers all possible scope transitions and handles the empty scope case correctly.
Consider using table-driven tests as suggested earlier for better maintainability.
67-83: LGTM! Complete coverage of asset visibility transitions.
The test thoroughly verifies both transitions of the asset visibility state.
85-94: LGTM! Thorough clone testing.
The test comprehensively verifies the cloning functionality:
- Ensures equality between original and clone
- Confirms deep copy with different memory addresses
- Properly handles the nil case
asset/assetdomain/thread/thread_test.go (2)
28-37: Add safety check in Thread.Comments() method
The test reveals that the Comments() method is expected to handle nil receivers. Ensure that the actual Thread.Comments() method includes a nil check to prevent panics in production.
74-109: 🛠️ Refactor suggestion
Add tests for edge cases in update and delete operations
Consider adding tests for:
- Updating a comment with empty content
- Concurrent modifications (if thread operations should be thread-safe)
- Maximum content length validation (if applicable)
asset/assetinfrastructure/assetmemory/event_test.go (1)
3-16: LGTM! Imports are well-organized.
The imports are properly structured and all dependencies are being used in the test file.
asset/assetdomain/file/file.go (1)
24-54: Validate presence of multipart form data.
The loop terminates after encountering the first matching part, but there's no stricter check that multipart data actually contains valid content (e.g., correct or non-empty). In some edge cases, an empty upload might pass. Consider additional validation on the file size or a minimal read to ensure the part is valid prior to returning.
asset/assetdomain/asset/preview_type.go (1)
20-24: Assess usage of custom type for references.
Using a pointer to a custom string type (PreviewType) is valid, but be aware that other code might expect a plain string pointer. Ensure consistent usage within the codebase to avoid confusion or accidental nil-pointer dereferences.
asset/asset.graphql (1)
51-57: Clarify concurrency rules for CreateAssetInput fields.
Since file uploads can be large or asynchronous, watch for potential concurrency issues if combining file-based input with URL-based input. Ensure that the server logic handles concurrency gracefully.
asset/assetdomain/asset/file.go (1)
63-67: Verify logic that excludes the parent path from SetFiles.
When filtering child files, confirm that ignoring any file with exactly the same path as the parent does not cause edge cases (e.g., file name collisions).
asset/assetinfrastructure/assetmemory/project.go (1)
118-120: Check for potential edge cases in blank name retrieval.
“FindByPublicName” returns nil if name is empty. Ensure there are no scenarios where an empty alias is a valid input.
asset/assetinfrastructure/assetfs/file.go (4)
27-37: Handle invalid or missing defaultBase more gracefully.
Line 30 sets urlBase to defaultBase if empty, but defaultBase is not shown in this snippet. Confirm that it is defined and is valid to avoid runtime issues.
92-98: Check file size limits more thoroughly.
You reject files ≥ fileSizeLimit, but the snippet does not show its definition or usage. Ensure that the limit is set to a reasonable default and is consistently applied.
122-125: Escaping asset filenames.
Ensure that path escaping in GetURL (line 124) is sufficient to avoid any potential path injection vulnerabilities.
193-199: Confirm path composition for potential path traversal vulnerabilities.
getFSObjectPath simply concatenates the assetDir, the first two characters, and the rest of fileUUID. Confirm that input can’t be manipulated to navigate outside the desired directory.
asset/assetdomain/id.go (1)
1-239: No critical issues detected.
All newly declared ID types, builder functions, and aliases appear consistent. The file is primarily domain definitions, with minimal logic. No major concerns.
asset/assetinfrastructure/assetmemory/project_test.go (8)
19-130: Good coverage for CountByWorkspace.
The tests methodically check different seeds, filters, and error mocks. This ensures robust coverage for workspace-based queries.
132-195: Filtered tests handle workspace permissions effectively.
The approach of passing a repo.WorkspaceFilter to validate r/w constraints is well structured.
197-310: Comprehensive coverage of FindByID.
Edge cases for filtering, missing projects, and error mocking are thoroughly tested. All looks good.
312-444: Extensive tests for FindByIDs.
Including scenarios with multiple seeds, filters, and error injection sets a strong foundation.
446-584: Consistent approach for FindByPublicName.
The results reflect correct usage of mocks, filters, and alias lookups.
586-732: Methodical pagination checks in FindByWorkspaces.
Though pagination is partially indicated as a TODO in code, this test suite verifies fundamental behavior with multi-page scenarios.
734-840: Remove operation tested thoroughly.
Different seeds, workspace filters, and error states highlight valuable coverage.
842-940: Save operation test.
These tests confirm read/write constraints, mocking errors, and verifying that the newly saved project is retrievable.
asset/assetdomain/integration/integration_test.go (2)
19-19: Great use of dynamic IDs.
Generating test-specific IDs (e.g., iId, uId) helps ensure isolation in concurrent tests and reduces the chance of ID collisions.
75-80: Parallelized subtests are beneficial, but verify resource safety.
Parallel execution can lead to race conditions if the tests share resources like global variables or environment settings. Make sure that all data is ephemeral or is set up/teardown in a thread-safe manner.
asset/assetdomain/integration/integration_type.go (1)
7-11: Enums are effectively represented.
Using string-based type definitions for "public"/"private" is a convenient approach for easy JSON serialization and comparisons. Just ensure all references use these constants rather than hard-coded strings.
asset/assetdomain/asset/map.go (1)
7-11: Efficient usage of lo.MapToSlice.
This approach keeps the code concise and avoids manual iteration. Good job leveraging the library’s utility.
asset/assetdomain/operator/id.go (1)
9-11: Clear type aliasing for domain concepts.
Aliasing types clarifies domain ownership, ensuring that the code cleanly integrates with the existing ID definitions. This is straightforward and readable.
asset/assetdomain/thread/list.go (1)
18-20: Verify thread cloning implementation
The Clone method assumes that Thread.Clone() exists and properly implements deep copying. Let's verify this assumption.
✅ Verification successful
Thread cloning implementation is properly implemented
The verification confirms that:
- Thread.Clone() exists and implements proper deep copying
- It handles nil receivers correctly
- It clones all fields: id, workspace, and comments
- Comment.Clone() is also implemented correctly
- The implementation is tested with proper assertions for nil handling and deep copying
- util.Map is used correctly for cloning comment slices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Thread.Clone() exists and its implementation
ast-grep --pattern 'func (t *Thread) Clone() *Thread {
$$$
}'
Length of output: 65
Script:
#!/bin/bash
# Let's first find the Thread type definition and its methods
rg "type Thread struct" -A 5
# Also search for Clone method with a more flexible pattern
rg "func.*Thread.*Clone"
# Let's also check the util.Map implementation since it's used in the Clone method
rg "func Map"
Length of output: 1888
Script:
#!/bin/bash
# Let's check the Thread.Clone() implementation and its test
rg "func \(th \*Thread\) Clone\(\)" -A 10 asset/assetdomain/thread/thread.go
# Also check the test implementation for additional context
rg "func TestThread_Clone" -A 10 asset/assetdomain/thread/thread_test.go
# Check util.Map implementation since it's used in List.Clone()
rg "func Map\[T any, V any\]" -A 5 util/slice.go
Length of output: 866
Script:
#!/bin/bash
# Let's check the Comment.Clone() implementation since it's used in Thread.Clone()
rg "func \(c \*Comment\) Clone\(\)" -A 10 asset/assetdomain/thread/
# Also check if there's any more of the Thread.Clone() implementation we didn't see
rg "func \(th \*Thread\) Clone\(\)" -A 15 asset/assetdomain/thread/thread.go
Length of output: 960
asset/assetdomain/asset/list.go (1)
23-29: LGTM! Robust nil handling in Map conversion.
The Map method correctly filters out nil entries before conversion and uses efficient functional operations from the lo library. The nil check prevents potential panics during map creation.
asset/assetdomain/event/id.go (2)
1-17: Well-organized domain separation with clear type hierarchies.
The code demonstrates good domain-driven design practices:
- Clear separation of concerns between account and asset domains
- Consistent type aliasing for domain objects
- Complete set of ID operations
3-7: Verify the version compatibility of imported packages.
The code imports packages from multiple domains. Let's ensure version compatibility.
asset/assetdomain/integration/id.go (1)
14-19: Consistent error handling across domains.
Good use of centralized error definition with ErrInvalidID. The ID operations provide a complete set of functionality.
✅ Verification successful
The search results show consistent error handling across domains, where ErrInvalidID is defined in the central idx package and reused across different domain packages. Each domain properly imports and uses this error, maintaining consistency in error handling for invalid IDs. The original review comment's approval of the centralized error definition is accurate.
Consistent error handling pattern confirmed across domains
The codebase demonstrates a uniform approach to ID-related error handling:
- Central definition in
idx/id.go - Consistent reuse across domain packages
- Proper error propagation in builder patterns
- Consistent testing patterns for ID validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent error handling across domains
rg "ErrInvalidID" --type go
Length of output: 2866
asset/assetinfrastructure/assetmemory/container.go (1)
11-23: Consider adding error handling for repository initialization
The New function assumes all repository initializations will succeed. Consider adding error handling to catch potential initialization failures, especially since this is part of a microservice architecture.
Additionally, using NopTransaction in production could lead to data consistency issues.
Let's verify if this container is used in tests only:
asset/assetdomain/thread/builder.go (2)
8-14: LGTM! Well-structured builder pattern implementation.
The builder pattern is implemented correctly with proper encapsulation and a clean constructor function.
51-54: 🛠️ Refactor suggestion
Ensure thread-safety in Comments method.
While slices.Clone provides a shallow copy of the slice, the Comment objects themselves are still shared. Consider deep copying the comments to prevent potential race conditions if the original comments are modified after building.
func (b *Builder) Comments(c []*Comment) *Builder {
- b.th.comments = slices.Clone(c)
+ comments := make([]*Comment, len(c))
+ for i, comment := range c {
+ if comment != nil {
+ comments[i] = comment.Clone()
+ }
+ }
+ b.th.comments = comments
return b
}asset/assetdomain/event/event_test.go (1)
15-31: 🛠️ Refactor suggestion
Split test into focused sub-tests and add error cases
The test covers multiple aspects but could be improved in several ways:
- Split into focused sub-tests for better organization
- Add error cases
- Use fixed time instead of
time.Now()
Consider restructuring like this:
-func TestEvent(t *testing.T) {
+func TestEvent(t *testing.T) {
+ fixedTime := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)
+
+ setupTestEvent := func() (*user.User, *asset.Asset, Event[*asset.Asset]) {
+ u := user.New().NewID().Email("[email protected]").Name("John").MustBuild()
+ a := asset.New().NewID().Thread(id.NewThreadID()).NewUUID().
+ Project(project.NewID()).Size(100).CreatedByUser(u.ID()).MustBuild()
+ ev := New[*asset.Asset]().ID(NewID()).Timestamp(fixedTime).Type(AssetCreate).
+ Operator(operator.OperatorFromUser(u.ID())).Object(a).MustBuild()
+ return u, a, ev
+ }
+
+ t.Run("successful creation", func(t *testing.T) {
+ u, a, ev := setupTestEvent()
+ assert.Equal(t, Type(AssetCreate), ev.Type())
+ assert.Equal(t, operator.OperatorFromUser(u.ID()), ev.Operator())
+ assert.Equal(t, a, ev.Object())
+ assert.Equal(t, fixedTime, ev.Timestamp())
+ })
+
+ t.Run("cloning", func(t *testing.T) {
+ _, _, ev := setupTestEvent()
+ clone := ev.Clone()
+ assert.Equal(t, ev, clone)
+ assert.NotSame(t, ev, clone)
+ })
+
+ t.Run("error cases", func(t *testing.T) {
+ t.Run("missing required fields", func(t *testing.T) {
+ _, err := New[*asset.Asset]().Build()
+ assert.Error(t, err)
+ })
+ })
+}Let's verify if there are any error cases we should test:
asset/assetinfrastructure/assetmemory/integration.go (3)
15-26: Well-structured implementation with good practices!
The implementation shows good attention to thread-safety using SyncMap and testability with the TimeNow utility.
58-80: Well-implemented bulk operations with consistent output
Good practices observed:
- Efficient bulk operations using FindAll
- Consistent sorting of results
- Clear error handling
103-109: Well-designed testing utilities
The mock functions provide good testing capabilities while maintaining type safety.
asset/assetdomain/project/project_test.go (1)
115-160: Well-implemented tests with good error handling
The tests demonstrate good practices:
- Error case validation
- Deep copy verification
- Nil handling
asset/assetdomain/asset/asset_test.go (1)
98-151: Well-implemented update and clone tests
Good practices observed:
- Proper verification of updates
- Use of builder pattern
- Deep copy validation in Clone
- Nil handling
asset/assetinfrastructure/assetfs/file_test.go (2)
23-31: LGTM!
The test effectively covers both successful creation and error handling for invalid URLs.
69-101:
Verify path traversal protection.
The test should include cases for potential path traversal attacks:
- File names with "../" sequences
- Absolute paths
- Special characters in file names
✅ Verification successful
Let me gather more information about the file path handling in the codebase, particularly focusing on the asset upload implementation.
Based on the gathered information, I can now generate the final response:
Path traversal protection is implemented securely.
The implementation is safe against path traversal attacks because:
- File paths are constructed using UUID-based directories:
assetDir/XX/YYYY.../filename - The UUID is validated before path construction (
IsValidUUIDcheck) filepath.Joinis used for safe path construction- The file system root is constrained by the
assetDirconstant
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for path traversal protection in the codebase
rg -l "filepath\.Clean|path\.Clean" | grep -v "_test.go"
Length of output: 87
Script:
#!/bin/bash
# Check the implementation of getFSObjectPath and related file path handling
ast-grep --pattern 'func getFSObjectPath($$$)'
# Also check the actual File.UploadAsset implementation
ast-grep --pattern 'func ($_ *File) UploadAsset($$$)'
# Look for any path sanitization in the codebase
rg -A 5 "filepath\.|path\." --type go
Length of output: 13962
asset/assetdomain/asset/builder_test.go (1)
209-235: Consider adding test coverage for concurrent builder usage.
The test suite runs tests in parallel but doesn't verify if the builder is safe for concurrent use. Consider adding tests that specifically verify thread safety.
asset/assetinfrastructure/assetmemory/asset_test.go (1)
1-15: LGTM! Well-organized package structure and imports.
The package structure follows Go conventions and the imports are properly organized, reflecting a clean domain-driven design architecture.
| func TestIntegrationRepo_Remove(t *testing.T) { | ||
| now := time.Now() | ||
| iId1 := id.NewIntegrationID() | ||
| i1 := integration.New().ID(iId1).UpdatedAt(now).MustBuild() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| seeds integration.List | ||
| arg *integration.Integration | ||
| want integration.List | ||
| wantErr error | ||
| mockErr bool | ||
| }{ | ||
| { | ||
| name: "Saved", | ||
| seeds: integration.List{}, | ||
| arg: i1, | ||
| want: integration.List{i1}, | ||
| wantErr: nil, | ||
| }, | ||
| { | ||
| name: "Saved same data", | ||
| seeds: integration.List{i1}, | ||
| arg: i1, | ||
| want: integration.List{i1}, | ||
| wantErr: nil, | ||
| }, | ||
| { | ||
| name: "must mock error", | ||
| wantErr: errors.New("test"), | ||
| mockErr: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| r := NewIntegration() | ||
| if tc.mockErr { | ||
| SetIntegrationError(r, tc.wantErr) | ||
| } | ||
| ctx := context.Background() | ||
| for _, p := range tc.seeds { | ||
| err := r.Save(ctx, p.Clone()) | ||
| if tc.wantErr != nil { | ||
| assert.ErrorIs(t, err, tc.wantErr) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| err := r.Save(ctx, tc.arg.Clone()) | ||
| if tc.wantErr != nil { | ||
| assert.ErrorIs(t, err, tc.wantErr) | ||
| return | ||
| } | ||
|
|
||
| assert.Equal(t, []*integration.Integration(tc.want), r.(*Integration).data.Values()) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical mismatch in test name versus actual method calls.
“TestIntegrationRepo_Remove” never calls the “Remove” method. Instead, it calls “Save” and verifies saved items. This appears to be reversed or a copy/paste mistake, likely preventing “Remove” from being tested as intended.
Below is a possible fix. Adjust the code to call “Remove” instead of “Save” at lines 304–305:
--- a/integration_test.go
+++ b/integration_test.go
func TestIntegrationRepo_Remove(t *testing.T) {
...
for _, tc := range tests {
...
- err := r.Save(ctx, tc.arg.Clone())
+ err := r.Remove(ctx, tc.arg.ID())
if tc.wantErr != nil {
assert.ErrorIs(t, err, tc.wantErr)
return
}
...
}
}Committable suggestion skipped: line range outside the PR's diff.
| func TestIntegrationRepo_Save(t *testing.T) { | ||
| now := time.Now() | ||
| iId1 := id.NewIntegrationID() | ||
| i1 := integration.New().ID(iId1).UpdatedAt(now).MustBuild() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| seeds integration.List | ||
| arg integration.ID | ||
| want integration.List | ||
| wantErr error | ||
| mockErr bool | ||
| }{ | ||
| { | ||
| name: "Saved", | ||
| seeds: integration.List{}, | ||
| arg: iId1, | ||
| want: integration.List{}, | ||
| wantErr: rerror.ErrNotFound, | ||
| }, | ||
| { | ||
| name: "Saved same data", | ||
| seeds: integration.List{i1}, | ||
| arg: iId1, | ||
| want: nil, | ||
| wantErr: nil, | ||
| }, | ||
| { | ||
| name: "must mock error", | ||
| wantErr: errors.New("test"), | ||
| mockErr: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| r := NewIntegration() | ||
| if tc.mockErr { | ||
| SetIntegrationError(r, tc.wantErr) | ||
| } | ||
| ctx := context.Background() | ||
| for _, p := range tc.seeds { | ||
| err := r.Save(ctx, p.Clone()) | ||
| if tc.wantErr != nil { | ||
| assert.ErrorIs(t, err, tc.wantErr) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| err := r.Remove(ctx, tc.arg) | ||
| if tc.wantErr != nil { | ||
| assert.ErrorIs(t, err, tc.wantErr) | ||
| return | ||
| } | ||
|
|
||
| assert.Equal(t, []*integration.Integration(tc.want), r.(*Integration).data.Values()) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Test name and method usage appear reversed.
Similarly, “TestIntegrationRepo_Save” invokes “Remove” (line 367). To properly test the “Save” method, call “Save” instead of “Remove,” or rename the function to match current behavior.
Here is a possible fix to switch from “Remove” to “Save” within “TestIntegrationRepo_Save”:
--- a/integration_test.go
+++ b/integration_test.go
func TestIntegrationRepo_Save(t *testing.T) {
...
for _, tc := range tests {
...
- err := r.Remove(ctx, tc.arg)
+ // Build the integration object using tc.arg, or reuse one from seeds:
+ integrationObj := integration.New().ID(tc.arg).UpdatedAt(now).MustBuild()
+ err := r.Save(ctx, integrationObj)
if tc.wantErr != nil {
assert.ErrorIs(t, err, tc.wantErr)
return
}
...
}
}Committable suggestion skipped: line range outside the PR's diff.
| func TestPublication_AssetPublic(t *testing.T) { | ||
| assert.True(t, (&Publication{assetPublic: true}).AssetPublic()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test case for false asset visibility.
The test only verifies the true case. Add a test for the false case to ensure complete coverage.
Add this test case:
func TestPublication_AssetPublic(t *testing.T) {
assert.True(t, (&Publication{assetPublic: true}).AssetPublic())
+ assert.False(t, (&Publication{assetPublic: false}).AssetPublic())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestPublication_AssetPublic(t *testing.T) { | |
| assert.True(t, (&Publication{assetPublic: true}).AssetPublic()) | |
| } | |
| func TestPublication_AssetPublic(t *testing.T) { | |
| assert.True(t, (&Publication{assetPublic: true}).AssetPublic()) | |
| assert.False(t, (&Publication{assetPublic: false}).AssetPublic()) | |
| } |
| func TestThread_Clone(t *testing.T) { | ||
| thread := &Thread{ | ||
| id: NewID(), | ||
| workspace: accountdomain.NewWorkspaceID(), | ||
| comments: []*Comment{ | ||
| {id: NewCommentID()}, | ||
| }, | ||
| } | ||
| assert.Nil(t, (*Thread)(nil).Clone()) | ||
| assert.Equal(t, thread, thread.Clone()) | ||
| assert.NotSame(t, thread, thread.Clone()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance clone testing to verify deep copy behavior
The current clone test verifies basic functionality but should also ensure that:
- The comments slice is deep copied
- Modifying the cloned thread's comments doesn't affect the original
func TestThread_Clone(t *testing.T) {
+ comment := NewComment(NewCommentID(), operator.OperatorFromUser(NewUserID()), "test")
thread := &Thread{
id: NewID(),
workspace: accountdomain.NewWorkspaceID(),
- comments: []*Comment{
- {id: NewCommentID()},
- },
+ comments: []*Comment{comment},
}
assert.Nil(t, (*Thread)(nil).Clone())
- assert.Equal(t, thread, thread.Clone())
- assert.NotSame(t, thread, thread.Clone())
+ clone := thread.Clone()
+ assert.Equal(t, thread, clone)
+ assert.NotSame(t, thread, clone)
+
+ // Verify deep copy of comments
+ clone.comments[0].content = "modified"
+ assert.NotEqual(t, thread.comments[0].content, clone.comments[0].content)
}Committable suggestion skipped: line range outside the PR's diff.
| func TestEvent_Save(t *testing.T) { | ||
| now := time.Now() | ||
| u := user.New().NewID().Email("[email protected]").Name("John").MustBuild() | ||
| a := asset.New().NewID().Project(project.NewID()).Size(100).NewUUID(). | ||
| CreatedByUser(u.ID()).Thread(id.NewThreadID()).MustBuild() | ||
| eID1 := event.NewID() | ||
| ev := event.New[any]().ID(eID1).Timestamp(now).Type(event.AssetCreate).Operator(operator.OperatorFromUser(u.ID())).Object(a).MustBuild() | ||
|
|
||
| r := NewEvent() | ||
| ctx := context.Background() | ||
| err := r.Save(ctx, ev) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, ev, r.(*Event).data.Values()[0]) | ||
|
|
||
| // already exist | ||
| _ = r.Save(ctx, ev) | ||
| assert.Equal(t, 1, len(r.(*Event).data.Values())) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test robustness and avoid implementation details.
The test has several areas for improvement:
- Accessing internal data structure via type assertion (
r.(*Event).data) couples the test to implementation details - Missing important error cases
- Could benefit from more comprehensive test cases
Consider this improved version:
func TestEvent_Save(t *testing.T) {
+ tests := []struct {
+ name string
+ ctx context.Context
+ event *event.Event[any]
+ wantErr error
+ }{
+ {
+ name: "success",
+ ctx: context.Background(),
+ event: setupTestData(t).ev,
+ wantErr: nil,
+ },
+ {
+ name: "duplicate",
+ ctx: context.Background(),
+ event: setupTestData(t).ev,
+ wantErr: nil,
+ },
+ {
+ name: "nil context",
+ ctx: nil,
+ event: setupTestData(t).ev,
+ wantErr: rerror.ErrInvalidContext,
+ },
+ {
+ name: "nil event",
+ ctx: context.Background(),
+ event: nil,
+ wantErr: rerror.ErrInvalidArgument,
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ r := NewEvent()
+ err := r.Save(tt.ctx, tt.event)
+ assert.Equal(t, tt.wantErr, err)
+
+ if tt.wantErr == nil {
+ // Verify save was successful by retrieving the event
+ saved, err := r.FindByID(tt.ctx, tt.event.ID())
+ assert.NoError(t, err)
+ assert.Equal(t, tt.event, saved)
+ }
+ })
+ }
}Instead of checking internal data structure:
- Use FindByID to verify saved state
- Add more test cases for error conditions
- Follow table-driven test pattern for better maintainability
Committable suggestion skipped: line range outside the PR's diff.
| func TestBuilder_LogoUrl(t *testing.T) { | ||
| type fields struct { | ||
| i *Integration | ||
| } | ||
| type args struct { | ||
| logoURL *url.URL | ||
| } | ||
| tests := []struct { | ||
| name string | ||
| fields fields | ||
| args args | ||
| want *Builder | ||
| }{ | ||
| { | ||
| name: "set", | ||
| fields: fields{i: &Integration{}}, | ||
| args: args{logoURL: lo.Must(url.Parse("https://sub.hugo.com/dir?p=1#test"))}, | ||
| want: &Builder{i: &Integration{logoUrl: lo.Must(url.Parse("https://sub.hugo.com/dir?p=1#test"))}}, | ||
| }, | ||
| { | ||
| name: "set", | ||
| fields: fields{i: &Integration{}}, | ||
| args: args{logoURL: nil}, | ||
| want: &Builder{i: &Integration{logoUrl: nil}}, | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| tt := tt | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| b := &Builder{ | ||
| i: tt.fields.i, | ||
| } | ||
| assert.Equalf(t, tt.want, b.LogoUrl(tt.args.logoURL), "LogoUrl(%v)", tt.args.logoURL) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestBuilder_Developer(t *testing.T) { | ||
| uId := accountdomain.NewUserID() | ||
| type fields struct { | ||
| i *Integration | ||
| } | ||
| type args struct { | ||
| developer UserID | ||
| } | ||
| tests := []struct { | ||
| name string | ||
| fields fields | ||
| args args | ||
| want *Builder | ||
| }{ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance URL validation tests.
The URL tests should verify:
- Invalid URL formats
- URL sanitization
- Security considerations (e.g., blocked schemes)
func TestBuilder_LogoUrl(t *testing.T) {
type fields struct {
i *Integration
}
type args struct {
logoURL *url.URL
}
tests := []struct {
name string
fields fields
args args
+ want *Builder
- want *Builder
+ wantErr error
}{
{
name: "set",
fields: fields{i: &Integration{}},
args: args{logoURL: lo.Must(url.Parse("https://sub.hugo.com/dir?p=1#test"))},
want: &Builder{i: &Integration{logoUrl: lo.Must(url.Parse("https://sub.hugo.com/dir?p=1#test"))}},
},
+ {
+ name: "invalid_scheme",
+ fields: fields{i: &Integration{}},
+ args: args{logoURL: lo.Must(url.Parse("file:///etc/passwd"))},
+ wantErr: ErrInvalidURLScheme,
+ },
+ {
+ name: "blocked_host",
+ fields: fields{i: &Integration{}},
+ args: args{logoURL: lo.Must(url.Parse("https://localhost/test"))},
+ wantErr: ErrBlockedHost,
+ },
+ {
+ name: "sanitize_path",
+ fields: fields{i: &Integration{}},
+ args: args{logoURL: lo.Must(url.Parse("https://example.com/../secrets"))},
+ want: &Builder{i: &Integration{logoUrl: lo.Must(url.Parse("https://example.com/secrets"))}},
+ },
}Committable suggestion skipped: line range outside the PR's diff.
| func TestBuilder_GenerateToken(t *testing.T) { | ||
| b := New() | ||
| b.GenerateToken() | ||
| assert.NotNil(t, b.i.token) | ||
| assert.True(t, strings.HasPrefix(b.i.token, "secret_")) | ||
| assert.Equal(t, 50, len(b.i.token)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance token generation tests.
The token generation test should verify:
- Token uniqueness across multiple generations
- Token entropy/randomness
- Token format compliance with security requirements
func TestBuilder_GenerateToken(t *testing.T) {
- b := New()
- b.GenerateToken()
- assert.NotNil(t, b.i.token)
- assert.True(t, strings.HasPrefix(b.i.token, "secret_"))
- assert.Equal(t, 50, len(b.i.token))
+ const iterations = 1000
+ tokens := make(map[string]bool)
+
+ for i := 0; i < iterations; i++ {
+ b := New()
+ b.GenerateToken()
+ token := b.i.token
+
+ // Verify token is not nil and follows format
+ assert.NotNil(t, token)
+ assert.True(t, strings.HasPrefix(token, "secret_"))
+ assert.Equal(t, 50, len(token))
+
+ // Verify uniqueness
+ assert.False(t, tokens[token], "Token collision detected")
+ tokens[token] = true
+
+ // Verify entropy (should contain mix of characters)
+ hasUpper := false
+ hasLower := false
+ hasDigit := false
+ hasSpecial := false
+ for _, c := range token[7:] { // Skip "secret_" prefix
+ switch {
+ case unicode.IsUpper(c):
+ hasUpper = true
+ case unicode.IsLower(c):
+ hasLower = true
+ case unicode.IsDigit(c):
+ hasDigit = true
+ case strings.ContainsRune("!@#$%^&*", c):
+ hasSpecial = true
+ }
+ }
+ assert.True(t, hasUpper && hasLower && hasDigit && hasSpecial,
+ "Token should contain mix of upper, lower, digits, and special chars")
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| func TestWebhook_Secret(t *testing.T) { | ||
| w := Webhook{} | ||
| w.SetSecret("xyz") | ||
| assert.Equal(t, "xyz", w.secret) | ||
| assert.Equal(t, "xyz", w.Secret()) | ||
| } |
There was a problem hiding this comment.
Enhance webhook secret security tests.
The secret testing should verify:
- Minimum length requirements
- Entropy requirements
- Secret storage security
func TestWebhook_Secret(t *testing.T) {
- w := Webhook{}
- w.SetSecret("xyz")
- assert.Equal(t, "xyz", w.secret)
- assert.Equal(t, "xyz", w.Secret())
+ t.Run("valid_secret", func(t *testing.T) {
+ w := Webhook{}
+ secret := "a_very_long_and_secure_secret_key_with_sufficient_entropy"
+ w.SetSecret(secret)
+ assert.Equal(t, secret, w.Secret())
+ })
+
+ t.Run("short_secret", func(t *testing.T) {
+ w := Webhook{}
+ assert.Panics(t, func() {
+ w.SetSecret("short")
+ })
+ })
+
+ t.Run("weak_secret", func(t *testing.T) {
+ w := Webhook{}
+ assert.Panics(t, func() {
+ w.SetSecret("1234567890123456") // All digits
+ })
+ })
+
+ t.Run("secret_storage", func(t *testing.T) {
+ w := Webhook{}
+ secret := "a_very_long_and_secure_secret_key_with_sufficient_entropy"
+ w.SetSecret(secret)
+
+ // Verify secret is not stored in plain text
+ assert.NotEqual(t, secret, w.secret)
+
+ // Verify secret is retrievable
+ assert.Equal(t, secret, w.Secret())
+ })
}Committable suggestion skipped: line range outside the PR's diff.
| args: args{pid1, usecasex.CursorPagination{First: lo.ToPtr(int64(1))}.Wrap()}, | ||
| want: []*asset.Asset{a1, a2}, | ||
| wantErr: nil, |
There was a problem hiding this comment.
Fix incorrect pagination test assertion.
The test case requests 1 item (First: lo.ToPtr(int64(1))) but expects 2 items []*asset.Asset{a1, a2}. This appears to be a bug in the test case.
Apply this fix:
args: args{pid1, usecasex.CursorPagination{First: lo.ToPtr(int64(1))}.Wrap()},
- want: []*asset.Asset{a1, a2},
+ want: []*asset.Asset{a1},
wantErr: nil,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| args: args{pid1, usecasex.CursorPagination{First: lo.ToPtr(int64(1))}.Wrap()}, | |
| want: []*asset.Asset{a1, a2}, | |
| wantErr: nil, | |
| args: args{pid1, usecasex.CursorPagination{First: lo.ToPtr(int64(1))}.Wrap()}, | |
| want: []*asset.Asset{a1}, | |
| wantErr: nil, |
| func TestAssetRepo_Delete(t *testing.T) { | ||
| pid1 := id.NewProjectID() | ||
| id1 := id.NewAssetID() | ||
| uid1 := accountdomain.NewUserID() | ||
| s := lo.ToPtr(asset.ArchiveExtractionStatusPending) | ||
| a1 := asset.New().NewID().Project(pid1).ArchiveExtractionStatus(s).NewUUID(). | ||
| CreatedByUser(uid1).Size(1000).Thread(id.NewThreadID()).MustBuild() | ||
| tests := []struct { | ||
| name string | ||
| seeds []*asset.Asset | ||
| arg id.AssetID | ||
| wantErr error | ||
| }{ | ||
| { | ||
| name: "Found 1", | ||
| seeds: []*asset.Asset{ | ||
| a1, | ||
| }, | ||
| arg: id1, | ||
| wantErr: nil, | ||
| }, | ||
| { | ||
| name: "Found 2", | ||
| seeds: []*asset.Asset{ | ||
| a1, | ||
| asset.New().NewID().Project(id.NewProjectID()).ArchiveExtractionStatus(s).NewUUID(). | ||
| CreatedByUser(accountdomain.NewUserID()).Size(1000).Thread(id.NewThreadID()).MustBuild(), | ||
| asset.New().NewID().Project(id.NewProjectID()).ArchiveExtractionStatus(s).NewUUID(). | ||
| CreatedByUser(accountdomain.NewUserID()).Size(1000).Thread(id.NewThreadID()).MustBuild(), | ||
| }, | ||
| arg: id1, | ||
| wantErr: nil, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| r := NewAsset() | ||
| ctx := context.Background() | ||
| for _, a := range tc.seeds { | ||
| err := r.Save(ctx, a.Clone()) | ||
| assert.NoError(t, err) | ||
| } | ||
|
|
||
| err := r.Delete(ctx, tc.arg) | ||
| if tc.wantErr != nil { | ||
| assert.ErrorIs(t, err, tc.wantErr) | ||
| return | ||
| } | ||
| assert.NoError(t, err) | ||
| _, err = r.FindByID(ctx, tc.arg) | ||
| assert.ErrorIs(t, err, rerror.ErrNotFound) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test case for deleting non-existent asset.
The test suite should include a case for attempting to delete a non-existent asset to verify the behavior in this scenario.
Add this test case:
tests := []struct {
name string
seeds []*asset.Asset
arg id.AssetID
wantErr error
}{
+ {
+ name: "Not found",
+ seeds: []*asset.Asset{},
+ arg: id.NewAssetID(),
+ wantErr: rerror.ErrNotFound,
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestAssetRepo_Delete(t *testing.T) { | |
| pid1 := id.NewProjectID() | |
| id1 := id.NewAssetID() | |
| uid1 := accountdomain.NewUserID() | |
| s := lo.ToPtr(asset.ArchiveExtractionStatusPending) | |
| a1 := asset.New().NewID().Project(pid1).ArchiveExtractionStatus(s).NewUUID(). | |
| CreatedByUser(uid1).Size(1000).Thread(id.NewThreadID()).MustBuild() | |
| tests := []struct { | |
| name string | |
| seeds []*asset.Asset | |
| arg id.AssetID | |
| wantErr error | |
| }{ | |
| { | |
| name: "Found 1", | |
| seeds: []*asset.Asset{ | |
| a1, | |
| }, | |
| arg: id1, | |
| wantErr: nil, | |
| }, | |
| { | |
| name: "Found 2", | |
| seeds: []*asset.Asset{ | |
| a1, | |
| asset.New().NewID().Project(id.NewProjectID()).ArchiveExtractionStatus(s).NewUUID(). | |
| CreatedByUser(accountdomain.NewUserID()).Size(1000).Thread(id.NewThreadID()).MustBuild(), | |
| asset.New().NewID().Project(id.NewProjectID()).ArchiveExtractionStatus(s).NewUUID(). | |
| CreatedByUser(accountdomain.NewUserID()).Size(1000).Thread(id.NewThreadID()).MustBuild(), | |
| }, | |
| arg: id1, | |
| wantErr: nil, | |
| }, | |
| } | |
| for _, tc := range tests { | |
| tc := tc | |
| t.Run(tc.name, func(t *testing.T) { | |
| t.Parallel() | |
| r := NewAsset() | |
| ctx := context.Background() | |
| for _, a := range tc.seeds { | |
| err := r.Save(ctx, a.Clone()) | |
| assert.NoError(t, err) | |
| } | |
| err := r.Delete(ctx, tc.arg) | |
| if tc.wantErr != nil { | |
| assert.ErrorIs(t, err, tc.wantErr) | |
| return | |
| } | |
| assert.NoError(t, err) | |
| _, err = r.FindByID(ctx, tc.arg) | |
| assert.ErrorIs(t, err, rerror.ErrNotFound) | |
| }) | |
| } | |
| } | |
| func TestAssetRepo_Delete(t *testing.T) { | |
| pid1 := id.NewProjectID() | |
| id1 := id.NewAssetID() | |
| uid1 := accountdomain.NewUserID() | |
| s := lo.ToPtr(asset.ArchiveExtractionStatusPending) | |
| a1 := asset.New().NewID().Project(pid1).ArchiveExtractionStatus(s).NewUUID(). | |
| CreatedByUser(uid1).Size(1000).Thread(id.NewThreadID()).MustBuild() | |
| tests := []struct { | |
| name string | |
| seeds []*asset.Asset | |
| arg id.AssetID | |
| wantErr error | |
| }{ | |
| { | |
| name: "Not found", | |
| seeds: []*asset.Asset{}, | |
| arg: id.NewAssetID(), | |
| wantErr: rerror.ErrNotFound, | |
| }, | |
| { | |
| name: "Found 1", | |
| seeds: []*asset.Asset{ | |
| a1, | |
| }, | |
| arg: id1, | |
| wantErr: nil, | |
| }, | |
| { | |
| name: "Found 2", | |
| seeds: []*asset.Asset{ | |
| a1, | |
| asset.New().NewID().Project(id.NewProjectID()).ArchiveExtractionStatus(s).NewUUID(). | |
| CreatedByUser(accountdomain.NewUserID()).Size(1000).Thread(id.NewThreadID()).MustBuild(), | |
| asset.New().NewID().Project(id.NewProjectID()).ArchiveExtractionStatus(s).NewUUID(). | |
| CreatedByUser(accountdomain.NewUserID()).Size(1000).Thread(id.NewThreadID()).MustBuild(), | |
| }, | |
| arg: id1, | |
| wantErr: nil, | |
| }, | |
| } | |
| for _, tc := range tests { | |
| tc := tc | |
| t.Run(tc.name, func(t *testing.T) { | |
| t.Parallel() | |
| r := NewAsset() | |
| ctx := context.Background() | |
| for _, a := range tc.seeds { | |
| err := r.Save(ctx, a.Clone()) | |
| assert.NoError(t, err) | |
| } | |
| err := r.Delete(ctx, tc.arg) | |
| if tc.wantErr != nil { | |
| assert.ErrorIs(t, err, tc.wantErr) | |
| return | |
| } | |
| assert.NoError(t, err) | |
| _, err = r.FindByID(ctx, tc.arg) | |
| assert.ErrorIs(t, err, rerror.ErrNotFound) | |
| }) | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
asset/assetinfrastructure/unzip/unzip_test.go (1)
14-18: Use t.TempDir for automatic cleanupInstead of manually creating and removing the temporary directory, consider using testing’s built-in t.TempDir() function. This approach automatically manages temp directory cleanup and may improve clarity.
- tempDir, err := os.MkdirTemp("", "unzip_test") - if err != nil { - t.Fatalf("Failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) + tempDir := t.TempDir()asset/assetinfrastructure/unzip/unzip.go (1)
94-133: Ensure robust file mode handling for extracted filesVerifying that the file modes in the archive are applied correctly is good practice. However, if the files originate from untrusted sources, consider normalizing or limiting the permissions for security reasons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
asset/assetinfrastructure/unzip/unzip.go(1 hunks)asset/assetinfrastructure/unzip/unzip_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
asset/assetinfrastructure/unzip/unzip_test.go
6-6: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
| import ( | ||
| "archive/zip" | ||
| "bytes" | ||
| "io/ioutil" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use “os” or “io” instead of “io/ioutil”
The “io/ioutil” package has been deprecated since Go 1.19. Please replace any usage of “io/ioutil” (including the import on line 6 and the call to “ioutil.WriteFile” on line 90) with equivalent functions in “os” or “io” packages, such as “os.WriteFile”.
Example fix:
-import (
- "archive/zip"
- "bytes"
- "io/ioutil"
- "os"
- "path/filepath"
- "testing"
-)
+import (
+ "archive/zip"
+ "bytes"
+ "io"
+ "os"
+ "path/filepath"
+ "testing"
+)
...
- return ioutil.WriteFile(zipFilePath, buf.Bytes(), 0644)
+ return os.WriteFile(zipFilePath, buf.Bytes(), 0644)Also applies to: 90-90
🧰 Tools
🪛 golangci-lint (1.62.2)
6-6: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
| totalFiles := len(reader.File) | ||
| completedFiles := 0 | ||
|
|
||
| go func() { | ||
| defer reader.Close() | ||
| defer close(au.progress) | ||
|
|
||
| for _, file := range reader.File { | ||
| au.wg.Add(1) | ||
| au.sem <- struct{}{} // acquire semaphore | ||
|
|
||
| go func(f *zip.File) { | ||
| defer au.wg.Done() | ||
| defer func() { <-au.sem }() // release semaphore | ||
|
|
||
| err := au.extractFile(f) | ||
| completedFiles++ | ||
| au.progress <- Progress{ |
There was a problem hiding this comment.
Protect shared counter “completedFiles” in concurrent environment
The variable “completedFiles” is incremented from multiple goroutines without synchronization. This can cause data races or incorrect counts. Consider using an atomic counter or locking mechanism to ensure proper concurrency control.
Example fix using atomic:
import (
"archive/zip"
"io"
"os"
"path/filepath"
"sync"
+ "sync/atomic"
)
...
var completedFiles int32
...
go func(f *zip.File) {
defer au.wg.Done()
defer func() { <-au.sem }()
err := au.extractFile(f)
- completedFiles++
+ atomic.AddInt32(&completedFiles, 1)
au.progress <- Progress{
CurrentFile: f.Name,
TotalFiles: totalFiles,
- CompletedFiles: completedFiles,
+ CompletedFiles: int(atomic.LoadInt32(&completedFiles)),
Error: err,
}
}Committable suggestion skipped: line range outside the PR's diff.
https://www.notion.so/eukarya/08-Asset-Management-Microservice-2f36dd6b8d534a768776f60fc912f879
Summary by CodeRabbit
New Features
Bug Fixes
Tests