Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🔥 feat: Add support for Neo4j #1562

Merged
merged 29 commits into from
Feb 3, 2025
Merged

Conversation

the-real-i9
Copy link
Contributor

@the-real-i9 the-real-i9 commented Jan 30, 2025

Neo4j storage driver: Configuration declaration (config.go), Storage implementation (neo4j.go), Tests (neo4j_test.go), and a README.md

arangodb/README.md: typo fix: I think the author meant arangodb, but mistakenly typed mysql

postgres/config.go: removed duplicate cfg.Table == "" condition check

fixes #1561

Summary by CodeRabbit

Summary by CodeRabbit

  • Documentation

    • Updated ArangoDB README to correct implementation reference.
    • Added comprehensive README for Neo4j storage driver, detailing functionality, installation, and configuration.
    • Added Neo4j to the list of storage implementations in the main README.
  • New Features

    • Introduced Neo4j storage driver with key-value store functionality.
    • Added configuration management for Neo4j database connections.
    • Implemented storage operations such as Get, Set, Delete, and Reset.
  • Bug Fixes

    • Streamlined logic in PostgreSQL configuration management by removing redundant checks.
  • Tests

    • Created extensive test suite for Neo4j storage driver, including unit and benchmark tests for performance evaluation.

@the-real-i9 the-real-i9 requested a review from a team as a code owner January 30, 2025 11:28
@the-real-i9 the-real-i9 requested review from gaby, sixcolors, ReneWerner87 and efectn and removed request for a team January 30, 2025 11:28
Copy link
Contributor

coderabbitai bot commented Jan 30, 2025

Walkthrough

This pull request introduces a new Neo4j storage driver implementation along with minor documentation and configuration updates. Key changes include the addition of a comprehensive README for the Neo4j driver, the creation of configuration and implementation files, and the development of a test suite. Additionally, a small optimization was made to the Postgres configuration function by removing a redundant check. The ArangoDB README was also slightly modified to correct a reference.

Changes

File Change Summary
arangodb/README.md Corrected reference from "mysql implementation" to "arangodb implementation"
neo4j/README.md Added comprehensive documentation for Neo4j storage driver
neo4j/config.go Introduced Config struct and default configuration for Neo4j storage
neo4j/neo4j.go Implemented storage interface for Neo4j database operations
neo4j/neo4j_test.go Added extensive test suite for Neo4j storage driver
postgres/config.go Removed redundant check in configDefault function
README.md Added entry for Neo4j storage driver in the list of implementations

Assessment against linked issues

