[IMPROVEMENT] Fix overly broad exception handling and improve error patterns
- 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
Assessment
This issue has not been assessed yet.