mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-04-10 13:02:59 +00:00
fix(config): Add defaults to schemas and fix config validation issues (#153)
* fix(web): Resolve font display and config API error handling issues - Fix font catalog display error where path.startsWith fails (path is object, not string) - Update save_main_config to use error_response() helper - Improve save_raw_main_config error handling consistency - Add proper error codes and traceback details to API responses * fix(web): Prevent fontCatalog redeclaration error on HTMX reload - Use window object to store global font variables - Check if script has already loaded before declaring variables - Update both window properties and local references on assignment - Fixes 'Identifier fontCatalog has already been declared' error * fix(web): Wrap fonts script in IIFE to prevent all redeclaration errors - Wrap entire script in IIFE that only runs once - Check if script already loaded before declaring variables/functions - Expose initializeFontsTab to window for re-initialization - Prevents 'Identifier has already been declared' errors on HTMX reload * fix(web): Exempt config save API endpoints from CSRF protection - Exempt save_raw_main_config, save_raw_secrets_config, and save_main_config from CSRF - These endpoints are called via fetch from JavaScript and don't include CSRF tokens - Fixes 500 error when saving config via raw JSON editor * fix(web): Exempt system action endpoint from CSRF protection - Exempt execute_system_action from CSRF - Fixes 500 error when using system action buttons (restart display, restart Pi, etc.) - These endpoints are called via HTMX and don't include CSRF tokens * fix(web): Exempt all API v3 endpoints from CSRF protection - Add before_request handler to exempt all api_v3.* endpoints - All API endpoints are programmatic (HTMX/fetch) and don't include CSRF tokens - Prevents future CSRF errors on any API endpoint - Cleaner than exempting individual endpoints * refactor(web): Remove CSRF protection for local-only application - CSRF is designed for internet-facing apps to prevent cross-site attacks - For local-only Raspberry Pi app, threat model is different - All endpoints were exempted anyway, so it wasn't protecting anything - Forms use HTMX without CSRF tokens - If exposing to internet later, can re-enable with proper token implementation * fix(web): Fix font path double-prefixing in font catalog display - Only prefix with 'assets/fonts/' if path is a bare filename - If path starts with '/' (absolute) or 'assets/' (already prefixed), use as-is - Fixes double-prefixing when get_fonts_catalog returns relative paths like 'assets/fonts/press_start.ttf' * fix(web): Remove fontsTabInitialized guard to allow re-initialization on HTMX reload - Remove fontsTabInitialized check that prevented re-initialization on HTMX content swap - The window._fontsScriptLoaded guard is sufficient to prevent function redeclaration - Allow initializeFontsTab() to run on each HTMX swap to attach listeners to new DOM elements - Fixes fonts UI breaking after HTMX reload (buttons, upload dropzone, etc. not working) * fix(api): Preserve empty strings for optional string fields in plugin config - Add _is_field_required() helper to check if fields are required in schema - Update _parse_form_value_with_schema() to preserve empty strings for optional string fields - Fixes 400 error when saving MQTT plugin config with empty username/password - Resolves validation error: 'Expected type string, got NoneType' * fix(config): Add defaults to schemas and fix None value handling - Updated merge_with_defaults to replace None values with defaults - Fixed form processing to skip empty optional fields without defaults - Added script to automatically add defaults to all plugin config schemas - Added defaults to 89 fields across 10 plugin schemas - Prevents validation errors from None values in configs Changes: - schema_manager.py: Enhanced merge_with_defaults to replace None with defaults - api_v3.py: Added _SKIP_FIELD sentinel to skip optional fields without defaults - add_defaults_to_schemas.py: Script to add sensible defaults to schemas - Plugin schemas: Added defaults for number, boolean, and array fields * fix(config): Fix save button spinner by checking HTTP status code - Fixed handleConfigSave to check xhr.status instead of event.detail.successful - With hx-swap="none", HTMX doesn't set event.detail.successful - Now properly detects successful saves (status 200-299) and stops spinner - Improved error message extraction from API responses - Also fixed handleToggleResponse for consistency --------- Co-authored-by: Chuck <chuck@example.com>
This commit is contained in:
Chuck
Submodule plugins/mqtt-notifications updated: 3bd955a329...5c23a842d8
231
scripts/add_defaults_to_schemas.py
Executable file
231
scripts/add_defaults_to_schemas.py
Executable file
@@ -0,0 +1,231 @@
|
||||
#!/usr/bin/env python3
|
||||
"""
|
||||
Script to add default values to plugin config schemas where missing.
|
||||
|
||||
This ensures that configs never start with None values, improving user experience
|
||||
and preventing validation errors.
|
||||
"""
|
||||
|
||||
import json
|
||||
import sys
|
||||
from pathlib import Path
|
||||
from typing import Any, Dict, List, Optional
|
||||
|
||||
|
||||
def get_default_for_field(prop: Dict[str, Any]) -> Any:
|
||||
"""
|
||||
Determine a sensible default value for a field based on its type and constraints.
|
||||
|
||||
Args:
|
||||
prop: Field property schema
|
||||
|
||||
Returns:
|
||||
Default value or None if no default should be added
|
||||
"""
|
||||
prop_type = prop.get('type')
|
||||
|
||||
# Handle union types (array with multiple types)
|
||||
if isinstance(prop_type, list):
|
||||
# Use the first non-null type
|
||||
prop_type = next((t for t in prop_type if t != 'null'), prop_type[0] if prop_type else 'string')
|
||||
|
||||
if prop_type == 'boolean':
|
||||
return False
|
||||
|
||||
elif prop_type == 'number':
|
||||
# For numbers, use minimum if available, or a sensible default
|
||||
minimum = prop.get('minimum')
|
||||
maximum = prop.get('maximum')
|
||||
|
||||
if minimum is not None:
|
||||
return minimum
|
||||
elif maximum is not None:
|
||||
# Use a reasonable fraction of max (like 30% or minimum 1)
|
||||
return max(1, int(maximum * 0.3))
|
||||
else:
|
||||
# No constraints, use 0
|
||||
return 0
|
||||
|
||||
elif prop_type == 'integer':
|
||||
# Similar to number
|
||||
minimum = prop.get('minimum')
|
||||
maximum = prop.get('maximum')
|
||||
|
||||
if minimum is not None:
|
||||
return minimum
|
||||
elif maximum is not None:
|
||||
return max(1, int(maximum * 0.3))
|
||||
else:
|
||||
return 0
|
||||
|
||||
elif prop_type == 'string':
|
||||
# Only add default for strings if it makes sense
|
||||
# Check if there's an enum - use first value
|
||||
enum_values = prop.get('enum')
|
||||
if enum_values:
|
||||
return enum_values[0]
|
||||
|
||||
# For optional string fields, empty string might be okay, but be cautious
|
||||
# We'll skip adding defaults for strings unless explicitly needed
|
||||
return None
|
||||
|
||||
elif prop_type == 'array':
|
||||
# Empty array as default
|
||||
return []
|
||||
|
||||
elif prop_type == 'object':
|
||||
# Empty object - but we'll handle nested objects separately
|
||||
return {}
|
||||
|
||||
return None
|
||||
|
||||
|
||||
def should_add_default(prop: Dict[str, Any], field_path: str) -> bool:
|
||||
"""
|
||||
Determine if we should add a default value to this field.
|
||||
|
||||
Args:
|
||||
prop: Field property schema
|
||||
field_path: Dot-separated path to the field
|
||||
|
||||
Returns:
|
||||
True if default should be added
|
||||
"""
|
||||
# Skip if already has a default
|
||||
if 'default' in prop:
|
||||
return False
|
||||
|
||||
# Skip secret fields (they should be user-provided)
|
||||
if prop.get('x-secret', False):
|
||||
return False
|
||||
|
||||
# Skip API keys and similar sensitive fields
|
||||
field_name = field_path.split('.')[-1].lower()
|
||||
sensitive_keywords = ['key', 'password', 'secret', 'token', 'auth', 'credential']
|
||||
if any(keyword in field_name for keyword in sensitive_keywords):
|
||||
return False
|
||||
|
||||
prop_type = prop.get('type')
|
||||
if isinstance(prop_type, list):
|
||||
prop_type = next((t for t in prop_type if t != 'null'), prop_type[0] if prop_type else None)
|
||||
|
||||
# Only add defaults for certain types
|
||||
if prop_type in ('boolean', 'number', 'integer', 'array'):
|
||||
return True
|
||||
|
||||
# For strings, only if there's an enum
|
||||
if prop_type == 'string' and 'enum' in prop:
|
||||
return True
|
||||
|
||||
return False
|
||||
|
||||
|
||||
def add_defaults_recursive(schema: Dict[str, Any], path: str = "", modified: List[str] = None) -> bool:
|
||||
"""
|
||||
Recursively add default values to schema fields.
|
||||
|
||||
Args:
|
||||
schema: Schema dictionary to modify
|
||||
path: Current path in the schema (for logging)
|
||||
modified: List to track which fields were modified
|
||||
|
||||
Returns:
|
||||
True if any modifications were made
|
||||
"""
|
||||
if modified is None:
|
||||
modified = []
|
||||
|
||||
if not isinstance(schema, dict) or 'properties' not in schema:
|
||||
return False
|
||||
|
||||
changes_made = False
|
||||
|
||||
for key, prop in schema['properties'].items():
|
||||
if not isinstance(prop, dict):
|
||||
continue
|
||||
|
||||
current_path = f"{path}.{key}" if path else key
|
||||
|
||||
# Check nested objects
|
||||
if prop.get('type') == 'object' and 'properties' in prop:
|
||||
if add_defaults_recursive(prop, current_path, modified):
|
||||
changes_made = True
|
||||
|
||||
# Add default if appropriate
|
||||
if should_add_default(prop, current_path):
|
||||
default_value = get_default_for_field(prop)
|
||||
if default_value is not None:
|
||||
prop['default'] = default_value
|
||||
modified.append(current_path)
|
||||
changes_made = True
|
||||
print(f" Added default to {current_path}: {default_value} (type: {prop.get('type')})")
|
||||
|
||||
return changes_made
|
||||
|
||||
|
||||
def process_schema_file(schema_path: Path) -> bool:
|
||||
"""
|
||||
Process a single schema file to add defaults.
|
||||
|
||||
Args:
|
||||
schema_path: Path to the schema file
|
||||
|
||||
Returns:
|
||||
True if file was modified
|
||||
"""
|
||||
print(f"\nProcessing: {schema_path}")
|
||||
|
||||
try:
|
||||
with open(schema_path, 'r', encoding='utf-8') as f:
|
||||
schema = json.load(f)
|
||||
except Exception as e:
|
||||
print(f" Error reading schema: {e}")
|
||||
return False
|
||||
|
||||
modified_fields = []
|
||||
changes_made = add_defaults_recursive(schema, modified=modified_fields)
|
||||
|
||||
if changes_made:
|
||||
# Write back with pretty formatting
|
||||
with open(schema_path, 'w', encoding='utf-8') as f:
|
||||
json.dump(schema, f, indent=2, ensure_ascii=False)
|
||||
f.write('\n') # Add trailing newline
|
||||
|
||||
print(f" ✓ Modified {len(modified_fields)} fields")
|
||||
return True
|
||||
else:
|
||||
print(f" ✓ No changes needed")
|
||||
return False
|
||||
|
||||
|
||||
def main():
|
||||
"""Main entry point."""
|
||||
project_root = Path(__file__).parent.parent
|
||||
plugins_dir = project_root / 'plugins'
|
||||
|
||||
if not plugins_dir.exists():
|
||||
print(f"Error: Plugins directory not found: {plugins_dir}")
|
||||
sys.exit(1)
|
||||
|
||||
# Find all config_schema.json files
|
||||
schema_files = list(plugins_dir.rglob('config_schema.json'))
|
||||
|
||||
if not schema_files:
|
||||
print("No config_schema.json files found")
|
||||
sys.exit(0)
|
||||
|
||||
print(f"Found {len(schema_files)} schema files")
|
||||
|
||||
modified_count = 0
|
||||
for schema_file in sorted(schema_files):
|
||||
if process_schema_file(schema_file):
|
||||
modified_count += 1
|
||||
|
||||
print(f"\n{'='*60}")
|
||||
print(f"Summary: Modified {modified_count} out of {len(schema_files)} schema files")
|
||||
print(f"{'='*60}")
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
main()
|
||||
|
||||
@@ -394,24 +394,54 @@ class SchemaManager:
|
||||
def merge_with_defaults(self, config: Dict[str, Any], defaults: Dict[str, Any]) -> Dict[str, Any]:
|
||||
"""
|
||||
Merge configuration with defaults, preserving user values.
|
||||
Also replaces None values with defaults to ensure config never has None from the start.
|
||||
|
||||
Args:
|
||||
config: User configuration
|
||||
defaults: Default values from schema
|
||||
|
||||
Returns:
|
||||
Merged configuration with defaults applied where missing
|
||||
Merged configuration with defaults applied where missing or None
|
||||
"""
|
||||
merged = defaults.copy()
|
||||
merged = copy.deepcopy(defaults)
|
||||
|
||||
def deep_merge(target: Dict[str, Any], source: Dict[str, Any]) -> None:
|
||||
"""Recursively merge source into target."""
|
||||
def deep_merge(target: Dict[str, Any], source: Dict[str, Any], default_dict: Dict[str, Any]) -> None:
|
||||
"""Recursively merge source into target, replacing None with defaults."""
|
||||
for key, value in source.items():
|
||||
if key in target and isinstance(target[key], dict) and isinstance(value, dict):
|
||||
deep_merge(target[key], value)
|
||||
else:
|
||||
target[key] = value
|
||||
default_value = default_dict.get(key)
|
||||
|
||||
deep_merge(merged, config)
|
||||
if key in target and isinstance(target[key], dict) and isinstance(value, dict):
|
||||
# Both are dicts, recursively merge
|
||||
if isinstance(default_value, dict):
|
||||
deep_merge(target[key], value, default_value)
|
||||
else:
|
||||
deep_merge(target[key], value, {})
|
||||
elif value is None and default_value is not None:
|
||||
# Value is None and we have a default, use the default
|
||||
target[key] = copy.deepcopy(default_value) if isinstance(default_value, (dict, list)) else default_value
|
||||
else:
|
||||
# Normal merge: user value takes precedence (copy if dict/list)
|
||||
if isinstance(value, (dict, list)):
|
||||
target[key] = copy.deepcopy(value)
|
||||
else:
|
||||
target[key] = value
|
||||
|
||||
deep_merge(merged, config, defaults)
|
||||
|
||||
# Final pass: replace any remaining None values at any level with defaults
|
||||
def replace_none_with_defaults(target: Dict[str, Any], default_dict: Dict[str, Any]) -> None:
|
||||
"""Recursively replace None values with defaults."""
|
||||
for key in list(target.keys()):
|
||||
value = target[key]
|
||||
default_value = default_dict.get(key)
|
||||
|
||||
if value is None and default_value is not None:
|
||||
# Replace None with default
|
||||
target[key] = copy.deepcopy(default_value) if isinstance(default_value, (dict, list)) else default_value
|
||||
elif isinstance(value, dict) and isinstance(default_value, dict):
|
||||
# Recursively process nested dicts
|
||||
replace_none_with_defaults(value, default_value)
|
||||
|
||||
replace_none_with_defaults(merged, defaults)
|
||||
return merged
|
||||
|
||||
|
||||
@@ -2914,6 +2914,43 @@ def _get_schema_property(schema, key_path):
|
||||
return None
|
||||
|
||||
|
||||
def _is_field_required(key_path, schema):
|
||||
"""
|
||||
Check if a field is required according to the schema.
|
||||
|
||||
Args:
|
||||
key_path: Dot-separated path like "mqtt.username"
|
||||
schema: The JSON schema dict
|
||||
|
||||
Returns:
|
||||
True if field is required, False otherwise
|
||||
"""
|
||||
if not schema or 'properties' not in schema:
|
||||
return False
|
||||
|
||||
parts = key_path.split('.')
|
||||
if len(parts) == 1:
|
||||
# Top-level field
|
||||
required = schema.get('required', [])
|
||||
return parts[0] in required
|
||||
else:
|
||||
# Nested field - navigate to parent object
|
||||
parent_path = '.'.join(parts[:-1])
|
||||
field_name = parts[-1]
|
||||
|
||||
# Get parent property
|
||||
parent_prop = _get_schema_property(schema, parent_path)
|
||||
if not parent_prop or 'properties' not in parent_prop:
|
||||
return False
|
||||
|
||||
# Check if field is required in parent
|
||||
required = parent_prop.get('required', [])
|
||||
return field_name in required
|
||||
|
||||
|
||||
# Sentinel object to indicate a field should be skipped (not set in config)
|
||||
_SKIP_FIELD = object()
|
||||
|
||||
def _parse_form_value_with_schema(value, key_path, schema):
|
||||
"""
|
||||
Parse a form value using schema information to determine correct type.
|
||||
@@ -2925,7 +2962,7 @@ def _parse_form_value_with_schema(value, key_path, schema):
|
||||
schema: The plugin's JSON schema
|
||||
|
||||
Returns:
|
||||
Parsed value with correct type
|
||||
Parsed value with correct type, or _SKIP_FIELD to indicate the field should not be set
|
||||
"""
|
||||
import json
|
||||
|
||||
@@ -2940,6 +2977,22 @@ def _parse_form_value_with_schema(value, key_path, schema):
|
||||
# If schema says it's an object, return empty dict instead of None
|
||||
if prop and prop.get('type') == 'object':
|
||||
return {}
|
||||
# If it's an optional string field, preserve empty string instead of None
|
||||
if prop and prop.get('type') == 'string':
|
||||
if not _is_field_required(key_path, schema):
|
||||
return "" # Return empty string for optional string fields
|
||||
# For number/integer fields, check if they have defaults or are required
|
||||
if prop:
|
||||
prop_type = prop.get('type')
|
||||
if prop_type in ('number', 'integer'):
|
||||
# If field has a default, use it
|
||||
if 'default' in prop:
|
||||
return prop['default']
|
||||
# If field is not required and has no default, skip setting it
|
||||
if not _is_field_required(key_path, schema):
|
||||
return _SKIP_FIELD
|
||||
# If field is required but empty, return None (validation will fail, which is correct)
|
||||
return None
|
||||
return None
|
||||
|
||||
# Handle string values
|
||||
@@ -3029,8 +3082,12 @@ def _set_nested_value(config, key_path, value):
|
||||
Args:
|
||||
config: The config dict to modify
|
||||
key_path: Dot-separated path (e.g., "customization.period_text.font")
|
||||
value: The value to set
|
||||
value: The value to set (or _SKIP_FIELD to skip setting)
|
||||
"""
|
||||
# Skip setting if value is the sentinel
|
||||
if value is _SKIP_FIELD:
|
||||
return
|
||||
|
||||
parts = key_path.split('.')
|
||||
current = config
|
||||
|
||||
@@ -3231,7 +3288,9 @@ def save_plugin_config():
|
||||
import logging
|
||||
logger = logging.getLogger(__name__)
|
||||
logger.debug(f"Combined indexed array field {base_path}: {values} -> {combined_value} -> {parsed_value}")
|
||||
_set_nested_value(plugin_config, base_path, parsed_value)
|
||||
# Only set if not skipped
|
||||
if parsed_value is not _SKIP_FIELD:
|
||||
_set_nested_value(plugin_config, base_path, parsed_value)
|
||||
|
||||
# Process remaining (non-indexed) fields
|
||||
# Skip any base paths that were processed as indexed arrays
|
||||
@@ -3249,8 +3308,9 @@ def save_plugin_config():
|
||||
import logging
|
||||
logger = logging.getLogger(__name__)
|
||||
logger.debug(f"Array field {key}: form value='{value}' -> parsed={parsed_value}")
|
||||
# Use helper to set nested values correctly
|
||||
_set_nested_value(plugin_config, key, parsed_value)
|
||||
# Use helper to set nested values correctly (skips if _SKIP_FIELD)
|
||||
if parsed_value is not _SKIP_FIELD:
|
||||
_set_nested_value(plugin_config, key, parsed_value)
|
||||
|
||||
# Post-process: Fix array fields that might have been incorrectly structured
|
||||
# This handles cases where array fields are stored as dicts (e.g., from indexed form fields)
|
||||
|
||||
@@ -4209,13 +4209,28 @@
|
||||
const btn = event.target.querySelector('[type=submit]');
|
||||
if (btn) btn.disabled = false;
|
||||
|
||||
if (event.detail.successful) {
|
||||
showNotification('Configuration saved successfully!', 'success');
|
||||
const xhr = event.detail.xhr;
|
||||
const status = xhr?.status || 0;
|
||||
|
||||
// Check if request was successful (2xx status codes)
|
||||
if (status >= 200 && status < 300) {
|
||||
// Try to get message from response JSON
|
||||
let message = 'Configuration saved successfully!';
|
||||
try {
|
||||
if (xhr?.responseJSON?.message) {
|
||||
message = xhr.responseJSON.message;
|
||||
} else if (xhr?.responseText) {
|
||||
const responseData = JSON.parse(xhr.responseText);
|
||||
message = responseData.message || message;
|
||||
}
|
||||
} catch (e) {
|
||||
// Use default message if parsing fails
|
||||
}
|
||||
showNotification(message, 'success');
|
||||
} else {
|
||||
// Log detailed error information
|
||||
const xhr = event.detail.xhr;
|
||||
// Request failed - log detailed error information
|
||||
console.error('Config save failed:', {
|
||||
status: xhr?.status,
|
||||
status: status,
|
||||
statusText: xhr?.statusText,
|
||||
responseText: xhr?.responseText
|
||||
});
|
||||
@@ -4223,7 +4238,13 @@
|
||||
// Try to parse error response
|
||||
let errorMessage = 'Failed to save configuration';
|
||||
try {
|
||||
if (xhr?.responseText) {
|
||||
if (xhr?.responseJSON) {
|
||||
const errorData = xhr.responseJSON;
|
||||
errorMessage = errorData.message || errorData.details || errorMessage;
|
||||
if (errorData.validation_errors) {
|
||||
errorMessage += ': ' + errorData.validation_errors.join(', ');
|
||||
}
|
||||
} else if (xhr?.responseText) {
|
||||
const errorData = JSON.parse(xhr.responseText);
|
||||
errorMessage = errorData.message || errorData.details || errorMessage;
|
||||
if (errorData.validation_errors) {
|
||||
@@ -4241,7 +4262,10 @@
|
||||
|
||||
// Handle toggle response
|
||||
window.handleToggleResponse = function(event, pluginId) {
|
||||
if (event.detail.successful) {
|
||||
const xhr = event.detail.xhr;
|
||||
const status = xhr?.status || 0;
|
||||
|
||||
if (status >= 200 && status < 300) {
|
||||
// Update UI in place instead of refreshing to avoid duplication
|
||||
const checkbox = document.getElementById(`plugin-enabled-${pluginId}`);
|
||||
const label = checkbox?.nextElementSibling;
|
||||
@@ -4252,14 +4276,39 @@
|
||||
label.className = `ml-2 text-sm ${isEnabled ? 'text-green-600' : 'text-gray-500'}`;
|
||||
}
|
||||
|
||||
showNotification('Plugin status updated', 'success');
|
||||
// Try to get message from response
|
||||
let message = 'Plugin status updated';
|
||||
try {
|
||||
if (xhr?.responseJSON?.message) {
|
||||
message = xhr.responseJSON.message;
|
||||
} else if (xhr?.responseText) {
|
||||
const responseData = JSON.parse(xhr.responseText);
|
||||
message = responseData.message || message;
|
||||
}
|
||||
} catch (e) {
|
||||
// Use default message
|
||||
}
|
||||
showNotification(message, 'success');
|
||||
} else {
|
||||
// Revert checkbox state on error
|
||||
const checkbox = document.getElementById(`plugin-enabled-${pluginId}`);
|
||||
if (checkbox) {
|
||||
checkbox.checked = !checkbox.checked;
|
||||
}
|
||||
showNotification('Failed to update plugin status', 'error');
|
||||
|
||||
// Try to get error message from response
|
||||
let errorMessage = 'Failed to update plugin status';
|
||||
try {
|
||||
if (xhr?.responseJSON?.message) {
|
||||
errorMessage = xhr.responseJSON.message;
|
||||
} else if (xhr?.responseText) {
|
||||
const errorData = JSON.parse(xhr.responseText);
|
||||
errorMessage = errorData.message || errorData.details || errorMessage;
|
||||
}
|
||||
} catch (e) {
|
||||
// Use default message
|
||||
}
|
||||
showNotification(errorMessage, 'error');
|
||||
}
|
||||
};
|
||||
|
||||
|
||||
Reference in New Issue
Block a user