refactor: drive endpoint generation from explicit surface spec#203
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors thetadatadx build-time code generation so endpoint-facing surfaces are driven by a checked-in endpoint_surface.toml spec, validated against proto/external.proto, and emits the registry/runtime/direct projections from that explicit contract.
Changes:
- Add
endpoint_surface.tomlas the normalized endpoint surface contract (templates + reusable param groups + concrete endpoints). - Split the previous monolithic
build.rsintobuild_support/modules and makebuild.rsa thin entrypoint. - Update maintenance/docs and packaging metadata to reflect the new generation/validation flow.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/thetadatadx/README.md | Documents the new endpoint surface spec + generator layout. |
| crates/thetadatadx/proto/MAINTENANCE.md | Updates maintenance flow to spec-first validation/generation. |
| crates/thetadatadx/endpoint_surface.toml | Introduces the explicit endpoint surface contract (param groups/templates/endpoints). |
| crates/thetadatadx/Cargo.toml | Ensures the spec and build_support are included in published artifacts. |
| crates/thetadatadx/build.rs | Replaces monolithic generation with build_support::run(). |
| crates/thetadatadx/build_support/mod.rs | Orchestrates proto compilation + endpoint + tick generation. |
| crates/thetadatadx/build_support/endpoints.rs | Implements spec resolution, validation vs proto, and codegen for registry/runtime/direct. |
| crates/thetadatadx/build_support/ticks.rs | Moves tick decoder generation into a dedicated module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if surface.wire_name.is_none() { | ||
| for wire_param in &wire.params { | ||
| if !surface_names.contains(&wire_param.name) { | ||
| return Err(format!( | ||
| "endpoint '{}' is missing wire parameter '{}' in endpoint_surface.toml", | ||
| surface.name, wire_param.name | ||
| ) | ||
| .into()); | ||
| } |
There was a problem hiding this comment.
validate_surface_endpoint skips the “missing wire parameters” check whenever wire_name is set. That means an aliased endpoint can accidentally omit required wire parameters without the validator catching it. Consider always requiring that all required wire params are present (even when wire_name is used), and only relaxing the check for optional wire params when aliasing is intended (e.g., allow dropping optional date for *_range endpoints).
| if surface.wire_name.is_none() { | |
| for wire_param in &wire.params { | |
| if !surface_names.contains(&wire_param.name) { | |
| return Err(format!( | |
| "endpoint '{}' is missing wire parameter '{}' in endpoint_surface.toml", | |
| surface.name, wire_param.name | |
| ) | |
| .into()); | |
| } | |
| for wire_param in &wire.params { | |
| let missing_from_surface = !surface_names.contains(&wire_param.name); | |
| let must_be_present = surface.wire_name.is_none() || wire_param.required; | |
| if missing_from_surface && must_be_present { | |
| return Err(format!( | |
| "endpoint '{}' is missing wire parameter '{}' in endpoint_surface.toml", | |
| surface.name, wire_param.name | |
| ) | |
| .into()); |
| fn emit_required_arg(out: &mut String, endpoint: &GeneratedEndpoint, param: &GeneratedParam) { | ||
| if endpoint.name == "stock_history_ohlc" && param.name == "date" && !param.required { | ||
| out.push_str(" let date = args.optional_date(\"date\")?.unwrap_or(\"\");\n"); | ||
| return; | ||
| } | ||
|
|
||
| if param.param_type == "Symbols" { | ||
| writeln!( | ||
| out, | ||
| " let symbol_values = args.required_symbols(\"{}\")?;", | ||
| param.name | ||
| ) | ||
| .unwrap(); | ||
| out.push_str( | ||
| " let symbol_refs: Vec<&str> = symbol_values.iter().map(String::as_str).collect();\n", | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| let getter = required_getter_name(¶m.param_type); | ||
| writeln!( | ||
| out, | ||
| " let {} = args.{getter}(\"{}\")?;", | ||
| param.name, param.name | ||
| ) | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
The generated shared-runtime dispatch treats all binding = "method" params as required inputs (via required_* getters), ignoring param.required. With the current surface spec, several method-bound params (notably date) are marked required = false, so the registry/CLI/REST will accept calls without them, but invoke_generated_endpoint will still fail at runtime. Either enforce that all method-bound params are required = true during surface validation, or update dispatch generation to use optional getters/defaulting when required = false.
| _ if field.proto_type == "string" => { | ||
| if field.is_optional { | ||
| if is_method_param { | ||
| format!("Some({arg_name}.clone())") | ||
| } else { | ||
| format!("{arg_name}.clone()") | ||
| } | ||
| } else if list_context { | ||
| format!("{arg_name}.to_string()") | ||
| } else { | ||
| format!("{arg_name}.clone()") | ||
| } | ||
| } |
There was a problem hiding this comment.
For optional wire string fields (e.g., date in many *History*RequestQuery messages), the generator emits Some(arg.clone()) whenever the param is method-bound. If the shared runtime uses an empty-string sentinel to represent “unset”, this will still send Some("") over the wire, which is not equivalent to None for protobuf optional fields and may break server-side union logic (date vs start_date/end_date). Consider emitting None when the method arg is empty for optional wire fields, or otherwise modeling this union explicitly so registry-driven calls can omit date safely.
| [param_groups.optional_date] | ||
|
|
||
| [[param_groups.optional_date.params]] | ||
| name = "date" | ||
| description = "Date YYYYMMDD" | ||
| param_type = "Date" | ||
| required = false | ||
| binding = "method" | ||
|
|
||
| [param_groups.interval] | ||
|
|
||
| [[param_groups.interval.params]] | ||
| name = "interval" | ||
| description = "Accepts milliseconds (60000) or shorthand (1m). Presets: 100ms, 500ms, 1s, 5s, 10s, 15s, 30s, 1m, 5m, 10m, 15m, 30m, 1h." | ||
| param_type = "Interval" | ||
| required = true | ||
| binding = "method" | ||
|
|
There was a problem hiding this comment.
optional_date is declared with binding = "method" but required = false. Because method-bound params become positional args, they’re still required at the Rust call site (and are always validated via the dates: hook in the generated parsed_endpoint! methods). This makes the registry advertise date as optional even though callers must supply it, and it also creates a required-after-optional ordering issue for CLI generation (e.g., interval comes after an “optional” date). Consider either making optional_date required when binding = "method", or moving date to a builder-bound optional param and introducing a separate endpoint for the date-required shape.
| [templates.stock_history_intraday_interval] | ||
| extends = "stock_parsed" | ||
| subcategory = "history" | ||
|
|
||
| [[templates.stock_history_intraday_interval.params]] | ||
| use = "stock_symbol" | ||
|
|
||
| [[templates.stock_history_intraday_interval.params]] | ||
| use = "optional_date" | ||
|
|
||
| [[templates.stock_history_intraday_interval.params]] | ||
| use = "interval" | ||
|
|
||
| [[templates.stock_history_intraday_interval.params]] | ||
| use = "history_time_filters" | ||
|
|
||
| [[templates.stock_history_intraday_interval.params]] | ||
| use = "stock_venue_filter" | ||
|
|
||
| [[templates.stock_history_intraday_interval.params]] | ||
| use = "date_range_optional" | ||
|
|
There was a problem hiding this comment.
In stock_history_intraday_interval, date (currently marked optional) appears before interval (required). The CLI generator treats all parameters after the first optional param as optional (due to clap positional constraints), so interval will become optional in the CLI/REST surfaces even though it’s wire-required. Reorder the params so all required method params come first, or mark date required for this call shape.
Summary
endpoint_surface.tomlas the normalized endpoint surface contractcrates/thetadatadx/build.rsinto focused build-support generatorsDirectClientmethods from the explicit surface spec validated againstproto/external.protoparam_groupsand inheritabletemplatesso repeated endpoint families no longer duplicate the same parameter blocksWhy
This is the next step after #201. The repository already moved multiple consumers onto a shared endpoint runtime, but the generator still depended on proto-derived heuristics and a monolithic build script. This change moves the endpoint-facing source of truth to a checked-in declarative spec, validates it against the upstream proto, and adds reusable spec primitives so the SDK surface can scale without turning the spec file into a hand-maintained wall of boilerplate.
Validation
cargo fmt --allcargo test -p thetadatadxcargo test --manifest-path tools/mcp/Cargo.tomlcargo test --manifest-path tools/server/Cargo.tomlcargo test --manifest-path tools/cli/Cargo.tomlcargo clippy -p thetadatadx -- -D warningscargo clippy --manifest-path tools/mcp/Cargo.toml -- -D warningscargo clippy --manifest-path tools/server/Cargo.toml -- -D warningscargo clippy --manifest-path tools/cli/Cargo.toml -- -D warningscargo check --manifest-path sdks/python/Cargo.tomlcargo build --release -p thetadatadx-ffiLD_LIBRARY_PATH=/home/theta-gamma/thetadx-next/target/release go test ./...(fromsdks/go)c++ -std=c++17 -fsyntax-only -I /home/theta-gamma/thetadx-next/sdks/cpp/include /home/theta-gamma/thetadx-next/sdks/cpp/src/thetadx.cppcmake -S /home/theta-gamma/thetadx-next/sdks/cpp -B /tmp/thetadx-cpp-audit-nextcmake --build /tmp/thetadx-cpp-audit-next --target thetadatadx_cppCloses #202.