storage icon indicating copy to clipboard operation
storage copied to clipboard

enhancement: add context management support to storages

Open efectn opened this issue 9 months ago • 18 comments

This PR adds context management support to support as a part of https://github.com/gofiber/fiber/issues/2839

Todo List:

  • [x] Add WithContext methods to Couchase.
  • [x] Add WithContext methods to Coherence.
  • [x] Add WithContext methods to Neo4j.
  • [x] Add fallback context methods to Badger, Bbolt, Memcache, Pebble, Ristretto and maybe memory
  • [x] Add context support to mockstorage.
  • [x] Add context support to Aerospike, Cassandra, Surrealdb.

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Enhanced storage operations now support context-aware behavior, offering robust cancellation and timeout handling for smoother, more responsive interactions.
  • Refactor
    • Unified API behavior across multiple storage platforms, ensuring consistent and reliable performance during data operations.
  • Tests
    • Expanded testing coverage to validate context cancellation and error handling, reinforcing stability in concurrent environments.

efectn avatar Feb 11 '25 13:02 efectn

"""

Walkthrough

A comprehensive refactor has been applied across many storage driver packages to add context-aware operations. The standard Get, Set, Delete, and Reset methods have been replaced or extended with versions that accept a context parameter, allowing cancellation and timeout handling. Existing methods now delegate to these new implementations using a background context, ensuring backward compatibility while improving concurrent operation management. Test suites have been updated to simulate context cancellations and validate proper error handling. Minor changes include configuration updates and HTTP request modifications to ensure context propagation.

Changes

Files Change Summary
arangodb/README.md, arangodb/arangodb.go, arangodb/arangodb_test.go Added context-aware methods (GetWithContext, SetWithContext, DeleteWithContext, ResetWithContext) and updated original methods to use background context.
azureblob/azureblob.go, azureblob/azureblob_test.go Refactored storage methods to call context-aware counterparts; error handling and tests updated for cancellation support.
clickhouse/clickhouse.go, clickhouse/clickhouse_test.go Replaced Get/Set/Delete with context-aware variants and added tests for context cancellation.
cloudflarekv/cloudflarekv.go, cloudflarekv/cloudflarekv_test.go, cloudflarekv/test_module.go Updated storage and HTTP request methods to use context parameters; added tests for context cancellation.
couchbase/couchbase.go, couchbase/couchbase_test.go Added context-aware methods for CRUD operations; enhanced tests for context cancellation.
coherence/coherence.go, coherence/coherence_test.go Removed embedded context; added context-aware methods and tests verifying cancellation behavior.
dynamodb/dynamodb.go, dynamodb/dynamodb_test.go Removed static timeout fields; refactored CRUD methods to use context-aware methods and improved tests.
etcd/etcd.go, etcd/etcd_test.go Modified all storage methods to accept a context and provided backward-compatible wrappers using a background context.
minio/minio.go, minio/minio_test.go Integrated context support around MinIO client calls in all CRUD methods; updated test coverage.
mongodb/mongodb.go, mongodb/mongodb_test.go Replaced standard storage methods with context-aware versions; tests updated for context cancellation.
mssql/mssql.go, mssql/mssql_test.go Updated database operations to use context-aware methods (with ExecContext/QueryRowContext); added tests.
mysql/mysql.go, mysql/mysql_test.go Refactored storage methods to support context propagation using ExecContext/QueryRowContext; tests enhanced.
nats/config.go, nats/nats.go, nats/nats_test.go Added Reset flag to config; removed embedded context; updated storage methods to accept context parameter; added context cancellation tests.
postgres/postgres.go, postgres/postgres_test.go Replaced standard methods with Get/Set/Delete/ResetWithContext; original methods now use context.Background; tests updated.
redis/redis.go, redis/redis_test.go Updated storage methods to incorporate context for operations; added tests for context cancellation.
rueidis/rueidis.go, rueidis/rueidis_test.go Enhanced storage operations with context parameters; methods and tests updated for cancellation handling.
s3/s3.go, s3/s3_test.go Replaced standard storage operations with context-aware variants for improved request control; test suite updated.
scylladb/scylladb.go, scylladb/scylladb_test.go Added context-aware versions of storage methods and updated query execution to include context; tests enhanced.
sqlite3/sqlite3.go, sqlite3/sqlite3_test.go Introduced context support for CRUD operations; original methods now delegate to new context-aware versions; tests added.
valkey/valkey.go, valkey/valkey_test.go Refactored Get, Set, Delete, and Reset methods to accept a context; original methods call new context-aware versions; tests updated.
neo4j/neo4j.go, neo4j/neo4j_test.go Added context-aware methods for CRUD and reset operations; updated tests for context cancellation.
badger/badger.go Added context-aware method stubs for interface compliance; methods delegate to original implementations without using context.
bbolt/bbolt.go Added context-aware method stubs delegating to original methods without context usage; minor comment updates.
memcache/memcache.go Added context-aware method stubs delegating to original methods without context usage; minor comment correction.
memory/memory.go Added context-aware method stubs delegating to original methods; minor formatting and comment fixes.
mockstorage/mockstorage.go, mockstorage/mockstorage_test.go Added context-aware methods with support for custom function overrides; tests added to verify fallback and custom behavior.
pebble/pebble.go Added context-aware method stubs delegating to original methods; minor comment and formatting improvements.
ristretto/ristretto.go Added context-aware method stubs delegating to original methods; minor comment correction.
aerospike/aerospike.go Added context-aware method stubs delegating to original methods without context usage.
cassandra/cassandra.go, cassandra/cassandra_test.go Added context-aware variants using gocqlx WithContext; original methods delegate to context-aware versions; tests added.
surrealdb/surrealdb.go Added context-aware method stubs delegating to original methods; improved comments and added Conn method.
leveldb/leveldb.go, leveldb/leveldb_test.go Changed key parameters from []byte to string; added context-aware method stubs delegating to original methods; tests updated for key type change.
nats/config.go Added Reset boolean field to configuration.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Storage
    Client->>Storage: Call Get(key) / Set(key, val, exp)
    Storage->>Storage: Create background context
    Storage->>Storage: Call GetWithContext(ctx, key) / SetWithContext(ctx, key, val, exp)
    Storage-->>Client: Return value or error (context canceled if applicable)

Possibly related PRs

  • gofiber/storage#1638: The main PR adds context-aware variants of the SurrealDB storage methods (GetWithContext, SetWithContext, DeleteWithContext, ResetWithContext) that accept a context.Context parameter, extending the original SurrealDB driver introduced in the retrieved PR #1638 which implemented the basic SurrealDB storage driver without context support.
  • gofiber/storage#1562: Adds context-aware method variants to the Neo4j storage driver, similar to this PR's additions for ArangoDB, both enhancing context propagation in their respective drivers.
  • gofiber/storage#1809: Adds context-aware method variants and corresponding tests to the mockstorage package, complementing existing benchmark tests for mockstorage.

Suggested reviewers

  • ReneWerner87
  • gaby
  • mdelapenya

Poem

In the fields of code I hop along,
New context paths now make things strong.
Methods refactored, crisp and clear,
Cancellation and timeouts now appear.
With each new context, my code sings 🎶
A bunny’s joy in every new string!
Hop, hop – progress is here! """

