feat(api): add root conversion endpoint#239
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a structured root conversion API contract with strict JSON validation, typed option errors, chart symbol validation, shared theme handling, revised shutdown and merge behavior, expanded tests, and updated serve documentation. ChangesServe REST API
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant handleConvertWithGenerator
participant core.Convert
participant HTTPResponse
Client->>handleConvertWithGenerator: POST / conversion request
handleConvertWithGenerator->>core.Convert: validated ConvertInput
core.Convert-->>handleConvertWithGenerator: Dataset, HTML, warnings, or OptionError
handleConvertWithGenerator-->>HTTPResponse: response or validation problem
HTTPResponse-->>Client: JSON or HTML result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
hfl0506
marked this pull request as ready for review
July 18, 2026 21:59
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
api/openapi_test.go (1)
109-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise uniqueness for every chart type.
The schema manually repeats six
containsconstraints, but this test only provesbar. Iterate through all supported types so a typo in another constraint cannot pass unnoticed.As per coding guidelines, “After every change, assess affected-feature coverage and add or update tests when needed.”
🤖 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 `@api/openapi_test.go` around lines 109 - 130, Update TestDatasetSettingsRequireUniqueChartTypes to verify duplicate settings for every supported chart type, rather than only bar. Iterate over the complete supported chart-type set, construct two settings entries with the current type, and assert validation returns the expected “want at most 1” error for each iteration.Source: Coding guidelines
pkg/core/core_test.go (1)
125-154: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert each exact
OptionError.Name.
NotEmptyallows every case to return the same incorrect name, which would misroute structured API error paths. Add expected names forselect,swap, andgrouping; ideally coverfilterandjsonPathtoo.As per coding guidelines, “After every change, assess affected-feature coverage and add or update tests when needed.”
🤖 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 `@pkg/core/core_test.go` around lines 125 - 154, The TestConvertIdentifiesInapplicableOptions test currently only checks that OptionError.Name is non-empty; update each test case to carry and assert its exact expected name, including select, swap, and grouping. Extend coverage with filter and jsonPath cases if supported by the existing Convert error paths, while preserving the current errors.Is assertion.Source: Coding guidelines
🤖 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 `@api/openapi.yaml`:
- Around line 608-610: Update the OpenAPI `symbol` schemas near the `symbol`
properties to constrain values to the server-supported ECharts built-in symbols
and valid `image://`, `path://`, or SVG path formats, instead of allowing
arbitrary strings. Apply the same validation to both schema occurrences so
generated clients reject unsupported values such as `star`.
In `@cmd/serve_contract.go`:
- Around line 39-42: Update statisticsOptions and its containing request
unmarshalling to reject explicit null for the statistics object, Enabled, and
Math fields instead of treating null as omitted/default; match the null-aware
unmarshalling behavior used by the conversion request types. Add or update tests
covering all three null cases while preserving valid omitted and non-null
values.
In `@cmd/serve_test.go`:
- Around line 145-159: Update the ServeSuite tests, including
TestCancellationShutsDownInMemoryListener and the additional affected test
cases, to replace direct receives from listener.acceptStarted and result with
the repository’s timeout-backed wait helper. Ensure every channel wait has a
bounded timeout while preserving the existing assertions and test behavior.
In `@cmd/serve.go`:
- Around line 143-155: Update the ctx.Done shutdown path in runServer so that
when shutdown returns an error, it calls server.Close(), drains and waits for
serveResult, then returns the shutdown error. Preserve the existing handling for
successful shutdown and update the shutdown-failure test to verify the serving
goroutine exits.
---
Nitpick comments:
In `@api/openapi_test.go`:
- Around line 109-130: Update TestDatasetSettingsRequireUniqueChartTypes to
verify duplicate settings for every supported chart type, rather than only bar.
Iterate over the complete supported chart-type set, construct two settings
entries with the current type, and assert validation returns the expected “want
at most 1” error for each iteration.
In `@pkg/core/core_test.go`:
- Around line 125-154: The TestConvertIdentifiesInapplicableOptions test
currently only checks that OptionError.Name is non-empty; update each test case
to carry and assert its exact expected name, including select, swap, and
grouping. Extend coverage with filter and jsonPath cases if supported by the
existing Convert error paths, while preserving the current errors.Is assertion.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3dbfed2f-ab4d-4cfc-a42d-1d77caec903f
📒 Files selected for processing (17)
api/openapi.yamlapi/openapi_test.gocmd/cli/dataflags.gocmd/cli/flagbag.gocmd/cli/flagbag_test.gocmd/serve.gocmd/serve_contract.gocmd/serve_test.gocmd/testing.godocs/src/content/docs/commands/serve.mdxinternal/flags/flag.gopkg/core/core.gopkg/core/core_test.gopkg/style/style.gopkg/style/style_test.goshared/chart_spec.goshared/chart_spec_test.go
hfl0506
force-pushed
the
feat/rest-root-endpoint
branch
from
July 19, 2026 06:56
8bcfa3a to
a2d5e9c
Compare
fahimfaisaal
requested changes
Jul 20, 2026
5 tasks
Member
|
@hfl0506 you have some conflicts and request changes, please review. Thanks. |
hfl0506
force-pushed
the
feat/rest-root-endpoint
branch
from
July 20, 2026 14:55
08d64f2 to
88f1794
Compare
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 `@cmd/serve_contract.go`:
- Around line 155-190: Update rejectNullFields and strictDecodeRequestObject to
preserve precise nested field paths for schema violations. Map
*json.UnmarshalTypeError paths using the relevant nullPath or prefix, and
convert non-object decoded values into bodyValidationError rather than allowing
root-level invalid_json errors. Ensure inputs such as grouping:"foo" and
grouping.pattern:123 identify the offending field path.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 072bfda2-8b96-4c04-98be-8fa5aea9a1a3
📒 Files selected for processing (13)
api/openapi.yamlapi/openapi_test.gocmd/cli/dataflags.gocmd/cli/flagbag_test.gocmd/serve.gocmd/serve_contract.gocmd/serve_test.godocs/src/content/docs/commands/serve.mdxpkg/core/core.gopkg/core/core_test.gopkg/style/style.gopkg/style/style_test.goshared/chart_spec_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/core/core_test.go
- shared/chart_spec_test.go
- pkg/style/style.go
- cmd/cli/flagbag_test.go
- pkg/core/core.go
- api/openapi_test.go
- cmd/serve.go
- api/openapi.yaml
- cmd/cli/dataflags.go
- cmd/serve_test.go
- pkg/style/style_test.go
Collaborator
Author
Hi, I resolved conflict, please take a look when you have a chance. Thanks |
fahimfaisaal
approved these changes
Jul 20, 2026
fahimfaisaal
left a comment
fahimfaisaal
left a comment
Member
There was a problem hiding this comment.
For the first version, LGTM. I found a bug btw
we have two regions but it's showing 0.
Could you create a bug issue on it to fix it at next release
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Resolve #214
Changes
Added strict POST / Dataset/HTML conversion flow through the shared core.
Added top-level Dataset metadata and structured parser, grouping, unit,
selection, chart, and output fields.
Added strict validation for unknown, null, invalid, and inapplicable options.
Updated api/openapi.yaml with schemas and examples.
Expanded cmd/serve.go, cmd/serve_contract.go, and test coverage.
Test Result
go run . serve --host 127.0.0.1 --port 8080Case 1
curl -i http://127.0.0.1:8080/ \ -H 'Content-Type: application/json' \ -H 'Accept: application/json' \ --data '{ "input": "region,latency\nwest,12\neast,18\n", "parser": "csv", "select": ["region,latency"], "charts": {"types": ["bar", "line"]}, "output": {"format": "dataset"} }' HTTP/1.1 200 OK Content-Type: application/json Date: Sat, 18 Jul 2026 18:29:56 GMT Content-Length: 326 {"timestamp":"2026-07-18T18:29:56Z","name":"Comparisons","theme":"default","axes":[{"key":"x","label":"region"},{"key":"y","label":"latency","type":"value"}],"settings":[{"type":"bar","scale":"linear"},{"type":"line","scale":"linear"}],"data":[{"xAxis":"west","yAxis":"12"},{"xAxis":"east","yAxis":"18"}],"preserveRows":true}Case 2
curl -i http://127.0.0.1:8080/ \ -H 'Content-Type: application/json' \ -H 'Accept: application/json' \ --data '{ "input": [ {"region":"west","latency":12}, {"region":"east","latency":18} ], "parser":"json", "grouping":{"pattern":"x","columns":["region"]}, "charts":{"types":["bar"]}, "output":{"format":"dataset"} }' HTTP/1.1 200 OK Content-Type: application/json Date: Sat, 18 Jul 2026 18:31:46 GMT Content-Length: 282 {"timestamp":"2026-07-18T18:31:46Z","name":"Comparisons","theme":"default","axes":[{"key":"x","label":"region"}],"settings":[{"type":"bar","scale":"linear"}],"data":[{"xAxis":"west","stats":[{"type":"latency","value":12}]},{"xAxis":"east","stats":[{"type":"latency","value":18}]}]}Case 3
curl -i http://127.0.0.1:8080/ \ -H 'Content-Type: application/json' \ -H 'Accept: application/json' \ --data '{ "input":"region,value\nwest,12\n", "id":"test-dataset", "name":"Local test", "description":"Testing REST conversion", "tag":"v1", "theme":"#5470C6,#3BA272", "parser":"csv", "charts":{"types":["bar"]} }' HTTP/1.1 200 OK Content-Type: application/json Date: Sat, 18 Jul 2026 18:32:55 GMT Content-Length: 301 {"id":"test-dataset","tag":"v1","timestamp":"2026-07-18T18:32:55Z","name":"Local test","theme":"#5470C6,#3BA272","description":"Testing REST conversion","axes":[{"key":"x","label":"region"}],"settings":[{"type":"bar","scale":"linear"}],"data":[{"xAxis":"west","stats":[{"type":"value","value":12}]}]}Case 4
curl -i http://127.0.0.1:8080/ \ -H 'Content-Type: application/json' \ -H 'Accept: application/json' \ --data '{ "input":"BenchmarkFoo-8 100 123 ns/op\n", "parser":"go", "charts":{"types":["bar"]} }' HTTP/1.1 200 OK Content-Type: application/json Date: Sat, 18 Jul 2026 18:33:40 GMT Content-Length: 223 {"timestamp":"2026-07-18T18:33:40Z","name":"Comparisons","theme":"default","axes":[{"key":"x"}],"settings":[{"type":"bar","scale":"linear"}],"data":[{"xAxis":"Foo","stats":[{"type":"Execution Time (ns/op)","value":123}]}]}Case 5
curl -sS http://127.0.0.1:8080/ \ -H 'Content-Type: application/json' \ -H 'Accept: text/html' \ --data '{ "input":"region,value\nwest,12\neast,18\n", "parser":"csv", "charts":{"types":["bar"]}, "output":{"format":"html"} }' \ --output /tmp/vizb.html open /tmp/vizb.html{"detail":"Request validation failed.","errors":[{"location":"body","path":"/unknownOption","code":"unknown_field","message":"unknown request field unknownOption"}],"instance":"/","status":400,"title":"Invalid request","type":"https://vizb.goptics.org/problems/validation"}
Case 8
curl -i http://127.0.0.1:8080/ \ -H 'Content-Type: application/json' \ --data '{ "input":"region,value\nwest,1\n", "parser":"csv", "charts":{ "types":["bar"], "configs":[{"type":"bar","threeDRotate":true}] } }' HTTP/1.1 400 Bad Request Content-Type: application/problem+json Date: Sat, 18 Jul 2026 18:37:29 GMT Content-Length: 323 {"detail":"Request validation failed.","errors":[{"location":"body","path":"/charts/configs/0/threeDRotate","code":"inapplicable_option","message":"flag \"3d-rotate\" skipped: requires axis \"z\" (present: [x])"}],"instance":"/","status":400,"title":"Invalid request","type":"https://vizb.goptics.org/problems/validation"}Case 9
curl -i http://127.0.0.1:8080/ \ -H 'Content-Type: application/json' \ -H 'Accept: text/html' \ --data '{ "input":"x,y\na,1\n", "output":{"format":"dataset"} }' HTTP/1.1 406 Not Acceptable Content-Type: application/problem+json Date: Sat, 18 Jul 2026 18:38:04 GMT Content-Length: 159 {"detail":"Accept must allow application/json","instance":"/","status":406,"title":"Not acceptable","type":"https://vizb.goptics.org/problems/not-acceptable"}Case 10
curl -i http://127.0.0.1:8080/ \ -H 'Content-Type: text/plain' \ --data '{"input":"x,y\na,1\n"}' HTTP/1.1 415 Unsupported Media Type Content-Type: application/problem+json Date: Sat, 18 Jul 2026 18:38:32 GMT Content-Length: 178 {"detail":"Content-Type must be application/json","instance":"/","status":415,"title":"Unsupported media type","type":"https://vizb.goptics.org/problems/unsupported-media-type"}Case 11
curl -i http://127.0.0.1:8080/ \ -H 'Content-Type: application/json' \ --data '{ "input":"region,value\nwest,\"unterminated\n", "parser":"csv", "charts":{"types":["bar"]} }' HTTP/1.1 422 Unprocessable Entity Content-Type: application/problem+json Date: Sat, 18 Jul 2026 18:39:16 GMT Content-Length: 214 {"detail":"read CSV: parse error on line 2, column 20: extraneous or missing \" in quoted-field","instance":"/","status":422,"title":"Input processing failed","type":"https://vizb.goptics.org/problems/processing"}Summary by CodeRabbit
New Features
Bug Fixes
null/empty inputs.Documentation
servecommand example payloads and selector format to match the new metadata fields.