From 56c6045e3bf7af08ca8cda6acff6a7d61fa28923 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sat, 15 Nov 2025 17:16:55 -0800 Subject: [PATCH] Use ngx_log_error! macro and fix formatting - Switched from ngx_log_error_core() to ngx_log_error! macro - Changed error_log level from debug to info (cleaner output) - Formatting cleanup across all modules (cargo fmt) - Removed trailing newlines and fixed indentation Logging now properly uses nginx's macro system for better integration with nginx's log handling. --- build.rs | 1 - conf/sqlite_serve.conf | 2 +- src/adapters.rs | 16 ++++---- src/config.rs | 1 - src/domain.rs | 36 +++++++----------- src/handler_types.rs | 86 ++++++++++++++++++++++-------------------- src/lib.rs | 12 +++--- src/logging.rs | 27 +++++-------- src/nginx_helpers.rs | 11 ++++-- src/parsing.rs | 16 ++++---- src/query.rs | 40 +++++++++----------- src/template.rs | 1 - src/types.rs | 51 ++++++++++++++----------- src/variable.rs | 5 +-- 14 files changed, 146 insertions(+), 159 deletions(-) diff --git a/build.rs b/build.rs index e70ba6d..08cd374 100644 --- a/build.rs +++ b/build.rs @@ -7,4 +7,3 @@ fn main() { println!("cargo:rustc-link-arg=dynamic_lookup"); } } - diff --git a/conf/sqlite_serve.conf b/conf/sqlite_serve.conf index c992411..5ba9784 100644 --- a/conf/sqlite_serve.conf +++ b/conf/sqlite_serve.conf @@ -8,7 +8,7 @@ events { worker_connections 1024; } -error_log logs/error.log debug; +error_log logs/error.log info; http { # Global templates for shared components diff --git a/src/adapters.rs b/src/adapters.rs index 7639107..7aff835 100644 --- a/src/adapters.rs +++ b/src/adapters.rs @@ -41,8 +41,7 @@ impl QueryExecutor for SqliteQueryExecutor { query: &SqlQuery, params: &[(String, String)], ) -> Result>, String> { - query::execute_query(db_path.as_str(), query.as_str(), params) - .map_err(|e| e.to_string()) + query::execute_query(db_path.as_str(), query.as_str(), params).map_err(|e| e.to_string()) } } @@ -54,7 +53,7 @@ pub struct HandlebarsAdapter { impl HandlebarsAdapter { /// Create adapter from mutable handlebars registry - /// + /// /// # Safety /// Caller must ensure the registry outlives this adapter pub unsafe fn new(registry: *mut Handlebars<'static>) -> Self { @@ -81,7 +80,11 @@ impl TemplateLoader for HandlebarsAdapter { impl TemplateRenderer for HandlebarsAdapter { fn render(&self, template_name: &str, data: &Value) -> Result { - unsafe { (*self.registry).render(template_name, data).map_err(|e| e.to_string()) } + unsafe { + (*self.registry) + .render(template_name, data) + .map_err(|e| e.to_string()) + } } } @@ -136,9 +139,7 @@ mod tests { let reg_ptr: *mut Handlebars<'static> = unsafe { std::mem::transmute(&mut reg) }; let adapter = unsafe { HandlebarsAdapter::new(reg_ptr) }; - adapter - .register_template("test", &template_path) - .unwrap(); + adapter.register_template("test", &template_path).unwrap(); let data = serde_json::json!({"name": "World"}); let rendered = adapter.render("test", &data).unwrap(); @@ -148,4 +149,3 @@ mod tests { let _ = fs::remove_dir_all(temp_dir); } } - diff --git a/src/config.rs b/src/config.rs index 7eeaae9..553b66f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -130,4 +130,3 @@ mod tests { assert_eq!(config.global_templates_dir, "templates/global"); } } - diff --git a/src/domain.rs b/src/domain.rs index fc6f238..6f627f6 100644 --- a/src/domain.rs +++ b/src/domain.rs @@ -25,7 +25,7 @@ impl ResolvedTemplate { pub fn full_path(&self) -> &str { &self.full_path } - + pub fn directory(&self) -> &str { &self.directory } @@ -231,11 +231,9 @@ mod tests { #[test] fn test_resolve_parameters_positional() { - let bindings = vec![ - ParameterBinding::Positional { - variable: NginxVariable::parse("$arg_id").unwrap(), - }, - ]; + let bindings = vec![ParameterBinding::Positional { + variable: NginxVariable::parse("$arg_id").unwrap(), + }]; let resolver = MockVariableResolver; let resolved = resolve_parameters(&bindings, &resolver).unwrap(); @@ -247,12 +245,10 @@ mod tests { #[test] fn test_resolve_parameters_named() { - let bindings = vec![ - ParameterBinding::Named { - name: ParamName::parse(":book_id").unwrap(), - variable: NginxVariable::parse("$arg_id").unwrap(), - }, - ]; + let bindings = vec![ParameterBinding::Named { + name: ParamName::parse(":book_id").unwrap(), + variable: NginxVariable::parse("$arg_id").unwrap(), + }]; let resolver = MockVariableResolver; let resolved = resolve_parameters(&bindings, &resolver).unwrap(); @@ -264,11 +260,9 @@ mod tests { #[test] fn test_resolve_parameters_literal() { - let bindings = vec![ - ParameterBinding::PositionalLiteral { - value: "constant".to_string(), - }, - ]; + let bindings = vec![ParameterBinding::PositionalLiteral { + value: "constant".to_string(), + }]; let resolver = MockVariableResolver; let resolved = resolve_parameters(&bindings, &resolver).unwrap(); @@ -291,11 +285,8 @@ mod tests { directory: "templates".to_string(), }; - let processor = RequestProcessor::new( - MockQueryExecutor, - MockTemplateLoader, - MockTemplateRenderer, - ); + let processor = + RequestProcessor::new(MockQueryExecutor, MockTemplateLoader, MockTemplateRenderer); let result = processor.process(&config, &resolved_template, &[], None); @@ -303,4 +294,3 @@ mod tests { assert!(result.unwrap().contains("Rendered")); } } - diff --git a/src/handler_types.rs b/src/handler_types.rs index e7d2a2d..7824377 100644 --- a/src/handler_types.rs +++ b/src/handler_types.rs @@ -6,7 +6,7 @@ use crate::domain::{RequestProcessor, ValidatedConfig}; use crate::logging; use crate::nginx_helpers::{get_doc_root_and_uri, send_response}; use crate::parsing; -use crate::{domain, Module}; +use crate::{Module, domain}; use ngx::core::Status; use ngx::http::{HttpModuleLocationConf, HttpModuleMainConf}; @@ -31,15 +31,15 @@ impl<'a> ValidConfigToken<'a> { /// Process a request with guaranteed valid configuration /// Returns Status directly - no Result needed, types prove correctness -pub fn process_request( - request: &mut ngx::http::Request, - config: ValidConfigToken, -) -> Status { +pub fn process_request(request: &mut ngx::http::Request, config: ValidConfigToken) -> Status { logging::log( request, logging::LogLevel::Debug, "handler", - &format!("Processing request for {}", request.unparsed_uri().to_str().unwrap_or("unknown")), + &format!( + "Processing request for {}", + request.unparsed_uri().to_str().unwrap_or("unknown") + ), ); // Parse config into validated types @@ -55,13 +55,19 @@ pub fn process_request( let (doc_root, uri) = match get_doc_root_and_uri(request) { Ok(paths) => paths, Err(e) => { - logging::log(request, logging::LogLevel::Error, "nginx", &format!("Path resolution failed: {}", e)); + logging::log( + request, + logging::LogLevel::Error, + "nginx", + &format!("Path resolution failed: {}", e), + ); return ngx::http::HTTPStatus::INTERNAL_SERVER_ERROR.into(); } }; // Resolve template path (pure function - cannot fail) - let resolved_template = domain::resolve_template_path(&doc_root, &uri, &validated_config.template_path); + let resolved_template = + domain::resolve_template_path(&doc_root, &uri, &validated_config.template_path); logging::log( request, @@ -72,27 +78,33 @@ pub fn process_request( // Resolve parameters let var_resolver = NginxVariableResolver::new(request); - let resolved_params = match domain::resolve_parameters(&validated_config.parameters, &var_resolver) { - Ok(params) => { - if !params.is_empty() { - logging::log( - request, - logging::LogLevel::Debug, - "params", - &format!("Resolved {} parameters", params.len()), - ); + let resolved_params = + match domain::resolve_parameters(&validated_config.parameters, &var_resolver) { + Ok(params) => { + if !params.is_empty() { + logging::log( + request, + logging::LogLevel::Debug, + "params", + &format!("Resolved {} parameters", params.len()), + ); + } + params } - params - } - Err(e) => { - logging::log_param_error(request, "variable", &e); - return ngx::http::HTTPStatus::BAD_REQUEST.into(); - } - }; + Err(e) => { + logging::log_param_error(request, "variable", &e); + return ngx::http::HTTPStatus::BAD_REQUEST.into(); + } + }; // Execute and render - let html = execute_with_processor(&validated_config, &resolved_template, &resolved_params, request); - + let html = execute_with_processor( + &validated_config, + &resolved_template, + &resolved_params, + request, + ); + // Send response send_response(request, &html) } @@ -107,21 +119,12 @@ fn execute_with_processor( let mut reg = handlebars::Handlebars::new(); let reg_ptr: *mut handlebars::Handlebars<'static> = unsafe { std::mem::transmute(&mut reg) }; let hbs_adapter = unsafe { HandlebarsAdapter::new(reg_ptr) }; - - let processor = RequestProcessor::new( - SqliteQueryExecutor, - hbs_adapter, - hbs_adapter, - ); + + let processor = RequestProcessor::new(SqliteQueryExecutor, hbs_adapter, hbs_adapter); let main_conf = Module::main_conf(request).expect("main config is none"); let global_dir = if !main_conf.global_templates_dir.is_empty() { - logging::log_template_loading( - request, - "global", - 0, - &main_conf.global_templates_dir, - ); + logging::log_template_loading(request, "global", 0, &main_conf.global_templates_dir); Some(main_conf.global_templates_dir.as_str()) } else { None @@ -137,7 +140,11 @@ fn execute_with_processor( "success", &format!( "Rendered {} with {} params", - resolved_template.full_path().split('/').last().unwrap_or("template"), + resolved_template + .full_path() + .split('/') + .last() + .unwrap_or("template"), resolved_params.len() ), ); @@ -235,4 +242,3 @@ mod tests { assert!(token.is_none()); } } - diff --git a/src/lib.rs b/src/lib.rs index c58bcb8..9508dba 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,11 +13,11 @@ mod types; mod variable; use config::{MainConfig, ModuleConfig}; -use handler_types::{process_request, ValidConfigToken}; +use handler_types::{ValidConfigToken, process_request}; use ngx::ffi::{ - NGX_CONF_TAKE1, NGX_CONF_TAKE2, NGX_HTTP_LOC_CONF, NGX_HTTP_MAIN_CONF, NGX_HTTP_MODULE, - NGX_HTTP_LOC_CONF_OFFSET, NGX_RS_MODULE_SIGNATURE, nginx_version, ngx_command_t, ngx_conf_t, - ngx_http_module_t, ngx_int_t, ngx_module_t, ngx_str_t, ngx_uint_t, + NGX_CONF_TAKE1, NGX_CONF_TAKE2, NGX_HTTP_LOC_CONF, NGX_HTTP_LOC_CONF_OFFSET, + NGX_HTTP_MAIN_CONF, NGX_HTTP_MODULE, NGX_RS_MODULE_SIGNATURE, nginx_version, ngx_command_t, + ngx_conf_t, ngx_http_module_t, ngx_int_t, ngx_module_t, ngx_str_t, ngx_uint_t, }; use ngx::http::{HttpModule, HttpModuleLocationConf, HttpModuleMainConf, NgxHttpCoreModule}; use ngx::{core::Status, http, http_request_handler, ngx_modules, ngx_string}; @@ -209,8 +209,8 @@ extern "C" fn ngx_http_howto_commands_set_template_path( conf.template_path = (*args.add(1)).to_string(); // Set the content handler for this location - let clcf = NgxHttpCoreModule::location_conf_mut(&*cf) - .expect("failed to get core location conf"); + let clcf = + NgxHttpCoreModule::location_conf_mut(&*cf).expect("failed to get core location conf"); clcf.handler = Some(howto_access_handler); }; diff --git a/src/logging.rs b/src/logging.rs index 0727dce..1ddb77e 100644 --- a/src/logging.rs +++ b/src/logging.rs @@ -15,29 +15,21 @@ pub enum LogLevel { /// Log a message with context using nginx's native logging pub fn log(request: &mut Request, level: LogLevel, module: &str, message: &str) { let r: *mut ngx::ffi::ngx_http_request_t = request.into(); - + let log_level = match level { - LogLevel::Error => 3, // NGX_LOG_ERR - LogLevel::Warn => 4, // NGX_LOG_WARN - LogLevel::Info => 6, // NGX_LOG_INFO - LogLevel::Debug => 7, // NGX_LOG_DEBUG + LogLevel::Error => 3, // NGX_LOG_ERR + LogLevel::Warn => 4, // NGX_LOG_WARN + LogLevel::Info => 6, // NGX_LOG_INFO + LogLevel::Debug => 7, // NGX_LOG_DEBUG }; + let r: *mut ngx::ffi::ngx_http_request_t = request.into(); unsafe { let connection = (*r).connection; if !connection.is_null() { - let log_ptr = (*connection).log; - if !log_ptr.is_null() { - // ngx_log_error! requires a string literal, so we use the C API directly - let c_msg = std::ffi::CString::new(format!("[sqlite-serve:{}] {}", module, message)) - .unwrap_or_else(|_| std::ffi::CString::new("log message error").unwrap()); - - ngx::ffi::ngx_log_error_core( - log_level as ngx::ffi::ngx_uint_t, - log_ptr, - 0, - c_msg.as_ptr(), - ); + let log = (*connection).log; + if !log.is_null() { + ngx_log_error!(log_level, log, "[sqlite-serve:{}] {}", module, message); } } } @@ -128,4 +120,3 @@ mod tests { assert_eq!(levels.len(), 4); } } - diff --git a/src/nginx_helpers.rs b/src/nginx_helpers.rs index 60ffd1a..cf0984d 100644 --- a/src/nginx_helpers.rs +++ b/src/nginx_helpers.rs @@ -28,7 +28,6 @@ pub fn get_doc_root_and_uri(request: &mut Request) -> Result<(String, String), S /// Create and send nginx response buffer pub fn send_response(request: &mut Request, body: &str) -> Status { - // Create output buffer let mut buf = match request.pool().create_buffer_from_str(body) { Some(buf) => buf, @@ -45,7 +44,7 @@ pub fn send_response(request: &mut Request, body: &str) -> Status { request.discard_request_body(); request.set_status(http::HTTPStatus::OK); - + let rc = request.send_header(); if rc == Status::NGX_ERROR || rc > Status::NGX_OK || request.header_only() { return rc; @@ -57,8 +56,12 @@ pub fn send_response(request: &mut Request, body: &str) -> Status { /// Log and return error status (deprecated - use logging module directly) #[allow(dead_code)] -pub fn log_error(request: &mut Request, context: &str, error: &str, status: http::HTTPStatus) -> Status { +pub fn log_error( + request: &mut Request, + context: &str, + error: &str, + status: http::HTTPStatus, +) -> Status { logging::log(request, logging::LogLevel::Error, context, error); status.into() } - diff --git a/src/parsing.rs b/src/parsing.rs index 11fc9f3..59f4d1f 100644 --- a/src/parsing.rs +++ b/src/parsing.rs @@ -2,15 +2,16 @@ use crate::config::ModuleConfig; use crate::domain::ValidatedConfig; -use crate::types::{DatabasePath, NginxVariable, ParamName, ParameterBinding, SqlQuery, TemplatePath}; +use crate::types::{ + DatabasePath, NginxVariable, ParamName, ParameterBinding, SqlQuery, TemplatePath, +}; /// Parse raw configuration into validated domain configuration pub fn parse_config(config: &ModuleConfig) -> Result { - let db_path = DatabasePath::parse(&config.db_path) - .map_err(|e| format!("invalid db_path: {}", e))?; + let db_path = + DatabasePath::parse(&config.db_path).map_err(|e| format!("invalid db_path: {}", e))?; - let query = SqlQuery::parse(&config.query) - .map_err(|e| format!("invalid query: {}", e))?; + let query = SqlQuery::parse(&config.query).map_err(|e| format!("invalid query: {}", e))?; let template_path = TemplatePath::parse(&config.template_path) .map_err(|e| format!("invalid template_path: {}", e))?; @@ -26,9 +27,7 @@ pub fn parse_config(config: &ModuleConfig) -> Result { } /// Parse parameter configuration into typed bindings -fn parse_parameter_bindings( - params: &[(String, String)], -) -> Result, String> { +fn parse_parameter_bindings(params: &[(String, String)]) -> Result, String> { let mut bindings = Vec::new(); for (param_name, var_name) in params { @@ -169,4 +168,3 @@ mod tests { } } } - diff --git a/src/query.rs b/src/query.rs index 4c63d02..3d94806 100644 --- a/src/query.rs +++ b/src/query.rs @@ -31,11 +31,9 @@ pub fn execute_query( let value: Value = match row.get_ref(i)? { rusqlite::types::ValueRef::Null => Value::Null, rusqlite::types::ValueRef::Integer(v) => Value::Number(v.into()), - rusqlite::types::ValueRef::Real(v) => { - serde_json::Number::from_f64(v) - .map(Value::Number) - .unwrap_or(Value::Null) - } + rusqlite::types::ValueRef::Real(v) => serde_json::Number::from_f64(v) + .map(Value::Number) + .unwrap_or(Value::Null), rusqlite::types::ValueRef::Text(v) => { Value::String(String::from_utf8_lossy(v).to_string()) } @@ -90,11 +88,8 @@ mod tests { { let conn = Connection::open(temp_path).unwrap(); - conn.execute( - "CREATE TABLE test (id INTEGER, name TEXT, value REAL)", - [], - ) - .unwrap(); + conn.execute("CREATE TABLE test (id INTEGER, name TEXT, value REAL)", []) + .unwrap(); conn.execute( "INSERT INTO test VALUES (1, 'first', 1.5), (2, 'second', 2.5)", [], @@ -104,10 +99,7 @@ mod tests { let results = execute_query(temp_path, "SELECT * FROM test ORDER BY id", &[]).unwrap(); assert_eq!(results.len(), 2); - assert_eq!( - results[0].get("id").unwrap(), - &Value::Number(1.into()) - ); + assert_eq!(results[0].get("id").unwrap(), &Value::Number(1.into())); assert_eq!( results[0].get("name").unwrap(), &Value::String("first".to_string()) @@ -157,8 +149,11 @@ mod tests { { let conn = Connection::open(temp_path).unwrap(); - conn.execute("CREATE TABLE books (id INTEGER, title TEXT, year INTEGER)", []) - .unwrap(); + conn.execute( + "CREATE TABLE books (id INTEGER, title TEXT, year INTEGER)", + [], + ) + .unwrap(); conn.execute( "INSERT INTO books VALUES (1, 'Old Book', 2000), (2, 'New Book', 2020), (3, 'Newer Book', 2023)", [], @@ -218,10 +213,7 @@ mod tests { let row = &results[0]; assert_eq!(row.get("id").unwrap(), &Value::Number(42.into())); - assert_eq!( - row.get("name").unwrap(), - &Value::String("test".to_string()) - ); + assert_eq!(row.get("name").unwrap(), &Value::String("test".to_string())); assert_eq!(row.get("price").unwrap().as_f64().unwrap(), 3.14); assert_eq!( row.get("data").unwrap(), @@ -242,8 +234,11 @@ mod tests { { let conn = Connection::open(temp_path).unwrap(); - conn.execute("CREATE TABLE books (id INTEGER, genre TEXT, rating REAL)", []) - .unwrap(); + conn.execute( + "CREATE TABLE books (id INTEGER, genre TEXT, rating REAL)", + [], + ) + .unwrap(); conn.execute( "INSERT INTO books VALUES (1, 'Fiction', 4.5), @@ -324,4 +319,3 @@ mod tests { let _ = fs::remove_file(temp_path); } } - diff --git a/src/template.rs b/src/template.rs index 3636ea2..bf46ad9 100644 --- a/src/template.rs +++ b/src/template.rs @@ -157,4 +157,3 @@ mod tests { let _ = fs::remove_dir_all(temp_dir); } } - diff --git a/src/types.rs b/src/types.rs index 7cdd772..79bea91 100644 --- a/src/types.rs +++ b/src/types.rs @@ -36,16 +36,16 @@ impl SqlQuery { pub fn parse(query: impl Into) -> Result { let query = query.into(); let trimmed = query.trim().to_uppercase(); - + if trimmed.is_empty() { return Err("query cannot be empty".to_string()); } - + // Ensure it's a SELECT query (read-only) if !trimmed.starts_with("SELECT") { return Err("only SELECT queries are allowed".to_string()); } - + Ok(SqlQuery(query)) } @@ -62,16 +62,16 @@ impl TemplatePath { /// Parse and validate a template path pub fn parse(path: impl AsRef) -> Result { let path = path.as_ref(); - + if path.as_os_str().is_empty() { return Err("template path cannot be empty".to_string()); } - + // Ensure it's a .hbs file if path.extension().and_then(|e| e.to_str()) != Some("hbs") { return Err("template must be a .hbs file".to_string()); } - + Ok(TemplatePath(path.to_path_buf())) } @@ -92,28 +92,28 @@ impl NginxVariable { /// Parse a nginx variable name pub fn parse(name: impl Into) -> Result { let name = name.into(); - + if name.is_empty() { return Err("variable name cannot be empty".to_string()); } - + if !name.starts_with('$') { return Err(format!("variable name must start with $: {}", name)); } - + // Get the part after $ let var_name = &name[1..]; if var_name.is_empty() { return Err("variable name after $ cannot be empty".to_string()); } - + Ok(NginxVariable(name)) } pub fn as_str(&self) -> &str { &self.0 } - + /// Get the variable name without the $ prefix pub fn name(&self) -> &str { &self.0[1..] @@ -128,23 +128,23 @@ impl ParamName { /// Parse a SQL parameter name pub fn parse(name: impl Into) -> Result { let name = name.into(); - + if name.is_empty() { return Err("parameter name cannot be empty".to_string()); } - + if !name.starts_with(':') { return Err(format!("parameter name must start with :: {}", name)); } - + Ok(ParamName(name)) } - + /// Create an empty (positional) parameter name pub fn positional() -> Self { ParamName(String::new()) } - + pub fn is_positional(&self) -> bool { self.0.is_empty() } @@ -157,10 +157,20 @@ impl ParamName { /// A parameter binding (param name + variable or literal) #[derive(Debug, Clone)] pub enum ParameterBinding { - Positional { variable: NginxVariable }, - PositionalLiteral { value: String }, - Named { name: ParamName, variable: NginxVariable }, - NamedLiteral { name: ParamName, value: String }, + Positional { + variable: NginxVariable, + }, + PositionalLiteral { + value: String, + }, + Named { + name: ParamName, + variable: NginxVariable, + }, + NamedLiteral { + name: ParamName, + value: String, + }, } impl ParameterBinding { @@ -300,4 +310,3 @@ mod tests { assert!(result.unwrap_err().contains(":")); } } - diff --git a/src/variable.rs b/src/variable.rs index d7766e6..8fb0140 100644 --- a/src/variable.rs +++ b/src/variable.rs @@ -86,8 +86,8 @@ mod tests { fn test_named_params_parsing() { // Test parameter name parsing logic let test_cases = vec![ - (2, false, ""), // sqlite_param $arg_id - (3, true, ":book_id"), // sqlite_param :book_id $arg_id + (2, false, ""), // sqlite_param $arg_id + (3, true, ":book_id"), // sqlite_param :book_id $arg_id ]; for (nelts, expected_is_named, expected_param_name) in test_cases { @@ -103,4 +103,3 @@ mod tests { } } } -