Skip to content

[IMPROVEMENT] Fix overly broad exception handling and improve error patterns #231

@vaishnaviasawale

Description

@vaishnaviasawale

🐛 Problem

The jhack codebase contains multiple instances of problematic error handling that can hide bugs, reduce debugging effectiveness, and in some cases create security issues.

🔍 Issue Analysis

Critical Issues Found:

  1. Bare except: clauses (most dangerous):

    # ❌ In jhack/charm/provision.py:105
    except:  # noqa: E722
        pass
    
    # ❌ In jhack/utils/charm_rpc_dispatch.py:81,88  
    except:  # noqa
        logger.error("failed serializing...")
  2. Overly broad Exception handling:

    # ❌ In jhack/helpers.py:69
    except Exception as e:
        logger.error(e, exc_info=True)
        return False
  3. Missing exception chaining:

    # ❌ Context lost
    except StopIteration:
        raise FileNotFoundError(f"could not find...")
  4. Poor logging practices:

    # ❌ F-strings in logging
    logger.error(f"Command {cmd} failed")

🎯 Impact

  • Hidden bugs: Bare except: catches KeyboardInterrupt, SystemExit, etc.
  • Poor debugging: Lost exception context makes issues hard to trace
  • Security risk: Broad exception handling can mask security issues
  • Inconsistent error reporting: Users get unclear error messages

🛠️ Proposed Solution

1. Replace bare except: with specific handling:

# ✅ Good
try:
    risky_operation()
except (OSError, subprocess.SubprocessError) as e:
    logger.warning("Expected error: %s", e)
except Exception as e:
    logger.error("Unexpected error: %s", e, exc_info=True)
    raise

2. Add proper exception chaining:

# ✅ Good  
except StopIteration as exc:
    raise FileNotFoundError(f"could not find...") from exc

3. Use lazy logging:

# ✅ Good
logger.error("Command %s failed with code %d", cmd, returncode)

4. Specify encoding for file operations:

# ✅ Good
Path(file).write_text(content, encoding='utf-8')

📋 Files Requiring Changes

Critical (bare except):

  • jhack/charm/provision.py:105
  • jhack/utils/charm_rpc_dispatch.py:81,88
  • jhack/utils/tail_logs.py:147
  • jhack/utils/event_recorder/recorder.py:82,98

High Priority:

  • jhack/helpers.py - Multiple logging and exception issues
  • Various files with f-string logging and missing exception chaining

🧪 Implementation Steps

  1. Phase 1: Fix bare except: clauses (security critical)
  2. Phase 2: Add exception chaining and specific exception types
  3. Phase 3: Convert logging to lazy formatting
  4. Phase 4: Add encoding specifications and resource management

Acceptance Criteria

  • Zero bare except: clauses in codebase
  • All custom exceptions use from chaining
  • All logging uses lazy formatting (%s instead of f-strings)
  • File operations specify encoding
  • Linting rules prevent regression
  • No functional regressions

🔧 Testing

def test_error_handling():
    """Verify proper error handling for edge cases."""
    # Test missing commands
    assert not check_command_available("nonexistent_cmd")
    
    # Test network failures  
    with patch('subprocess.run', side_effect=OSError):
        with pytest.raises(GetStatusError):
            juju_status()

📚 Additional Context

This addresses technical debt that impacts:

  • Developer experience: Better error messages and debugging
  • Code maintainability: Clear error handling patterns
  • Security posture: Prevents masking of security issues
  • User experience: More informative error messages

Priority: High (affects stability and security)
Effort: Medium (systematic changes, but well-defined)
Risk: Low (improvements don't change functionality)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions