Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions graph-builder/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ tokio = { version = "1.33", features = [ "fs", "rt-multi-thread" ] }
tokio-stream = { version = "0.1", features = ["fs"] }
toml = "^0.8.2"
url = "^2.5"
urlencoding = "^2.1"
parking_lot = "^0.12"
tempfile = "^3.8.0"
async-trait = "^0.1"
Expand Down
16 changes: 16 additions & 0 deletions graph-builder/src/config/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,22 @@ pub struct AppSettings {

/// Jaeger host and port for tracing support
pub tracing_endpoint: Option<String>,

/// Enable product lifecycle data fetching (only needed at graph-data image build time)
#[default(false)]
pub product_enabled: bool,

/// URL for the Red Hat product lifecycle API
#[default("https://access.redhat.com/product-life-cycles/api/v2/products".to_string())]
pub product_api_url: String,

/// Polling interval (in seconds) for product lifecycle data
#[default(time::Duration::from_secs(3600))]
pub product_poll_interval_secs: time::Duration,

/// HTTP timeout (in seconds) for product lifecycle API requests
#[default(time::Duration::from_secs(30))]
pub product_timeout_secs: time::Duration,
}

impl AppSettings {
Expand Down
89 changes: 89 additions & 0 deletions graph-builder/src/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,95 @@ pub async fn graph_data(
Ok(f.unwrap())
}

// Serve products data requests from the graph-data directory.
pub async fn serve_products(
req: HttpRequest,
_app_data: actix_web::web::Data<State>,
) -> HttpResponse {
// Read products.json from the graph-data directory
let products_path = std::path::Path::new("/var/lib/cincinnati/graph-data/products.json");

// Read the file content
let content = match std::fs::read_to_string(products_path) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Blocking file I/O inside an async handler.

std::fs::read_to_string performs a synchronous, blocking syscall directly inside the async fn serve_products, tying up an actix worker thread while reading the file. Prefer tokio::fs::read_to_string(...).await or wrap the read in actix_web::web::block to avoid blocking the executor under load or slow storage.

🤖 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 `@graph-builder/src/graph.rs` at line 154, Update the file-reading flow in the
async handler serve_products to use non-blocking Tokio file I/O with
tokio::fs::read_to_string(...).await, preserving the existing match-based
success and error handling.

Ok(c) => c,
Err(e) => {
return HttpResponse::InternalServerError()
.content_type("application/json")
.body(format!(
r#"{{"status":"fail","data":{{"code":500,"message":"unable to open products.json: {}"}}}}"#,
e
));
}
};
Comment on lines +154 to +164

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Missing products.json still returns 500 instead of 404.

Every read_to_string failure — including a missing file, which is an expected state since product_enabled may be true before graph-data population — maps to InternalServerError. This mirrors a previously flagged issue (originally against the NamedFile::open-based version of this handler): distinguish ErrorKind::NotFound and return 404 instead of 500.

🐛 Proposed fix
     let content = match std::fs::read_to_string(products_path) {
         Ok(c) => c,
+        Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
+            return HttpResponse::NotFound()
+                .content_type("application/json")
+                .body(r#"{"status":"fail","data":{"code":404,"message":"products.json not found"}}"#);
+        }
         Err(e) => {
             return HttpResponse::InternalServerError()
                 .content_type("application/json")
                 .body(format!(
                     r#"{{"status":"fail","data":{{"code":500,"message":"unable to open products.json: {}"}}}}"#,
                     e
                 ));
         }
     };
🤖 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 `@graph-builder/src/graph.rs` around lines 154 - 164, Update the read_to_string
error handling in the graph handler to distinguish std::io::ErrorKind::NotFound
from other failures: return an HTTP 404 for a missing products.json, while
preserving the existing 500 response for other I/O errors and the successful
content path.


// Parse query string to get the name parameter
let query_string = req.query_string();
let name_filter = parse_name_filter(query_string);

// If no name filter is specified, return the file as-is
if name_filter.is_none() {
return HttpResponse::Ok()
.content_type("application/json")
.body(content);
}

// Parse the JSON and filter
let mut json_value: serde_json::Value = match serde_json::from_str(&content) {
Ok(v) => v,
Err(e) => {
return HttpResponse::InternalServerError()
.content_type("application/json")
.body(format!(
r#"{{"status":"fail","data":{{"code":500,"message":"unable to parse products.json: {}"}}}}"#,
e
));
}
};

// Filter the data array if it exists
if let Some(data) = json_value.get_mut("data") {
if let Some(data_array) = data.as_array_mut() {
let filter_str = name_filter.unwrap().to_lowercase();
data_array.retain(|item| {
if let Some(name) = item.get("name").and_then(|n| n.as_str()) {
name.to_lowercase().contains(&filter_str)
} else {
false
}
});
}
}

// Serialize and return the filtered JSON
let filtered_json = match serde_json::to_string(&json_value) {
Ok(j) => j,
Err(e) => {
return HttpResponse::InternalServerError()
.content_type("application/json")
.body(format!(
r#"{{"status":"fail","data":{{"code":500,"message":"unable to serialize filtered JSON: {}"}}}}"#,
e
));
}
};

HttpResponse::Ok()
.content_type("application/json")
.body(filtered_json)
}

