rework node query to apply correct visibility rules as with tree traversal and listing #204
Conversation
…ersal and listing also improve tests for library nodes
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis pull request involves significant structural changes to the library's database handling. It removes the Changes
Possibly related PRs
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: 10
🧹 Outside diff range and nitpick comments (12)
tests/library/helpers.go (1)
8-14: LGTM with suggestions: Consider edge cases and test flexibility.The
UniqueNodefunction effectively generates a unique node for testing purposes. However, consider the following improvements:
- Add input validation for the
nameparameter to handle empty strings or invalid characters.- Consider implementing a max length for the generated slug to avoid potential issues with very long identifiers.
- For increased test flexibility, consider adding an option to provide a custom slug or use a deterministic slug generation for specific test cases.
Here's a suggested implementation incorporating these improvements:
func UniqueNode(name string, opts ...func(*openapi.NodeInitialProps)) openapi.NodeInitialProps { if name == "" { name = "unnamed_node" } slug := name + "_" + uuid.NewString() if len(slug) > 100 { slug = slug[:100] } node := openapi.NodeInitialProps{ Name: name, Slug: &slug, } for _, opt := range opts { opt(&node) } return node } // Usage example: // customSlug := UniqueNode("test", func(n *openapi.NodeInitialProps) { // *n.Slug = "custom_slug" // })This implementation adds input validation, limits the slug length, and allows for custom modifications through optional functional parameters.
app/resources/library/node_children/db.go (1)
Line range hint
1-72: Summary of changes and potential impact.The changes in this file reflect a shift from using a generic
library.Repositoryto a more specific*node_querier.Querier. This change is consistently applied to both thedatabasestruct and theNewfunction. TheMovefunction remains unchanged, which suggests that theQuerierinterface likely provides similar functionality to the previousRepositoryinterface.These changes may have broader implications:
- Other parts of the codebase that interact with this package might need to be updated to use the new
Querier.- The
Repositoryinterface might need to be reviewed to ensure it still accurately represents the abstraction needed.- There might be performance or behavior changes due to the use of
Querierinstead ofRepository, which should be verified through testing.Overall, this change appears to be part of a larger refactoring effort to improve the specificity and possibly the performance of database operations. Ensure that comprehensive testing is performed to validate that existing functionality is maintained and that any performance improvements are measurable.
app/services/semdex/semdexer/refhydrate/hydrator.go (1)
54-54: LGTM: Hydrate method updated correctlyThe
Hydratemethod has been appropriately updated to use the newnodeQuerier.Probemethod instead of the previouslibrary.GetByID. This change aligns with the PR objectives and is consistent with the other modifications in the file.Consider enhancing the error handling to provide more context:
if err != nil { return nil, fault.Wrap(err, fctx.With(ctx, fctx.With("kind", "node"))) }This addition would make debugging easier by specifying which kind of item (node) caused the error.
tests/library/children/children_test.go (3)
1-18: Consider aligning the package name with the directory structure.The package is named
datagraph_test, but the file is located intests/library/children. It might be more appropriate to name the packagechildren_testorlibrary_testto better reflect its location and purpose.
30-36: Good test context setup, but contains unused code.The test context and session setup look good, following appropriate practices for end-to-end testing. However, there's an unused commented-out variable
iurlthat should be removed to keep the code clean.Consider removing the unused commented-out variable:
- // iurl := "https://picsum.photos/500/500"
37-77: Well-structured node creation and relationship testing.The test effectively creates three nodes and establishes the correct parent-child relationships between them. The use of UUIDs for unique slugs and proper error checking after each operation are good practices.
To improve readability, consider adding a comment before each node creation to clearly indicate the intended hierarchy structure.
Add comments to clarify the node hierarchy:
+ // Create root node name1 := "test-node-1" slug1 := name1 + uuid.NewString() node1, err := cl.NodeCreateWithResponse(ctx, openapi.NodeInitialProps{ Name: name1, Slug: &slug1, }, session) tests.Ok(t, err, node1) - // Add a child node + // Create first child node (node1 -> node2) name2 := "test-node-2" slug2 := name2 + uuid.NewString() node2, err := cl.NodeCreateWithResponse(ctx, openapi.NodeInitialProps{ Name: name2, Slug: &slug2, }, session) tests.Ok(t, err, node2) + // Establish relationship: node1 -> node2 cadd, err := cl.NodeAddNodeWithResponse(ctx, slug1, slug2, session) tests.Ok(t, err, cadd) r.Equal(node1.JSON200.Id, cadd.JSON200.Id) - // Add another child to this child - // node1 - // |- node2 - // |- node3 + // Create second child node (node1 -> node2 -> node3) name3 := "test-node-3" slug3 := name3 + uuid.NewString() node3, err := cl.NodeCreateWithResponse(ctx, openapi.NodeInitialProps{ Name: name3, Slug: &slug3, }, session) tests.Ok(t, err, node3) + // Establish relationship: node2 -> node3 cadd, err = cl.NodeAddNodeWithResponse(ctx, slug2, slug3, session) tests.Ok(t, err, cadd) r.Equal(node2.JSON200.Id, cadd.JSON200.Id)app/resources/seed/seed.go (1)
Line range hint
1-89: Summary of changes and potential impactThe modifications in this file reflect a significant architectural shift in how node operations are handled:
- The import change from
librarytonode_writerindicates a move towards more specialized components.- The
Newfunction'snode_repoparameter type change fromlibrary.Repositoryto*node_writer.Writeraligns with this new approach.These changes are consistent with the PR's objective of reworking the node query functionality. However, they may have broader implications:
- The use of a concrete type (
*node_writer.Writer) instead of an interface could impact the flexibility of the codebase.- Other parts of the system that relied on the
library.Repositoryinterface might need adjustments.Ensure that these changes are consistently applied throughout the codebase and that all affected components are updated accordingly.
tests/library/deletion/deletion_test.go (3)
1-122: LGTM! Consider adding package documentation.The overall structure of the test file is well-organized and follows good testing practices. The use of parallel testing and dependency injection is commendable.
Consider adding a package comment at the beginning of the file to provide an overview of the test suite's purpose and structure. For example:
// Package deletion_test provides integration tests for the node deletion functionality // in the library. It tests various scenarios of deleting nodes in a tree structure, // ensuring that the deletion logic maintains the integrity of the tree. package deletion_test
41-65: LGTM! Consider adding an assertion for node1.The test case for deleting a leaf node is well-structured and covers the basic scenario effectively.
To make the test more comprehensive, consider adding an assertion to verify that node1's state remains unchanged after the deletion of node3. This would ensure that the deletion operation doesn't have unexpected side effects on the root node. For example:
node1get, err := cl.NodeGetWithResponse(ctx, node1.JSON200.Slug, session) tests.Ok(t, err, node1get) a.Len(node1get.JSON200.Children, 1, "node1 should still have one child (node2)")
67-91: LGTM! Consider adding an assertion for node3.The test case for deleting a parent node with a target is well-structured and effectively tests the scenario.
To make the test more robust, consider adding an assertion to directly verify node3's new parent after the deletion of node2. This would ensure that the child node is correctly reassigned. For example:
node3get, err := cl.NodeGetWithResponse(ctx, node3.JSON200.Slug, session) tests.Ok(t, err, node3get) a.NotNil(node3get.JSON200.Parent, "node3 should have a parent") a.Equal(node1.JSON200.Id, node3get.JSON200.Parent.Id, "node3's new parent should be node1")app/services/library/node_visibility/visibility.go (1)
Line range hint
72-74: Implement event emission and notifications for published nodesThe TODO comment indicates that when a node's visibility changes to
Published, events should be emitted, and notifications sent. Implementing this ensures users are informed about new published content.Would you like assistance in generating the code to emit events and send notifications, or should we open a new GitHub issue to track this task?
app/resources/library/node_querier/node_querier.go (1)
103-103: Address the TODO: Implement a lightweight Node representation for probingThe
Probemethod is intended for quick existence checks without loading related edges. Implementing a slimmed-down version of theNodestruct or adjusting the query to exclude unnecessary fields can improve performance.Would you like assistance in refactoring the
Probemethod to enhance efficiency? I can help develop a minimalNodestruct or modify the query accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (24)
- app/resources/library/db.go (0 hunks)
- app/resources/library/library.go (0 hunks)
- app/resources/library/node_children/db.go (1 hunks)
- app/resources/library/node_querier/node_querier.go (1 hunks)
- app/resources/library/node_writer/node_writer.go (1 hunks)
- app/resources/resources.go (2 hunks)
- app/resources/seed/seed.go (2 hunks)
- app/services/asset/analyse/analyse.go (2 hunks)
- app/services/asset/analyse/pdf.go (1 hunks)
- app/services/asset/asset_upload/uploader.go (4 hunks)
- app/services/library/node_mutate/node.go (11 hunks)
- app/services/library/node_read/reader.go (2 hunks)
- app/services/library/node_visibility/visibility.go (4 hunks)
- app/services/library/nodetree/nodetree.go (6 hunks)
- app/services/link/fetcher/fetcher.go (0 hunks)
- app/services/link/scrape_job/scrape.go (2 hunks)
- app/services/semdex/index_job/indexer.go (5 hunks)
- app/services/semdex/semdexer/refhydrate/hydrator.go (2 hunks)
- app/services/semdex/summarise_job/summariser.go (5 hunks)
- tests/datagraph/datagraph_test.go (0 hunks)
- tests/library/children/children_test.go (1 hunks)
- tests/library/deletion/deletion_test.go (1 hunks)
- tests/library/helpers.go (1 hunks)
- tests/library/visibility/visibility_test.go (10 hunks)
💤 Files with no reviewable changes (4)
- app/resources/library/db.go
- app/resources/library/library.go
- app/services/link/fetcher/fetcher.go
- tests/datagraph/datagraph_test.go
🧰 Additional context used
🔇 Additional comments (67)
tests/library/helpers.go (1)
3-6: LGTM: Imports are appropriate and necessary.The imports are well-chosen for the function's requirements. The
openapipackage from the project and theuuidpackage from Google are both relevant to theUniqueNodefunction's implementation.app/services/semdex/summarise_job/summariser.go (4)
12-12: LGTM: Import statement added correctly.The new import for the
node_writerpackage is consistent with the changes made in the rest of the file.
22-22: LGTM: Struct field updated appropriately.The replacement of
nodeRepowithnodeWriteris consistent with the architectural changes mentioned in the PR objectives. The use of a pointer type fornodeWriteris appropriate for a complex type likeWriter.
32-32: LGTM: Function signature and struct initialization updated correctly.The changes to the
newSummariseConsumerfunction signature and the corresponding struct initialization are consistent with the modifications made to thesummariseConsumerstruct. The parameter type and field assignment correctly reflect the newnodeWriterfield.Also applies to: 41-41
55-55: LGTM with a suggestion: Method call updated correctly, but review error handling.The update to use
i.nodeWriter.Updatewithnode_writer.WithDescriptionis consistent with the architectural changes. However, please review the error handling to ensure it's still appropriate for the newnodeWriter.Updatemethod, as its error semantics might differ from the previousnodeRepo.Update.To verify the error handling, please run the following script:
app/services/asset/analyse/pdf.go (1)
25-25: LGTM. Verify implications of usingProbeinstead ofGetByID.The change from
GetByIDtoProbealigns with the PR objective of applying correct visibility rules. This modification likely improves how nodes are accessed or validated.To ensure this change doesn't introduce unintended side effects, please verify:
- Error handling: Confirm that
Probereturns the same error types asGetByID.- Performance: Check if
Probehas any performance implications compared toGetByID.- Visibility rules: Ensure that
Probecorrectly applies the intended visibility rules.Run the following script to compare the implementations of
GetByIDandProbe:app/resources/library/node_children/db.go (2)
13-13: LGTM: New import added for node_querier package.The addition of this import is consistent with the changes made to the
databasestruct andNewfunction. It's good to see that thenode_querierpackage is part of the same project structure.
23-23: Approve New function signature change, but verify Repository interface.The update to the
Newfunction signature is consistent with the changes made to thedatabasestruct. It's good that the function still returns aRepositoryinterface, maintaining the abstraction.Please verify if the
Repositoryinterface needs to be updated to reflect any changes in the methods ofnode_querier.Querier. Run the following script to compare the methods:#!/bin/bash # Description: Compare methods of Repository interface and node_querier.Querier # Test: Extract and compare method signatures echo "Repository interface methods:" ast-grep --lang go --pattern 'type Repository interface { $$$ }' | sed -n '/type Repository interface {/,/}/p' | grep -v '^\s*$' | grep -v '}' echo "\nnode_querier.Querier methods:" ast-grep --lang go --pattern 'type Querier struct { $$$ }' | sed -n '/type Querier struct {/,/}/p' | grep -v '^\s*$' | grep -v '}'app/services/link/scrape_job/scrape.go (4)
14-14: LGTM: Import statement addition is appropriate.The addition of the
node_writerpackage import is consistent with the changes made to thescrapeConsumerstruct and its methods. This import is necessary to support the newnodeWriterfield.
30-35: LGTM: Constructor updated consistently with struct changes.The modifications to the
newScrapeConsumerfunction signature and struct initialization are consistent with the changes made to thescrapeConsumerstruct.To ensure this change doesn't break any existing code, please run the following script:
#!/bin/bash # Description: Check for any calls to the old constructor signature # Test: Search for calls to newScrapeConsumer with the old signature rg --type go "newScrapeConsumer\(.+,\s*\w+\s*library\.Repository\)"If this search returns results, it may indicate areas of the code that need to be updated to use the new constructor signature.
55-55: LGTM: Method call updated consistently with struct changes.The replacement of
s.nodes.Updatewiths.nodeWriter.Updateis consistent with the changes made to thescrapeConsumerstruct. The use ofnode_writer.WithContentLinksinstead oflibrary.WithContentLinkssuggests a change in package structure or functionality.To ensure the
WithContentLinksfunction in thenode_writerpackage has the same functionality as the previouslibrary.WithContentLinks, please run the following script:#!/bin/bash # Description: Compare the WithContentLinks functions in library and node_writer packages # Test: Search for the WithContentLinks function in the library package echo "library.WithContentLinks:" ast-grep --lang go --pattern 'func WithContentLinks($_) $_' # Test: Search for the WithContentLinks function in the node_writer package echo "node_writer.WithContentLinks:" ast-grep --lang go --pattern 'func WithContentLinks($_) $_'Compare the results to ensure that the functionality of
WithContentLinksis preserved in thenode_writerpackage.
22-24: LGTM: Struct field change improves code organization.The replacement of
nodes library.RepositorywithnodeWriter *node_writer.Writeraligns with the PR objectives and potentially improves code organization by using a dedicated writer for node operations.To ensure this change doesn't negatively impact other parts of the codebase, please run the following script:
If these searches return results, it may indicate areas of the code that need to be updated to use the new
nodeWriterfield.✅ Verification successful
Verification Successful: No remaining references to
nodes library.Repository.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usages of the old 'nodes' field in the codebase # Test: Search for any remaining references to 'nodes library.Repository' rg --type go "nodes\s+library\.Repository" # Test: Search for any method calls on the old 'nodes' field rg --type go "s\.nodes\."Length of output: 3649
app/services/asset/analyse/analyse.go (2)
11-11: LGTM: Import statement added for node_querier packageThe addition of the import statement for the
node_querierpackage is consistent with the changes made to theAnalyserstruct and its constructor. This import is necessary to support the new*node_querier.Queriertype.
20-20: LGTM: Analyser struct and constructor updated to use node_querier.QuerierThe changes to the
Analyserstruct and its constructor are consistent with the new import and reflect a shift from usinglibrary.Repositoryto*node_querier.Querier. This change likely represents an architectural improvement, possibly moving to a more specialized querying component.To ensure these changes don't negatively impact other parts of the codebase, please run the following verification script:
This script will help identify any areas of the codebase that might need attention due to these changes.
Also applies to: 27-27
✅ Verification successful
Verification Successful: No remaining references to library.Repository
The verification script did not find any remaining uses of
library.Repositoryin the codebase. The changes to theAnalyserstruct and its constructor have been successfully applied without introducing any inconsistencies elsewhere.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining uses of library.Repository that might need updating # Test 1: Search for any remaining uses of library.Repository echo "Searching for remaining uses of library.Repository:" rg --type go "library\.Repository" # Test 2: Check for any files that might need updating due to the Analyser changes echo "Checking for files that might need updating due to Analyser changes:" rg --type go "analyse\.Analyser|analyse\.New"Length of output: 529
app/services/semdex/semdexer/refhydrate/hydrator.go (3)
17-17: LGTM: New import added correctlyThe new import for
node_querieris correctly added and aligns with the changes made to theHydratorstruct and its methods.
24-26: LGTM: Hydrator struct updated correctlyThe
Hydratorstruct has been appropriately modified to replace thelibraryfield with anodeQuerierfield. This change aligns with the PR objectives and is consistent with the other modifications in the file.
32-37: LGTM: New function updated correctlyThe
Newfunction has been properly updated to reflect the changes in theHydratorstruct. The function signature and struct initialization are consistent with the newnodeQuerierfield, replacing the previouslibrarydependency.app/services/asset/asset_upload/uploader.go (4)
15-15: LGTM: New import aligns with architectural changes.The addition of the
node_writerimport is consistent with the shift fromlibrary.Repositoryto a more specialized*node_writer.Writer. This change supports better modularity in the codebase.
68-68: LGTM: Method call updated to use node_writer package.The update from
library.WithAssetstonode_writer.WithAssetsis consistent with the architectural changes. This maintains the functionality while adhering to the new structure.To ensure the
WithAssetsfunction in thenode_writerpackage has the correct signature and functionality, please run the following verification:#!/bin/bash # Check the definition of WithAssets in the node_writer package ast-grep --lang go --pattern 'func WithAssets($_) $_' app/resources/library/node_writer
35-35: LGTM: Constructor signature updated correctly.The
Newfunction signature has been properly updated to match the changes in theUploaderstruct. This ensures consistency in the use of*node_writer.Writer.To ensure all callers of this function have been updated, please run the following verification:
#!/bin/bash # Find all calls to the New function in the asset_upload package rg --type go -e "New\s*\(" app/services/asset/asset_upload
26-26: Approve change and verify impact on node writing operations.The update from
library.Repositoryto*node_writer.Writeraligns with the PR's objective of reworking node queries. This change likely provides more specific and efficient node writing capabilities.To ensure this change doesn't introduce unintended side effects, please run the following verification:
✅ Verification successful
Verification Successful: All usages of
nodewriterhave been correctly updated.No issues found in the codebase regarding the
nodewriterfield.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all usages of the nodewriter field are updated correctly rg --type go -e "nodewriter\." app/services/asset/asset_uploadLength of output: 456
tests/library/children/children_test.go (1)
20-29: Well-structured test setup using dependency injection.The test function is appropriately named and set to run in parallel. The use of
fx.Invokefor dependency injection is a good practice, allowing for a clean and maintainable test setup.app/services/semdex/index_job/indexer.go (4)
14-14: LGTM: Import statement added correctly.The new import for the
node_querierpackage is consistent with the changes made in the struct and methods.
41-41: LGTM: Constructor function updated correctly.The changes in the
newIndexConsumerfunction are consistent with the struct modification. Both the parameter and the field assignment have been updated to use the newnodeQuerier *node_querier.Querier.To ensure this change doesn't break any existing code, please run the following verification:
#!/bin/bash # Search for calls to newIndexConsumer function rg --type go "newIndexConsumer\("Also applies to: 55-55
75-75: LGTM: Method call updated to use new querier.The change from
i.nodeRepo.GetByID(ctx, id)toi.nodeQuerier.Probe(ctx, id)is consistent with the new querying approach. However, the change in method name fromGetByIDtoProbemight indicate a difference in functionality.To ensure compatibility and correct behavior, please verify the following:
- Check that the
Probemethod returns the same type of result as the previousGetByIDmethod.- Confirm that the
Probemethod handles errors in the same way asGetByID.You can use the following command to inspect the
Probemethod:
26-26: LGTM: Struct field updated to use new querier.The change from
nodeRepo library.RepositorytonodeQuerier *node_querier.Querieraligns with the new querying approach. This shift in data access pattern is consistent with the PR objectives.To ensure this change doesn't negatively impact other parts of the codebase, please run the following verification:
app/resources/seed/seed.go (2)
14-14: Import change reflects architectural shiftThe replacement of the
libraryimport withnode_writeraligns with the PR's objective of reworking the node query functionality. This change suggests a move towards more specialized components for handling node operations.
61-61: Verify impact ofnode_repotype changeThe
node_repoparameter type has changed fromlibrary.Repositoryto*node_writer.Writer. This change aligns with the new import and suggests a shift towards a more concrete implementation for node writing operations.While this may provide more direct access to writing functionalities, it could potentially reduce flexibility in terms of dependency injection.
Please verify that this change doesn't negatively impact other parts of the codebase that might have depended on the more abstract
library.Repositoryinterface. Run the following script to check for any remaining usages oflibrary.Repository:app/resources/resources.go (2)
24-27: Import changes align with PR objectivesThe addition of
node_querierandnode_writerimports, along with the removal of the generallibraryimport, aligns well with the PR objective of reworking node query functionality. This change indicates a more granular approach to handling node-related operations.
72-73: Build function updates reflect improved node handlingThe replacement of
library.Newwithnode_querier.Newandnode_writer.Newin theBuildfunction reflects the shift towards more specific node-related functionalities. This change is consistent with the PR objectives and should provide better granularity in handling node operations.tests/library/deletion/deletion_test.go (1)
93-119: LGTM! Comprehensive test case.The test case for deleting a parent node without a target is well-structured and comprehensive. It effectively verifies that:
- The parent node (node2) is deleted.
- The child node (node3) is moved to the root level.
- The original parent (node1) no longer has any children.
The assertions cover all necessary aspects of the operation, ensuring the tree structure is maintained correctly after the deletion.
tests/library/visibility/visibility_test.go (7)
28-41: Improved context naming and session managementThe changes in this segment enhance the clarity and organization of the test setup:
- Renaming the context parameter from
ctxtorootbetter reflects its scope.- Introduction of session variables (
adminSession,authorSession,randoSession) improves readability and maintainability.These modifications will make the tests easier to understand and maintain.
49-52: Consistent context and session usage in "public_only" testThe changes in this segment maintain consistency with the earlier modifications:
- Using
rootinstead ofctxfor the context in node creation.- Utilizing the pre-defined session variables instead of creating new sessions inline.
These changes improve the readability and consistency of the test case.
72-77: Consistent context and session usage in "public_filter_by_author" testThe changes in this segment follow the same pattern as the previous test case:
- Using
rootfor the context in node creation.- Utilizing pre-defined session variables.
This consistency across test cases improves the overall readability and maintainability of the test suite.
96-104: Improved clarity in "admin_change_visibility" test with added commentThe changes in this segment continue the pattern of consistent context and session usage. Additionally:
- A valuable comment has been added explaining the expected behavior of this test case.
- The comment highlights that the admin should not be able to list a node in draft status, which is crucial for understanding the test's purpose.
These changes not only maintain consistency but also improve the test's documentation, making it easier for developers to understand the intended behavior.
Line range hint
124-232: Consistent application of context and session changes across multiple test casesThe changes in this large segment demonstrate a consistent application of the modifications seen earlier:
- All occurrences of
ctxhave been replaced withrootfor context.- Pre-defined session variables are used consistently across all test cases.
This consistency spans multiple test cases including "author_change_visibility", "author_can_view_own_drafts", "admin_lists_in_review_but_not_drafts", "author_submits_for_review", and "author_submmits_unlisted".
The uniform application of these changes across the entire test file significantly improves its overall consistency and readability.
248-253: Consistent changes in "visibility_affects_children" test while maintaining test integrityThe changes in this segment continue the pattern of using
rootfor context and pre-defined session variables. Importantly:
- The core logic of the test case remains unchanged, ensuring that the visibility rules for parent-child relationships are still being tested correctly.
- The modifications do not alter the intent or coverage of the test, maintaining its validity and importance in the test suite.
These changes improve consistency while preserving the crucial checks for visibility inheritance in the node hierarchy.
263-287: Excellent addition of "only_author_sees_non_published_children" test caseThis new test case is a valuable addition to the visibility test suite:
- It explicitly tests the visibility of non-published (draft) child nodes for different user roles (author, random authenticated user, guest).
- The test covers an important edge case in the visibility rules that wasn't directly addressed in previous tests.
- The logic is comprehensive, checking visibility for the node owner, other authenticated users, and guests separately.
This addition enhances the overall coverage of the visibility rules and helps ensure that the system correctly handles the visibility of draft child nodes in various scenarios. Great job on improving the test suite!
app/services/library/node_read/reader.go (2)
23-23: Type change fornodereaderaligns with new implementationUpdating the
nodereaderfield and constructor parameter to use*node_querier.Querierreflects the updated architecture and ensures consistency throughout the code.Also applies to: 30-30
44-50: Verify behavior whenoptsis emptyWhen there is no account session, the
optsslice remains empty. Please verify that callingq.nodereader.Getwith an emptyoptsslice does not cause unintended behavior or errors.If necessary, consider handling the no-session case explicitly or ensuring that
q.nodereader.Getcan safely handle empty options.app/services/library/node_visibility/visibility.go (1)
68-68: Ensure consistency with child nodes when updating visibilityWhen updating a node's visibility using
m.nodeWriter.Update, consider if child nodes should also have their visibility updated to maintain consistency across the node hierarchy.To verify if child nodes require visibility updates, you can check for child nodes associated with the current node:
Please replace
REPLACE_WITH_NODE_IDwith the actual node ID to run the script.app/resources/library/node_querier/node_querier.go (1)
66-87: 🛠️ Refactor suggestionEnsure visibility rules are consistently applied and optimized
The visibility filtering in the
WithNodesclause correctly enforces the visibility rules based on whether therequestingAccountis provided. However, the logic could be further optimized.Consider simplifying the conditionals or using predefined predicates to improve readability and maintainability. Additionally, ensure that the visibility conditions are thoroughly tested to prevent any unauthorized data access.
Would you like assistance in refactoring the visibility logic or creating tests to verify the correctness of these rules?
app/resources/library/node_writer/node_writer.go (1)
139-145: Ensure consistent error handling withfmsg.WithDescWhen wrapping constraint errors, you use
fmsg.WithDescto provide a user-friendly message. To maintain consistency across the codebase, verify thatfmsg.WithDescis used uniformly with appropriate descriptions. This helps in providing clear and actionable error messages to users.Run the following script to check
fmsg.WithDescusage:✅ Verification successful
Consistent usage of
fmsg.WithDescconfirmed.All instances of
fmsg.WithDescin the codebase utilize appropriate descriptions, ensuring uniform and clear error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of fmsg.WithDesc in error handling. # Test: Search for all instances of fmsg.WithDesc in Go files. rg --type go 'fmsg\.WithDesc\('Length of output: 6708
app/services/library/nodetree/nodetree.go (10)
13-14: Import necessary packages for node querying and writingThe additions of
node_querierandnode_writerpackages are appropriate and necessary for the specialized node operations introduced.
35-36: Update service struct with nodeQuerier and nodeWriterThe
servicestruct now includesnodeQuerierandnodeWriter, providing a clearer separation of concerns compared to the previous genericlibrary.Repository.
67-67: Retrieve the child node using nodeQuerierThe retrieval of the child node now correctly utilizes
s.nodeQuerier.Get, aligning with the updated architecture.
72-72: Retrieve the parent node using nodeQuerierThe parent node is now fetched using
s.nodeQuerier.Get, which is consistent with the new modular design.
91-91: Update child node to sever circular relationship if necessaryThe use of
s.nodeWriter.Updatewithnode_writer.WithChildNodeRemoveproperly handles the potential circular relationship by removing the parent node from the child’s children.
98-98: Add child node to the new parent using nodeWriterAdding the child node to the new parent via
s.nodeWriter.Updatewithnode_writer.WithChildNodeAddis correctly implemented.
120-120: Retrieve the child node in Sever method using nodeQuerierIn the
Severmethod, fetching the child node withs.nodeQuerier.Getmaintains consistency with the rest of the service.
125-125: Retrieve the parent node in Sever method using nodeQuerierSimilarly, retrieving the parent node using
s.nodeQuerier.Getaligns with the updated design.
134-134: Update parent node to remove child link in Sever methodThe use of
s.nodeWriter.Updatewithnode_writer.WithChildNodeRemoveeffectively removes the child node from the parent’s children, properly executing the sever operation.
40-49: Verify all calls to the updatedNewconstructor functionThe
Newfunction signature has been updated to acceptnodeQuerier,nodeWriter, andaccountQuery. Ensure all instances wherenodetree.Newis called are updated to match the new signature to prevent potential runtime errors.Run the following script to locate and review all calls to
nodetree.New:app/services/library/node_mutate/node.go (14)
21-22: New Imports fornode_querierandnode_writerAddedThe imports for
node_querierandnode_writerhave been correctly added to support the new modular architecture.
64-71: UpdatedOptsMethod to Usenode_writer.OptionThe
Optsmethod in thePartialstruct has been updated to return[]node_writer.Optionand usenode_writeroptions. This refactoring aligns with the new design and is correctly implemented.
77-78: Service Struct Now IncludesnodeQuerierandnodeWriterFieldsThe
servicestruct has been updated to include*node_querier.Querierand*node_writer.Writer, replacing the previouslibrary.Repository. This change supports the updated node handling logic.
88-89: Constructor Function Updated to Initialize New FieldsThe
Newfunction correctly initializes the newnodeQuerierandnodeWriterfields, and theservicestruct assignments are appropriately updated.Also applies to: 98-99
141-141: Assets Added Usingnode_writer.WithAssetsAssets from the asset sources are correctly appended to the options using
node_writer.WithAssets.
150-150: Link Added to Options Usingnode_writer.WithLinkThe link node is correctly added to the options using
node_writer.WithLink.
154-154: Node Creation Now UsesnodeWriter.CreateThe
Createmethod now utilizess.nodeWriter.Create, which aligns with the new architecture.
179-179: Node Retrieval Updated to UsenodeQuerier.GetThe
Updatemethod now retrieves the node usings.nodeQuerier.Get, reflecting the refactored design.
201-201: Assets Added Usingnode_writer.WithAssetsAssets are appended to the options using
node_writer.WithAssets, consistent with the earlier changes.
223-223: Link Added to Options Usingnode_writer.WithLinkThe link is appended to the options using
node_writer.WithLink, similar to the change in theCreatemethod.
227-227: Node Update Now UsesnodeWriter.UpdateThe
Updatemethod now callss.nodeWriter.Update, aligning with the updated architecture.
252-252: Node Retrieval inDeleteMethod UpdatedThe
Deletemethod now usess.nodeQuerier.Getto retrieve the node, reflecting the architectural changes.
273-273: Node Deletion Now UsesnodeWriter.DeleteThe node is now deleted using
s.nodeWriter.Delete, consistent with the refactored node handling.
281-281: UpdatedapplyOptsMethod to Usenode_querierThe
applyOptsmethod now usess.nodeQuerier.Getto retrieve the parent node and appends the parent to the options usingnode_writer.WithParent. This change is correctly implemented.
| "github.com/Southclaws/storyden/internal/ent" | ||
| "github.com/Southclaws/storyden/internal/ent/node" | ||
| ) | ||
|
|
||
| type database struct { | ||
| db *ent.Client | ||
| nr library.Repository | ||
| nr *node_querier.Querier |
There was a problem hiding this comment.
💡 Codebase verification
Resolve remaining library.Repository references.
The change to replace library.Repository with *node_querier.Querier has not been fully applied. The following files still reference library.Repository and need to be updated accordingly:
app/resources/tag/repo.goapp/resources/settings/repo.goapp/resources/library/node_traversal/traversal.goapp/resources/profile/profile_search/search.goapp/resources/library/node_children/children.goapp/resources/asset/asset.goapp/resources/post/thread_writer/writer.goapp/resources/collection/repo.goapp/resources/post/thread/repo.goapp/resources/post/post_search/repo.goapp/resources/post/category/repo.goapp/resources/account/authentication/repo.goapp/resources/post/reply/repo.go
Please update these files to ensure consistent dependency injection across the codebase.
🔗 Analysis chain
Approve change to database struct, but verify impact.
The change from library.Repository to *node_querier.Querier for the nr field reflects a more specific dependency injection. This is generally a good practice as it provides clearer intentions and potentially better type safety.
Please ensure that this change is consistently applied throughout the codebase. Run the following script to check for any remaining uses of library.Repository:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for remaining uses of library.Repository
# Test: Search for library.Repository usage
rg --type go 'library\.Repository'
Length of output: 12882
| package datagraph_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/google/uuid" | ||
| "github.com/stretchr/testify/require" | ||
| "go.uber.org/fx" | ||
|
|
||
| "github.com/Southclaws/storyden/app/resources/account/account_writer" | ||
| "github.com/Southclaws/storyden/app/resources/seed" | ||
| "github.com/Southclaws/storyden/app/transports/http/middleware/session" | ||
| "github.com/Southclaws/storyden/app/transports/http/openapi" | ||
| "github.com/Southclaws/storyden/internal/integration" | ||
| "github.com/Southclaws/storyden/internal/integration/e2e" | ||
| "github.com/Southclaws/storyden/tests" | ||
| ) | ||
|
|
||
| func TestLibraryNodeChildren(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| integration.Test(t, nil, e2e.Setup(), fx.Invoke(func( | ||
| lc fx.Lifecycle, | ||
| ctx context.Context, | ||
| cl *openapi.ClientWithResponses, | ||
| cj *session.Jar, | ||
| aw *account_writer.Writer, | ||
| ) { | ||
| lc.Append(fx.StartHook(func() { | ||
| r := require.New(t) | ||
|
|
||
| ctx, _ := e2e.WithAccount(ctx, aw, seed.Account_001_Odin) | ||
| session := e2e.WithSession(ctx, cj) | ||
|
|
||
| // iurl := "https://picsum.photos/500/500" | ||
|
|
||
| name1 := "test-node-1" | ||
| slug1 := name1 + uuid.NewString() | ||
| node1, err := cl.NodeCreateWithResponse(ctx, openapi.NodeInitialProps{ | ||
| Name: name1, | ||
| Slug: &slug1, | ||
| }, session) | ||
| tests.Ok(t, err, node1) | ||
|
|
||
| // Add a child node | ||
|
|
||
| name2 := "test-node-2" | ||
| slug2 := name2 + uuid.NewString() | ||
| node2, err := cl.NodeCreateWithResponse(ctx, openapi.NodeInitialProps{ | ||
| Name: name2, | ||
| Slug: &slug2, | ||
| }, session) | ||
| tests.Ok(t, err, node2) | ||
|
|
||
| cadd, err := cl.NodeAddNodeWithResponse(ctx, slug1, slug2, session) | ||
| tests.Ok(t, err, cadd) | ||
|
|
||
| r.Equal(node1.JSON200.Id, cadd.JSON200.Id) | ||
|
|
||
| // Add another child to this child | ||
| // node1 | ||
| // |- node2 | ||
| // |- node3 | ||
|
|
||
| name3 := "test-node-3" | ||
| slug3 := name3 + uuid.NewString() | ||
| node3, err := cl.NodeCreateWithResponse(ctx, openapi.NodeInitialProps{ | ||
| Name: name3, | ||
| Slug: &slug3, | ||
| }, session) | ||
| tests.Ok(t, err, node3) | ||
|
|
||
| cadd, err = cl.NodeAddNodeWithResponse(ctx, slug2, slug3, session) | ||
| tests.Ok(t, err, cadd) | ||
|
|
||
| r.Equal(node2.JSON200.Id, cadd.JSON200.Id) | ||
| })) | ||
| })) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more test cases for completeness.
While the current test covers the basic functionality of node creation and parent-child relationships, it would be beneficial to add more test cases to ensure comprehensive coverage. Consider adding tests for:
- Attempting to create a node with a duplicate slug.
- Trying to establish a circular relationship (e.g., making a parent node a child of its own child).
- Verifying that a node cannot be added as a child to multiple parents.
- Testing the behavior when adding a non-existent node as a child.
- Checking the system's response to invalid input (e.g., empty names or slugs).
These additional test cases would help ensure the robustness of the node hierarchy system.
There was a problem hiding this comment.
all these cases are already tested in other tests
There was a problem hiding this comment.
@Southclaws, understood. Thanks for clarifying that these cases are already tested in other tests.
✏️ Learnings added
Learnt from: Southclaws
PR: Southclaws/storyden#204
File: tests/library/children/children_test.go:1-80
Timestamp: 2024-10-10T12:29:02.898Z
Learning: The test cases for duplicate slugs, circular relationships, multiple parent assignments, adding non-existent nodes, and invalid inputs are already covered in other tests in the codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| "github.com/Southclaws/storyden/app/resources/library/node_querier" | ||
| "github.com/Southclaws/storyden/app/resources/library/node_search" | ||
| "github.com/Southclaws/storyden/app/resources/library/node_traversal" | ||
| "github.com/Southclaws/storyden/app/resources/library/node_writer" |
There was a problem hiding this comment.
💡 Codebase verification
Incomplete Refactoring Detected
Several files still import or utilize the old library resource. Please ensure all dependencies are updated to use node_querier and node_writer to maintain consistency and prevent potential issues.
app/services/library/perms.goapp/services/semdex/summarise_job/summariser.goapp/resources/library/node_traversal/db.go- (and others as identified)
🔗 Analysis chain
Verify impact on dependent code
The changes from library to node_querier and node_writer seem to be part of a larger refactoring effort. While these changes look good, it's important to ensure that all code previously depending on the library resource has been updated to use the new node_querier and node_writer resources.
To verify this, please run the following script:
This script will help identify any areas of the codebase that might still be using the old library resource and need updating.
Also applies to: 72-73
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of the old library resource
# Test 1: Search for any remaining imports of the old library package
echo "Checking for remaining imports of the old library package:"
rg --type go 'github\.com/Southclaws/storyden/app/resources/library"$'
# Test 2: Search for any remaining usage of library.New
echo "Checking for remaining usage of library.New:"
rg --type go 'library\.New'
# Test 3: Search for any files that might need updating to use node_querier or node_writer
echo "Files that might need updating to use node_querier or node_writer:"
rg --type go -l 'library\.' | xargs -I {} echo "File: {}"
Length of output: 5086
| nodeQuerier *node_querier.Querier, | ||
| nodeWriter *node_writer.Writer, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using interfaces for dependency injection
Injecting interfaces instead of concrete types for nodeQuerier and nodeWriter in the New function enhances testability and decouples the Controller from specific implementations.
Apply this diff to modify the function signature and struct fields:
type Controller struct {
accountQuery *account_querier.Querier
- nodeQuerier *node_querier.Querier
- nodeWriter *node_writer.Writer
+ nodeQuerier node_querier.QuerierInterface
+ nodeWriter node_writer.WriterInterface
nc node_children.Repository
}
func New(
accountQuery *account_querier.Querier,
- nodeQuerier *node_querier.Querier,
- nodeWriter *node_writer.Writer,
+ nodeQuerier node_querier.QuerierInterface,
+ nodeWriter node_writer.WriterInterface,
nc node_children.Repository,
) *Controller {This change allows for easier mocking and testing of the Controller by injecting different implementations of nodeQuerier and nodeWriter.
Committable suggestion was skipped due to low confidence.
| query. | ||
| WithOwner(func(aq *ent.AccountQuery) { | ||
| aq.WithAccountRoles(func(arq *ent.AccountRolesQuery) { arq.WithRole() }) | ||
| }). | ||
| WithAssets(). | ||
| WithLink(func(lq *ent.LinkQuery) { | ||
| lq.WithAssets().Order(link.ByCreatedAt(sql.OrderDesc())) | ||
| }). | ||
| WithParent(func(cq *ent.NodeQuery) { | ||
| cq. | ||
| WithAssets(). | ||
| WithOwner(func(aq *ent.AccountQuery) { | ||
| aq.WithAccountRoles(func(arq *ent.AccountRolesQuery) { arq.WithRole() }) | ||
| }) | ||
| }) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing eager loading of related entities to improve performance
In the Get method, multiple related entities are eagerly loaded using WithOwner, WithAssets, WithLink, and WithParent. While this ensures all related data is available, it could lead to performance issues, especially with large datasets or deep relationships.
Evaluate whether all these relations are necessary for every retrieval. If not all related data is needed in every case, consider loading only the required relations or using lazy loading to defer loading until the data is actually needed.
| } | ||
|
|
||
| // Probe does not pull edges, only the node itself, it's fast for quick checks. | ||
| // TODO: Provide a more slimmed-down invariant of Node struct for this purpose. |
There was a problem hiding this comment.
Typo in comment: 'invariant' should be 'variant' or 'version'
In the TODO comment on line 103, the term "invariant" seems out of context. It likely should be "variant" or "version".
-// TODO: Provide a more slimmed-down invariant of Node struct for this purpose.
+// TODO: Provide a more slimmed-down variant of Node struct for this purpose.📝 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.
| // TODO: Provide a more slimmed-down invariant of Node struct for this purpose. | |
| // TODO: Provide a more slimmed-down variant of Node struct for this purpose. |
| func WithContent(v datagraph.Content) Option { | ||
| return func(c *ent.NodeMutation) { | ||
| c.SetContent(v.HTML()) | ||
| c.SetDescription(v.Short()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Clarify the scope of WithContent to avoid unexpected side effects
The WithContent option function sets both the Content and Description fields. This might be unexpected for users who assume it only sets the Content. Consider separating the description logic into a separate WithDescription option or clearly document that WithContent also sets the description based on the content provided.
| func (w *Writer) Update(ctx context.Context, qk library.QueryKey, opts ...Option) (*library.Node, error) { | ||
| // NOTE: Should be a probe not a full read. Query is necessary because of | ||
| // the Mark being used (id or slug) for updates. Cannot use UpdateOneID. | ||
| pre, err := w.querier.Get(ctx, qk) | ||
| if err != nil { | ||
| return nil, fault.Wrap(err, fctx.With(ctx)) | ||
| } | ||
|
|
||
| update := w.db.Node.Update() | ||
| update.Where(qk.Predicate()) | ||
|
|
||
| mutate := update.Mutation() | ||
| for _, fn := range opts { | ||
| fn(mutate) | ||
| } | ||
|
|
||
| err = update.Exec(ctx) | ||
| if err != nil { | ||
| return nil, fault.Wrap(err, fctx.With(ctx)) | ||
| } | ||
|
|
||
| qk = library.QueryKey{mark.NewQueryKeyID(pre.Mark.ID())} | ||
|
|
||
| return w.querier.Get(ctx, qk) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the Update method to avoid unnecessary reads
The Update method first retrieves the node using w.querier.Get before performing the update. This additional read operation may impact performance, especially under heavy load. If the primary goal is to update existing nodes and ensure they exist, consider using UpdateOne with appropriate error handling to perform the update without the initial fetch.
| _, err := delete.Exec(ctx) | ||
| if err != nil { |
There was a problem hiding this comment.
Remove unnecessary assignment to the blank identifier
In the Delete method, the result of delete.Exec(ctx) is assigned to the blank identifier _, but Exec returns only an error. The assignment is unnecessary and can be omitted.
Apply this diff to simplify the code:
- _, err := delete.Exec(ctx)
+ err := delete.Exec(ctx)📝 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.
| _, err := delete.Exec(ctx) | |
| if err != nil { | |
| err := delete.Exec(ctx) | |
| if err != nil { |
| func (w *Writer) Delete(ctx context.Context, qk library.QueryKey) error { | ||
| delete := w.db.Node.Delete() | ||
|
|
||
| delete.Where(qk.Predicate()) | ||
|
|
||
| _, err := delete.Exec(ctx) | ||
| if err != nil { | ||
| return fault.Wrap(err, fctx.With(ctx)) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid shadowing built-in functions with variable names
In the Delete method, the variable delete shadows Go's built-in delete function. While this doesn't cause an error, it's best practice to avoid using names that shadow built-ins to prevent potential confusion and maintain code clarity.
Apply this diff to rename the variable:
-func (w *Writer) Delete(ctx context.Context, qk library.QueryKey) error {
- delete := w.db.Node.Delete()
- delete.Where(qk.Predicate())
- _, err := delete.Exec(ctx)
+func (w *Writer) Delete(ctx context.Context, qk library.QueryKey) error {
+ del := w.db.Node.Delete()
+ del.Where(qk.Predicate())
+ _, err := del.Exec(ctx)📝 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 (w *Writer) Delete(ctx context.Context, qk library.QueryKey) error { | |
| delete := w.db.Node.Delete() | |
| delete.Where(qk.Predicate()) | |
| _, err := delete.Exec(ctx) | |
| if err != nil { | |
| return fault.Wrap(err, fctx.With(ctx)) | |
| } | |
| return nil | |
| } | |
| func (w *Writer) Delete(ctx context.Context, qk library.QueryKey) error { | |
| del := w.db.Node.Delete() | |
| del.Where(qk.Predicate()) | |
| _, err := del.Exec(ctx) | |
| if err != nil { | |
| return fault.Wrap(err, fctx.With(ctx)) | |
| } | |
| return nil | |
| } |
When "Visibility rules" were implemented, I neglected to apply the logic to "Node Get" operations and only to tree traversal and mutations.
This meant that a page that was published which had un-published child pages were visible to anyone. Un-published pages included those that were: unlisted, draft or in-review.
This fix applies the same visibility rules so when a page (node) is requested, the children are returned with the following logic:
This ensures:
Also in this PR: