Conversation
WalkthroughAdds a PlanOutputFormat type (text/json), updates Planner.PlanOperation to accept the format and produce text or JSON, introduces a CLI Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
SkArchon
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
router/pkg/plan_generator/plan_generator.go (1)
145-169: Keep reportfile_namealigned with the operation nameWhen
Rawis enabled we now append.jsontofileNameand reuse that string everywhere, includingQueryPlanResult.FileName. That field previously matched the original operation file name, and downstream tooling (and even users readingreport.jsonwhenprint-reportis enabled butprint-per-fileis disabled) relies on it to map results back to the source.graphqlfile. With this change the report will list entries such asfoo.graphql.jsoneven though the actual query file on disk is stillfoo.graphql, which breaks that mapping and forces consumers to strip suffixes manually.Please keep
QueryPlanResult.FileNamepointing to the operation file, and derive the on-disk plan filename separately for the write step. Conceptually:- outContent, err := planner.PlanOperation(queryFilePath, cfg.Raw) - fileName := queryFile.Name() - if cfg.Raw { - fileName += ".json" - } - res := QueryPlanResult{ - FileName: fileName, - Plan: outContent, - } + outContent, err := planner.PlanOperation(queryFilePath, cfg.Raw) + operationFileName := queryFile.Name() + planFileName := operationFileName + if cfg.Raw { + planFileName = operationFileName + ".json" + } + res := QueryPlanResult{ + FileName: operationFileName, + Plan: outContent, + } … - if cfg.OutputFiles { - outFileName := filepath.Join(outPath, fileName) + if cfg.OutputFiles { + outFileName := filepath.Join(outPath, planFileName)That keeps the report backward-compatible while still allowing the per-file plan to use a
.jsonsuffix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
router/cmd/plan_generator.go(1 hunks)router/core/plan_generator.go(3 hunks)router/pkg/plan_generator/plan_generator.go(3 hunks)router/pkg/plan_generator/plan_generator_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
router/pkg/plan_generator/plan_generator_test.go (1)
router/pkg/plan_generator/plan_generator.go (4)
QueryPlanConfig(22-37)PlanGenerator(51-215)ReportFileName(20-20)QueryPlanResults(39-42)
⏰ 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). (5)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
router/cmd/plan_generator.go (1)
39-39: Thanks for wiring the CLI flag through the configThe new
-rawflag integrates cleanly with the existing flag set and keeps the defaults untouched. 👍router/core/plan_generator.go (1)
95-101: Good guardrail on raw marshalingReturning early with a wrapped error if
json.Marshalfails keeps the caller’s error-handling path unchanged and prevents us from writing partially serialized plans. Nice touch.
|
converting to draft again for a few changes |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router/pkg/plan_generator/plan_generator.go(2 hunks)router/pkg/plan_generator/plan_generator_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router/pkg/plan_generator/plan_generator.go
🧰 Additional context used
🧬 Code graph analysis (1)
router/pkg/plan_generator/plan_generator_test.go (2)
router/pkg/plan_generator/plan_generator.go (2)
QueryPlanConfig(22-37)PlanGenerator(51-211)router/cmd/plan_generator.go (1)
PlanGenerator(22-112)
⏰ 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). (6)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
router/core/plan_generator.go (1)
56-61: Solid: explicit output format enumClear public type with string constants. Consider adding a short doc comment for exported identifiers to improve GoDoc.
router/pkg/plan_generator/plan_generator.go (2)
80-86: Filter parsing: trim and ignore blank lines (handle CRLF, trailing newlines)Direct
strings.Splitcan leave""entries and CRLF tails, causing false negatives on Windows or with trailing newlines.- filter = strings.Split(string(filterContent), "\n") + for _, line := range strings.Split(string(filterContent), "\n") { + l := strings.TrimSpace(line) + if l != "" { + filter = append(filter, l) + } + }
102-107: Optional: set a sane default timeout when emptyCurrently an empty
cfg.Timeoutwill fail parsing. If the CLI already guarantees a value, ignore; otherwise default here.- duration, parseErr := time.ParseDuration(cfg.Timeout) + if cfg.Timeout == "" { + cfg.Timeout = "30s" + } + duration, parseErr := time.ParseDuration(cfg.Timeout)router/pkg/plan_generator/plan_generator_test.go (4)
21-24: Use filepath.Join for filesystem paths
path.Joinis for slash‑separated URLs; preferfilepath.Joinfor OS‑agnostic filesystem paths.func getTestDataDir() string { _, filename, _, _ := runtime.Caller(0) - return path.Join(filepath.Dir(filename), "testdata") + return filepath.Join(filepath.Dir(filename), "testdata") }Consider updating other occurrences of
path.Joinin this file similarly.
26-35: Precompile the regexAvoid recompiling the regex on each call to
sanitizeName.You can add this at file scope (outside the function):
var sanitizeRe = regexp.MustCompile(`[^a-zA-Z0-9_]+`)Then inside
sanitizeName:- re := regexp.MustCompile(`[^a-zA-Z0-9_]+`) - name = re.ReplaceAllString(name, "_") + name = sanitizeRe.ReplaceAllString(name, "_")
391-428: Optionally assert JSON validity for raw outputsIf you want a stronger check, validate JSON for entries that look like JSON, while still allowing Warning/Error fallbacks.
- for _, de := range entriesInOutDir { + for _, de := range entriesInOutDir { name := de.Name() content, err := os.ReadFile(path.Join(outDir, name)) assert.NoError(t, err) assert.Equal(t, string(results[name]), string(content)) + s := strings.TrimSpace(string(content)) + if strings.HasPrefix(s, "{") { + var v any + assert.NoError(t, json.Unmarshal(content, &v)) + } }Also consider switching
path.Jointofilepath.Joinhere.
430-467: Mirror the JSON-validity check in the text-format test (optional)Not required, but keeping similar checks increases parity and future‑proofs the tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
router/cmd/plan_generator.go(3 hunks)router/core/plan_generator.go(4 hunks)router/pkg/plan_generator/plan_generator.go(3 hunks)router/pkg/plan_generator/plan_generator_test.go(9 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_generates_a_plan_for_every_file_filtered/1.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_generates_non_raw_textual_plans_when_raw_is_disabled/1.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_generates_non_raw_textual_plans_when_raw_is_disabled/2.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_generates_non_raw_textual_plans_when_raw_is_disabled/3.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_generates_non_raw_textual_plans_when_raw_is_disabled/4.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_generates_non_raw_textual_plans_when_raw_is_disabled/5.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_generates_raw_json_plans_when_raw_is_enabled/1.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_generates_raw_json_plans_when_raw_is_enabled/2.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_generates_raw_json_plans_when_raw_is_enabled/3.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_generates_raw_json_plans_when_raw_is_enabled/4.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_generates_raw_json_plans_when_raw_is_enabled/5.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_will_not_fail_on_warnings_and_files_should_have_warnings_and_generate_files/1.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_will_not_fail_on_warnings_and_files_should_have_warnings_and_generate_files/2.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_will_not_fail_on_warnings_and_files_should_have_warnings_and_generate_files/3.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_will_not_fail_on_warnings_and_files_should_have_warnings_and_generate_files/4.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_will_not_fail_on_warnings_and_files_should_have_warnings_and_generate_files/5.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/testplangenerator_will_not_fail_on_warnings_and_results_should_return_the_warnings_and_generate_results_file/report.json(1 hunks)
✅ Files skipped from review due to trivial changes (11)
- router/pkg/plan_generator/testdata/plans/testplangenerator_generates_a_plan_for_every_file_filtered/1.graphql
- router/pkg/plan_generator/testdata/plans/testplangenerator_generates_non_raw_textual_plans_when_raw_is_disabled/3.graphql
- router/pkg/plan_generator/testdata/plans/testplangenerator_generates_raw_json_plans_when_raw_is_enabled/4.graphql
- router/pkg/plan_generator/testdata/plans/testplangenerator_will_not_fail_on_warnings_and_files_should_have_warnings_and_generate_files/3.graphql
- router/pkg/plan_generator/testdata/plans/testplangenerator_generates_non_raw_textual_plans_when_raw_is_disabled/1.graphql
- router/pkg/plan_generator/testdata/plans/testplangenerator_will_not_fail_on_warnings_and_files_should_have_warnings_and_generate_files/5.graphql
- router/pkg/plan_generator/testdata/plans/testplangenerator_will_not_fail_on_warnings_and_files_should_have_warnings_and_generate_files/2.graphql
- router/pkg/plan_generator/testdata/plans/testplangenerator_generates_raw_json_plans_when_raw_is_enabled/1.graphql
- router/pkg/plan_generator/testdata/plans/testplangenerator_generates_raw_json_plans_when_raw_is_enabled/3.graphql
- router/pkg/plan_generator/testdata/plans/testplangenerator_will_not_fail_on_warnings_and_results_should_return_the_warnings_and_generate_results_file/report.json
- router/pkg/plan_generator/testdata/plans/testplangenerator_generates_raw_json_plans_when_raw_is_enabled/2.graphql
🚧 Files skipped from review as they are similar to previous changes (1)
- router/cmd/plan_generator.go
🧰 Additional context used
🧬 Code graph analysis (2)
router/pkg/plan_generator/plan_generator.go (1)
router/core/plan_generator.go (2)
PlanOutputFormat(56-56)PlanOutputFormatText(59-59)
router/pkg/plan_generator/plan_generator_test.go (2)
router/pkg/plan_generator/plan_generator.go (3)
QueryPlanConfig(22-37)PlanGenerator(51-215)ReportFileName(20-20)router/core/plan_generator.go (3)
PlanGenerator(43-47)PlanOutputFormatJSON(60-60)PlanOutputFormatText(59-59)
⏰ 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). (10)
- GitHub Check: build-router
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
router/pkg/plan_generator/testdata/plans/testplangenerator_will_not_fail_on_warnings_and_files_should_have_warnings_and_generate_files/4.graphql (1)
1-26: LGTM on the new plan fixture.Plan structure and entity resolution look consistent with the rest of the suite. No issues spotted.
router/pkg/plan_generator/testdata/plans/testplangenerator_will_not_fail_on_warnings_and_files_should_have_warnings_and_generate_files/1.graphql (1)
1-32: Fixture matches the expected non-JSON plan.The sequence and fetch blocks line up with the textual formatter’s current output, so this fixture looks good to me.
router/pkg/plan_generator/testdata/plans/testplangenerator_generates_non_raw_textual_plans_when_raw_is_disabled/2.graphql (1)
1-1: Fixture LGTMThe warning fixture aligns with the generator’s "Warning:" prefix behavior.
router/pkg/plan_generator/testdata/plans/testplangenerator_generates_non_raw_textual_plans_when_raw_is_disabled/5.graphql (1)
1-29: Textual plan fixture LGTMMatches the expected non‑raw pretty‑printed output shape.
router/pkg/plan_generator/testdata/plans/testplangenerator_generates_non_raw_textual_plans_when_raw_is_disabled/4.graphql (1)
1-26: Subscription plan fixture LGTMCovers the subscription + entity fetch path as intended.
router/pkg/plan_generator/testdata/plans/testplangenerator_generates_raw_json_plans_when_raw_is_enabled/5.graphql (1)
1-1: Raw JSON fixture LGTMMinified JSON matches the expected structure for OutputFormatJSON.
router/pkg/plan_generator/plan_generator.go (1)
56-58: Defaulting format to text is goodReasonable default when not specified.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
router/core/plan_generator.go (3)
56-62: Add brief doc comments for the new enumSmall clarity win for readers and godoc.
Apply this diff:
type PlanOutputFormat string const ( - PlanOutputFormatText PlanOutputFormat = "text" - PlanOutputFormatJSON PlanOutputFormat = "json" + // PlanOutputFormatText renders a human-readable, pretty-printed plan. + PlanOutputFormatText PlanOutputFormat = "text" + // PlanOutputFormatJSON renders the raw plan as JSON. + PlanOutputFormatJSON PlanOutputFormat = "json" )
106-110: Nit: rename temporary variable for clarityAvoid naming the variable “marshal” (reads like a function). Use a neutral name.
Apply this diff:
- marshal, err := json.Marshal(rawPlan) + b, err := json.Marshal(rawPlan) if err != nil { return "", fmt.Errorf("failed to marshal raw plan: %w", err) } - return string(marshal), nil + return string(b), nil
113-113: Error message: prefer “output format” over “type”More precise wording and consistent with the API.
Apply this diff:
- return "", fmt.Errorf("invalid type specified: %q", outputFormat) + return "", fmt.Errorf("invalid output format: %q", outputFormat)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router/core/plan_generator.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
router/core/plan_generator.go (1)
router/pkg/pubsub/datasource/planner.go (1)
Planner(14-21)
⏰ 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). (10)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
router/core/plan_generator.go (3)
5-5: Import of encoding/json is appropriateRequired for JSON output; no issues.
102-114: Previous “wrap nil error” bug is fixedThe default case now returns a concrete error string and no longer wraps a nil error. Good catch and fix.
76-76: Signature change verified: all call sites updated
Call in router/pkg/plan_generator/plan_generator.go passes the newoutputFormatargument; no missing invocations remain.
SkArchon
force-pushed
the
milinda/eng-8110-view-query-plan-as-a-json
branch
from
d8339cb to
cb39241
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
router/core/plan_generator.go (4)
56-62: Add brief GoDoc for exported type/constants (optional).Improves API clarity and linting.
+// PlanOutputFormat controls how a query plan is serialized for output. type PlanOutputFormat string const ( - PlanOutputFormatText PlanOutputFormat = "text" - PlanOutputFormatJSON PlanOutputFormat = "json" + // PlanOutputFormatText pretty-prints the plan as human-readable text. + PlanOutputFormatText PlanOutputFormat = "text" + // PlanOutputFormatJSON emits the raw plan as JSON. + PlanOutputFormatJSON PlanOutputFormat = "json" )
102-111: Optional: pretty-print JSON for readability.If the JSON is meant for humans too, consider MarshalIndent.
- marshal, err := json.Marshal(rawPlan) + marshal, err := json.MarshalIndent(rawPlan, "", " ")
76-86: Early-validate outputFormat to fail fast (optional).Return on invalid formats before doing heavy planning work.
func (pl *Planner) PlanOperation(operationFilePath string, outputFormat PlanOutputFormat) (string, error) { + switch outputFormat { + case PlanOutputFormatText, PlanOutputFormatJSON: + // ok + default: + return "", fmt.Errorf("invalid output format: %q", outputFormat) + }
113-113: Clarify error message wording (nit).“invalid output format” is more specific than “invalid type”.
-return "", fmt.Errorf("invalid type specified: %q", outputFormat) +return "", fmt.Errorf("invalid output format: %q", outputFormat)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router/cmd/plan_generator.go(3 hunks)router/core/plan_generator.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
router/cmd/plan_generator.go (2)
router/pkg/plan_generator/plan_generator.go (1)
QueryPlanConfig(22-37)router/core/plan_generator.go (3)
PlanOutputFormatText(59-59)PlanOutputFormat(56-56)PlanOutputFormatJSON(60-60)
⏰ 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: build-router
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
router/cmd/plan_generator.go (3)
10-10: LGTM: case-insensitive flag parsing.Importing strings to normalize the flag value is appropriate.
26-28: Good: explicit default to text.Initializing OutputFormat to PlanOutputFormatText provides a clear default.
42-52: Consider renaming flag to--formatand clarify default in help.For CLI consistency and brevity, prefer
--formatover--print-format, and mention the default in the help string. No behavior change.- f.Func("print-format", "output format (text|json)", func(s string) error { + f.Func("format", "output format (text|json) [default: text]", func(s string) error {If you keep
--print-format, consider registering both flags to avoid churn:+ f.Func("print-format", "DEPRECATED: use --format", func(s string) error { + return nil // keep for backward compatibility; ignore value + })router/core/plan_generator.go (3)
5-5: LGTM: JSON import added for new output mode.Import is correct and scoped.
102-111: Confirm JSON marshaling safety (cycles/unsupported types).Verify resolve.FetchTreeQueryPlanNode contains only JSON-encodable fields and no parent pointers that create cycles. If not, introduce a DTO for serialization.
76-76: All PlanOperation call sites updated The call in router/pkg/plan_generator/plan_generator.go now passescfg.OutputFormat; no further updates needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
router/cmd/plan_generator.go (2)
26-28: Avoid duplicated defaults for OutputFormatYou're defaulting OutputFormat here and (per package code) also inside pkg PlanGenerator. Prefer a single source of defaulting to prevent drift; keep it in pkg and let CLI usage text advertise the default.
42-52: Harden flag parsing (trim input) and clarify help textTrim whitespace before lowercasing; also reflect the default in the flag help for discoverability.
Apply this diff:
- f.Func("print-format", "output format (text|json)", func(s string) error { - switch value := core.PlanOutputFormat(strings.ToLower(s)); value { + f.Func("print-format", "output format (text|json) (default: text)", func(s string) error { + switch value := core.PlanOutputFormat(strings.ToLower(strings.TrimSpace(s))); value { case core.PlanOutputFormatInvalid: cfg.OutputFormat = core.PlanOutputFormatText return nil case core.PlanOutputFormatText, core.PlanOutputFormatJSON: cfg.OutputFormat = value return nil } return fmt.Errorf("must be one of: text, json (got %q)", s) })router/pkg/plan_generator/plan_generator_test.go (2)
82-85: Use t.TempDir consistentlyThis test still uses os.MkdirTemp + t.Cleanup. Prefer t.TempDir for consistency with the rest and less code.
Apply this diff:
- tempDir, err := os.MkdirTemp("", "plans-") - require.NoError(t, err) - t.Cleanup(func() { - _ = os.RemoveAll(tempDir) - }) + tempDir := t.TempDir()
316-349: Test names: reflect new API (optional)Consider renaming “Raw is enabled/disabled” to “OutputFormatJSON/Text” to match the new configuration surface.
router/core/plan_generator.go (1)
114-114: Clarify error wording“invalid type specified” could be “invalid output format” for precision and consistency with CLI wording.
Apply this diff:
- return "", fmt.Errorf("invalid type specified: %q", outputFormat) + return "", fmt.Errorf("invalid output format: %q", outputFormat)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
router/cmd/plan_generator.go(3 hunks)router/core/plan_generator.go(4 hunks)router/pkg/plan_generator/plan_generator_test.go(21 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
router/pkg/plan_generator/plan_generator_test.go (2)
router/core/plan_generator.go (3)
PlanGenerator(43-47)PlanOutputFormatJSON(61-61)PlanOutputFormatText(60-60)router/pkg/plan_generator/plan_generator.go (2)
PlanGenerator(51-215)QueryPlanConfig(22-37)
router/cmd/plan_generator.go (2)
router/pkg/plan_generator/plan_generator.go (1)
QueryPlanConfig(22-37)router/core/plan_generator.go (4)
PlanOutputFormatText(60-60)PlanOutputFormat(56-56)PlanOutputFormatInvalid(59-59)PlanOutputFormatJSON(61-61)
⏰ 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: build-router
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
router/cmd/plan_generator.go (1)
10-10: LGTM: needed importstrings import is appropriate for case-insensitive flag parsing.
router/pkg/plan_generator/plan_generator_test.go (3)
35-35: Nice: switched to t.TempDirUsing t.TempDir reduces boilerplate and auto-cleans temp dirs.
341-347: Allow "Error:" outputs in JSON-mode assertionPlanGenerator can write "Error: ..." (non-validation failures) even when OutputFormatJSON is set. The current assertion only allows "Warning:". Accept both to avoid false negatives. Based on relevant code snippets.
Apply this diff:
- if err := json.Unmarshal(content, &m); err != nil { - assert.True(t, strings.HasPrefix(string(content), "Warning:")) + if err := json.Unmarshal(content, &m); err != nil { + s := string(content) + assert.True(t, strings.HasPrefix(s, "Warning:") || strings.HasPrefix(s, "Error:"))
375-379: Allow "Error:" outputs in text-mode assertionNon-raw path can also emit "Error: ..." fallbacks. Include it in accepted prefixes to prevent false failures. Based on relevant code snippets.
Apply this diff:
- assert.True(t, strings.HasPrefix(s, "QueryPlan {") || strings.HasPrefix(s, "Warning:")) + assert.True(t, strings.HasPrefix(s, "QueryPlan {") || + strings.HasPrefix(s, "Warning:") || + strings.HasPrefix(s, "Error:"))router/core/plan_generator.go (3)
56-63: LGTM: explicit output format type and zero-valuePlanOutputFormat with an explicit Invalid zero-value is a solid pattern.
103-114: LGTM: format switch and JSON marshallingSwitching on outputFormat with proper error propagation is correct.
77-77: No in-repo calls toPlanOperationfound; manually verify external callers – automated search returned no usages beyond its definition; ensure CLI commands, tests or other consumers supply the newoutputFormatargument.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
router/core/plan_generator.go (4)
56-63: Enums look good; handle zero value by defaulting to textYou introduced PlanOutputFormatUnset = "". Ensure zero-value inputs don’t error by defaulting to text in PlanOperation.
103-114: Default unset to text and clarify error messageAvoid surprising errors on zero-value format and make the error precise.
Apply:
- switch outputFormat { + if outputFormat == PlanOutputFormatUnset { + outputFormat = PlanOutputFormatText + } + switch outputFormat { case PlanOutputFormatText: return rawPlan.PrettyPrint(), nil case PlanOutputFormatJSON: marshal, err := json.Marshal(rawPlan) if err != nil { return "", fmt.Errorf("failed to marshal raw plan: %w", err) } return string(marshal), nil } - return "", fmt.Errorf("invalid type specified: %q", outputFormat) + return "", fmt.Errorf("invalid output format: %q", outputFormat)
107-111: Optional: support pretty JSON output variantIf human readability is desired, consider an additional format (e.g., "json-pretty") that uses json.MarshalIndent.
77-77: All PlanOperation call sites updated with the new outputFormat parameter. No remaining breaking calls detected. Optionally add a convenience wrapperfunc (pl *Planner) PlanOperationText(path string) (string, error) { return pl.PlanOperation(path, PlanOutputFormatText) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
router/cmd/plan_generator.go(3 hunks)router/core/plan_generator.go(4 hunks)router/pkg/plan_generator/plan_generator.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- router/pkg/plan_generator/plan_generator.go
- router/cmd/plan_generator.go
🧰 Additional context used
🧬 Code graph analysis (1)
router/core/plan_generator.go (1)
router/pkg/pubsub/datasource/planner.go (1)
Planner(14-21)
⏰ 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: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
router/core/plan_generator.go (1)
5-5: LGTM on JSON importNeeded for JSON output; no issues.
3 participants
What happens now
Right now we can view query plan from the plan generator in the router using a textual pretty printed format
With this Pr
This PR introduces a "format" attribute, which is "text" by default, upon being set to "json", this would cause query plans to be generated as a raw json. This is NOT similar to the apollo jsons, to make it similar would require separate work.
Summary by CodeRabbit
New Features
Tests
Checklist