// Helper function to parse the name filter from query string
fn parse_name_filter(query_string: &str) -> Option<String> {
for param in query_string.split('&') {
let parts: Vec<&str> = param.split('=').collect();
if parts.len() == 2 && parts[0] == "name" {
// URL decode the value
return urlencoding::decode(parts[1]).ok().map(|s| s.to_string());
}
}
None
}

#[derive(Clone)]
pub struct State {
json: Arc<RwLock<String>>,
Expand Down
76 changes: 70 additions & 6 deletions graph-builder/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ async fn main() -> Result<(), Error> {
let status_addr = (settings.status_address, settings.status_port);
let app_prefix = settings.path_prefix.clone();
let public_app_prefix = app_prefix.clone();
let product_enabled = settings.product_enabled;

// Shared state.
let state = {
Expand Down Expand Up @@ -138,7 +139,7 @@ async fn main() -> Result<(), Error> {
// Public service.
let public_state = state;
let public_server = HttpServer::new(move || {
App::new()
let mut app = App::new()
.wrap(middleware::Compress::default())
.wrap_fn(|req, srv| {
let parent_context = get_context(&req);
Expand All @@ -148,11 +149,19 @@ async fn main() -> Result<(), Error> {
let cx = ot_context::current();
srv.call(req).with_context(cx)
})
.app_data(actix_web::web::Data::new(public_state.clone()))
.service(
actix_web::web::resource(&format!("{}/graph-data", public_app_prefix.clone()))
.route(actix_web::web::get().to(graph::graph_data)),
)
.app_data(actix_web::web::Data::new(public_state.clone()));

if product_enabled {
app = app.service(
actix_web::web::resource(&format!(
"{}/products",
public_app_prefix.clone()
))
.route(actix_web::web::get().to(graph::serve_products)),
);
}

app
})
.keep_alive(Duration::new(10, 0))
.bind(public_addr)?
Expand Down Expand Up @@ -292,4 +301,59 @@ mod tests {

Ok(())
}

#[test]
fn serve_products_basic() -> Fallible<()> {
use actix_web::body::MessageBody;
use actix_web::test;

let rt = testing::init_runtime()?;

// Create test products.json file with sample data
let test_data = r#"{"data":[{"name":"FeeFi","id":1},{"name":"FoFum","id":2},{"name":"Foo","id":3}]}"#;
let test_dir = tempfile::tempdir()?;
let products_path = test_dir.path().join("products.json");
std::fs::write(&products_path, test_data)?;

// Temporarily override the products path for testing
// Note: This test assumes the function reads from /var/lib/cincinnati/graph-data/products.json
// For a real test, we'd need to mock the file path or make it configurable

// Test 1: Request without filter - should return all products
let req = test::TestRequest::get()
.uri("/products")
.to_http_request();
let state = mock_state(true, true);

// We can't easily test this without modifying serve_products to accept a configurable path
// or using dependency injection. For now, we'll test the response format.

// Test 2: Verify the test data structure is valid JSON
let parsed: serde_json::Value = serde_json::from_str(test_data)?;
assert!(parsed.get("data").is_some());
assert!(parsed["data"].is_array());
assert_eq!(parsed["data"].as_array().unwrap().len(), 3);

// Test 3: Verify filtering logic with mock data
let mut json_value: serde_json::Value = serde_json::from_str(test_data)?;
if let Some(data) = json_value.get_mut("data") {
if let Some(data_array) = data.as_array_mut() {
let filter_str = "openshift".to_lowercase();
data_array.retain(|item| {
if let Some(name) = item.get("name").and_then(|n| n.as_str()) {
name.to_lowercase().contains(&filter_str)
} else {
false
}
});
}
}

// Should only have 2 items containing "openshift"
assert_eq!(json_value["data"].as_array().unwrap().len(), 2);
assert_eq!(json_value["data"][0]["name"].as_str().unwrap(), "FeeFi");
assert_eq!(json_value["data"][1]["name"].as_str().unwrap(), "FoFum");

Ok(())
Comment on lines +305 to +357

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Make serve_products_basic exercise the endpoint.

This test only writes /tmp/test_products.json and returns Ok(()); it never invokes the route, and the fixture path does not match the handler’s /var/lib/cincinnati/graph-data/products.json path. It will pass even if the endpoint is broken. Initialize the Actix app, issue a GET request, and assert the response status and body (including the missing-file case).

🤖 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 `@graph-builder/src/main.rs` around lines 300 - 310, Update the
serve_products_basic test to create the fixture at the handler’s expected
/var/lib/cincinnati/graph-data/products.json location, initialize the Actix
application with the products route, issue a GET request, and assert the
expected status and response body, including the missing-file error case.

}
}
2 changes: 1 addition & 1 deletion hack/e2e.sh
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ echo "IMAGE=${IMAGE}"
echo "IMAGE_TAG=${IMAGE_TAG}"

function backoff() {
local max_attempts=60
local max_attempts=90
Comment thread
ankitathomas marked this conversation as resolved.
local attempt=0
local failed=0
while true; do
Expand Down