Create a new Config parse endpoint for the IDEs#887
Conversation
abrooksv
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a new IDE-facing API to parse static analysis configuration YAML into a structured, per-product JSON response, while preserving legacy “best-effort” behavior for older rulesets endpoints.
Changes:
- Introduces
/v2/config/parsereturning a structured JSON response (currently SAST rulesets). - Refactors
StaticAnalysisConfigFileusage toward instance methods and distinguishes raw YAML vs base64-encoded inputs via aBase64Stringwrapper. - Updates legacy rulesets endpoints to return
[]on decode/parse failures and adds regression tests for that contract.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
crates/bins/src/bin/datadog_static_analyzer_server/ide/mod.rs |
Registers the new parse route and adds/updates endpoint contract tests. |
crates/bins/src/bin/datadog_static_analyzer_server/ide/configuration_file/static_analysis_config_file.rs |
Adds Base64String wrapper; shifts parsing to raw YAML for TryFrom<String>; introduces instance method sast_rulesets(). |
crates/bins/src/bin/datadog_static_analyzer_server/ide/configuration_file/models.rs |
Adds request/response models for the new parse API. |
crates/bins/src/bin/datadog_static_analyzer_server/ide/configuration_file/endpoints.rs |
Implements /v2/config/parse, updates legacy endpoints to use new parsing paths and preserve legacy behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
* Create a new route to parse the config file that returns a structured output so we can add more products to output * Switch to instance methods for manipulating the StaticAnalysisConfigFile * Make APIs aware of Base64 encoded String vs raw YAML String since new API does not encode the payload
abrooksv
force-pushed
the
createIDEConfigParse
branch
from
a755519 to
496925b
Compare
albertvaka
left a comment
albertvaka
left a comment
There was a problem hiding this comment.
Left some comments but I think this new API is much nicer :)
| .map_err(|e| Custom(Status::InternalServerError, e))?; | ||
| Ok(Json(ParseConfigResponse { | ||
| sast: SastParsedConfig { | ||
| rulesets: config.sast_rulesets(), |
There was a problem hiding this comment.
In get_rulesets there's some handling to return empty array if config.sast_rulesets() fails. What happens in this endpoint if it fails? Since this new endpoint will unify both validation and parsing, I think it should provide some specific error in that case. If it already implicitly does somehow (I don't know rust enough to tell), it's at least lacking documentation in the comment above the method.
There was a problem hiding this comment.
It will return a ConfigFileError, in particular the Parser variant (see here).
The error type in the signature gives you a hint, but we could definitely add that to the documentation of the method.
There was a problem hiding this comment.
👍 It still could explain the possible error responses in the comment above IMO.
jasonforal
left a comment
jasonforal
left a comment
There was a problem hiding this comment.
(Note: I didn't review correctness of the new routes/request/response)
Just added a couple comments about how the parsing is done
|
|
||
| impl StaticAnalysisConfigFile { | ||
| /// Parses a raw YAML configuration string (not base64-encoded) into a [`StaticAnalysisConfigFile`]. | ||
| fn from_yaml_content(content: String) -> Result<Self, ConfigFileError> { |
There was a problem hiding this comment.
I don't know if TryFrom<String> is the correct level of abstraction because here, we need to indicate whether the file is a legacy config or a v1 config. Otherwise, we could parse v1 syntax from a legacy file (static-analysis.datadog.yml)
There was a problem hiding this comment.
The parser seems to internally have that data though, right? After talking with Roberto, we thought we could rely on the parser determining it based on the content shape:
The only caveat is when we pass an empty string, we need to default that to an empty config file with unified format (which we would do in the 1.0.0 release), pre-1.0.0 that would still fall back to legacy.
Are you seeing something I am overlooking?
| if content.trim().is_empty() { | ||
| return Ok(Self::default()); | ||
| } | ||
| let parsed = if cfg!(test) { |
There was a problem hiding this comment.
Similarly, we'd likely want to remove this -- this was a guard I put in place so I could ship the v1 config in the analyzer without introducing it to the server (specifically because the IDE extension didn't have support)
There was a problem hiding this comment.
Yep, going to make a dedicated PR for that when we get closer to having the IDEs ready
What problem are you trying to solve?
The IDEs current call
get_rulesetsbut this doesn't scale to the new unified format file where we may need to extract out multiple fields from the config file. Instead we will add a new config/parse API that returns a JSON object with per-product top level keys.What is your solution?
Alternatives considered
What the reviewer should know
Starting to implement https://docs.google.com/document/d/1PpSy12I1xMGmxOOA3qAdw05DFveUUqmETK7pnTjpnS0/