fix: correctly serialize RFC3339 datetimes in EI API types - #2565
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR updates Exploit Intelligence API models and tests so OffsetDateTime fields are serialized as RFC3339 strings, aligning the implementation with the OpenAPI spec and adding coverage to ensure the returned JSON uses the expected string format. Sequence diagram for RFC3339 serialization in Exploit Intelligence API responsessequenceDiagram
actor Client
participant ExploitIntelligenceEndpoint as ExploitIntelligenceEndpoint
participant ExploitIntelligenceJobSummary as ExploitIntelligenceJobSummary
participant ExploitIntelligenceJobDetails as ExploitIntelligenceJobDetails
participant ComponentResult as ComponentResult
participant time_serde_rfc3339 as time::serde::rfc3339
participant JsonResponse as JsonResponse
Client->>ExploitIntelligenceEndpoint: GET /exploit-intelligence/jobs
ExploitIntelligenceEndpoint->>ExploitIntelligenceJobSummary: build job summaries
ExploitIntelligenceEndpoint->>ExploitIntelligenceJobDetails: build job details
ExploitIntelligenceEndpoint->>ComponentResult: build component results
ExploitIntelligenceJobSummary->>time_serde_rfc3339: serialize created OffsetDateTime
ExploitIntelligenceJobSummary->>time_serde_rfc3339: serialize updated OffsetDateTime
ExploitIntelligenceJobDetails->>time_serde_rfc3339: serialize created OffsetDateTime
ExploitIntelligenceJobDetails->>time_serde_rfc3339: serialize updated OffsetDateTime
ComponentResult->>time_serde_rfc3339: serialize created OffsetDateTime
ComponentResult->>time_serde_rfc3339: serialize updated OffsetDateTime
time_serde_rfc3339-->>JsonResponse: RFC3339 datetime strings
JsonResponse-->>Client: JSON with RFC3339 created and updated fields
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the updated test,
body["created"].as_str().unwrap()andbody["updated"].as_str().unwrap()can panic if the fields are missing or not strings; consider usingexpectwith a clear message or checkingis_string()before parsing to make failures easier to diagnose. - Instead of comparing
OffsetDateTime::parse(...).err() == None, usingOffsetDateTime::parse(..., &Rfc3339).is_ok()would be more idiomatic and directly express the intent of validating RFC3339 formatting.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the updated test, `body["created"].as_str().unwrap()` and `body["updated"].as_str().unwrap()` can panic if the fields are missing or not strings; consider using `expect` with a clear message or checking `is_string()` before parsing to make failures easier to diagnose.
- Instead of comparing `OffsetDateTime::parse(...).err() == None`, using `OffsetDateTime::parse(..., &Rfc3339).is_ok()` would be more idiomatic and directly express the intent of validating RFC3339 formatting.
## Individual Comments
### Comment 1
<location path="modules/exploit-intelligence/src/endpoints/test.rs" line_range="156-161" />
<code_context>
+ assert_eq!(body["status"], "completed");
+ assert_eq!(body["vulnerability_id"], "CVE-2024-1234");
+ assert_eq!(body["finding"], "not_vulnerable");
+ assert_eq!(
+ OffsetDateTime::parse(body["created"].as_str().unwrap(), &Rfc3339).err(),
+ None
+ );
assert_eq!(
- body.finding,
- Some(ExploitIntelligenceFinding::NotVulnerable)
+ OffsetDateTime::parse(body["updated"].as_str().unwrap(), &Rfc3339).err(),
+ None
);
</code_context>
<issue_to_address>
**nitpick:** Use `is_ok()` (or similar) instead of checking `err() == None` for clearer intent in datetime parsing assertions.
The `created` and `updated` assertions currently use `OffsetDateTime::parse(...).err() == None`, which hides the intent and produces less clear failure output (an `Option` instead of a parse `Result`). Please switch to `assert!(OffsetDateTime::parse(..., &Rfc3339).is_ok())` (optionally with a custom message) so the tests clearly assert successful parsing and produce more readable failures.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
rh-jfuller
left a comment
There was a problem hiding this comment.
you might have to run >cargo xtask precommit to refresh openapi defs ... otherwise LGTM
This was bringing the implementation in-line with what openapi was declaring. CI would catch precommit yielding a diff though, so we're all good there |
|
Successfully created backport PR for |
OpenAPI spec currently states that these date fields should be strings, but the implementation didnt match. Not currently used anywhere so the issue wasn't caught.
Fixes TC-5538
Summary by Sourcery
Ensure exploit intelligence API timestamps are serialized as RFC3339 strings and update tests accordingly.
Bug Fixes:
Tests: