-
Notifications
You must be signed in to change notification settings - Fork 19
feat(ide/config): add unified config support to the static-analyzer server #923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
2324543
245ba4f
ac249d3
e351d00
d4227c6
b94a76f
8073057
86dbd4d
584828a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,9 @@ use crate::datadog_static_analyzer_server::ide::configuration_file::models::{ | |
|
|
||
| use super::error::ConfigFileError; | ||
| use super::models::{AddRuleSetsRequest, IgnoreRuleRequest}; | ||
| use super::static_analysis_config_file::{Base64String, StaticAnalysisConfigFile}; | ||
| use super::static_analysis_config_file::{ | ||
| Base64String, ConfigFormat, StaticAnalysisConfigFile, WithHint, | ||
| }; | ||
| use kernel::utils::encode_base64_string; | ||
| use rocket::http::Status; | ||
| use rocket::response::status::Custom; | ||
|
|
@@ -35,26 +37,35 @@ pub fn post_ignore_rule( | |
| rule, | ||
| configuration_base64, | ||
| encoded, | ||
| .. | ||
| schema_version, | ||
| } = request.into_inner(); | ||
| tracing::debug!(rule, content = &configuration_base64); | ||
| let result = StaticAnalysisConfigFile::with_ignored_rule( | ||
| rule.into(), | ||
| Base64String(configuration_base64), | ||
| ConfigFormat::from(schema_version.as_deref()), | ||
| ); | ||
| to_response_result(result, encoded) | ||
| } | ||
|
|
||
| /// Checks if onboarding is allowed for the configuration file (deprecated). | ||
|
Copilot marked this conversation as resolved.
Outdated
|
||
| /// | ||
| /// # Arguments | ||
| /// * `content` - The path to the configuration file. | ||
| /// * `content` - The base64-encoded configuration file content, passed as a URL path segment. | ||
| #[instrument()] | ||
| #[rocket::get("/v1/config/can-onboard/<content..>")] | ||
| #[deprecated(note = "IDEs stopped supporting onboarding")] | ||
| pub fn get_can_onboard(content: PathBuf) -> Result<Json<bool>, Custom<ConfigFileError>> { | ||
| let content_str = content.to_string_lossy().into_owned(); | ||
| can_onboard(content_str) | ||
| // NOTE: this method is still used by Visual Studio. | ||
| let mut content_str = content.to_string_lossy().into_owned(); | ||
| if cfg!(target_os = "windows") { | ||
| // NOTE: this is needed due to how Rocket works with multiple segment captures. | ||
| content_str = content_str.replace("\\", "/"); | ||
| } | ||
| let config = StaticAnalysisConfigFile::try_from(WithHint(Base64String(content_str), None)) | ||
| .map_err(|e| Custom(Status::InternalServerError, e))?; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like from the description that this 500 is intentional. Would you please add a comment explaining why? I would otherwise wonder why it's not 400 like elsewhere
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! You're right, it should be 400 and I've corrected it. Any error on this route is a parse failure on client-supplied content (bad base64 or invalid YAML), so 500 was semantically wrong. I initially kept the original 500 to avoid inadvertently breaking the Visual Studio extension (the only known caller of this deprecated route), but after checking its source I confirmed it uses a plain try/catch with no status-code inspection, so the change is safe. A comment has been added to the code explaining this. Thanks for flagging it! |
||
|
|
||
| let can_onboard = config.is_onboarding_allowed(); | ||
| Ok(Json(can_onboard)) | ||
| } | ||
|
|
||
| /// Checks if onboarding is allowed for the configuration file (v2). | ||
|
|
@@ -67,15 +78,27 @@ pub fn get_can_onboard(content: PathBuf) -> Result<Json<bool>, Custom<ConfigFile | |
| format = "application/json", | ||
| data = "<request>" | ||
| )] | ||
| #[deprecated(note = "IDEs stopped supporting onboarding")] | ||
| pub fn post_can_onboard_v2( | ||
| request: Json<CanOnboardRequest>, | ||
| ) -> Result<Json<bool>, Custom<ConfigFileError>> { | ||
| let CanOnboardRequest { | ||
| configuration_base64, | ||
| .. | ||
| schema_version, | ||
| } = request.into_inner(); | ||
| can_onboard(configuration_base64) | ||
|
|
||
| let format = ConfigFormat::from(schema_version.as_deref()); | ||
| let config = StaticAnalysisConfigFile::try_from(WithHint( | ||
| Base64String(configuration_base64), | ||
| Some(format), | ||
| )) | ||
| .map_err(|e| Custom(Status::InternalServerError, e))?; | ||
|
|
||
| config | ||
| .validate_format(format) | ||
| .map_err(|e| Custom(Status::InternalServerError, e))?; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And maybe a comment why this isn't 422?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed, changed to 422 Unprocessable Entity, consistent with every other SchemaMismatch mapping in the file. I initially kept the original 500 behavior here as well out of caution, to avoid breaking older clients. After verifying that the Visual Studio extension (the only client calling the deprecated v1 route) does not inspect specific HTTP status codes, and that VS Code's canOnboard uses a generic try/catch with no status-code check either, we confirmed the change is safe to make. |
||
|
|
||
| let can_onboard = config.is_onboarding_allowed(); | ||
| Ok(Json(can_onboard)) | ||
| } | ||
|
|
||
| /// Gets the rulesets from the static analysis configuration file (deprecated). | ||
|
|
@@ -117,16 +140,26 @@ pub fn post_get_rulesets_v2(request: Json<GetRulesetsRequest>) -> Json<Vec<Strin | |
|
|
||
| /// Parses the static analysis configuration YAML and returns per-product fields. | ||
| /// | ||
| /// Returns `400` when the content cannot be parsed or when the detected schema does not | ||
| /// match the caller-declared `schema_version` — mirroring the CLI's filename-strict behavior. | ||
| /// | ||
| /// # Arguments | ||
| /// * `request` - The request containing the raw YAML string. | ||
| #[instrument()] | ||
| #[rocket::post("/v2/config/parse", format = "application/json", data = "<request>")] | ||
| pub fn post_parse_config( | ||
| request: Json<ParseConfigRequest>, | ||
| ) -> Result<Json<ParseConfigResponse>, Custom<ConfigFileError>> { | ||
| let ParseConfigRequest { configuration } = request.into_inner(); | ||
| let ParseConfigRequest { | ||
| configuration, | ||
| schema_version, | ||
| } = request.into_inner(); | ||
| tracing::debug!(%configuration); | ||
| let config = StaticAnalysisConfigFile::try_from(configuration) | ||
| let format = ConfigFormat::from(schema_version.as_deref()); | ||
| let config = StaticAnalysisConfigFile::try_from(WithHint(configuration, Some(format))) | ||
| .map_err(|e| Custom(Status::BadRequest, e))?; | ||
| config | ||
| .validate_format(format) | ||
| .map_err(|e| Custom(Status::BadRequest, e))?; | ||
| Ok(Json(ParseConfigResponse { | ||
| sast: SastParsedConfig { | ||
|
|
@@ -169,22 +202,21 @@ pub fn post_add_rulesets_v2( | |
|
|
||
| /// Adds rulesets to the configuration file. | ||
| /// | ||
| /// # Arguments | ||
| /// * `request` - The request containing rulesets and configuration file (base64). | ||
| /// When no config content is supplied, creates a new file in the format implied by | ||
| /// `schema_version` (unified when `"v1"`, legacy otherwise). When content is supplied, | ||
| /// validates that the detected format matches the declared one before mutating. | ||
| fn add_rulesets(request: Json<AddRuleSetsRequest>) -> Result<String, Custom<ConfigFileError>> { | ||
| let AddRuleSetsRequest { | ||
| rulesets, | ||
| configuration_base64, | ||
| encoded, | ||
| .. | ||
| schema_version, | ||
| } = request.into_inner(); | ||
| tracing::debug!( | ||
| rulesets=?&rulesets, | ||
| content=&configuration_base64 | ||
| ); | ||
| tracing::debug!(rulesets=?&rulesets, content=&configuration_base64); | ||
| let result = StaticAnalysisConfigFile::with_added_rulesets( | ||
| &rulesets, | ||
| configuration_base64.map(Base64String), | ||
| ConfigFormat::from(schema_version.as_deref()), | ||
| ); | ||
| to_response_result(result, encoded) | ||
| } | ||
|
|
@@ -207,7 +239,7 @@ fn get_rulesets(mut content: String) -> Json<Vec<String>> { | |
| content = content.replace("\\", "/"); | ||
| } | ||
| tracing::debug!(%content); | ||
| let rulesets = StaticAnalysisConfigFile::try_from(Base64String(content)) | ||
| let rulesets = StaticAnalysisConfigFile::try_from(WithHint(Base64String(content), None)) | ||
| .map(|config| config.sast_rulesets()) | ||
| .unwrap_or_else(|e| { | ||
| tracing::error!(error = ?e, "Error trying to parse config file"); | ||
|
|
@@ -216,25 +248,11 @@ fn get_rulesets(mut content: String) -> Json<Vec<String>> { | |
| Json(rulesets) | ||
| } | ||
|
|
||
| /// Checks if onboarding is allowed for the configuration file. | ||
| /// | ||
| /// # Arguments | ||
| /// * `content` - The configuration file content (base64). | ||
| fn can_onboard(mut content: String) -> Result<Json<bool>, Custom<ConfigFileError>> { | ||
| if cfg!(target_os = "windows") { | ||
| // NOTE: this is needed due to how Rocket works with multiple segment captures. | ||
| // we may get rid of this once v1 endpoints are no longer needed. | ||
| content = content.replace("\\", "/"); | ||
| } | ||
| tracing::debug!(%content); | ||
| let config = StaticAnalysisConfigFile::try_from(Base64String(content)) | ||
| .map_err(|e| Custom(Status::InternalServerError, e))?; | ||
| let can_onboard = config.is_onboarding_allowed(); | ||
| Ok(Json(can_onboard)) | ||
| } | ||
|
|
||
| /// Converts the result to a response string, optionally encoding it in base64. | ||
| /// | ||
| /// `SchemaMismatch` is a client error and maps to `400 Bad Request`; all other | ||
| /// failures map to `500 Internal Server Error`. | ||
| /// | ||
| /// # Arguments | ||
| /// * `result` - The result string or error. | ||
| /// * `encode` - Whether to encode the result in base64. | ||
|
|
@@ -244,5 +262,11 @@ fn to_response_result( | |
| ) -> Result<String, Custom<ConfigFileError>> { | ||
| result | ||
| .map(|r| if encode { encode_base64_string(r) } else { r }) | ||
| .map_err(|e| Custom(Status::InternalServerError, e)) | ||
| .map_err(|e| { | ||
| let status = match e { | ||
| ConfigFileError::SchemaMismatch => Status::BadRequest, | ||
| _ => Status::InternalServerError, | ||
| }; | ||
| Custom(status, e) | ||
| }) | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.