feat(project): add sql and restore commands - #1326
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## next #1326 +/- ##
==========================================
+ Coverage 54.01% 59.17% +5.15%
==========================================
Files 303 318 +15
Lines 23457 24319 +862
==========================================
+ Hits 12671 14390 +1719
+ Misses 10758 9901 -857
Partials 28 28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds host-accessible project database resolution, project SQL and restore commands, Compose database port publishing, and a delimiter-aware SQL shell with streaming execution and multiple output formats. ChangesProject database tools
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant projectSQLCmd
participant connectProjectDatabase
participant Executor.DatabaseConnection
participant sqlshell.InteractiveShell
participant MySQL
projectSQLCmd->>connectProjectDatabase: resolve project database
connectProjectDatabase->>Executor.DatabaseConnection: request host credentials
Executor.DatabaseConnection-->>connectProjectDatabase: return DatabaseConnection
connectProjectDatabase->>MySQL: open dedicated connection
projectSQLCmd->>sqlshell.InteractiveShell: execute interactive SQL
sqlshell.InteractiveShell->>MySQL: execute statements and queries
MySQL-->>sqlshell.InteractiveShell: return formatted results
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
cmd/project/project_sql.go (2)
29-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate
--formatbefore you open the database connection.
connectProjectDatabaseruns first. An invalid--formatvalue therefore starts a container lookup and a database handshake, and only then fails with a usage error. MoveresolveSQLFormatabove the connect call.♻️ Proposed reorder
- conn, dbConn, cleanup, err := connectProjectDatabase(cmd) - if err != nil { - return err - } - defer cleanup() - - format, err := resolveSQLFormat(cmd) - if err != nil { - return err - } + format, err := resolveSQLFormat(cmd) + if err != nil { + return err + } + + conn, dbConn, cleanup, err := connectProjectDatabase(cmd) + if err != nil { + return err + } + defer cleanup()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/project/project_sql.go` around lines 29 - 40, Move the resolveSQLFormat call before connectProjectDatabase in the RunE command handler, returning its error immediately before any database connection or cleanup setup occurs. Preserve the existing format value and subsequent connection flow.
41-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTerminal detection uses the process streams, but I/O uses the Cobra streams.
Line 45 checks
os.Stdin, and Line 46 reads fromcmd.InOrStdin(). Line 68 checksos.Stdout, and the output goes tocmd.OutOrStdout(). If a caller or a test replaces the Cobra streams withcmd.SetIn/cmd.SetOut, the command still inspects the real process descriptors. The command then selects the wrong mode, for example the interactive shell on a piped test input.Derive the check from the Cobra streams instead, and fall back to non-terminal when the stream is not an
*os.File.♻️ Proposed helper
func isTerminalStream(s any) bool { f, ok := s.(*os.File) return ok && term.IsTerminal(f.Fd()) }- if !term.IsTerminal(os.Stdin.Fd()) { + if !isTerminalStream(cmd.InOrStdin()) {- if term.IsTerminal(os.Stdout.Fd()) { + if isTerminalStream(cmd.OutOrStdout()) {Also applies to: 64-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/project/project_sql.go` around lines 41 - 52, Update terminal detection in the command’s input and output mode selection to inspect cmd.InOrStdin() and cmd.OutOrStdout() rather than os.Stdin and os.Stdout. Add or reuse an isTerminalStream helper that only calls term.IsTerminal for *os.File values and returns false for other stream types, preserving non-terminal behavior for Cobra-injected buffers and pipes.internal/sqlshell/shell.go (1)
30-34: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffContext cancellation does not interrupt the blocking read.
scanner.Scan()blocks until the user sends a line. Thectx.Err()check runs only afterScanreturns. If the root command cancels the context, for example onSIGINT, the shell stays blocked until the user presses Enter or Ctrl+D.For an interactive shell the process normally terminates on the signal, so the impact is limited. If you want the loop to react, read lines in a goroutine and select on
ctx.Done().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sqlshell/shell.go` around lines 30 - 34, Update the scanner loop around scanner.Scan so context cancellation can interrupt a blocking interactive read: perform line reads in a goroutine and select between the read result and ctx.Done(), returning ctx.Err() immediately when the context is canceled while preserving normal EOF and scan-error handling.internal/sqlshell/run.go (1)
376-397: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueNumeric-column JSON passthrough can emit values that consumers cannot read back.
renderJSONconverts a numeric column value tojson.RawMessagewhenjson.Validaccepts it.json.Validaccepts any well-formed JSON document, not only numbers. For a numeric column the driver returns a numeric literal, so this is safe today. Two edge cases remain:
- A
DECIMALwith high precision, for example12345678901234567890.123, is emitted as a raw number. Many JSON consumers parse it as a float64 and lose precision.numeric[i]is derived fromDatabaseTypeName(), which is driver-dependent for aliases such asUNSIGNED BIGINT.If precision matters for downstream
jquse, consider restricting the raw passthrough to integer types and emittingDECIMALas a string.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sqlshell/run.go` around lines 376 - 397, Update renderJSON to distinguish integer numeric columns from DECIMAL and other high-precision values instead of treating every numeric[i] value as a raw JSON number. Preserve raw-number passthrough only for supported integer database types, including driver aliases such as UNSIGNED BIGINT, and emit DECIMAL values as strings so downstream consumers do not lose precision.internal/sqlshell/stream.go (1)
42-55: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBuffer growth is quadratic for very large single statements.
buffer += string(chunk[:n])copies the whole pending buffer on every read. Normal dumps split statements belowmax_allowed_packet, so the pending buffer stays small. A dump with one very large statement (for example a multi-hundred-MB extendedINSERTor aLOADpayload) makes each 1 MiB read copy the full buffer again.If you want to bound this, accumulate into a
strings.Builder(or[]byte) and only materialize a string when you call the splitter, resetting the builder from the returned remainder. The remainder itself is a substring and stays cheap.This is not blocking for typical dumps.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sqlshell/stream.go` around lines 42 - 55, The statement streaming loop in stream.go is growing the pending buffer with repeated string concatenation, which becomes quadratic for very large statements. Update the read/execute path around buffer and SplitStatementsWithDelimiter to accumulate chunks in a strings.Builder or []byte, materialize a string only when splitting, and then reset the accumulator to the returned remainder so large single statements stay linear-time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/project/executor.go`:
- Around line 50-60: Update the MySQL configuration setup around mysqlCfg to
require certificate-validating TLS when dbConn targets a non-loopback host,
while leaving loopback connections unchanged as appropriate. Configure the
driver with a strict TLS mode or registered TLS configuration that verifies
certificates and hostnames; do not use the "preferred" mode or allow plaintext
fallback. Ensure the existing connection fields and MaxAllowedPacket behavior
remain intact.
In `@cmd/project/project_restore.go`:
- Around line 33-49: Update the project restore command’s RunE flow to prompt
for confirmation before executing the dump when interaction is enabled, using
system.IsInteractionEnabled as in project_sql.go; bypass the prompt when
interaction is disabled or the --force flag is provided. Add and honor the
--force command option, and abort without restoring when the user declines.
In `@internal/executor/database.go`:
- Around line 42-67: Update applyDatabaseURL in database.go to reject
DATABASE_URL values unless url.Parse returns both a non-empty parsed.Scheme and
parsed.Hostname() before mutating conn; if either is missing, return an error
instead of preserving the default connection settings. Keep the existing parsing
of parsed.User, parsed.Port(), and parsed.Path for valid URLs, and add
regression tests covering the shopware and mysql:///shopware inputs so project
restore cannot fall back to the default database.
In `@internal/sqlshell/run.go`:
- Around line 162-171: Update returnsResultSet and the statement-dispatch logic
to recognize MariaDB INSERT, REPLACE, and single-table DELETE statements
containing RETURNING as result sets. Ensure these statements use
Query/QueryContext and flow through renderResultSet instead of ExecContext,
while preserving existing handling for other statements.
---
Nitpick comments:
In `@cmd/project/project_sql.go`:
- Around line 29-40: Move the resolveSQLFormat call before
connectProjectDatabase in the RunE command handler, returning its error
immediately before any database connection or cleanup setup occurs. Preserve the
existing format value and subsequent connection flow.
- Around line 41-52: Update terminal detection in the command’s input and output
mode selection to inspect cmd.InOrStdin() and cmd.OutOrStdout() rather than
os.Stdin and os.Stdout. Add or reuse an isTerminalStream helper that only calls
term.IsTerminal for *os.File values and returns false for other stream types,
preserving non-terminal behavior for Cobra-injected buffers and pipes.
In `@internal/sqlshell/run.go`:
- Around line 376-397: Update renderJSON to distinguish integer numeric columns
from DECIMAL and other high-precision values instead of treating every
numeric[i] value as a raw JSON number. Preserve raw-number passthrough only for
supported integer database types, including driver aliases such as UNSIGNED
BIGINT, and emit DECIMAL values as strings so downstream consumers do not lose
precision.
In `@internal/sqlshell/shell.go`:
- Around line 30-34: Update the scanner loop around scanner.Scan so context
cancellation can interrupt a blocking interactive read: perform line reads in a
goroutine and select between the read result and ctx.Done(), returning ctx.Err()
immediately when the context is canceled while preserving normal EOF and
scan-error handling.
In `@internal/sqlshell/stream.go`:
- Around line 42-55: The statement streaming loop in stream.go is growing the
pending buffer with repeated string concatenation, which becomes quadratic for
very large statements. Update the read/execute path around buffer and
SplitStatementsWithDelimiter to accumulate chunks in a strings.Builder or
[]byte, materialize a string only when splitting, and then reset the accumulator
to the returned remainder so large single statements stay linear-time.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d7fac925-d7a0-48e9-a8fb-20809b7e44f6
📒 Files selected for processing (19)
cmd/project/executor.gocmd/project/project_restore.gocmd/project/project_restore_test.gocmd/project/project_sql.gointernal/docker/compose.gointernal/docker/compose_test.gointernal/executor/database.gointernal/executor/database_test.gointernal/executor/docker.gointernal/executor/executor.gointernal/executor/local.gointernal/executor/symfony_cli.gointernal/sqlshell/run.gointernal/sqlshell/run_test.gointernal/sqlshell/shell.gointernal/sqlshell/split.gointernal/sqlshell/split_test.gointernal/sqlshell/stream.gointernal/sqlshell/stream_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sqlshell/run.go`:
- Around line 203-218: Update hasReturningClause’s comment scanner so -- starts
a line comment only when its following character is whitespace, while still
recognizing valid end-of-line comments. Treat completed block comments as token
separators when checking RETURNING boundaries, including comments before or
after the clause; preserve detection for 1--1 RETURNING, /* comment */RETURNING,
and RETURNING/* comment */id. Add regression tests covering these cases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a459f546-472a-4bb8-9ce6-c1a6f27e5595
📒 Files selected for processing (11)
cmd/project/project_restore.gocmd/project/project_sql.gocmd/project/project_sql_test.gointernal/executor/database.gointernal/executor/database_docker_test.gointernal/executor/database_test.gointernal/sqlshell/fakedb_test.gointernal/sqlshell/run.gointernal/sqlshell/run_db_test.gointernal/sqlshell/run_test.gointernal/sqlshell/split_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/sqlshell/split_test.go
- cmd/project/project_sql.go
- cmd/project/project_restore.go
- internal/executor/database_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/sqlshell/interactive_test.go (2)
146-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test locks in the no-cancel behavior.
TestInteractiveIgnoresKeysWhileRunningasserts that every key is ignored whilem.runningis true. I raised the missing cancellation path atinternal/sqlshell/interactive.golines 94-97. If you addctrl+ccancellation there, narrow this test to keys that must stay ignored, and add a case that assertsctrl+ccancels the running statement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sqlshell/interactive_test.go` around lines 146 - 153, Update TestInteractiveIgnoresKeysWhileRunning to cover only non-cancellation keys that remain ignored while m.running is true. Add a separate test case for ctrl+c that verifies the running statement is cancelled, matching the cancellation handling in the interactive model.
128-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClean up the package-level fake query.
Register
fakeQueries.Delete("SELECT 3")witht.Cleanupafter the store call. This keeps the test isolated if another test later uses the same query key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sqlshell/interactive_test.go` around lines 128 - 132, After the fakeQueries.Store call for "SELECT 3" in the test, register fakeQueries.Delete("SELECT 3") with t.Cleanup so the package-level fake query is removed when the test finishes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sqlshell/interactive.go`:
- Around line 94-97: Update the execution flow around Update’s tea.KeyPressMsg
handling to create a per-statement cancellable context derived from the session
context, retain its cancel function, and invoke it for ctrl+c while m.running
instead of dropping the key. Propagate that statement context into query
execution, and update View to render a running indicator such as a spinner or
“running…” line while execution is active.
- Around line 17-21: Update InteractiveShell to return nil when errors.Is(err,
context.Canceled) matches, accounting for Bubble Tea’s wrapped cancellation
error; preserve all other errors, including unrelated tea.ErrProgramKilled
cases, and return them unchanged.
---
Nitpick comments:
In `@internal/sqlshell/interactive_test.go`:
- Around line 146-153: Update TestInteractiveIgnoresKeysWhileRunning to cover
only non-cancellation keys that remain ignored while m.running is true. Add a
separate test case for ctrl+c that verifies the running statement is cancelled,
matching the cancellation handling in the interactive model.
- Around line 128-132: After the fakeQueries.Store call for "SELECT 3" in the
test, register fakeQueries.Delete("SELECT 3") with t.Cleanup so the
package-level fake query is removed when the test finishes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c3714d6-8837-4288-aa86-92678f3197eb
📒 Files selected for processing (13)
cmd/project/executor.gocmd/project/project_dump.gocmd/project/project_dump_test.gocmd/project/project_restore.gocmd/project/project_sql.gocmd/project/project_sql_test.gointernal/executor/database.gointernal/executor/database_test.gointernal/sqlshell/fakedb_test.gointernal/sqlshell/interactive.gointernal/sqlshell/interactive_test.gointernal/sqlshell/run.gointernal/sqlshell/shell.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/project/project_sql.go
- internal/sqlshell/shell.go
- cmd/project/project_sql_test.go
- cmd/project/project_restore.go
- internal/sqlshell/run.go
18c61d9 to
2cc4c47
Compare
Adds `project sql` and `project restore` (alias `import`), working against the database of the current environment so no host or credentials knowledge is needed. project sql: - one-shot queries, piped scripts and an interactive shell - the interactive shell is an inline bubbletea program with real line editing (alt+backspace/ctrl+w/alt+d word deletion, cursor movement), arrow-key session history and cancellable queries via ctrl+c - output formats: mysql-style table (tty), tsv (piped), json; binary columns render as hex, numeric columns as real JSON numbers project restore: - plain, gzip or zstd compressed dumps, detected via magic bytes, streamed in chunks so dumps larger than memory work - progress reporting, confirmation prompt (skippable with --force, auto-skipped for stdin dumps and --no-interaction) Infrastructure: - Executor gains DatabaseConnection() resolving host-reachable credentials: DATABASE_URL with Symfony precedence for local and symfony-cli, the published compose port for docker; MySQLConfig() and Open() keep all driver knowledge in the executor package - the generated dev compose publishes the database on a random loopback port so host-side tools can connect without conflicts - project dump resolves its base connection through the executor too, gaining docker support; flags still override individual parts - internal/sqlshell statement splitter handles quotes, comments, executable comments (/*!40014 ... */) and DELIMITER directives as emitted by the dumper around triggers, with state that survives stream chunks and shell lines - commands run on a single dedicated connection so session state like SET FOREIGN_KEY_CHECKS sticks - reusable CountingReader and DecompressReader in internal/system Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2cc4c47 to
9e39da5
Compare
2 participants
What
Two new project commands that talk to the project database without the user needing to know which MySQL/MariaDB, host or credentials are in play:
project sql [query]shopware-cli project sql "SELECT id, tax_rate FROM tax"shopware-cli project sql < script.sql,--format json | jqsql>/->continuation prompts, multi-line statements,exit/quit/\qtable(default on a TTY),tsv(default when piped),json(numeric columns as real JSON numbers)BINARY(16)ids) render as0x…hexproject restore <file>(aliasproject import).sql, gzip and zstd dumps — compression detected from magic bytes, not the filename;-reads from stdinHow
Executor.DatabaseConnection(ctx)method resolves host-reachable credentials:DATABASE_URLwith Symfony precedence (executor env > real env > env files), defaults matchingproject dumpDATABASE_URLfrom thewebcontainer, then swaps the compose service host for the port published on the host (docker compose port); external DB hosts pass through127.0.0.1::3306) so host-side tools can connect without port conflicts; picked up on the nextproject devrun*sql.Connso session state (SET FOREIGN_KEY_CHECKS,SET NAMES,USE) sticks;max_allowed_packetis fetched from the serverinternal/sqlshellpackage with a statement splitter that understands quotes, escapes, line/block comments, executable comments (/*!40014 … */) andDELIMITERdirectives (as emitted by our dumper around triggers) — delimiter state survives stream chunk boundaries and REPL linesTesting
project dump→project restoreround trip in all three compressions, stdin restore, trigger round trip (restored trigger fires), 10 MB / 100k-row gzipped dump in ~0.8 s, error pathsgo test ./...andgolangci-lint runcleanLimitations
DELIMITERsupport covers directive lines as dumps emit them; interactive redefinition mid-line is not supported🤖 Generated with Claude Code
fixes #1243
Summary by CodeRabbit
project sqlfor direct queries, piped SQL, and interactive sessions.project restore/project importfor plain, gzip, and zstd SQL dumps from files or standard input.