Objective Addressed Explanation
Introduce a Neo4j storage driver for GoFiber (#1561)

Suggested labels

📒 Documentation

Suggested reviewers

  • sixcolors
  • efectn
  • ReneWerner87

🐰 In the garden, a new path we weave,
With Neo4j’s magic, we truly believe.
A README so bright, with config in tow,
Our storage now ready, let’s watch it grow!
With tests all in place, we hop with delight,
In the world of databases, we shine ever bright! 🌟


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (10)
neo4j/neo4j.go (2)

116-116: Validate the use of utils.UnsafeBytes().
While it efficiently converts strings to byte slices, changes to the underlying string might lead to unexpected behavior or data races in concurrent settings. Verify that it’s safe for your use case.


172-175: Ensure goroutine shutdown before closing the Neo4j driver.
Closing the driver immediately after signaling s.done could lead to a race if the gcTicker goroutine attempts to execute Cypher queries at the same time. Consider waiting for the goroutine to complete if you need strict ordering.

neo4j/config.go (1)

51-54: Clarify documentation for the Reset field.
The comment references “existing Table” rather than the node name. Consider updating to reference the actual entity in Neo4j for consistency.

-// Reset clears any existing keys in existing Table
+// Reset clears any existing nodes in the Neo4j database node.
neo4j/neo4j_test.go (2)

64-65: Use duration-based constants to improve test stability.
Relying on 1100ms might lead to flaky tests. A small buffer above exp improves reliability in CI environments with variable performance.

 time.Sleep(1100 * time.Millisecond)
+time.Sleep(exp + 200*time.Millisecond)

119-121: Reevaluate test data for non-UTF8 validation.
"0xF5" is a valid ASCII sequence, so it may not fully test non-UTF8 handling. If genuine binary data is expected, consider using truly non-UTF8 byte patterns.

neo4j/README.md (5)

6-8: Enhance version information clarity.

Consider adding more specific version details:

  1. Specify the minimum supported version of the neo4j-go-driver.
  2. Include the exact minimum Go version (e.g., "Requires Go 1.20.0 and above").

20-28: Add method descriptions and error types.

Consider enhancing the signatures section with:

  1. Brief descriptions of each method's purpose
  2. Specific error types that each method can return
  3. Example usage for complex methods

Example format:

// Get retrieves a value by key from the storage
// Returns: value ([]byte), ErrNotFound or other storage-specific errors
func (s *Storage) Get(key string) ([]byte, error)

54-65: Expand examples section with common use cases.

Consider adding examples for:

  1. Error handling
  2. Basic CRUD operations (Get/Set/Delete)
  3. Authentication setup
  4. Connection with TLS
  5. Working with expiration times

Example:

// Example with error handling and basic operations
store := neo4jstore.New(config)
defer store.Close()

// Set with expiration
if err := store.Set("key", []byte("value"), 1*time.Hour); err != nil {
    log.Fatal(err)
}

// Get with error handling
value, err := store.Get("key")
if err != nil {
    if errors.Is(err, neo4jstore.ErrNotFound) {
        // Handle not found
    }
    log.Fatal(err)
}

123-134: Enhance security documentation in authentication section.

Consider adding:

  1. Security best practices for authentication
  2. Warning about storing credentials in code
  3. Environment variables usage example
  4. TLS/SSL configuration recommendations

Example addition:

#### Security Best Practices
- Use environment variables for credentials
- Enable TLS for production environments
- Implement proper credential rotation
- Use least privilege principle when setting up database users

Example using environment variables:
```go
store := neo4jstore.New(neo4jstore.Config{
    Username: os.Getenv("NEO4J_USER"),
    Password: os.Getenv("NEO4J_PASS"),
})
🧰 Tools
🪛 LanguageTool

[grammar] ~129-~129: Two determiners in a row. Choose either “the” or “this”.
Context: ... and Password fields: This will cause the this storage driver to use Basic Auth. Othe...

(DT_DT)


140-145: Add production configuration guidance.

Consider adding a note about production-ready configuration, including:

  1. Recommended GCInterval for different workloads
  2. Production-appropriate TargetBoltURI format
  3. Node naming best practices
  4. Performance implications of Reset flag
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f29aa52 and cb30085.

⛔ Files ignored due to path filters (2)
  • neo4j/go.mod is excluded by !**/*.mod
  • neo4j/go.sum is excluded by !**/*.sum, !**/*.sum
📒 Files selected for processing (6)
  • arangodb/README.md (1 hunks)
  • neo4j/README.md (1 hunks)
  • neo4j/config.go (1 hunks)
  • neo4j/neo4j.go (1 hunks)
  • neo4j/neo4j_test.go (1 hunks)
  • postgres/config.go (0 hunks)
💤 Files with no reviewable changes (1)
  • postgres/config.go
✅ Files skipped from review due to trivial changes (1)
  • arangodb/README.md
🧰 Additional context used
🪛 LanguageTool
neo4j/README.md

[grammar] ~129-~129: Two determiners in a row. Choose either “the” or “this”.
Context: ... and Password fields: This will cause the this storage driver to use Basic Auth. Othe...

(DT_DT)

🪛 golangci-lint (1.62.2)
neo4j/neo4j.go

204-204: Error return value of json.Unmarshal is not checked

(errcheck)

🔇 Additional comments (2)
neo4j/neo4j.go (1)

55-60: Double-check the “FINISH” keyword in Cypher queries.
Standard Neo4j Cypher does not typically use a FINISH clause. Confirm that this syntax is valid/executable in your environment or remove it if unnecessary.

Also applies to: 75-78

neo4j/config.go (1)

85-91: Validate partial credentials.
Currently, if only Username or Password is provided (not both), neo4j.BasicAuth is not invoked. If partial credentials are unintended or invalid, consider handling that case or clarifying it in the documentation.

@ReneWerner87
Copy link
Member

ReneWerner87 commented Jan 30, 2025

thx
don´t forget the test setup/workflow and the readme in the first level for all storages

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
neo4j/README.md (1)

59-64: Enhance custom configuration example.

Consider expanding the example to demonstrate authentication setup, which is a crucial aspect of Neo4j configuration. This would help users understand how to properly set up authentication using the available options (DB, Auth, or Username/Password).

 store := neo4jstore.New(neo4jstore.Config{
  DB:              driver,
  Node:           "fiber_storage",
  Reset:           false,
  GCInterval:      10 * time.Second,
+ // Authentication example:
+ Username:        "neo4j",
+ Password:        "password",
+ // Or using Auth:
+ // Auth:         neo4j.BasicAuth("neo4j", "password", ""),
 })
neo4j/neo4j.go (1)

197-199: Consider logging garbage collection errors.

While it's acceptable for GC to fail silently, logging errors could help with debugging issues.

 func (s *Storage) gc(t time.Time) {
-	_, _ = neo4j.ExecuteQuery(context.Background(), s.db, s.cypherGC, map[string]any{"exp": t.Unix()}, neo4j.EagerResultTransformer)
+	if _, err := neo4j.ExecuteQuery(context.Background(), s.db, s.cypherGC, map[string]any{"exp": t.Unix()}, neo4j.EagerResultTransformer); err != nil {
+		// Use your preferred logging mechanism
+		fmt.Printf("neo4j gc error: %v\n", err)
+	}
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cb30085 and c5fa260.

📒 Files selected for processing (2)
  • neo4j/README.md (1 hunks)
  • neo4j/neo4j.go (1 hunks)
🧰 Additional context used
📓 Learnings (1)
neo4j/neo4j.go (1)
Learnt from: the-real-i9
PR: gofiber/storage#1562
File: neo4j/neo4j.go:101-101
Timestamp: 2025-01-30T11:50:03.517Z
Learning: In Neo4j Cypher queries, using `OPTIONAL MATCH` ensures at least one record is always returned, with unmatched patterns resulting in null values. This makes additional length checks on the records array unnecessary when combined with proper null checking.
🔇 Additional comments (3)
neo4j/README.md (2)

38-38: Fix typo in installation instructions.

"noe4j" should be "neo4j".


70-120: LGTM! Well-documented configuration options.

The Config struct documentation clearly explains all available options and their defaults.

neo4j/neo4j.go (1)

85-117: LGTM! Proper implementation of Get method.

The implementation correctly uses OPTIONAL MATCH and handles null values appropriately, ensuring safe record access.

@the-real-i9
Copy link
Contributor Author

Oh! Wait. I'm yet to write the github workflow yaml file and update the READMEs

@gaby gaby changed the title Neo4j storage driver (with little fixes in arangodb and postgres) 🔥 feat: Add support for Neo4j Jan 30, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
README.md (1)

70-70: LGTM! Consider adding alt text to the workflow badge.

The Neo4j entry follows the consistent format and structure of other storage implementations. However, for better accessibility:

Add alt text to the workflow badge image:

-- [Neo4j](./neo4j/README.md) <a href="https://github.com/gofiber/storage/actions?query=workflow%3A%22Tests+Neo4j%22"> <img src="https://img.shields.io/github/actions/workflow/status/gofiber/storage/test-neo4j.yml?branch=main&label=%F0%9F%A7%AA%20&style=flat&color=75C46B" /> </a>
++ [Neo4j](./neo4j/README.md) <a href="https://github.com/gofiber/storage/actions?query=workflow%3A%22Tests+Neo4j%22"> <img alt="Neo4j Tests Status" src="https://img.shields.io/github/actions/workflow/status/gofiber/storage/test-neo4j.yml?branch=main&label=%F0%9F%A7%AA%20&style=flat&color=75C46B" /> </a>
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

70-70: Images should have alternate text (alt text)
null

(MD045, no-alt-text)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5e5858e and 4a7a7f0.

📒 Files selected for processing (1)
  • README.md (1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md

70-70: Images should have alternate text (alt text)
null

(MD045, no-alt-text)

@the-real-i9
Copy link
Contributor Author

the-real-i9 commented Jan 31, 2025

@gaby Done.

- [Neo4j](./neo4j/README.md) <a href="https://github.com/gofiber/storage/actions?query=workflow%3A%22Tests+Neo4j%22"> <img src="https://img.shields.io/github/actions/workflow/status/gofiber/storage/test-neo4j.yml?branch=main&label=%F0%9F%A7%AA%20&style=flat&color=75C46B" /> </a>

I can see the tag beside it now. It should change from "no status" after this pull request is merged.

@the-real-i9 the-real-i9 requested a review from gaby January 31, 2025 14:51
@the-real-i9
Copy link
Contributor Author

@gaby Thanks for this opportunity. And this conversation.

@gaby
Copy link
Member

gaby commented Feb 2, 2025

@gaby Thanks for this opportunity. And this conversation.

Will review today, thanks!

neo4j/config.go Outdated
@@ -0,0 +1,102 @@
package neo4jstore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@the-real-i9 I was looking at the other drivers and this can be named neo4j. For example our redis one is named package redis same as the upstream one and it doesn't collide.

Copy link
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing, overall looks good.


// Start a Neo4j test container
neo4jContainer, err := neo4j.Run(ctx,
"neo4j:5.26",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to a const outside this function. That way it's easier to find/update in the future.

@the-real-i9
Copy link
Contributor Author

the-real-i9 commented Feb 3, 2025

@gaby Now I get it. It's ok to panic during initializations where the server is yet to start. But never in the code where the server has already started, and users are already sending requests.

I've been trying to get a clear understanding of where to panic and where not to. Thanks!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
neo4j/neo4j_test.go (1)

106-127: Improve test reliability by adding pre-expiration check.

While the test is functionally correct, it could be more reliable by verifying the value exists before expiration.

Apply this diff to improve test reliability:

 err := testStore.Set(key, val, exp)
 require.NoError(t, err)

+// Verify value exists before expiration
+result, err := testStore.Get(key)
+require.NoError(t, err)
+require.Equal(t, val, result)
+
 time.Sleep(200 * time.Millisecond)

 result, err := testStore.Get(key)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4a7a7f0 and 93c211d.

📒 Files selected for processing (3)
  • neo4j/config.go (1 hunks)
  • neo4j/neo4j.go (1 hunks)
  • neo4j/neo4j_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • neo4j/config.go
🧰 Additional context used
📓 Learnings (1)
neo4j/neo4j.go (6)
Learnt from: gaby
PR: gofiber/storage#1562
File: neo4j/neo4j.go:221-221
Timestamp: 2025-02-02T23:41:30.921Z
Learning: In the Neo4j storage implementation, the `mapToStruct` function should return an error instead of panicking when JSON unmarshaling fails to prevent server crashes.
Learnt from: the-real-i9
PR: gofiber/storage#1562
File: neo4j/neo4j.go:50-52
Timestamp: 2025-01-30T12:29:41.774Z
Learning: Storage drivers in the gofiber/storage repository intentionally panic on connection errors as a design choice to fail fast when storage is unavailable.
Learnt from: the-real-i9
PR: gofiber/storage#1562
File: neo4j/neo4j.go:50-52
Timestamp: 2025-01-30T12:29:41.774Z
Learning: In the Neo4j storage driver initialization, connectivity verification errors are considered unexpected according to the maintainer.
Learnt from: the-real-i9
PR: gofiber/storage#1562
File: neo4j/neo4j.go:44-45
Timestamp: 2025-01-30T12:32:54.196Z
Learning: In the Neo4j storage driver, connection errors are handled by panicking during connectivity verification, making additional error logging redundant.
Learnt from: the-real-i9
PR: gofiber/storage#1562
File: neo4j/neo4j.go:44-45
Timestamp: 2025-01-30T12:32:54.196Z
Learning: In the Neo4j storage driver, connection errors are handled through `VerifyConnectivity` which panics on failure, making additional error logging at connection creation time redundant.
Learnt from: the-real-i9
PR: gofiber/storage#1562
File: neo4j/neo4j.go:101-101
Timestamp: 2025-01-30T11:50:03.517Z
Learning: In Neo4j Cypher queries, using `OPTIONAL MATCH` ensures at least one record is always returned, with unmatched patterns resulting in null values. This makes additional length checks on the records array unnecessary when combined with proper null checking.
🔇 Additional comments (22)
neo4j/neo4j.go (10)

40-42: LGTM!

The function correctly creates a new driver with context and properly handles error propagation.


44-98: LGTM!

The function correctly:

  • Initializes a new storage instance
  • Creates an index for better query performance
  • Starts a garbage collection ticker
  • Follows the design choice of panicking on connection errors

100-136: LGTM!

The function correctly:

  • Handles empty keys
  • Handles expired values
  • Handles non-existent values
  • Handles JSON unmarshaling errors

138-164: LGTM!

The function correctly:

  • Handles empty keys and values
  • Handles expiration time
  • Handles error propagation

166-174: LGTM!

The function correctly:

  • Handles empty keys
  • Handles error propagation

176-185: LGTM!

The function correctly:

  • Executes the reset query
  • Handles error propagation

187-192: LGTM!

The function correctly:

  • Signals the gc ticker to stop
  • Closes the database connection

194-197: LGTM!

The function correctly returns the database client.


199-211: LGTM!

The function correctly:

  • Starts a ticker
  • Handles cleanup
  • Handles shutdown signal

213-216: LGTM!

The function correctly:

  • Executes the gc query
  • Ignores errors as this is a background task
neo4j/neo4j_test.go (12)

15-49: LGTM!

The function correctly:

  • Starts a Neo4j test container
  • Handles container termination errors
  • Initializes the store

51-65: LGTM!

The function correctly:

  • Uses t.Parallel() for concurrent test execution
  • Uses t.Cleanup() for proper cleanup
  • Tests setting a key-value pair

67-84: LGTM!

The function correctly:

  • Uses t.Parallel() for concurrent test execution
  • Uses t.Cleanup() for proper cleanup
  • Tests upserting a key-value pair

86-104: LGTM!

The function correctly:

  • Uses t.Parallel() for concurrent test execution
  • Uses t.Cleanup() for proper cleanup
  • Tests getting a key-value pair

129-141: LGTM!

The function correctly:

  • Uses t.Parallel() for concurrent test execution
  • Uses t.Cleanup() for proper cleanup
  • Tests getting an expired key

143-153: LGTM!

The function correctly:

  • Uses t.Parallel() for concurrent test execution
  • Uses t.Cleanup() for proper cleanup
  • Tests getting a non-existent key

155-176: LGTM!

The function correctly:

  • Uses t.Parallel() for concurrent test execution
  • Uses t.Cleanup() for proper cleanup
  • Tests deleting a key

178-203: LGTM!

The function correctly:

  • Uses t.Parallel() for concurrent test execution
  • Uses t.Cleanup() for proper cleanup
  • Tests resetting all keys

205-220: LGTM!

The function correctly:

  • Uses t.Parallel() for concurrent test execution
  • Uses t.Cleanup() for proper cleanup
  • Tests non-UTF8 values

222-230: LGTM!

The function correctly:

  • Uses t.Parallel() for concurrent test execution
  • Uses t.Cleanup() for proper cleanup
  • Tests closing the store

232-239: LGTM!

The function correctly:

  • Uses t.Parallel() for concurrent test execution
  • Uses t.Cleanup() for proper cleanup
  • Tests getting the connection

241-290: 🛠️ Refactor suggestion

Check error from Set in SetAndDelete benchmark.

The benchmark ignores the error from Set operation, which could mask issues.

Apply this diff to improve error handling:

 var err error
 for i := 0; i < b.N; i++ {
-    _ = testStore.Set("john", []byte("doe"), 0)
+    err = testStore.Set("john", []byte("doe"), 0)
+    require.NoError(b, err)
     err = testStore.Delete("john")
 }

Likely invalid or redundant comment.

@the-real-i9
Copy link
Contributor Author

the-real-i9 commented Feb 3, 2025

I like the idea of starting with one container per test, so instead of having a TestMain function, having a function that creates the Neo4j store, and calling it por test would be my choice. It will allow parallel execution without the need of taking care of the test data

@mdelapenya
Hmm... It takes a sometime for the container to start, you know... I'll do this and compare the time it takes for the whole test to run in both alternatives.


[UPDATE]:
Ok. So I tried it. Turns out the simplest test function took around 60s, while others took around 40s. As of this writing the running time is currently at 7mins. on github workflow, and it's not done.


[UPDATE]:
The tests eventually failed due to timeout.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
neo4j/neo4j_test.go (2)

94-109: 🛠️ Refactor suggestion

Strengthen expiration test reliability.

The current expiration test could be flaky and doesn't verify the actual expiration process.

 func Test_Neo4jStore_Set_Expiration(t *testing.T) {
+    t.Parallel()
     var (
         key = "john"
         val = []byte("doe")
         exp = 100 * time.Millisecond
     )
 
     err := testStore.Set(key, val, exp)
     require.NoError(t, err)
 
+    // Verify value exists before expiration
+    result, err := testStore.Get(key)
+    require.NoError(t, err)
+    require.Equal(t, val, result)
+
+    // Wait for expiration with a small buffer
     time.Sleep(200 * time.Millisecond)
 
-    result, err := testStore.Get(key)
+    result, err = testStore.Get(key)
     require.NoError(t, err)
     require.Zero(t, len(result))
 }

208-219: ⚠️ Potential issue

Fix error handling in SetAndDelete benchmark.

The Set operation error is ignored, which could mask issues.

 func Benchmark_Neo4jStore_SetAndDelete(b *testing.B) {
     b.ReportAllocs()
     b.ResetTimer()
 
     var err error
     for i := 0; i < b.N; i++ {
-        _ = testStore.Set("john", []byte("doe"), 0)
+        err = testStore.Set("john", []byte("doe"), 0)
+        require.NoError(b, err)
         err = testStore.Delete("john")
     }
 
     require.NoError(b, err)
 }
🧹 Nitpick comments (1)
neo4j/neo4j_test.go (1)

182-219: Add more comprehensive benchmark scenarios.

The current benchmarks cover basic operations. Consider adding more complex scenarios to better understand performance characteristics.

Suggested additional benchmarks:

  1. Concurrent operations
  2. Batch operations
  3. Operations with varying key/value sizes
  4. Operations with expiration

Example:

func Benchmark_Neo4jStore_ConcurrentSet(b *testing.B) {
    b.ReportAllocs()
    b.ResetTimer()

    b.RunParallel(func(pb *testing.PB) {
        for pb.Next() {
            err := testStore.Set("john", []byte("doe"), 0)
            require.NoError(b, err)
        }
    })
}

func Benchmark_Neo4jStore_BatchSet(b *testing.B) {
    b.ReportAllocs()
    keys := make([]string, 100)
    vals := make([][]byte, 100)
    for i := 0; i < 100; i++ {
        keys[i] = fmt.Sprintf("key%d", i)
        vals[i] = []byte(fmt.Sprintf("val%d", i))
    }
    b.ResetTimer()

    for i := 0; i < b.N; i++ {
        for j := 0; j < 100; j++ {
            err := testStore.Set(keys[j], vals[j], 0)
            require.NoError(b, err)
        }
    }
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 93c211d and 7e74ccf.

⛔ Files ignored due to path filters (2)
  • neo4j/go.mod is excluded by !**/*.mod
  • neo4j/go.sum is excluded by !**/*.sum, !**/*.sum
📒 Files selected for processing (1)
  • neo4j/neo4j_test.go (1 hunks)
🔇 Additional comments (2)
neo4j/neo4j_test.go (2)

21-24: Move container configuration to constants.

The container version and credentials should be defined as package-level constants for easier maintenance and reusability.

+const (
+    neo4jVersion     = "neo4j:5.26"
+    neo4jAdminPasswd = "pass#w*#d"
+)

 func TestMain(m *testing.M) {
     ctx := context.Background()
 
     // Start a Neo4j test container
     neo4jContainer, err := neo4j.Run(ctx,
-        "neo4j:5.26",
-        neo4j.WithAdminPassword("pass#w*#d"),
+        neo4jVersion,
+        neo4j.WithAdminPassword(neo4jAdminPasswd),
     )

16-55: Consider per-test container setup for parallel execution.

Instead of using TestMain, consider creating a helper function that sets up a Neo4j container for each test. This would allow for parallel test execution and better isolation.

Here's a suggested approach:

func setupTestStore(t *testing.T) (*Storage, func()) {
    t.Helper()
    ctx := context.Background()

    neo4jContainer, err := neo4j.Run(ctx,
        neo4jVersion,
        neo4j.WithAdminPassword(neo4jAdminPasswd),
    )
    require.NoError(t, err)

    uri, err := neo4jContainer.BoltUrl(ctx)
    require.NoError(t, err)

    store := New(Config{
        Reset:    true,
        URI:      uri,
        Username: "neo4j",
        Password: neo4jAdminPasswd,
    })

    cleanup := func() {
        store.Close()
        if err := neo4jContainer.Terminate(ctx); err != nil {
            t.Logf("Failed to terminate Neo4j container: %v", err)
        }
    }

    return store, cleanup
}

// Usage in tests:
func Test_Neo4jStore_Set(t *testing.T) {
    t.Parallel()
    store, cleanup := setupTestStore(t)
    defer cleanup()
    // ... test logic
}

@the-real-i9
Copy link
Contributor Author

I like the idea of starting with one container per test, so instead of having a TestMain function, having a function that creates the Neo4j store, and calling it por test would be my choice. It will allow parallel execution without the need of taking care of the test data

@mdelapenya Hmm... It takes a sometime for the container to start, you know... I'll do this and compare the time it takes for the whole test to run in both alternatives.

[UPDATE]: Ok. So I tried it. Turns out the simplest test function took around 60s, while others took around 40s. As of the writing the running time is currently at 7mins. on github workflow, and it's not done.

[UPDATE]: The tests eventually failed due to timeout.

@mdelapenya With a single container in TestMain, running all tests took around 20s.

@the-real-i9
Copy link
Contributor Author

@gaby I've addressed all the reviews. Thanks. The PR has been updated.

Copy link
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
neo4j/README.md (1)

52-65: Initialization Examples Evaluation

The dual examples demonstrate both default and custom configurations effectively. For the custom configuration example, consider clarifying what the driver variable refers to (e.g., mention that it should be an initialized neo4j.DriverWithContext instance) to help users understand the context.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7e74ccf and 69a51dc.

📒 Files selected for processing (1)
  • neo4j/README.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
neo4j/README.md

[grammar] ~8-~8: After the number ‘two’, use a plural noun. Did you mean “releases”?
Context: ...driver). > Note: Requires latest two release of Golang ### Table of Contents - [...

(CD_NNU)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: compare (neo4j)
🔇 Additional comments (8)
neo4j/README.md (8)

1-4: YAML Front Matter Validation

The YAML front matter is concise, clear, and correctly declares the document's identity and title for Neo4j.


10-17: Table of Contents Clarity

The Table of Contents is well structured and easy to navigate. Just ensure the links remain updated with any future additions or adjustments.


18-29: Public API Signatures Overview

The signatures listed for the public API (including New, Get, Set, Delete, Reset, Close, and Conn) are clearly presented. This gives users a comprehensive view of the supported methods.


30-42: Installation Instructions Review

The Installation section provides clear instructions for initializing a Go module and installing the Neo4j storage driver. Consider updating the placeholder github.com/<user>/<repo> with a note indicating that users should replace it with their actual repository.


44-50: Import Statement Example

The example for importing the Neo4j storage package is concise and correct. This snippet should help users integrate the package without confusion.


67-121: Configuration Struct Documentation

The configuration struct and its associated comments are thorough and clear. Each field is well explained, and default values are documented, aiding in usability and maintenance.


123-134: Authentication Note Details

The authentication section clearly explains the three prioritized methods for providing credentials. The instructions are helpful in avoiding misconfigurations. It might be beneficial to emphasize that if authentication is not needed, leaving the fields empty is acceptable.


135-147: Default Configuration Block Accuracy

The default configuration block is correctly defined and aligns with the configuration struct. It provides sensible defaults for a basic Neo4j setup. Make sure to update these defaults if the implementation changes in the future.


A Neo4j storage driver using [neo4j/neo4j-go-driver](https://github.com/neo4j/neo4j-go-driver).

> **Note: Requires latest two release of Golang**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Grammar Correction for Go Version Note

The note currently reads "Requires latest two release of Golang." Please update it to "Requires latest two releases of Golang" to correct the pluralization.

🧰 Tools
🪛 LanguageTool

[grammar] ~8-~8: After the number ‘two’, use a plural noun. Did you mean “releases”?
Context: ...driver). > Note: Requires latest two release of Golang ### Table of Contents - [...

(CD_NNU)

@ReneWerner87 ReneWerner87 merged commit e4abddc into gofiber:main Feb 3, 2025
19 checks passed
@gaby
Copy link
Member

gaby commented Feb 3, 2025

@the-real-i9 Thank you 💪

@the-real-i9
Copy link
Contributor Author

@the-real-i9 Thank you 💪

@gaby @ReneWerner87 🙏 It's an honour to be a part of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🤔 [Question]: Is there a particular reason why Neo4j isn't included among the storage drivers?
4 participants