chore/bump-version-0.6.1

LOGGING_REVIEW.md

@@ -0,0 +1,377 @@
+# Logging Review - Clean Code Compliance
+
+**Date**: 2025-10-06
+**Reviewer**: Claude Code
+**Guidelines**: Clean Code principles (Chapters 2, 3, 4, 7, 10, 17)
+
+## Executive Summary
+
+The nostr-java codebase uses SLF4J logging with Lombok's `@Slf4j` annotation consistently across the project. The logging implementation is generally good, with proper log levels and meaningful messages. However, there are several areas where the logging does not fully comply with Clean Code principles.
+
+**Overall Grade**: B+
+
+**Key Findings**:
+- ✅ Consistent use of SLF4J with Lombok `@Slf4j`
+- ✅ No sensitive data (private keys, passwords) logged in plain text
+- ✅ Appropriate log levels used in most cases
+- ⚠️ Some empty or non-descriptive error messages
+- ⚠️ Excessive debug logging in low-level classes (PrivateKey, PublicKey)
+- ⚠️ Test methods using log.info for test names (should use JUnit display names)
+- ⚠️ Some log messages lack context
+
+## Detailed Findings
+
+### 1. Clean Code Chapter 2: Meaningful Names
+
+**Principle**: Use intention-revealing, searchable names in log messages.
+
+#### Issues Found
+
+**❌ Empty error message** (`nostr-java-event/src/main/java/nostr/event/entities/UserProfile.java:46`)
+```java
+log.error("", ex);
+```
+
+**Problem**: Empty string provides no context about what failed.
+**Fix**: Add meaningful error message
+```java
+log.error("Failed to encode UserProfile to Bech32 format", ex);
+```
+
+**❌ Generic warning** (`nostr-java-event/src/main/java/nostr/event/impl/GenericEvent.java:196`)
+```java
+log.warn(ex.getMessage());
+```
+
+**Problem**: Only logs exception message without context about what operation failed.
+**Fix**: Add context
+```java
+log.warn("Failed to update event during serialization: {}", ex.getMessage(), ex);
+```
+
+### 2. Clean Code Chapter 3: Functions
+
+**Principle**: Functions should do one thing. Logging should not be the primary purpose.
+
+#### Issues Found
+
+**⚠️ Excessive constructor logging** (`nostr-java-base/src/main/java/nostr/base/PrivateKey.java:16,21,29`)
+```java
+public PrivateKey(byte[] rawData) {
+    super(KeyType.PRIVATE, rawData, Bech32Prefix.NSEC);
+    log.debug("Created private key from byte array");
+}
+
+public PrivateKey(String hexPrivKey) {
+    super(KeyType.PRIVATE, NostrUtil.hexToBytes(hexPrivKey), Bech32Prefix.NSEC);
+    log.debug("Created private key from hex string");
+}
+
+public static PrivateKey generateRandomPrivKey() {
+    PrivateKey key = new PrivateKey(Schnorr.generatePrivateKey());
+    log.debug("Generated new random private key");
+    return key;
+}
+```
+
+**Problem**: Low-level constructors should not log. This creates noise and violates single responsibility. These classes are used frequently, and logging every creation adds overhead.
+
+**Recommendation**: Remove these debug logs. If tracking object creation is needed, use a profiler or instrumentation.
+
+**Same issue in** `PublicKey.java:17,22` and `BaseKey.java:32,48`
+
+### 3. Clean Code Chapter 4: Comments
+
+**Principle**: Code should be self-documenting. Logs should not explain what code does, but provide runtime context.
+
+#### Good Examples
+
+**✅ Context-rich logging** (`SpringWebSocketClient.java:38-42`)
+```java
+log.debug(
+    "Sending {} to relay {} (size={} bytes)",
+    eventMessage.getCommand(),
+    relayUrl,
+    json.length());
+```
+
+**Good**: Provides runtime context (command, relay, size) without explaining code logic.
+
+**✅ Error recovery logging** (`SpringWebSocketClient.java:112-116`)
+```java
+log.error(
+    "Failed to send message to relay {} after retries (size={} bytes)",
+    relayUrl,
+    json.length(),
+    ex);
+```
+
+**Good**: Logs failure with context and includes exception for debugging.
+
+#### Issues Found
+
+**⚠️ Verbose serialization logging** (`GenericEvent.java:277`)
+```java
+log.debug("Serialized event: {}", new String(this.get_serializedEvent()));
+```
+
+**Problem**: Logs entire serialized event at debug level. This could be very verbose and is called frequently. Consider:
+1. Using TRACE level instead of DEBUG
+2. Truncating output
+3. Removing this log entirely (serialization is expected behavior)
+
+**Recommendation**: Remove or change to TRACE level with size limit.
+
+### 4. Clean Code Chapter 7: Error Handling
+
+**Principle**: Error handling should be complete. Don't pass null or empty messages to logging.
+
+#### Issues Found
+
+**❌ Empty error log** (`UserProfile.java:46`)
+```java
+catch (Exception ex) {
+    log.error("", ex);  // Empty message
+    throw new RuntimeException(ex);
+}
+```
+
+**Fix**:
+```java
+catch (Exception ex) {
+    log.error("Failed to convert UserProfile to Bech32 format", ex);
+    throw new RuntimeException("Failed to convert UserProfile to Bech32 format", ex);
+}
+```
+
+**⚠️ Generic RuntimeException wrapping** (multiple locations)
+```java
+catch (Exception ex) {
+    log.error("Error converting key to Bech32", ex);
+    throw new RuntimeException(ex);
+}
+```
+
+**Better approach**: Create specific exception types or include original message:
+```java
+catch (Exception ex) {
+    log.error("Error converting {} key to Bech32 format with prefix {}", type, prefix, ex);
+    throw new RuntimeException("Failed to convert key to Bech32: " + ex.getMessage(), ex);
+}
+```
+
+### 5. Clean Code Chapter 10: Classes
+
+**Principle**: Classes should have a single responsibility. Excessive logging can indicate unclear responsibilities.
+
+#### Good Examples
+
+**✅ Client handler logging** (`SpringWebSocketClient.java`)
+- Logs connection lifecycle events
+- Logs retry failures
+- Logs subscription events
+- All appropriate for a client handler class
+
+**✅ Validator logging** (`Nip05Validator.java:110,123,133`)
+- Logs validation errors with context
+- Logs HTTP request failures
+- Logs public key lookup results
+- All appropriate for a validator class
+
+#### Issues Found
+
+**⚠️ Low-level utility logging** (`PrivateKey.java`, `PublicKey.java`, `BaseKey.java`)
+
+These classes are data containers with minimal behavior. Logging in constructors and conversion methods adds noise without value.
+
+**Recommendation**: Remove all debug logging from these low-level classes. If needed, add logging at the application layer where these objects are used.
+
+### 6. Clean Code Chapter 17: Smells and Heuristics
+
+**Principle**: Avoid code smells that indicate poor design.
+
+#### Code Smells Found
+
+**G5: Duplication**
+
+**⚠️ Duplicated recovery logging** (`SpringWebSocketClient.java:112-116, 129-133, 145-151, 166-171`)
+
+Four nearly identical recovery methods with duplicated logging logic.
+
+**Recommendation**: Extract common recovery logging:
+```java
+private void logRecoveryFailure(String operation, String relayUrl, int size, IOException ex) {
+    log.error("Failed to {} to relay {} after retries (size={} bytes)",
+              operation, relayUrl, size, ex);
+}
+```
+
+**G15: Selector Arguments**
+
+Test classes use `log.info()` to log test names:
+```java
+@Test
+void testEventFilterEncoder() {
+    log.info("testEventFilterEncoder");  // Unnecessary
+    // test code
+}
+```
+
+**Recommendation**: Remove these. Use JUnit's `@DisplayName` instead:
+```java
+@Test
+@DisplayName("Event filter encoder should serialize filters correctly")
+void testEventFilterEncoder() {
+    // test code
+}
+```
+
+**G31: Hidden Temporal Couplings**
+
+**⚠️ Potential issue** (`GenericTagDecoder.java:56`)
+```java
+log.info("Decoded GenericTag: {}", genericTag);
+```
+
+**Problem**: Using INFO level for routine decoding operation. This should be DEBUG or removed entirely. INFO level implies something noteworthy, but decoding is expected behavior.
+
+**Recommendation**: Change to DEBUG or remove.
+
+### 7. Security Concerns
+
+**✅ No Sensitive Data Logged**
+
+Analysis of all logging statements confirms:
+- Private keys are NOT logged (only existence is logged: "Created private key")
+- Passwords/secrets are NOT logged
+- Public keys are logged only at DEBUG level (appropriate since they're public)
+
+**Good security practice observed**.
+
+### 8. Performance Concerns
+
+**⚠️ Expensive Operations at DEBUG Level**
+
+Several locations log expensive operations:
+
+1. **Full event serialization** (`GenericEvent.java:277`)
+```java
+log.debug("Serialized event: {}", new String(this.get_serializedEvent()));
+```
+
+2. **GenericTag decoding** (`GenericTagDecoder.java:56`)
+```java
+log.info("Decoded GenericTag: {}", genericTag);
+```
+
+**Problem**: Even if DEBUG is disabled, `toString()` is still called on objects passed to log methods.
+
+**Recommendation**: Use lazy evaluation:
+```java
+if (log.isDebugEnabled()) {
+    log.debug("Serialized event: {}", new String(this.get_serializedEvent()));
+}
+```
+
+Or better, remove entirely.
+
+## Recommendations by Priority
+
+### High Priority (Fix Immediately)
+
+1. **Fix empty error message** in `UserProfile.java:46`
+   ```java
+   // Before
+   log.error("", ex);
+
+   // After
+   log.error("Failed to convert UserProfile to Bech32 format", ex);
+   ```
+
+2. **Fix generic warning** in `GenericEvent.java:196`
+   ```java
+   // Before
+   log.warn(ex.getMessage());
+
+   // After
+   log.warn("Failed to update event during serialization: {}", ex.getMessage(), ex);
+   ```
+
+3. **Change INFO to DEBUG** in `GenericTagDecoder.java:56`
+   ```java
+   // Before
+   log.info("Decoded GenericTag: {}", genericTag);
+
+   // After
+   log.debug("Decoded GenericTag: {}", genericTag);
+   // Or remove entirely
+   ```
+
+### Medium Priority (Should Fix)
+
+4. **Remove constructor logging** from `PrivateKey.java`, `PublicKey.java`, `BaseKey.java`
+   - Lines: `PrivateKey.java:16,21,29`
+   - Lines: `PublicKey.java:17,22`
+   - Lines: `BaseKey.java:32,48`
+
+5. **Remove or optimize expensive debug logging**
+   - `GenericEvent.java:277` - Full event serialization
+   - Add `if (log.isDebugEnabled())` guard or remove
+
+6. **Remove test method name logging**
+   - All files in `nostr-java-event/src/test/java/`
+   - Replace with `@DisplayName` annotations
+
+### Low Priority (Nice to Have)
+
+7. **Extract duplicated recovery logging** in `SpringWebSocketClient.java`
+   - Create helper method to reduce duplication
+
+8. **Add more context to error messages**
+   - Include variable values that help debugging
+   - Use structured logging where appropriate
+
+## Compliance Summary
+
+| Clean Code Chapter | Compliance | Issues |
+|-------------------|------------|---------|
+| Ch 2: Meaningful Names | 🟡 Partial | Empty error messages, generic warnings |
+| Ch 3: Functions | 🟡 Partial | Constructor logging, excessive debug logs |
+| Ch 4: Comments | ✅ Good | Most logs provide runtime context, not code explanation |
+| Ch 7: Error Handling | 🟡 Partial | Empty error messages, generic exceptions |
+| Ch 10: Classes | ✅ Good | Logging appropriate for class responsibilities (except low-level utils) |
+| Ch 17: Smells | 🟡 Partial | Duplication, test name logging, INFO for routine operations |
+
+**Legend**: ✅ Good | 🟡 Partial | ❌ Poor
+
+## Positive Observations
+
+1. **Consistent framework usage**: SLF4J with Lombok `@Slf4j` throughout
+2. **Proper log levels**: DEBUG for detailed info, ERROR for failures, WARN for issues
+3. **Parameterized logging**: Uses `{}` placeholders (avoids string concatenation)
+4. **Security**: No sensitive data logged
+5. **Context-rich messages**: Most logs include relay URLs, subscription IDs, sizes
+6. **Exception logging**: Properly includes exception objects in error logs
+
+## Action Items
+
+Create issues or tasks for:
+- [ ] Fix empty error message in UserProfile.java
+- [ ] Fix generic warning in GenericEvent.java
+- [ ] Change INFO to DEBUG in GenericTagDecoder.java
+- [ ] Remove constructor logging from key classes
+- [ ] Optimize or remove expensive debug logging
+- [ ] Replace test log.info with @DisplayName
+- [ ] Extract duplicated recovery logging
+- [ ] Review and enhance error message context
+
+## Conclusion
+
+The logging implementation in nostr-java is solid overall, with proper use of SLF4J and good security practices. The main areas for improvement are:
+
+1. **Meaningful error messages** (avoid empty strings)
+2. **Reduce noise** (remove constructor logging in low-level classes)
+3. **Optimize performance** (guard expensive debug operations)
+4. **Improve tests** (use JUnit features instead of logging)
+
+Implementing the high-priority fixes will bring the codebase to an **A-** grade for logging practices.

nostr-java-api/pom.xml

@@ -4,7 +4,7 @@
     <parent>
         <groupId>xyz.tcheeric</groupId>
         <artifactId>nostr-java</artifactId>
-        <version>0.6.0</version>
+        <version>0.6.1</version>
         <relativePath>../pom.xml</relativePath>
     </parent>