improve thread read performance #348
Conversation
split nested query into separate concurrent queries
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis pull request introduces significant changes across multiple packages, focusing on enhancing data mapping, lookup, and relationship handling. The modifications primarily involve creating new types like Changes
Sequence DiagramsequenceDiagram
participant Client
participant Mapper
participant Repository
participant Database
Client->>Mapper: Request data mapping
Mapper->>Repository: Fetch related data
Repository->>Database: Execute queries
Database-->>Repository: Return data
Repository-->>Mapper: Provide lookup data
Mapper->>Mapper: Transform and map data
Mapper-->>Client: Return mapped objects
Possibly related PRs
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: 0
🧹 Nitpick comments (6)
app/resources/tag/tag.go (1)
54-55: Commented outItemsfield might indicate dead code.
If theItemsfield is no longer required for performance, remove theitemsappending logic to avoid confusion. If it will be used in the future, consider documenting why it is commented out.internal/ent/mutation.go (1)
18715-18715: Clarify naming consistency between “author” and “account_posts”.
Clearing the "author" edge also clearspost.FieldAccountPostshere. This is presumably intentional, but it could confuse maintainers if the naming diverges or if the fields represent different concepts.internal/ent/schema/post.go (1)
41-41: Consider renaming or clarifying usage ofaccount_posts.Currently, the newly introduced field is named
account_posts, which might not align intuitively with storing a single account ID (xid.ID{}). Consider renaming it to something more descriptive, for exampleaccount_id, to improve clarity and consistency. If storing multiple accounts is intended in the future, a different type (like a multi-value field) may be more suitable.internal/infrastructure/db/db.go (2)
22-22: Dependency onzapintroduced.
The newloggerdependency is a valuable addition to track queries and mutations. Ensure the logging level is configurable in different environments (development vs. production) to avoid verbosity or performance overhead.
70-113: Commented-out logging interceptor code.
The logging interceptor code is helpful for debugging but is currently disabled. If it’s not needed immediately, consider removing it or marking it with aTODOcomment so others are aware of its purpose and potential usage.internal/ent/post_create.go (1)
531-533: Validate necessity of requiring "account_posts" field.
This enforces the field as mandatory during creation. If there's a use case where a post might not belong to a specific account, consider making this field nullable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (23)
app/resources/account/account.go(2 hunks)app/resources/account/mapping.go(1 hunks)app/resources/event/event.go(1 hunks)app/resources/post/reaction/react.go(3 hunks)app/resources/post/reply/db.go(3 hunks)app/resources/post/reply/reply.go(4 hunks)app/resources/post/thread/db.go(6 hunks)app/resources/post/thread/thread.go(3 hunks)app/resources/profile/profile.go(1 hunks)app/resources/tag/tag.go(2 hunks)internal/ent/account_query.go(2 hunks)internal/ent/category_query.go(0 hunks)internal/ent/er.html(1 hunks)internal/ent/link_query.go(0 hunks)internal/ent/mutation.go(8 hunks)internal/ent/post.go(4 hunks)internal/ent/post/post.go(3 hunks)internal/ent/post/where.go(2 hunks)internal/ent/post_create.go(6 hunks)internal/ent/post_query.go(2 hunks)internal/ent/post_update.go(2 hunks)internal/ent/schema/post.go(2 hunks)internal/infrastructure/db/db.go(3 hunks)
💤 Files with no reviewable changes (2)
- internal/ent/category_query.go
- internal/ent/link_query.go
🔇 Additional comments (86)
app/resources/post/thread/thread.go (7)
9-9: Import forgithub.com/rs/xidlooks good.
This package provides a concise way to generate and parse globally unique IDs, which can help avoid collisions across distributed systems.
11-11: Import forgithub.com/Southclaws/storyden/app/resources/accountis appropriate.
This aligns with the new lookups for account data in theMapperfunction.
52-99: NewMapfunction is well-structured and consistent with the existing patterns.
Constructing theThreadobject directly froment.Postis straightforward, and the error handling withfault.Wrapis good. The approach of mapping category, link, content, and profile is clear and maintainable.
101-107:Mapperfunction signature is clear.
Defining a higher-order function that provides a specialized mapper is a nice approach for injecting additional lookups (accounts, likes, etc.).
108-113: Category handling remains correct.
The retrieval of theCategoryedge and graceful error handling continues the pattern established inMap().
124-132: Potential nil pointer when accessing the author fromam[m.AccountPosts].
Ifam[m.AccountPosts]is nil, dereferencing it will panic. If the schema or usage guarantees the presence of an account for each post, this should be fine. Otherwise, consider a nil check or fallback logic.
135-135: Reaction lookup is straightforward.
Retrieving the set of reactions viarl[xid.ID(m.ID)]is a seamless way to gather them without re-querying.app/resources/tag/tag.go (1)
35-35: Switch tothread.Mapis consistent and maintains error handling.
Replacing the old call withthread.Mapunifies the new mapping approach across the codebase.app/resources/post/reaction/react.go (3)
19-19: Newtargetfield improves association clarity.
Storing the target ID directly in eachReacthelps group them efficiently by post or entity.
22-28:Reactstype andMap()method provide a clean way to group reactions.
Usinglo.GroupByontargetis concise and well-suited for reusing grouped data.
52-63: Check for missing accounts inMapper.
Ifam[xid.ID(in.AccountID)]returns nil, dereferencing it as*accwill cause a panic. Ensure the lookup is guaranteed by schema or handle the nil case.app/resources/account/account.go (1)
50-52:Accounts.Map()leverageslo.KeyByeffectively.
This method cleanly creates a lookup map keyed by account ID. Just be aware of potential collisions if multipleAccountobjects share the same ID (which is unlikely if IDs are unique).app/resources/event/event.go (1)
44-44: Switch tothread.Mapis coherent with the broader refactor.
Callingthread.Map(threadEdge)aligns event mapping with the new thread-mapping approach across the codebase.app/resources/profile/profile.go (1)
51-51: Double-check the removal of error handling for AccountRoles.By directly assigning
rolesEdge := a.Edges.AccountRoleswithoutAccountRolesOrErr(), the code now silently ignores any missing or invalid roles data. Ifa.Edges.AccountRolesmight be missing or inconsistent, it could cause downstream errors. Consider verifying that the roles edge always exists and is valid in this context.app/resources/account/mapping.go (1)
19-19: Confirm the absence of role edge error handling.Similar to
profile.go, the direct assignmentrolesEdge := a.Edges.AccountRolesremoves any checks for potential edge retrieval errors. If there's a scenario whereAccountRolesdata is incomplete or unavailable, this might create silent failures. Verify that no upstream logic depends on the oldAccountRolesOrErr()behavior.app/resources/post/thread/db.go (30)
16-16: Add concurrency library import.
No issues with introducing"github.com/alitto/pond/v2"for parallel task management.
20-21: Add new utility and logging imports.
These imports ("github.com/samber/lo"and"go.uber.org/zap") are common and stable.
24-31: Additional resource imports.
Imports for assets, reactions, and tags look consistent with the rest of the file.
33-34: Ent account and asset imports.
No issues found; matches likely usage for ent-based queries.
38-39: Import ent reaction and tag.
Looks correct for referencing ent-based reaction and tag entities.
43-44: Add logger and db fields to the database struct.
Elevates maintainability by centralizing the logger.
48-53: New constructor function with logger injection.
This approach is good for dependency injection and keeps logging consistent across operations.
128-128: Return mapped thread object in Create.
UsingMap(p)aligns with the new mapping approach.
166-166: Use of Map again in Update.
Consistent usage for mapping query results.
232-232: Ordering assets by updated time then created time.
Verify that this meets your intended sort order: updated first, then created.
285-285: Instantiate mapper with nil parameters.
Confirm that theMapperfunction correctly handles nil values to avoid potential runtime errors.
343-347: Initialize a new pond group and set up logger context.
This concurrent group creation is a solid design choice for parallel queries.
349-354: Create a logduration helper function.
A concise way to measure and log execution time of concurrent tasks.
356-368: Concurrent retrieval of reply stats.
Well-structured concurrency block with proper error wrapping.
369-383: Concurrent retrieval of likes.
Follows the same concurrency pattern; no concerns.
384-398: Concurrent retrieval of collection statuses.
Implementation appears consistent and correct.
399-412: Concurrent retrieval of tags.
Same concurrency approach, error handling is good.
413-426: Concurrent retrieval of assets.
No concerns with the parallel fetching pattern.
427-449: Concurrent retrieval of replies.
Logic aligns with the rest of the concurrency approach.
450-477: Concurrent retrieval of the thread root.
Error handling is robust.
478-479: Wait for all concurrent tasks to finish.
Ensures data consistency before proceeding.
484-503: Build combined post lists and deduplicate account IDs.
Use oflo.Uniqis concise and effective.
504-508: Query relevant account edges.
Code correctly fetches all referenced accounts.
513-515: Map ent accounts to domain accounts.
No issues beyond verifying that mapping covers all necessary fields.
518-519: Construct account lookup.
Straightforward approach for quick account references.
520-529: Combine all data using domain-specific mappers.
Keeps reaction, like, and collection data organized.
531-534: Map replies with concurrency results.
Error handling is consistent; no concerns.
536-536: Obtain the main thread object with threadMapper.
Implementation fits the established mapping pattern.
541-543: Prepare pagination info for replies.
The pagination logic seems correct.
545-546: Attach tags and assets to the final thread object.
No concerns, final data struct is updated appropriately.internal/ent/mutation.go (7)
18573-18608: Implementation follows standard Ent mutation patterns.
These methods for setting, retrieving, and resetting the "account_posts" field (stored internally asm.author) appear consistent with Ent’s generated code approach. The logic is sound, and the error handling forOldAccountPostslooks correct.
19455-19455: Good capacity increment for new field.
Allocating a capacity of 17 accommodates the new "account_posts" field. This minor detail supports efficient appending.
19498-19500: Conditionally appending the "account_posts" field is correct.
This ensures the field is included only when it’s set, avoiding unintended updates.
19543-19544: Switch case to retrieve the "account_posts" field is valid.
This maintains consistency with other fields within the same logical block.
19586-19587: Switch case to retrieve old "account_posts" value is correct.
Relying onOldAccountPosts(ctx)ensures the historical value is fetched only when valid forUpdateOne.
19699-19705: Type safety for setting "account_posts" is well-implemented.
Forcing a cast toxid.IDprevents runtime type mismatches and ensures consistent data handling.
19868-19870: Reset logic aligns with Ent’s mutation pattern.
Clearingm.authorinResetAccountPostsproperly undoes any mutation to the field, preserving data integrity.internal/ent/schema/post.go (1)
51-51: Enforce correct integrity checks on the new field.This edge uses
.Field("account_posts")for theauthorrelationship. Verify that the database enforces foreign key constraints or validations to ensure the field references a valid account, preventing orphan records.app/resources/post/reply/db.go (3)
87-87: ReplacingFromModel(nil)(p)withMap(p)
The switch to a directMap(p)call looks simpler. However, ensure that all fields previously handled byFromModelare now correctly mapped byMapto prevent any missing data.
110-110: Retaining consistency with existing business logic.
Again, returningMap(p)is consistent with the new approach. Verify that any filtering or transformations that previously occurred inFromModelhave been moved into theMapfunction if needed.
148-148: Confirm the deprecation ofFromModelusage.
This change finalizes the migration toMap. Ensure the old function is fully removed or marked as deprecated in code comments to discourage future use.app/resources/post/reply/reply.go (3)
10-10: Dependency import added.
Importing"github.com/Southclaws/storyden/app/resources/account"makes sense if you are now leveraging account lookups here. Confirm that this import is needed in all relevant build configs and not left unused in certain build variants.
69-102: NewMapfunction to transforment.PostintoReply.
This function is straightforward, focusing on converting anent.Postinto aReplyby extracting author, rich text content, and optional reply references. It’s a good approach to keep the function short and clear.
- Ensure error handling covers all required fields (e.g., if
Edges.Authoris missing).- Double-check whether the
DeletedAtfield is relevant beyond soft-deletes in your domain logic.
Line range hint
104-133: NewMapperfunction for advanced mappings.
This function extends theMaplogic with account, likes, and reaction lookups.
- The parameter-based approach (
am,ls,rl) introduces a clear separation of concerns.- Confirm that concurrency or caching strategies for these lookups are not inadvertently impacted by heavy usage in production.
internal/infrastructure/db/db.go (1)
61-61:newEntClientsignature updated to accept alogger.
Accepting a logger innewEntClientis a clean way to integrate structured logging. Make sure that any existing callers ofnewEntClientare updated accordingly, preventing compilation or injection failures.internal/ent/post.go (5)
53-54: Well-structured field addition forAccountPosts.Adding
AccountPosts xid.IDmakes the relationship clearer. Ensure external usage references this field consistently.
61-62: No issues noted.Lines declaring
EdgesandselectValuesremain consistent with the ent code generation pattern.
260-260: Good inclusion inscanValues.Including
post.FieldAccountPostsin thescanValuesswitch statement correctly ensures that the field is recognized for scanning.
371-376: Correct assignment logic.The assignment logic for
post.FieldAccountPostsappears correct, properly handling the scanned value.
546-548: EnhancedString()representation.Including
account_postsin the string builder improves debuggability and clarity of object logs.internal/ent/post/post.go (3)
47-48: Meaningful constant addition.Declaring
FieldAccountPosts = "account_posts"aligns with ent's naming convention for schema fields.
189-189: Correct field inclusion inColumnsslice.Adding
FieldAccountPoststo theColumnsslice ensures it’s properly recognized by queries and mutations.
331-334: Convenient ordering utility.
ByAccountPostsis a well-structured helper for sorting byaccount_posts. No concerns detected.internal/ent/post/where.go (2)
119-123: Consistent naming for equality predicate.
AccountPostsfunction neatly mirrors established naming patterns, ensuring consistent usage in queries.
804-873: Comprehensive predicates foraccount_posts.These new functions (e.g.,
AccountPostsEQ,AccountPostsNEQ, etc.) enrich query flexibility. Implementation follows ent’s best practices.internal/ent/post_query.go (2)
1022-1022: Clear foreign-key lookup.Accessing
fk := nodes[i].AccountPoststo loadAuthoris correct and ensures the right reference.
1617-1619: Conditional column loading is appropriate.Only adding
post.FieldAccountPostswhenwithAuthoris set helps avoid unnecessary data loading.internal/ent/post_create.go (8)
193-197: Add null-check for ID parameters if needed.
Currently, the setter accepts anxid.IDwithout any validation. Ensure that a nil or zero-value ID won't break the logic downstream, if that scenario is feasible.
1121-1125: SetAccountPosts method looks straightforward.
This setter properly updates the PostUpsert struct with the new field.
1127-1131: UpdateAccountPosts ensures upsert uses the create-time value.
Good addition for concurrency scenarios in upsert logic.
1451-1456: Add “SetAccountPosts” to PostUpsertOne.
Method is consistent with the overall pattern for setting fields upon upsert one.
1458-1463: “UpdateAccountPosts” in PostUpsertOne parallels other fields.
Maintains consistency in upsert behavior.
1956-1961: Bulk upsert “SetAccountPosts.”
Code conveys the new field to bulk operations effectively.
1963-1969: Bulk upsert “UpdateAccountPosts.”
Appears correct for ensuring post creation-time value is used during conflicts.
640-640: Confirm correct assignment to 'AccountPosts'.
The code assignsnodes[0](from author edge) to_node.AccountPosts. Double-check this logic to ensure you don’t inadvertently overwrite the correct field.internal/ent/account_query.go (1)
Line range hint
1430-1444: Ensure mapping is correct for multi-post scenarios.
The logic sets a single node perAccountPostsID. If multiple posts belong to one account, check for potential collisions or missing associations in the loop.internal/ent/post_update.go (4)
259-263: Setter added for “account_posts.”
This is consistent with the create methods. No issues spotted.
265-271: Nillable setter matches other fields’ pattern.
Ensures optional assignment if the field can be omitted.
1706-1710: SetAccountPosts for PostUpdateOne.
Maintains a consistent approach in single-entity updates.
1712-1718: Nillable setter for PostUpdateOne.
Good addition for avoiding forced assignment of “account_posts.”internal/ent/er.html (1)
206-206: Added “account_posts” to ER diagram.
If it denotes a required one-to-one reference, consider making that explicit with the appropriate notation or clarifying in the docs.
split nested query into separate concurrent queries