feat(asset): add asset management functionality and validation #60
Conversation
Replaced global entropy lock with `sync.Pool` for better concurrency in ULID generation. Simplified and restructured the `asset` package by removing unused methods, consolidating responsibilities, and adding support for metadata and extraction status handling.
Updated `ID` and `nid` types to leverage pointers, improving clarity and simplifying nil handling. Adjusted associated methods, test cases, and related structures like `Set` and `List` to accommodate the refactored pointer-based approach. This improves overall code maintainability and robustness.
…s; enhance list operations with nil checks
…epository and service structure
…us asset handling
…proved comparison
…tives and clean up unused code
|
Warning Rate limit exceeded@kasugamirai has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces several significant updates across multiple packages, focusing on improving ID management, asset handling, and utility functions. Key changes include modifications to ID generation and handling in the Changes
Sequence DiagramsequenceDiagram
participant Client
participant AssetService
participant AssetRepository
participant PubSub
Client->>AssetService: Create Asset
AssetService->>AssetRepository: Save Asset
AssetRepository-->>AssetService: Asset Saved
AssetService->>PubSub: Publish Asset Event
PubSub-->>Client: Event Published
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: 6
🧹 Nitpick comments (17)
account/accountdomain/workspace/initializer.go (1)
21-22: LGTM! Consider consistent variable naming.The introduction of intermediate variables improves code readability by making ID creation explicit. For better consistency, consider aligning the variable names:
-newID := user.NewID() -p.UserID = newID.Ref() +newUserID := user.NewID() +p.UserID = newUserID.Ref()Also applies to: 25-26
go.mod (1)
53-129: Indirect Dependencies
The added and updated indirect dependencies, such as Google Cloud libraries, AWS internal packages, and x/ packages, may affect downstream functionalities (e.g., logging, credentials, or tracing). Make sure your integration tests cover these areas to catch any unexpected regressions.asset/utils.go (1)
7-9: Minimalist UUID Generation
The function “generateID” cleanly returns a UUID string without extraneous handling. While this is sufficient for most use cases, consider:
• Exporting the function (e.g., “GenerateID”) if other packages need to access it.
• Adding doc comments if it’s part of the package’s public API.
Otherwise this is appropriate and concise.asset/repository.go (1)
8-15: Repository Interface Completeness
Defining a clear interface is excellent for inversion of control. Potential improvements or considerations:
• Returning typed errors for known conditions (e.g., “AssetNotFoundErr”) to simplify error handling.
• Providing context usage guidelines to ensure consistent cancellation and deadlines.
• Ensuring concurrency safety if multiple goroutines fetch/save the same asset concurrently.asset/types.go (3)
7-7: Consider Using Strongly Typed IDs
Currently, ID is an alias for string. If collisions or format validations become a concern, consider a dedicated struct with validation logic or a custom type that ensures valid UUID patterns.
9-17: Asset Struct Initialization & Timestamps
The asset struct includes timestamps (CreatedAt, UpdatedAt).
• Ensure these fields are correctly managed (e.g., updated during create/update).
• Confirm time zones are handled consistently.
19-24: Validation for CreateAssetInput
As size and content type can vary, you might want to add basic validation (e.g., max/min size checks, allowed content types) before persisting assets.asset/pubsub/pubsub.go (1)
8-11: Consider adding event type constants.The
AssetEvent.Typeis currently a string without predefined values. Consider adding constants for common event types (e.g., ASSET_CREATED, ASSET_UPDATED, ASSET_DELETED) to prevent typos and ensure consistency.const ( AssetEventCreated = "ASSET_CREATED" AssetEventUpdated = "ASSET_UPDATED" AssetEventDeleted = "ASSET_DELETED" )asset/service.go (1)
34-56: Add transaction support and optimize Update method.The Update method could benefit from:
- Transaction support to ensure atomicity
- Optimistic locking to prevent concurrent modifications
- Validation of the updated fields
Consider implementing optimistic locking:
func (s *Service) Update(ctx context.Context, id ID, input UpdateAssetInput) (*Asset, error) { + if err := input.Validate(); err != nil { + return nil, fmt.Errorf("invalid input: %w", err) + } asset, err := s.repo.Fetch(ctx, id) if err != nil { return nil, fmt.Errorf("failed to fetch asset: %w", err) } + // Store original version for optimistic locking + originalVersion := asset.Version // Update fields... asset.UpdatedAt = time.Now() + asset.Version++ // Increment version - if err := s.repo.Save(ctx, asset); err != nil { + if err := s.repo.SaveWithVersion(ctx, asset, originalVersion); err != nil { return nil, fmt.Errorf("failed to save asset: %w", err) } return asset, nil }idx/nid.go (1)
158-163: Potentially nil receiver in UnmarshalText.
If id is a nil pointer, assigning *id = *newID will panic. Ensure that callers never invoke UnmarshalText on a nil receiver or add defensive checks.idx/ulid.go (1)
72-77: Efficient concurrency in generateAllID.
The worker approach is well-designed. Consider whether partial errors in BatchResult might need unified error handling rather than ignoring them.idx/list.go (1)
31-47: Potential performance concern with Has.
The Has method uses a nested loop (O(n*m)) for larger lists. This is often fine but can degrade performance. Consider using a set-based approach if performance is a concern.idx/set.go (1)
62-67: Consider pre-allocating the mapWhen initializing the map in the Add method, consider pre-allocating with an estimated capacity to reduce reallocations.
-s.m = map[string]ID[T]{} +s.m = make(map[string]ID[T], len(id))idx/id.go (1)
94-103: Consider documenting nil comparison behaviorThe Compare method implements a complete nil handling strategy where nil values are considered less than non-nil values. This behavior should be documented as it affects sorting and comparison operations.
Add documentation above the Compare method:
+// Compare implements comparison between two ID pointers. +// nil values are considered less than non-nil values. +// When both values are nil, they are considered equal. func (id *ID[T]) Compare(id2 *ID[T]) int {idx/set_test.go (1)
55-57: Consider adding edge case testsWhile the basic functionality is well-tested, consider adding tests for:
- Adding/merging nil values
- Concurrent access scenarios
- Large set operations
Also applies to: 67-69, 78-81, 92-94
idx/id_test.go (1)
97-98: Consider adding marshaling error testsWhile the happy path is well-tested, consider adding tests for:
- Marshaling nil ID
- Unmarshaling invalid JSON
- Concurrent marshaling scenarios
Also applies to: 106-109
util/list.go (1)
24-26: LGTM: Clean implementation using lo.SomeByThe change to use lo.SomeBy makes the code more idiomatic and clearer in intent while maintaining the same functionality.
Consider adding a brief comment explaining the method's behavior with multiple elements, e.g.:
func (l List[T]) Has(elements ...T) bool { + // Returns true if any of the provided elements exist in the list return lo.SomeBy(elements, func(e T) bool { return slices.Contains(l, e) }) }
📜 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 (25)
.gitignore(1 hunks)account/accountdomain/workspace/initializer.go(1 hunks)account/accountdomain/workspace/member.go(3 hunks)account/accountinfrastructure/accountmemory/workspace.go(1 hunks)asset/decompress/zip.go(1 hunks)asset/pubsub/pubsub.go(1 hunks)asset/repository.go(1 hunks)asset/service.go(1 hunks)asset/types.go(1 hunks)asset/utils.go(1 hunks)go.mod(1 hunks)idx/id.go(4 hunks)idx/id_test.go(2 hunks)idx/list.go(2 hunks)idx/list_test.go(2 hunks)idx/nid.go(4 hunks)idx/nid_test.go(1 hunks)idx/set.go(4 hunks)idx/set_test.go(7 hunks)idx/string.go(2 hunks)idx/string_test.go(2 hunks)idx/ulid.go(1 hunks)util/list.go(1 hunks)util/slice.go(0 hunks)util/slice_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- util/slice.go
- util/slice_test.go
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 golangci-lint (1.62.2)
asset/decompress/zip.go
32-32: func (*ZipDecompressor).processZipFile is unused
(unused)
idx/nid.go
74-74: func refNIDTo is unused
(unused)
🔇 Additional comments (32)
account/accountinfrastructure/accountmemory/workspace.go (1)
65-70: LGTM! Improved ID comparison logic.
The changes properly handle ID comparison using pointers and intermediate variables, which is both safer and more efficient by avoiding multiple method calls.
account/accountdomain/workspace/member.go (1)
85-85: LGTM! Consistent pointer comparison across sorting operations.
The changes establish a consistent pattern of pointer-based ID comparison across all sorting operations (UserIDs, IntegrationIDs, and UsersByRole), aligning with the changes in the workspace package.
Also applies to: 97-97, 258-258
go.mod (3)
3-3: Upgrade to Go 1.22.7 Considerations
Updating the Go version to 1.22.7 is beneficial for newer language features and performance improvements. Ensure that any modules or tooling pinned to earlier versions remain compatible.
Would you like me to generate a script to search for any references to older Go features that might no longer be valid?
5-5: Toolchain Specification
Defining an explicit toolchain version (go1.23.4) is helpful for reproducible builds. Confirm that your CI pipeline or build environment supports this version to avoid mismatches.
8-47: Dependency Updates
Multiple dependencies (e.g., gqlgen, opentelemetry, AWS SDK, Echo, testify, etc.) have been upgraded. Generally, this is good practice but may introduce breaking changes or deprecations. Check associated changelogs, especially for major or minor version increments, to ensure continued compatibility and that any new features or migration steps have been applied.
✅ Verification successful
Based on the search results, I can generate the final response as I have enough information to verify the dependency updates. The codebase shows active usage of the updated dependencies:
Dependency Updates Look Safe
The codebase shows proper integration with the updated dependencies:
gqlgenandotelgqlgenare used correctly inappx/gql.go- OpenTelemetry packages are properly integrated in
appx/tracer.go - AWS SDK v2 is used correctly in
mailer/aws_ses.go - Echo framework is properly used in multiple files like
log/echo.go,authserver/endpoint.go - MongoDB driver is used extensively throughout the codebase in
mongox/*packages - Testify is used consistently across test files with proper assertions
The usage patterns align with the APIs of the updated versions and there are no obvious compatibility issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# This script checks for references to old APIs in code, helping detect regressions after dependency updates.
rg -A 8 '(gqlgen|opentelemetry|aws-sdk-go-v2|echo|testify|mongo-driver|opentelemetry.io/otel)'
Length of output: 65849
asset/types.go (1)
25-29: Optional Fields in UpdateAssetInput
Using pointers in UpdateAssetInput helps distinguish between “not set” and “set to empty.” This is a solid approach for partial updates.
idx/nid.go (3)
18-21: Return an empty ID when nid is nil.
This approach is consistent with the rest of the codebase, returning an empty ID struct on nil. No issues here.
25-26: Ensure newNID won’t panic in loops.
Here, newNIDs calls newNID in a loop. Double-check that all items in ids have valid .nid fields to avoid potential nil pointer dereference in newNID.
14-15: Guard against possible nil pointer dereference.
If i.nid happens to be nil, accessing i.nid.id could cause a runtime panic. Consider adding a nil check to prevent unexpected crashes.
idx/ulid.go (3)
20-26: Appropriate usage of sync.Pool and customized error.
The definition of entropyPool for ULID generation and the ErrInvalidID error seem well-structured. This is a solid concurrency and error-handling approach.
32-35: Good use of pooled entropy for ID generation.
Leveraging sync.Pool to avoid repeated allocations is a performance win.
96-103: Validating length and uppercase in parseID.
Checking for exactly 26 characters and uppercase characters is correct for ULIDs. Properly returns ErrInvalidID when invalid.
idx/list.go (4)
53-55: Safe At method.
Returning nil on out-of-bounds access is safer than panicking.
97-110: Clear and straightforward Delete.
Good approach to building a result slice and excluding matches, maintaining readability.
189-203: MoveAt handles partial shifts well.
Copying the slice segments ensures correct reordering. The logic is solid.
258-266: Deep copy in Clone.
Cloning each nid is essential for avoiding unintended side effects in the cloned list. Nicely done.
idx/string.go (2)
13-17: Ref method handles nil & empty strings safely.
Returning nil for nil or empty string pointers avoids confusion.
28-32: String method gracefully defaults to empty.
Returning "" on a nil pointer is user-friendly. No issues here.
idx/string_test.go (2)
16-20: LGTM! Improved pointer comparison logic
The change from using lo.ToPtr to direct pointer comparison simplifies the code while maintaining the same functionality. This is a good improvement in test clarity.
32-36: LGTM! Good addition of nil pointer handling test
The addition of explicit nil pointer handling test improves the test coverage and ensures proper behavior with nil values.
idx/nid_test.go (3)
15-15: LGTM! Updated assertion to match pointer semantics
The assertion now correctly compares slices of pointers, maintaining consistency with the updated type system.
27-31: LGTM! Good addition of nil handling test for Text marshaling
The addition of explicit nil handling test for Text marshaling improves the robustness of the implementation.
42-46: LGTM! Good addition of nil handling test for JSON marshaling
The addition of explicit nil handling test for JSON marshaling ensures proper serialization behavior.
idx/set.go (3)
21-24: LGTM! Improved nil handling
Good addition of nil checks before accessing ID properties. This prevents potential panics and makes the code more robust.
Also applies to: 59-61, 98-100
42-51: LGTM! Proper deep clone implementation
The Clone method now properly creates a deep copy of the map and list, preventing any shared state between original and cloned sets.
5-5: Consider performance implications of using string keys
The change from map[ID[T]]struct{} to map[string]ID[T] might impact performance due to:
- Additional string allocations for keys
- Increased memory usage (storing full IDs vs empty structs)
Consider benchmarking to evaluate the performance impact.
Also applies to: 9-11
idx/id.go (2)
66-70: LGTM: Robust nil handling in methods
The nil handling in Clone and GoString methods is comprehensive and consistent. The implementation properly handles both nil receiver and nil nid field cases.
Also applies to: 87-90
13-17:
Breaking change: nid field changed to pointer type
This is a significant breaking change that affects all users of the ID type. While the change improves memory efficiency and enables proper nil handling, it requires careful migration from existing codebases.
Let's verify the impact on existing usages:
✅ Verification successful
Change to pointer type is properly handled throughout the codebase
The change of nid to a pointer type has been properly implemented with:
- Nil checks added in all relevant places (e.g.,
IsNil(), list operations) - Proper pointer handling in comparison operations
- Safe cloning with nil checks
- Consistent nil-safety across all files that use the
nidfield
The changes maintain correctness while improving memory efficiency. All direct accesses to .nid are already properly guarded with nil checks, making this a safe change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct field access to nid that might break
rg -l "\.nid\." --type go | grep -v "_test.go"
Length of output: 79
Script:
#!/bin/bash
# Let's examine the actual usage of .nid in these files
echo "=== id.go ==="
rg "\.nid\." idx/id.go -A 2 -B 2
echo -e "\n=== nid.go ==="
rg "\.nid\." idx/nid.go -A 2 -B 2
echo -e "\n=== list.go ==="
rg "\.nid\." idx/list.go -A 2 -B 2
Length of output: 1079
idx/set_test.go (1)
13-18: LGTM: Map initialization and key handling
The change to use string keys in the map is well-tested, with proper initialization and key generation using String() method.
idx/id_test.go (1)
19-19: LGTM: Comprehensive test coverage for pointer semantics
The tests properly verify both the pointer initialization and the underlying ULID value, ensuring complete coverage of the new pointer-based implementation.
Also applies to: 24-25, 31-34
idx/list_test.go (2)
39-39: LGTM: Good test coverage enhancement
Added test case verifies the variadic functionality of the Has method.
226-230: LGTM: Improved test robustness
The changes enhance the clone test by:
- Storing the cloned list once, improving performance
- Adding explicit instance comparison with NotSame
- Making the test more readable
| func (p *AssetPubSub) PublishAssetEvent(ctx context.Context, eventType string, assetID asset.ID) error { | ||
| event := AssetEvent{ | ||
| Type: eventType, | ||
| AssetID: assetID, | ||
| } | ||
| return p.publisher.Publish(ctx, p.topic, event) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for eventType parameter.
The PublishAssetEvent method accepts any string as eventType without validation. Consider adding validation to ensure only valid event types are published.
func (p *AssetPubSub) PublishAssetEvent(ctx context.Context, eventType string, assetID asset.ID) error {
+ if eventType == "" {
+ return errors.New("event type cannot be empty")
+ }
event := AssetEvent{
Type: eventType,
AssetID: assetID,
}
return p.publisher.Publish(ctx, p.topic, event)
}| func (d *ZipDecompressor) processZipFile(ctx context.Context, zipReader *zip.Reader) error { | ||
| // Process each file in the zip | ||
| // Create new assets for each file | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Implement the processZipFile method or remove if unused.
The processZipFile method is currently a stub and marked as unused by static analysis.
Either implement the method with proper ZIP file processing logic or remove it if not needed. Consider:
func (d *ZipDecompressor) processZipFile(ctx context.Context, zipReader *zip.Reader) error {
for _, file := range zipReader.File {
select {
case <-ctx.Done():
return ctx.Err()
default:
if err := d.processZipEntry(ctx, file); err != nil {
return fmt.Errorf("failed to process %s: %w", file.Name, err)
}
}
}
return nil
}🧰 Tools
🪛 golangci-lint (1.62.2)
32-32: func (*ZipDecompressor).processZipFile is unused
(unused)
| func (d *ZipDecompressor) DecompressAsync(ctx context.Context, assetID asset.ID) error { | ||
| // Get the zip file from asset service | ||
| zipFile, err := d.assetService.GetFile(ctx, assetID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer zipFile.Close() | ||
|
|
||
| // Create a temporary file to store the zip content | ||
| // Implementation of async zip extraction | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Implementation incomplete and missing error handling.
The DecompressAsync method:
- Returns nil without actually processing the ZIP file
- Missing ZIP reader initialization
- Should handle context cancellation
Consider implementing the missing functionality:
func (d *ZipDecompressor) DecompressAsync(ctx context.Context, assetID asset.ID) error {
zipFile, err := d.assetService.GetFile(ctx, assetID)
if err != nil {
return fmt.Errorf("failed to get zip file: %w", err)
}
defer zipFile.Close()
+ // Create zip reader
+ zipReader, err := zip.NewReader(zipFile, -1)
+ if err != nil {
+ return fmt.Errorf("failed to create zip reader: %w", err)
+ }
+
+ // Process asynchronously
+ go func() {
+ if err := d.processZipFile(ctx, zipReader); err != nil {
+ // Consider implementing error callback or logging
+ log.Printf("Error processing zip file: %v", err)
+ }
+ }()
return nil
}Committable suggestion skipped: line range outside the PR's diff.
| func (s *Service) Upload(ctx context.Context, id ID, file io.Reader) error { | ||
| return s.repo.Upload(ctx, id, file) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add file validation in Upload method.
The Upload method should validate the file before uploading:
- Check file size limits
- Validate content type if applicable
- Consider adding progress tracking for large files
func (s *Service) Upload(ctx context.Context, id ID, file io.Reader) error {
+ // Wrap reader with size limitation
+ limitedReader := io.LimitReader(file, MaxFileSize)
+
+ // Optionally wrap with progress tracking
+ progressReader := &ProgressReader{Reader: limitedReader}
+
- return s.repo.Upload(ctx, id, file)
+ return s.repo.Upload(ctx, id, progressReader)
}Committable suggestion skipped: line range outside the PR's diff.
| func (s *Service) Create(ctx context.Context, input CreateAssetInput) (*Asset, error) { | ||
| asset := &Asset{ | ||
| ID: ID(generateID()), | ||
| Name: input.Name, | ||
| Size: input.Size, | ||
| ContentType: input.ContentType, | ||
| CreatedAt: time.Now(), | ||
| UpdatedAt: time.Now(), | ||
| } | ||
|
|
||
| if err := s.repo.Save(ctx, asset); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return asset, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and validation in Create method.
The Create method needs additional validation and error wrapping:
- Input validation is missing
- ID generation error handling is not implemented
- Error messages could be more descriptive
func (s *Service) Create(ctx context.Context, input CreateAssetInput) (*Asset, error) {
+ if err := input.Validate(); err != nil {
+ return nil, fmt.Errorf("invalid input: %w", err)
+ }
+
+ id, err := generateID()
+ if err != nil {
+ return nil, fmt.Errorf("failed to generate ID: %w", err)
+ }
asset := &Asset{
- ID: ID(generateID()),
+ ID: ID(id),
Name: input.Name,
Size: input.Size,
ContentType: input.ContentType,
CreatedAt: time.Now(),
UpdatedAt: time.Now(),
}
if err := s.repo.Save(ctx, asset); err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to save asset: %w", err)
}
return asset, 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.
| func (s *Service) Create(ctx context.Context, input CreateAssetInput) (*Asset, error) { | |
| asset := &Asset{ | |
| ID: ID(generateID()), | |
| Name: input.Name, | |
| Size: input.Size, | |
| ContentType: input.ContentType, | |
| CreatedAt: time.Now(), | |
| UpdatedAt: time.Now(), | |
| } | |
| if err := s.repo.Save(ctx, asset); err != nil { | |
| return nil, err | |
| } | |
| return asset, nil | |
| } | |
| func (s *Service) Create(ctx context.Context, input CreateAssetInput) (*Asset, error) { | |
| if err := input.Validate(); err != nil { | |
| return nil, fmt.Errorf("invalid input: %w", err) | |
| } | |
| id, err := generateID() | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to generate ID: %w", err) | |
| } | |
| asset := &Asset{ | |
| ID: ID(id), | |
| Name: input.Name, | |
| Size: input.Size, | |
| ContentType: input.ContentType, | |
| CreatedAt: time.Now(), | |
| UpdatedAt: time.Now(), | |
| } | |
| if err := s.repo.Save(ctx, asset); err != nil { | |
| return nil, fmt.Errorf("failed to save asset: %w", err) | |
| } | |
| return asset, nil | |
| } |
| func refNIDTo[T Type](n *nid) *ID[T] { | ||
| if n == nil { | ||
| return nil | ||
| } | ||
| nid2 := nidTo[T](*n) | ||
| nid2 := nidTo[T](n) | ||
| return &nid2 | ||
| } | ||
|
|
||
| func (id nid) Ref() *nid { | ||
| return &id | ||
| } | ||
|
|
||
| func (id nid) Clone() nid { | ||
| return nid{id: id.id} | ||
| func (id *nid) Ref() *nid { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function refNIDTo is unused.
Static analysis indicates that refNIDTo is unused. Consider removing it if it has no planned usage.
-func refNIDTo[T Type](n *nid) *ID[T] {
- if n == nil {
- return nil
- }
- nid2 := nidTo[T](n)
- return &nid2
-}📝 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 refNIDTo[T Type](n *nid) *ID[T] { | |
| if n == nil { | |
| return nil | |
| } | |
| nid2 := nidTo[T](*n) | |
| nid2 := nidTo[T](n) | |
| return &nid2 | |
| } | |
| func (id nid) Ref() *nid { | |
| return &id | |
| } | |
| func (id nid) Clone() nid { | |
| return nid{id: id.id} | |
| func (id *nid) Ref() *nid { | |
| func (id *nid) Ref() *nid { |
🧰 Tools
🪛 golangci-lint (1.62.2)
74-74: func refNIDTo is unused
(unused)
…n and simplifying insertion and movement logic
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
idx/list.go (3)
40-58: Consider using a map for better performanceThe current implementation has O(n*m) complexity where n is the list length and m is the number of IDs to check. For better performance, consider using a temporary map for lookups.
func (l List[T]) Has(ids ...ID[T]) bool { if l == nil || len(ids) == 0 { return false } + // Create temporary map for O(1) lookups + idMap := make(map[string]struct{}, len(l)) + for _, lid := range l { + if lid.nid != nil { + idMap[lid.String()] = struct{}{} + } + } for _, id := range ids { if id.nid == nil { continue } - found := false - for _, lid := range l { - if lid.nid != nil && lid.nid.Compare(id.nid) == 0 { - found = true - break - } - } - if !found { + if _, exists := idMap[id.String()]; !exists { return false } } return true }
298-302: Pre-allocate map capacity for better performanceConsider pre-allocating the map with the expected capacity to avoid rehashing.
s := &Set[T]{ - m: make(map[string]ID[T]), + m: make(map[string]ID[T], len(l)), }
Line range hint
1-302: Consider architectural improvements for better maintainability and performanceSeveral architectural improvements could enhance the package:
- Consistent Error Handling: Consider returning errors instead of silently handling invalid states
- Concurrency: For large lists, operations like Has, Intersect could benefit from concurrent processing
- Builder Pattern: Consider implementing a builder pattern for chaining operations
- Documentation: Add examples and performance characteristics to method documentation
Would you like me to provide specific implementation details for any of these suggestions?
idx/list.go
Outdated
| // Convert util.List[nid] to []*nid | ||
| convertedList := convertToNidSlice(l.list().Move(*newNID(e), to)) | ||
| return nidsTo[T](convertedList) | ||
| } | ||
|
|
||
| func (l List[T]) MoveAt(from, to int) List[T] { | ||
| if l == nil { | ||
| return nil | ||
| } | ||
|
|
||
| return nidsTo[T](l.list().MoveAt(from, to)) | ||
| // Convert util.List[nid] to []*nid | ||
| convertedList := convertToNidSlice(l.list().MoveAt(from, to)) | ||
| return nidsTo[T](convertedList) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Move operations and remove unnecessary conversions
The current implementation creates unnecessary intermediate copies through conversions. Consider implementing the move operations directly.
func (l List[T]) Move(e ID[T], to int) List[T] {
if l == nil {
return nil
}
- // Convert util.List[nid] to []*nid
- convertedList := convertToNidSlice(l.list().Move(*newNID(e), to))
- return nidsTo[T](convertedList)
+ from := l.Index(e)
+ if from == -1 || to < 0 || to >= len(l) {
+ return l
+ }
+ return l.MoveAt(from, to)
}
func (l List[T]) MoveAt(from, to int) List[T] {
if l == nil {
return nil
}
+ if from < 0 || from >= len(l) || to < 0 || to >= len(l) || from == to {
+ return l
+ }
+ result := l.Clone()
+ item := result[from]
+ if from < to {
+ copy(result[from:], result[from+1:to+1])
+ } else {
+ copy(result[to+1:], result[to:from])
+ }
+ result[to] = item
+ return result
- // Convert util.List[nid] to []*nid
- convertedList := convertToNidSlice(l.list().MoveAt(from, to))
- return nidsTo[T](convertedList)
}
-// Helper function to convert util.List[nid] to []*nid
-func convertToNidSlice(list util.List[nid]) []*nid {
- var result []*nid
- for _, item := range list {
- result = append(result, &item)
- }
- return result
-}Also applies to: 189-195
| nids := newNIDs(l) | ||
| values := make([]nid, len(nids)) | ||
| for i, n := range nids { | ||
| values[i] = *n | ||
| } | ||
| return util.List[nid](values) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add nil checks and optimize memory usage
The current implementation could panic if newNIDs returns any nil pointers. Consider adding nil checks and reducing unnecessary copying.
func (l List[T]) list() util.List[nid] {
nids := newNIDs(l)
values := make([]nid, 0, len(nids))
for _, n := range nids {
+ if n == nil {
+ continue
+ }
values = append(values, *n)
}
return util.List[nid](values)
}📝 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.
| nids := newNIDs(l) | |
| values := make([]nid, len(nids)) | |
| for i, n := range nids { | |
| values[i] = *n | |
| } | |
| return util.List[nid](values) | |
| nids := newNIDs(l) | |
| values := make([]nid, 0, len(nids)) | |
| for _, n := range nids { | |
| if n == nil { | |
| continue | |
| } | |
| values = append(values, *n) | |
| } | |
| return util.List[nid](values) |
| nids := newNIDs(ids) | ||
| values := make([]nid, len(nids)) | ||
| for i, n := range nids { | ||
| values[i] = *n | ||
| } | ||
| inserted := l.list().Insert(i, values...) | ||
| pointers := make([]*nid, len(inserted)) | ||
| for i, v := range inserted { | ||
| pointers[i] = &v | ||
| } | ||
| return nidsTo[T](pointers) |
There was a problem hiding this comment.
Add index validation and optimize memory usage
The method should validate the input index and could be optimized to reduce allocations.
func (l List[T]) Insert(i int, ids ...ID[T]) List[T] {
+ if i < 0 || (l != nil && i > len(l)) {
+ return l
+ }
+ if l == nil {
+ return append(List[T]{}, ids...)
+ }
- nids := newNIDs(ids)
- values := make([]nid, len(nids))
- for i, n := range nids {
- values[i] = *n
- }
- inserted := l.list().Insert(i, values...)
- pointers := make([]*nid, len(inserted))
- for i, v := range inserted {
- pointers[i] = &v
- }
- return nidsTo[T](pointers)
+ result := make(List[T], len(l)+len(ids))
+ copy(result[:i], l[:i])
+ copy(result[i:], ids)
+ copy(result[i+len(ids):], l[i:])
+ return result
}📝 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.
| nids := newNIDs(ids) | |
| values := make([]nid, len(nids)) | |
| for i, n := range nids { | |
| values[i] = *n | |
| } | |
| inserted := l.list().Insert(i, values...) | |
| pointers := make([]*nid, len(inserted)) | |
| for i, v := range inserted { | |
| pointers[i] = &v | |
| } | |
| return nidsTo[T](pointers) | |
| if i < 0 || (l != nil && i > len(l)) { | |
| return l | |
| } | |
| if l == nil { | |
| return append(List[T]{}, ids...) | |
| } | |
| result := make(List[T], len(l)+len(ids)) | |
| copy(result[:i], l[:i]) | |
| copy(result[i:], ids) | |
| copy(result[i+len(ids):], l[i:]) | |
| return result |
| if l[i].nid == nil || l[j].nid == nil { | ||
| return false | ||
| } | ||
| return l[i].nid.Compare(l[j].nid) < 0 |
There was a problem hiding this comment.
Add bounds checking and clarify nil handling behavior
The Less method should include bounds checking and have a more explicit handling of nil values.
func (l List[T]) Less(i, j int) bool {
+ if i < 0 || i >= len(l) || j < 0 || j >= len(l) {
+ return false
+ }
+ // nil values are considered greater than non-nil values
+ if l[i].nid == nil {
+ return false
+ }
+ if l[j].nid == nil {
+ return true
+ }
- if l[i].nid == nil || l[j].nid == nil {
- return false
- }
return l[i].nid.Compare(l[j].nid) < 0
}📝 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.
| if l[i].nid == nil || l[j].nid == nil { | |
| return false | |
| } | |
| return l[i].nid.Compare(l[j].nid) < 0 | |
| func (l List[T]) Less(i, j int) bool { | |
| if i < 0 || i >= len(l) || j < 0 || j >= len(l) { | |
| return false | |
| } | |
| // nil values are considered greater than non-nil values | |
| if l[i].nid == nil { | |
| return false | |
| } | |
| if l[j].nid == nil { | |
| return true | |
| } | |
| return l[i].nid.Compare(l[j].nid) < 0 | |
| } |
…ty functions for improved readability and performance
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
go.modfile.Chores