🔥 feat: Support for DisableStartupCheck for several drivers #1927
Conversation
gaby
requested review from
ReneWerner87,
efectn and
sixcolors
and removed request for
a team
Summary of ChangesHello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the flexibility of several database and caching drivers (Memcache, MySQL, PostgreSQL, and Redis) by introducing a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a DisableStartupCheck boolean across memcache, postgres, redis, and rueidis configs/defaults; constructors now optionally skip startup ping/reset when set. Rueidis additionally records initialization errors (initErr), defers error propagation to operations, and guards Close/get/set/delete/reset against nil client. READMEs and tests updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant C as Config
participant New as New(...)
participant DB as Backend
App->>C: Provide Config (DisableStartupCheck = true/false)
App->>New: Call New(C)
New->>DB: Establish connection
alt DisableStartupCheck = false
New->>DB: Ping (startup validation)
alt Reset enabled
New->>DB: Reset (Flush/Drop/DeleteAll)
DB-->>New: Reset result
end
DB-->>New: Ping result
New-->>App: Return initialized Storage (db != nil, initErr nil)
else DisableStartupCheck = true
Note over New,DB: Skip Ping and Reset
alt connection error occurred
New-->>New: store initErr (db may be nil)
New-->>App: Return Storage with initErr
else
New-->>App: Return Storage (db set, initErr nil)
end
end
rect rgba(0,128,0,0.08)
Note right of App: Subsequent ops (Get/Set/Delete/Reset) check initErr and nil db
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
gaby
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for a DisableStartupCheck configuration option to the memcache, mysql, postgres, and redis storage drivers. The changes are consistent across the drivers and include corresponding documentation updates and tests. The implementation appears correct.
However, there is a critical discrepancy between the changes in this PR and its title and description. The title is 'Revert DisableStartupCheck support from selected drivers' and the description states that the goal is to revert this feature for the mongodb, rueidis, and sqlite drivers. The actual changes do the opposite: they add the feature to a different set of drivers (memcache, mysql, postgres, redis). This is very misleading and will cause confusion for anyone looking at the repository's history. The PR description also incorrectly states that no tests were run, but new tests have been added for each modified driver.
Please update the pull request title and description to accurately reflect the changes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| sqlGC: fmt.Sprintf("DELETE FROM %s WHERE e <= $1 AND e != 0", cfg.Table), | ||
| } | ||
|
|
||
| store.checkSchema(cfg.Table) | ||
| if !cfg.DisableStartupCheck { | ||
| store.checkSchema(cfg.Table) | ||
| } |
There was a problem hiding this comment.
Similar to the MySQL change, guarding the entire initialization block with if !cfg.DisableStartupCheck now prevents the storage from creating or resetting its table when the flag is true. With startup checks disabled, New returns a store whose underlying table may not exist and whose schema is never verified, causing later operations or the GC ticker to return errors once the application first touches the store. The flag should only suppress the ping, not the schema creation/reset logic.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
memcache/memcache_test.go (1)
181-189: LGTM! Test validates DisableStartupCheck behavior.The test appropriately verifies that
Newconstructs a storage instance without panicking whenDisableStartupCheckis enabled, even with an unreachable server address.Consider adding
defer store.Close()for consistency with other tests (likeTest_MYSQL_New_DisableStartupCheck), even though Memcache'sClose()is a no-op:store := New(Config{ Servers: "127.0.0.1:11210", DisableStartupCheck: true, }) require.NotNil(t, store) + defer store.Close() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
memcache/README.md(1 hunks)memcache/config.go(2 hunks)memcache/memcache.go(1 hunks)memcache/memcache_test.go(1 hunks)mysql/README.md(2 hunks)mysql/config.go(2 hunks)mysql/mysql.go(2 hunks)mysql/mysql_test.go(1 hunks)postgres/README.md(2 hunks)postgres/config.go(2 hunks)postgres/postgres.go(2 hunks)postgres/postgres_test.go(1 hunks)redis/README.md(2 hunks)redis/config.go(2 hunks)redis/redis.go(1 hunks)redis/redis_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
memcache/memcache_test.go (2)
memcache/memcache.go (1)
New(19-57)memcache/config.go (1)
Config(6-35)
postgres/postgres_test.go (2)
postgres/postgres.go (1)
New(57-134)postgres/config.go (1)
Config(13-73)
redis/redis_test.go (2)
redis/redis.go (1)
New(24-86)redis/config.go (1)
Config(9-91)
mysql/mysql_test.go (2)
mysql/mysql.go (1)
New(43-111)mysql/config.go (1)
Config(10-73)
redis/config.go (1)
testhelpers/redis/redis.go (1)
Port(15-15)
🪛 markdownlint-cli2 (0.18.1)
mysql/README.md
130-130: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1
(MD010, no-hard-tabs)
132-132: Hard tabs
Column: 1
(MD010, no-hard-tabs)
133-133: Hard tabs
Column: 1
(MD010, no-hard-tabs)
151-151: Hard tabs
Column: 1
(MD010, no-hard-tabs)
memcache/README.md
77-77: Hard tabs
Column: 1
(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
postgres/README.md
122-122: Hard tabs
Column: 1
(MD010, no-hard-tabs)
123-123: Hard tabs
Column: 1
(MD010, no-hard-tabs)
124-124: Hard tabs
Column: 1
(MD010, no-hard-tabs)
143-143: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: compare (mysql)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
🔇 Additional comments (17)
redis/README.md (1)
183-186: LGTM! Documentation is clear and consistent.The DisableStartupCheck field is well-documented with appropriate comments explaining its purpose and default value.
mysql/config.go (1)
56-59: LGTM! Configuration is correctly implemented.The DisableStartupCheck field is properly added to the Config struct with clear documentation, and the default value of
falsepreserves existing behavior.Also applies to: 84-84
postgres/postgres_test.go (1)
206-216: LGTM! Test correctly verifies the DisableStartupCheck behavior.The test appropriately uses an invalid connection configuration to ensure that startup checks are skipped when DisableStartupCheck is true, and verifies no panic occurs.
redis/redis_test.go (1)
201-212: LGTM! Test correctly verifies the DisableStartupCheck behavior.The test appropriately uses an invalid connection configuration to ensure that startup checks are skipped when DisableStartupCheck is true, and verifies no panic occurs.
postgres/postgres.go (1)
71-112: LGTM! Startup checks are correctly gated.The implementation properly guards all startup validation logic (Ping, schema parsing, Reset, table existence check, initialization, and schema validation) behind the DisableStartupCheck flag. The default value of
falsepreserves existing behavior.Also applies to: 126-128
redis/redis.go (1)
68-80: LGTM! Startup checks are correctly gated.The implementation properly guards the Ping test and Reset/FlushDB operations behind the DisableStartupCheck flag, consistent with the pattern in other drivers. The default value of
falsepreserves existing behavior.mysql/mysql_test.go (1)
111-121: LGTM! Test correctly verifies DisableStartupCheck behavior.The test appropriately validates that
Newcan construct a storage instance without panicking whenDisableStartupCheckis enabled, even with an invalid connection. The test uses an unreachable host/port (127.0.0.1:3308), which would normally fail during startup validation.memcache/memcache.go (1)
33-44: LGTM! Startup validation properly guarded.The implementation correctly wraps startup health checks (Ping and potential DeleteAll) behind the
DisableStartupCheckflag. This allows users to opt out of startup validation when needed (e.g., for testing or lazy initialization scenarios) while maintaining safe defaults.memcache/config.go (2)
18-21: LGTM! Configuration field properly defined.The
DisableStartupCheckfield is clearly documented and follows the existing configuration pattern. The comment accurately describes its purpose.
39-42: LGTM! Safe default configuration.The
ConfigDefaultinitialization correctly setsDisableStartupCheck: false, ensuring startup validation remains enabled by default. This preserves backward compatibility and provides safe-by-default behavior.postgres/README.md (1)
120-123: LGTM! Configuration option properly documented.The
DisableStartupCheckfield is clearly documented in the Config section with appropriate comments about its purpose and default value.postgres/config.go (2)
64-67: LGTM! Configuration field properly defined.The
DisableStartupCheckfield is clearly documented and follows the existing configuration pattern.
77-85: LGTM! Safe default configuration with proper formatting.The
ConfigDefaultinitialization correctly setsDisableStartupCheck: falseand maintains clean field alignment. The safe default ensures startup validation remains enabled by default.redis/config.go (2)
71-74: LGTM! Configuration field properly defined.The
DisableStartupCheckfield is clearly documented and follows the existing configuration pattern.
95-110: LGTM! Safe default configuration.The
ConfigDefaultinitialization correctly setsDisableStartupCheck: falseat line 102, ensuring startup validation remains enabled by default. This provides safe-by-default behavior while allowing users to opt out when needed.mysql/mysql.go (2)
66-89: LGTM! Startup validation properly guarded.The implementation correctly wraps all startup operations (Ping, Reset with table drop, and initialization queries) behind the
DisableStartupCheckflag. This allows users to bypass startup validation when needed while maintaining safe defaults.
103-105: LGTM! Schema validation properly guarded.The
checkSchemacall is appropriately wrapped in theDisableStartupCheckcondition, ensuring schema validation is skipped when startup checks are disabled.
| // DisableStartupCheck skips the initial connection validation during New. | ||
| // | ||
| // Optional. Default is false | ||
| DisableStartupCheck bool |
There was a problem hiding this comment.
Replace hard tabs with spaces to comply with markdown linting rules.
The static analysis tool detected hard tabs in the documentation. The content is correct, but formatting should use spaces for consistency.
Apply this fix:
- // DisableStartupCheck skips the initial connection validation during New.
- //
- // Optional. Default is false
- DisableStartupCheck bool
+ // DisableStartupCheck skips the initial connection validation during New.
+ //
+ // Optional. Default is false
+ DisableStartupCheck bool- DisableStartupCheck: false,
+ DisableStartupCheck: false,Based on static analysis hints.
Also applies to: 88-88
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
77-77: Hard tabs
Column: 1
(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🤖 Prompt for AI Agents
In memcache/README.md around lines 77-80 and also line 88, replace any hard tab
characters with spaces to satisfy markdown linting; open the file, locate the
indented lines shown (the comment block for DisableStartupCheck and the other
occurrence at line 88), convert tabs to the project’s preferred number of spaces
(typically 2 or 4) and save the file so the documentation uses only spaces for
indentation.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rueidis/rueidis.go (1)
66-68: Critical: Constructor still panics on connection failure despite DisableStartupCheck.When
rueidis.NewClient()fails (e.g., connection refused), line 67 panics regardless of theDisableStartupChecksetting. This contradicts the PR objective to "prevent the Redis storage constructor from panicking on network or flushing errors" and causes the test atrueidis_test.go:52-61to fail.The
DisableStartupCheckflag should also guard this panic. Apply this pattern:db, err := rueidis.NewClient(rueidis.ClientOption{ Username: cfg.Username, Password: cfg.Password, ClientName: cfg.ClientName, SelectDB: cfg.SelectDB, InitAddress: cfg.InitAddress, TLSConfig: cfg.TLSConfig, CacheSizeEachConn: cfg.CacheSizeEachConn, RingScaleEachConn: cfg.RingScaleEachConn, ReadBufferEachConn: cfg.ReadBufferEachConn, WriteBufferEachConn: cfg.WriteBufferEachConn, BlockingPoolSize: cfg.BlockingPoolSize, PipelineMultiplex: cfg.PipelineMultiplex, DisableRetry: cfg.DisableRetry, DisableCache: cfg.DisableCache, AlwaysPipelining: cfg.AlwaysPipelining, }) - if err != nil { + if err != nil && !cfg.DisableStartupCheck { panic(err) }Note: Verify whether a nil or partially-initialized
dbclient can be safely used in the returnedStoragewhenDisableStartupCheckis true andNewClientfails. If the rueidis client cannot be used after a failed initialization, the constructor signature should be changed toNew(...Config) (*Storage, error)as suggested in issue #1924.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
rueidis/README.md(2 hunks)rueidis/config.go(3 hunks)rueidis/rueidis.go(1 hunks)rueidis/rueidis_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
rueidis/rueidis_test.go (2)
rueidis/rueidis.go (1)
New(18-88)rueidis/config.go (1)
Config(11-107)
🪛 GitHub Actions: Tests Rueidis
rueidis/rueidis.go
[error] 67-67: New() triggered a panic due to failed startup connection (dial tcp 127.0.0.1:6390: connect: connection refused).
rueidis/rueidis_test.go
[error] 53-53: Test_Rueidis_New_DisableStartupCheck panicked. Dial to 127.0.0.1:6390 was refused. Command: 'go test ./... -v -race'. Panic indicates startup connection could not be established.
[warning] 384-384: --- SKIP: Test_Rueidis_Cluster: TODO: Replace with containerized cluster when testcontainers-go Redis module supports clustering
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: compare (mysql)
- GitHub Check: Tests (1.23.x, 7)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
🔇 Additional comments (3)
rueidis/config.go (1)
98-101: LGTM! Configuration changes are well-structured.The
DisableStartupCheckfield is properly defined with clear documentation, correctly initialized tofalseinConfigDefaultfor backward compatibility, and the merge logic inconfigDefaultfollows the same pattern as other boolean flags.Also applies to: 128-128, 203-205
rueidis/rueidis.go (1)
70-82: Ping and reset are correctly guarded, but incomplete without fixing line 67.The explicit
PingandFlushdboperations are properly skipped whenDisableStartupCheckis true, which aligns with the intent to avoid "implicit network IO (PING) during object creation." However, this guard is insufficient without also addressing the panic at line 67.rueidis/README.md (1)
187-190: Documentation is accurate but implementation is incomplete.The README correctly documents
DisableStartupCheckas skipping "initial connection validation during New" and defaulting tofalse. However, per the critical issue inrueidis.go:66-68, the current implementation doesn't fully prevent panics during construction - only the explicit ping/reset are skipped, not the underlying client creation errors.Once the implementation bug is fixed, ensure this documentation accurately reflects the behavior. If the constructor remains panic-prone for client creation errors even with
DisableStartupCheck: true, update the docs to clarify the limitation.Also applies to: 218-218
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rueidis/rueidis.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
rueidis/rueidis.go (1)
valkey/valkey.go (1)
Storage(13-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.24.x, 6)
- GitHub Check: Tests (1.23.x, 6)
- GitHub Check: Tests (1.23.x, 7)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.23.x, 7)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.24.x, 6)
- GitHub Check: Tests (1.23.x, 6)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
🔇 Additional comments (1)
rueidis/rueidis.go (1)
148-150: Defensive nil check is reasonable, but treats symptom rather than root cause.The nil check prevents a panic if
s.dbis nil, which can happen when client creation fails withDisableStartupCheckenabled. However, this is a defensive measure treating the symptom. The root cause is thatStorageshould never be instantiated with a nil client in the first place (see previous comment on lines 66-81).Assuming the client creation logic is fixed, this nil check remains a reasonable safety measure for defensive programming.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rueidis/rueidis.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.24.x, 6)
- GitHub Check: Tests (1.23.x, 6)
- GitHub Check: Tests (1.23.x, 7)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.23.x, 7)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.24.x, 6)
- GitHub Check: Tests (1.23.x, 6)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
🔇 Additional comments (1)
rueidis/rueidis.go (1)
148-151: Good: nil-safety in Close prevents panic with partially initialized clientEarly-return on nil client is correct and avoids crashes when initialization fails or is skipped.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rueidis/rueidis.go (1)
69-81: Fix the comment typo from "Clear" to "Reset".The conditional startup check logic correctly skips Ping and Reset operations when
DisableStartupCheckistrue, which aligns with the PR objectives. However, line 75's comment still says "Empty collection if Clear is true" but should say "Empty collection if Reset is true" to match the actual code (cfg.Reset). This typo was flagged in a previous review but remains unfixed.Apply this diff to fix the comment:
- // Empty collection if Clear is true + // Empty collection if Reset is true if cfg.Reset {Note: This PR partially addresses issue #1924 by allowing startup checks to be skipped, but doesn't fully implement the requested change to return
(*Storage, error)instead of panicking. The constructor still panics on connection errors (lines 66, 72, 78), which may be acceptable for backward compatibility but doesn't achieve the goal of "letting callers decide whether to verify connectivity."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
postgres/README.md(1 hunks)rueidis/rueidis.go(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Tests Rueidis
rueidis/rueidis.go
[error] 66-66: New() panicked during startup check due to connection failure (dial tcp 127.0.0.1:6390: connect: connection refused)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tests (1.23.x, 6)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x, 7)
- GitHub Check: Tests (1.24.x, 6)
🔇 Additional comments (1)
rueidis/rueidis.go (1)
148-150: LGTM: Defensive nil check prevents potential panics.The nil guard in
Close()is good defensive programming. While the current code always panics on client creation failure (line 66), this guard protects against future code changes or edge cases wheres.dbmight be nil, preventing a nil pointer dereference.
| // DisableStartupCheck skips the initial connection validation during New. | ||
| // | ||
| // Optional. Default is false | ||
| DisableStartupCheck bool | ||
|
|
There was a problem hiding this comment.
Docs still show New returning Storage.
Code now returns (*Storage, error); update the README signature/comments to match.
🤖 Prompt for AI Agents
In postgres/README.md around lines 120 to 124 the docs still state that New
returns Storage; update the README comments/signature to reflect the current
function signature which returns (*Storage, error). Edit the text to show New
returns a pointer and an error (e.g., "New returns (*Storage, error)") and
adjust any surrounding examples or descriptions to mention the error return
value and that callers must handle the returned error.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
rueidis/rueidis.go (4)
116-118: LGTM: nil‑safety guard correctly implemented.
136-138: LGTM: nil‑safety guard correctly implemented.
152-154: LGTM: nil‑safety guard correctly implemented.
74-86: Fix comment typo: "Clear" → "Reset"Line 80 reads "Empty collection if Clear is true" but should reference
cfg.Reset, not a non-existent "Clear" field.Apply this diff:
- // Empty collection if Clear is true + // Empty collection if Reset is true
🧹 Nitpick comments (2)
rueidis/rueidis.go (1)
67-72: Client creation error handling implements deferred failure, but doesn't distinguish error types.When
DisableStartupCheckis true and client creation fails, the code returns a Storage withinitErrset, deferring the panic until first use. This aligns with the PR objectives to avoid network‑related panics during construction.However, note that
rueidis.NewClienterrors can stem from both configuration issues (e.g., invalid addresses) and network failures. Configuration errors are programming mistakes that ideally should always panic for immediate feedback, while network errors are runtime conditions that benefit from deferred handling. The current implementation treats all client creation errors uniformly.Consider a future enhancement to inspect the error type and always panic for configuration errors while deferring only network errors. For now, the uniform approach is acceptable given the PR objectives.
rueidis/README.md (1)
187-192: Hard tabs are consistent with file style but diverge from Markdown conventions.Static analysis flags hard tabs on these lines. While the entire file uses tabs for indentation (consistent internal style), Markdown style guides typically recommend spaces. If you plan to enforce a consistent style across all documentation, consider converting tabs to spaces file‑wide in a separate formatting commit.
For now, the changes are consistent with the existing file style, so this is not blocking.
Also applies to: 220-220
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rueidis/README.md(2 hunks)rueidis/config.go(3 hunks)rueidis/rueidis.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rueidis/config.go
🧰 Additional context used
🧬 Code graph analysis (1)
rueidis/rueidis.go (2)
mysql/mysql.go (1)
Storage(14-24)valkey/valkey.go (1)
Storage(13-15)
🪛 markdownlint-cli2 (0.18.1)
rueidis/README.md
187-187: Hard tabs
Column: 1
(MD010, no-hard-tabs)
188-188: Hard tabs
Column: 1
(MD010, no-hard-tabs)
189-189: Hard tabs
Column: 1
(MD010, no-hard-tabs)
190-190: Hard tabs
Column: 1
(MD010, no-hard-tabs)
191-191: Hard tabs
Column: 1
(MD010, no-hard-tabs)
192-192: Hard tabs
Column: 1
(MD010, no-hard-tabs)
220-220: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (4)
rueidis/rueidis.go (3)
14-15: Deferred error pattern correctly introduced.The
initErrfield enables graceful degradation whenDisableStartupCheckis true and initialization fails. Operations will surface the initialization error instead of panicking on a nildb, aligning with the PR objective to allow deferred connectivity validation.
96-98: LGTM: nil‑safety guard correctly returns initialization error.The nil check ensures that if client creation failed with
DisableStartupCheckenabled, the storedinitErris surfaced to the caller instead of dereferencing a nildb. This pattern is applied consistently across all public methods.
165-167: LGTM: Close gracefully handles nil client.Returning
nilearly whendbis nil prevents a panic if Close is called on a Storage that failed initialization. This is the correct no‑op behavior.rueidis/README.md (1)
187-192: LGTM: Documentation clearly describes the new field.The description accurately explains that
DisableStartupCheckskips initial connection validation and that operations will surface initialization errors instead of panicking when the flag is enabled.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
rueidis/rueidis.go (2)
14-15: Reconsider the initErr field—it signals incomplete error handling.Adding
initErrto the publicStoragestruct is a workaround for the constructor not returning(*Storage, error). This approach creates a "zombie" Storage instance that appears valid but defers all failures to the first operation. According to the linked issue #1924, constructors should return(*Storage, error)instead of panicking or returning partially initialized objects.Consider changing the constructor signature to properly propagate initialization errors:
-func New(config ...Config) *Storage { +func New(config ...Config) (*Storage, error) {If backward compatibility is a concern, introduce a
NewWithError(config ...Config) (*Storage, error)and have the existingNewwrap it while markingNewas deprecated.
80-80: Fix comment: "Clear" should be "Reset".The comment references
cfg.Clear, but the actual field iscfg.Reset(line 81).Apply this diff:
- // Empty collection if Clear is true + // Empty collection if Reset is true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rueidis/README.md(2 hunks)rueidis/config.go(3 hunks)rueidis/rueidis.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rueidis/config.go
🧰 Additional context used
🧬 Code graph analysis (1)
rueidis/rueidis.go (2)
mysql/mysql.go (1)
Storage(14-24)valkey/valkey.go (1)
Storage(13-15)
🪛 markdownlint-cli2 (0.18.1)
rueidis/README.md
187-187: Hard tabs
Column: 1
(MD010, no-hard-tabs)
188-188: Hard tabs
Column: 1
(MD010, no-hard-tabs)
189-189: Hard tabs
Column: 1
(MD010, no-hard-tabs)
190-190: Hard tabs
Column: 1
(MD010, no-hard-tabs)
191-191: Hard tabs
Column: 1
(MD010, no-hard-tabs)
192-192: Hard tabs
Column: 1
(MD010, no-hard-tabs)
220-220: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: lint (rueidis)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x, 6)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x, 6)
- GitHub Check: Tests (1.23.x, 7)
- GitHub Check: Tests (1.23.x, 7)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.24.x, 6)
- GitHub Check: Tests (1.23.x, 6)
🔇 Additional comments (1)
rueidis/rueidis.go (1)
174-176: Ensure Conn() handles nil client when DisableStartupCheck is enabled
Conn() will return nil if client creation fails and DisableStartupCheck is true (db stays nil, initErr set). Either document that callers must check initErr before using Conn(), or add a guard in Conn() (e.g. panic or return a sentinel) to prevent unexpected nil derefs.
| // DisableStartupCheck skips the initial connection validation during New. | ||
| // When true and client creation fails, New returns a Storage whose | ||
| // operations surface the initialization error instead of panicking. | ||
| // | ||
| // Optional. Default is false | ||
| DisableStartupCheck bool |
There was a problem hiding this comment.
Replace hard tabs with spaces.
The markdownlint tool flags hard tabs in these lines. For consistency with the rest of the file, use spaces for indentation.
Apply this diff to replace tabs with spaces:
- // DisableStartupCheck skips the initial connection validation during New.
- // When true and client creation fails, New returns a Storage whose
- // operations surface the initialization error instead of panicking.
- //
- // Optional. Default is false
- DisableStartupCheck bool
+ // DisableStartupCheck skips the initial connection validation during New.
+ // When true and client creation fails, New returns a Storage whose
+ // operations surface the initialization error instead of panicking.
+ //
+ // Optional. Default is false
+ DisableStartupCheck boolCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
187-187: Hard tabs
Column: 1
(MD010, no-hard-tabs)
188-188: Hard tabs
Column: 1
(MD010, no-hard-tabs)
189-189: Hard tabs
Column: 1
(MD010, no-hard-tabs)
190-190: Hard tabs
Column: 1
(MD010, no-hard-tabs)
191-191: Hard tabs
Column: 1
(MD010, no-hard-tabs)
192-192: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🤖 Prompt for AI Agents
In rueidis/README.md around lines 187 to 192, the block comment uses hard tab
characters for indentation which markdownlint flags; replace each hard tab with
equivalent spaces to match the file's existing indentation style (use 2 or 4
spaces consistent with the rest of the file), ensuring the comment lines and the
"DisableStartupCheck bool" line are indented using spaces only and that no tabs
remain.
| DisableCache: false, | ||
| AlwaysPipelining: true, | ||
| Reset: false, | ||
| DisableStartupCheck: false, |
There was a problem hiding this comment.
Replace hard tab with spaces.
Same indentation issue as above.
Apply this diff:
- DisableStartupCheck: false,
+ DisableStartupCheck: false,Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
220-220: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🤖 Prompt for AI Agents
In rueidis/README.md around line 220, the indentation uses a hard tab character
for the line "DisableStartupCheck: false,"; replace the hard tab with spaces to
match the file's existing indentation style (use the same number of spaces used
on surrounding lines, typically 2 or 4), ensuring the file contains no hard-tab
characters on that line.
| if cfg.DisableStartupCheck { | ||
| return &Storage{initErr: err} | ||
| } |
There was a problem hiding this comment.
Returning a Storage with nil db creates a confusing API.
When DisableStartupCheck is true and client creation fails, this returns a Storage with db == nil and initErr set. Callers of New() receive what looks like a valid pointer but is actually unusable. This violates the principle that constructors should either fully succeed or clearly fail.
The flag should only skip optional startup validation (Ping, Reset), not suppress critical client creation failures. Client creation failing is always fatal.
If the constructor signature cannot be changed immediately, at minimum document this behavior prominently in the function comment and README, warning users that DisableStartupCheck may return an unusable Storage.
🤖 Prompt for AI Agents
In rueidis/rueidis.go around lines 67-69, the constructor currently returns
&Storage{initErr: err} when client creation fails and cfg.DisableStartupCheck is
true, leaving db nil and producing a misleading usable pointer; change the
constructor so client creation failures always return an error (do not return a
Storage with a nil db) — i.e., propagate the creation error to the caller
(return nil, err) so New() either fully succeeds or fails; if you cannot change
the function signature right now, at minimum update the function comment and
README to clearly document that DisableStartupCheck can cause New() to return a
Storage with an unusable nil db and initErr set, and add/update unit tests and
caller sites to handle the error-returning behavior.
Summary
We can add this feature to other drivers in the future if needed.
Fixes #1924
Summary by CodeRabbit