canonical / canonical/jhack

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

Open
#231 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
60
Forks
31
Avg merge
2d 15h
Merged PRs (30d)
1

Description

## 🐛 **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):
```python
# ❌ 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**:
```python
# ❌ In jhack/helpers.py:69
except Exception as e:
logger.error(e, exc_info=True)
return False
```

3. **Missing exception chaining**:
```python
# ❌ Context lost
except StopIteration:
raise FileNotFoundError(f"could not find...")
```

4. **Poor logging practices**:
```python
# ❌ 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:
```python
# ✅ 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:
```python
# ✅ Good
except StopIteration as exc:
raise FileNotFoundError(f"could not find...") from exc
```

### 3. Use lazy logging:
```python
# ✅ Good
logger.error("Command %s failed with code %d", cmd, returncode)
```

### 4. Specify encoding for file operations:
```python
# ✅ 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**

```python
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)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.