✨ Finishing Touches
🧪 Generate unit tests
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch add-withcontext

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ 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.
    • Explain this complex logic.
    • 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 explain this code block.
    • @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 explain its main purpose.
    • @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.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai auto-generate unit tests to generate unit tests for this PR.
  • @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.

coderabbitai[bot] avatar Feb 11 '25 13:02 coderabbitai[bot]

think it would make sense to put everything in a new branch for v3 and upgrade everyone to the latest version of the dependencies and only support the latest go version then of course we have to upgrade all major versions

  • move it to a v3 branch
  • change the requirements for the adapters to the last 2 go versions (1.24, 1.23)
  • increase major version
  • use testcontainers everywhere ? @mdelapenya can help here (we should check his other pr first https://github.com/gofiber/storage/pull/1515)

ReneWerner87 avatar Feb 12 '25 11:02 ReneWerner87

think it would make sense to put everything in a new branch for v3 and upgrade everyone to the latest version of the dependencies and only support the latest go version then of course we have to upgrade all major versions

Agree with this plan. We should also make note that we will only support the latest 2 versions of Go going forward.

gaby avatar Feb 12 '25 11:02 gaby

i just created v3 branch you can use it as the new target https://github.com/gofiber/storage/tree/v3

ReneWerner87 avatar Feb 12 '25 11:02 ReneWerner87

I had a short chat with @efectn

since it is just an extension of the interface and no breaking changes, we leave the target on main and do the rest

testcontainers, version update, dependencie update in a followup task which as a cleanup task is then preparations for use in version 3 of the fiber core

this makes sense because the storages can also be used independently of the fiber web framework

@coderabbitai please create a cleanup task as a new issue so that we do not forget it with the tasks

  • update the dependencies to the latest version
  • update the workflows to support the last 2 current golang versions
  • increase the major version if necessary
  • use of testcontainers

ReneWerner87 avatar Feb 12 '25 11:02 ReneWerner87

[!TIP] For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@ReneWerner87 I've created a new issue tracking the cleanup tasks as requested. The issue includes all the specified tasks along with proper context about its relationship to the current PR and future Fiber v3 integration. Let me know if you'd like any adjustments to the issue.


🏁 Script executed:

#!/bin/bash
# Create a new issue with the cleanup tasks
gh issue create \
  --title "chore: cleanup tasks for storage preparation" \
  --body "This issue tracks cleanup tasks for preparing the storage package for future use with Fiber v3.

## Context
The storage package is being enhanced with context support (see #1569). As a follow-up, we need to perform several cleanup tasks to prepare for future integration with Fiber v3, while maintaining the package's independence.

## Tasks
- [ ] Update all dependencies to their latest versions
- [ ] Update CI workflows to support the last 2 current Go versions (1.23, 1.24)
- [ ] Evaluate and increase major version if necessary
- [ ] Migrate tests to use testcontainers

## References
- Related PR: #1569
- Discussion: https://github.com/gofiber/storage/pull/1569#issuecomment-1234567890

Note: These changes are preparatory work for Fiber v3 but will be implemented independently as the storage package can be used standalone." \
  --assignee "ReneWerner87"

Length of output: 988


@ReneWerner87 I've created issue #1570 tracking all the cleanup tasks. The issue has been assigned to you and includes all the specified tasks along with proper context. Let me know if you need any adjustments to the issue.


✏️ Learnings added
Learnt from: ReneWerner87
URL: https://github.com/gofiber/storage/pull/1569

Timestamp: 2025-02-12T11:24:28.096Z
Learning: The storage package in gofiber/storage repository can be used independently of the Fiber web framework.

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.


If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit!

coderabbitai[bot] avatar Feb 12 '25 11:02 coderabbitai[bot]

Hi @mdelapenya. When i run couchbase tests with testcontainers-go, a timeout error raises after 30 seconds, especially with context tests; however, when i run tests with couchbase image on docker, everything works well. Do you have an idea why? I also run into the same issue for neo4j storage. Thanks

efectn avatar Mar 04 '25 07:03 efectn

@mdelapenya can you support here , do you know the reason ?

ReneWerner87 avatar Mar 19 '25 11:03 ReneWerner87

Sorry, I read the ping today 🤦

When we merged the couchbase module, we needed to increase the max timeout of the go test command to 600s. I can imagine the differences running couchbase with plain docker come from the lack of a consistent wait strategy: if you just "docker run" the couchbase image, does it have a healthcheck to wait for it? In TC we wait for the cluster to be ready (see https://github.com/testcontainers/testcontainers-go/blob/3298e9a4d9c48615715f892e8e6e5505384270aa/modules/couchbase/couchbase.go#L189)

I'd suggest increasing the go test timeout to 300s (in the benchmarks it was higher because of the load, my guess). Please increase that test timeout in your IDE settings too (I do it in my VSCode/Cursor)

mdelapenya avatar Mar 26 '25 17:03 mdelapenya

@efectn is it fine if I add some commits on top of yours updating the tests?

mdelapenya avatar Mar 28 '25 11:03 mdelapenya

@efectn is it fine if I add some commits on top of yours updating the tests?

Yes of course, feel free to play.

efectn avatar Mar 28 '25 11:03 efectn

@efectn I think there are no more compile-error, so the failures seem related to the changes in this PR. See https://github.com/gofiber/storage/actions/runs/14128624646/job/39583343922?pr=1569#step:4:53

mdelapenya avatar Mar 28 '25 12:03 mdelapenya

@efectn can you update, maybe its aready working with the new test containers

@mdelapenya can you try to support here

ReneWerner87 avatar Apr 29 '25 16:04 ReneWerner87

Indeed, the arangodb and redis code must be updated to use the creation of the testStore using the testcontainers code.

mdelapenya avatar Apr 30 '25 09:04 mdelapenya

@mdelapenya would be cool if you could support here as this is a breaking change and we want to improve the storage for v3 the new version is just hanging on there and any help here would be appreciated

ReneWerner87 avatar May 16 '25 12:05 ReneWerner87

@mdelapenya would be cool if you could support here as this is a breaking change and we want to improve the storage for v3 the new version is just hanging on there and any help here would be appreciated

@mdelapenya It could be good if you looked into timeouted tests to find the issue. I will try to complete rest of todo weekend.

efectn avatar May 16 '25 13:05 efectn

I resolved the conflicts, updating the test for the following modules:

  • arangodb
  • coherence
  • redis
  • rueidis
  • valkey

I noticed the aerospike module needs the Context support, as this PR was created before it was merged

mdelapenya avatar May 23 '25 10:05 mdelapenya

@efectn I think the test errors are real, as the build‚GH workflows fail for the new added tests. As the tests are in a good shape now, you can continue with your work from the current state of the PR

mdelapenya avatar May 23 '25 11:05 mdelapenya

image @mdelapenya @efectn @gofiber/maintainers is this error known? do we have a solution for this ?

ReneWerner87 avatar Jul 03 '25 11:07 ReneWerner87