feat(firmware): allow custom version detection regexes#3420
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🤖 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 `@crates/api-core/src/handlers/firmware.rs`:
- Around line 529-556: Update parse_component_regex to trim the supplied value
before emptiness validation, length validation, and Regex compilation, so
leading or trailing whitespace does not become part of the pattern. Preserve the
existing error handling and add a regression case to
parse_component_regex_validates_supplied_values for a whitespace-padded anchored
regex.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c3096136-6435-4cdb-b13e-048b0edb4795
⛔ Files ignored due to path filters (2)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/sdk/standard/model_host_firmware_component_config.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (9)
crates/api-core/src/handlers/firmware.rscrates/api-core/src/tests/host_firmware_config.rscrates/rpc/proto/forge.protorest-api/api/pkg/api/model/hostfirmwareconfig.gorest-api/api/pkg/api/model/hostfirmwareconfig_test.gorest-api/docs/index.htmlrest-api/openapi/spec.yamlrest-api/openapitools.jsonrest-api/proto/core/src/v1/nico_nico.proto
| fn parse_component_regex( | ||
| vendor: bmc_vendor::BMCVendor, | ||
| model: &str, | ||
| component_type: FirmwareComponentType, | ||
| value: Option<&str>, | ||
| ) -> Result<Option<Regex>, CarbideError> { | ||
| value | ||
| .map(|value| { | ||
| if value.trim().is_empty() { | ||
| return Err(CarbideError::InvalidArgument(format!( | ||
| "current_version_reported_as is empty for {vendor} {model} {component_type}" | ||
| ))); | ||
| } | ||
| if value.chars().count() > MAX_CURRENT_VERSION_REPORTED_AS_LENGTH { | ||
| return Err(CarbideError::InvalidArgument(format!( | ||
| "current_version_reported_as exceeds {MAX_CURRENT_VERSION_REPORTED_AS_LENGTH} characters for {vendor} {model} {component_type}" | ||
| ))); | ||
| } | ||
|
|
||
| Regex::new(value).map_err(|error| { | ||
| CarbideError::InvalidArgument(format!( | ||
| "invalid current_version_reported_as for {vendor} {model} {component_type}: {error}" | ||
| )) | ||
| }) | ||
| }) | ||
| .transpose() | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Trim the regex value before validating/compiling it.
parse_component_regex checks emptiness via value.trim() but compiles and length-checks the untrimmed value. A value with stray leading/trailing whitespace (e.g. " ^BMC-Firmware$") passes the emptiness check yet compiles into a Regex that can never match anything, since the literal space forces a position that contradicts the ^ anchor — silently breaking version detection for that component with no error surfaced. The sibling parse_preingest_upgrade_when_below already trims consistently; this function should do the same.
🐛 Proposed fix: trim before validating/compiling
fn parse_component_regex(
vendor: bmc_vendor::BMCVendor,
model: &str,
component_type: FirmwareComponentType,
value: Option<&str>,
) -> Result<Option<Regex>, CarbideError> {
value
.map(|value| {
- if value.trim().is_empty() {
+ let trimmed = value.trim();
+ if trimmed.is_empty() {
return Err(CarbideError::InvalidArgument(format!(
"current_version_reported_as is empty for {vendor} {model} {component_type}"
)));
}
- if value.chars().count() > MAX_CURRENT_VERSION_REPORTED_AS_LENGTH {
+ if trimmed.chars().count() > MAX_CURRENT_VERSION_REPORTED_AS_LENGTH {
return Err(CarbideError::InvalidArgument(format!(
"current_version_reported_as exceeds {MAX_CURRENT_VERSION_REPORTED_AS_LENGTH} characters for {vendor} {model} {component_type}"
)));
}
- Regex::new(value).map_err(|error| {
+ Regex::new(trimmed).map_err(|error| {
CarbideError::InvalidArgument(format!(
"invalid current_version_reported_as for {vendor} {model} {component_type}: {error}"
))
})
})
.transpose()
}Consider also adding a case to parse_component_regex_validates_supplied_values for a value like " ^BMC-Firmware$ " to lock in the trimmed-compile behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn parse_component_regex( | |
| vendor: bmc_vendor::BMCVendor, | |
| model: &str, | |
| component_type: FirmwareComponentType, | |
| value: Option<&str>, | |
| ) -> Result<Option<Regex>, CarbideError> { | |
| value | |
| .map(|value| { | |
| if value.trim().is_empty() { | |
| return Err(CarbideError::InvalidArgument(format!( | |
| "current_version_reported_as is empty for {vendor} {model} {component_type}" | |
| ))); | |
| } | |
| if value.chars().count() > MAX_CURRENT_VERSION_REPORTED_AS_LENGTH { | |
| return Err(CarbideError::InvalidArgument(format!( | |
| "current_version_reported_as exceeds {MAX_CURRENT_VERSION_REPORTED_AS_LENGTH} characters for {vendor} {model} {component_type}" | |
| ))); | |
| } | |
| Regex::new(value).map_err(|error| { | |
| CarbideError::InvalidArgument(format!( | |
| "invalid current_version_reported_as for {vendor} {model} {component_type}: {error}" | |
| )) | |
| }) | |
| }) | |
| .transpose() | |
| } | |
| fn parse_component_regex( | |
| vendor: bmc_vendor::BMCVendor, | |
| model: &str, | |
| component_type: FirmwareComponentType, | |
| value: Option<&str>, | |
| ) -> Result<Option<Regex>, CarbideError> { | |
| value | |
| .map(|value| { | |
| let trimmed = value.trim(); | |
| if trimmed.is_empty() { | |
| return Err(CarbideError::InvalidArgument(format!( | |
| "current_version_reported_as is empty for {vendor} {model} {component_type}" | |
| ))); | |
| } | |
| if trimmed.chars().count() > MAX_CURRENT_VERSION_REPORTED_AS_LENGTH { | |
| return Err(CarbideError::InvalidArgument(format!( | |
| "current_version_reported_as exceeds {MAX_CURRENT_VERSION_REPORTED_AS_LENGTH} characters for {vendor} {model} {component_type}" | |
| ))); | |
| } | |
| Regex::new(trimmed).map_err(|error| { | |
| CarbideError::InvalidArgument(format!( | |
| "invalid current_version_reported_as for {vendor} {model} {component_type}: {error}" | |
| )) | |
| }) | |
| }) | |
| .transpose() | |
| } |
🤖 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 `@crates/api-core/src/handlers/firmware.rs` around lines 529 - 556, Update
parse_component_regex to trim the supplied value before emptiness validation,
length validation, and Regex compilation, so leading or trailing whitespace does
not become part of the pattern. Preserve the existing error handling and add a
regression case to parse_component_regex_validates_supplied_values for a
whitespace-padded anchored regex.
1 participant
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes