|
|
|
@@ -0,0 +1,415 @@ |
|
|
|
# Code Review: AFASystems Presence Detection System |
|
|
|
|
|
|
|
**Date**: 2026-01-15 |
|
|
|
**Reviewer**: Claude Code |
|
|
|
**Project**: BLE Beacon Presence Detection System |
|
|
|
|
|
|
|
--- |
|
|
|
|
|
|
|
## Overall Assessment |
|
|
|
|
|
|
|
Your system is a well-structured microservices architecture for BLE beacon presence detection. The code is functional but has several areas that need refactoring for production readiness, maintainability, and robustness. |
|
|
|
|
|
|
|
**Code Quality Score**: 6.5/10 |
|
|
|
|
|
|
|
- ✅ Good architecture and separation |
|
|
|
- ✅ Thread-safe concurrent access |
|
|
|
- ❌ No testing |
|
|
|
- ❌ Poor error handling |
|
|
|
- ❌ Security concerns |
|
|
|
- ❌ Code duplication |
|
|
|
|
|
|
|
--- |
|
|
|
|
|
|
|
## 🔴 Critical Issues (Fix Immediately) |
|
|
|
|
|
|
|
### 1. Hardcoded Credentials in Config |
|
|
|
**Location**: [internal/pkg/config/config.go:46-49](internal/pkg/config/config.go#L46) |
|
|
|
**Risk**: Security vulnerability - default credentials exposed in source code |
|
|
|
|
|
|
|
```go |
|
|
|
ClientSecret: getEnv("ClientSecret", "wojuoB7Z5xhlPFrF2lIxJSSdVHCApEgC"), |
|
|
|
HTTPPassword: getEnv("HTTPPassword", "C0r3_us3r_Cr3d3nt14ls"), |
|
|
|
``` |
|
|
|
|
|
|
|
**Fix**: Remove default credentials, require explicit environment configuration: |
|
|
|
```go |
|
|
|
ClientSecret: getEnvOrFatal("ClientSecret"), // Helper that panics if not set |
|
|
|
HTTPPassword: getEnvOrFatal("HTTPPassword"), |
|
|
|
``` |
|
|
|
|
|
|
|
### 2. Global Database Variable |
|
|
|
**Location**: [internal/pkg/database/database.go:12](internal/pkg/database/database.go#L12) |
|
|
|
```go |
|
|
|
var DB *gorm.DB // ❌ Global variable |
|
|
|
``` |
|
|
|
|
|
|
|
**Issues**: |
|
|
|
- Hard to test |
|
|
|
- Implicit dependencies |
|
|
|
- Cannot have multiple DB connections |
|
|
|
|
|
|
|
**Fix**: Return `*gorm.DB` from `Connect()` and inject it into services. |
|
|
|
|
|
|
|
### 3. Missing Error Context |
|
|
|
Errors are logged but lose context: |
|
|
|
```go |
|
|
|
fmt.Println("Error in sending Kafka message:", err) // ❌ No context |
|
|
|
``` |
|
|
|
|
|
|
|
**Fix**: Use structured logging: |
|
|
|
```go |
|
|
|
slog.Error("failed to send kafka message", |
|
|
|
"topic", topic, |
|
|
|
"beacon_id", id, |
|
|
|
"error", err) |
|
|
|
``` |
|
|
|
|
|
|
|
### 4. Unsafe Map Access |
|
|
|
**Location**: [cmd/bridge/main.go:28](cmd/bridge/main.go#L28) |
|
|
|
```go |
|
|
|
hostname := strings.Split(topic, "/")[1] // ❌ Panic if index doesn't exist |
|
|
|
``` |
|
|
|
|
|
|
|
**Fix**: Validate before accessing: |
|
|
|
```go |
|
|
|
parts := strings.Split(topic, "/") |
|
|
|
if len(parts) < 2 { |
|
|
|
slog.Warn("invalid topic format", "topic", topic) |
|
|
|
return |
|
|
|
} |
|
|
|
hostname := parts[1] |
|
|
|
``` |
|
|
|
|
|
|
|
--- |
|
|
|
|
|
|
|
## 🟡 High Priority Refactoring (Fix Soon) |
|
|
|
|
|
|
|
### 5. Code Duplication Across Services |
|
|
|
**Affected Files**: All 4 main files |
|
|
|
|
|
|
|
**Pattern**: Logging setup (lines 124-131 in all services) |
|
|
|
```go |
|
|
|
logFile, err := os.OpenFile("server.log", os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0666) |
|
|
|
w := io.MultiWriter(os.Stderr, logFile) |
|
|
|
logger := slog.New(slog.NewJSONHandler(w, nil)) |
|
|
|
slog.SetDefault(logger) |
|
|
|
``` |
|
|
|
|
|
|
|
**Refactor**: Create `internal/pkg/common/logger/logger.go`: |
|
|
|
```go |
|
|
|
func SetupLogger(filename string) *slog.Logger { |
|
|
|
logFile, err := os.OpenFile(filename, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0666) |
|
|
|
if err != nil { |
|
|
|
log.Fatalf("Failed to open log file: %v\n", err) |
|
|
|
} |
|
|
|
w := io.MultiWriter(os.Stderr, logFile) |
|
|
|
logger := slog.New(slog.NewJSONHandler(w, nil)) |
|
|
|
slog.SetDefault(logger) |
|
|
|
return logger |
|
|
|
} |
|
|
|
``` |
|
|
|
|
|
|
|
### 6. Inconsistent Error Handling |
|
|
|
Mixed error handling patterns across codebase: |
|
|
|
|
|
|
|
**In controller**: [internal/pkg/controller/trackers_controller.go:48](internal/pkg/controller/trackers_controller.go#L48) |
|
|
|
```go |
|
|
|
if err != nil { |
|
|
|
fmt.Println("error in sending Kafka POST message") |
|
|
|
http.Error(w, "Error in sending kafka message", 500) |
|
|
|
return |
|
|
|
} |
|
|
|
``` |
|
|
|
|
|
|
|
**In main**: [cmd/decoder/main.go:97](cmd/decoder/main.go#L97) |
|
|
|
```go |
|
|
|
if err != nil { |
|
|
|
eMsg := fmt.Sprintf("Error in decoding: %v", err) |
|
|
|
fmt.Println(eMsg) |
|
|
|
return |
|
|
|
} |
|
|
|
``` |
|
|
|
|
|
|
|
**Fix**: Use consistent error handling with structured responses. |
|
|
|
|
|
|
|
### 7. Missing Configuration Validation |
|
|
|
**Location**: [internal/pkg/config/config.go](internal/pkg/config/config.go) |
|
|
|
|
|
|
|
Config doesn't validate required fields: |
|
|
|
```go |
|
|
|
func Load() *Config { |
|
|
|
return &Config{ |
|
|
|
// No validation |
|
|
|
} |
|
|
|
} |
|
|
|
``` |
|
|
|
|
|
|
|
**Fix**: Add validation: |
|
|
|
```go |
|
|
|
func (c *Config) Validate() error { |
|
|
|
if c.DBHost == "" { |
|
|
|
return errors.New("DBHost is required") |
|
|
|
} |
|
|
|
if c.KafkaURL == "" { |
|
|
|
return errors.New("KafkaURL is required") |
|
|
|
} |
|
|
|
// ... other validations |
|
|
|
return nil |
|
|
|
} |
|
|
|
``` |
|
|
|
|
|
|
|
### 8. Potential Memory Inefficiency |
|
|
|
**Location**: [cmd/location/main.go:217-222](cmd/location/main.go#L217) |
|
|
|
```go |
|
|
|
if len(beacon.BeaconMetrics) >= settings.BeaconMetricSize { |
|
|
|
copy(beacon.BeaconMetrics, beacon.BeaconMetrics[1:]) |
|
|
|
beacon.BeaconMetrics[settings.BeaconMetricSize-1] = metric |
|
|
|
} else { |
|
|
|
beacon.BeaconMetrics = append(beacon.BeaconMetrics, metric) |
|
|
|
} |
|
|
|
``` |
|
|
|
|
|
|
|
**Issue**: This logic is correct but could be optimized with a circular buffer. |
|
|
|
|
|
|
|
--- |
|
|
|
|
|
|
|
## 🟢 Medium Priority Improvements (Plan Refactor) |
|
|
|
|
|
|
|
### 9. Tight Coupling in Controllers |
|
|
|
Controllers directly use `*gorm.DB` instead of repository pattern: |
|
|
|
|
|
|
|
```go |
|
|
|
func TrackerAdd(db *gorm.DB, writer *kafka.Writer, ctx context.Context) http.HandlerFunc |
|
|
|
``` |
|
|
|
|
|
|
|
**Refactor**: Introduce repository interface: |
|
|
|
```go |
|
|
|
type TrackerRepository interface { |
|
|
|
Create(tracker *model.Tracker) error |
|
|
|
Find(id string) (*model.Tracker, error) |
|
|
|
// ... |
|
|
|
} |
|
|
|
|
|
|
|
func TrackerAdd(repo TrackerRepository, writer *kafka.Writer, ctx context.Context) http.HandlerFunc |
|
|
|
``` |
|
|
|
|
|
|
|
### 10. Poor Separation of Concerns |
|
|
|
**Location**: [internal/pkg/common/appcontext/context.go](internal/pkg/common/appcontext/context.go) |
|
|
|
|
|
|
|
The AppState mixes state management with Kafka client creation (lines 60-76). |
|
|
|
|
|
|
|
**Refactor**: Separate concerns: |
|
|
|
```go |
|
|
|
// internal/pkg/infrastructure/kafka/pool.go |
|
|
|
type KafkaPool struct { |
|
|
|
writers []*kafka.Writer |
|
|
|
readers []*kafka.Reader |
|
|
|
} |
|
|
|
|
|
|
|
// internal/pkg/domain/appstate/state.go |
|
|
|
type AppState struct { |
|
|
|
beacons *model.BeaconsList |
|
|
|
settings *model.Settings |
|
|
|
kafkaPool *kafka.KafkaPool // Composed, not owned |
|
|
|
} |
|
|
|
``` |
|
|
|
|
|
|
|
### 11. Magic Numbers |
|
|
|
**Location**: [cmd/location/main.go:107-108](cmd/location/main.go#L107) |
|
|
|
```go |
|
|
|
seenW := 1.5 // What does this mean? |
|
|
|
rssiW := 0.75 // What does this mean? |
|
|
|
``` |
|
|
|
|
|
|
|
**Fix**: Extract to named constants in settings: |
|
|
|
```go |
|
|
|
type SettingsVal struct { |
|
|
|
LocationConfidence int64 `json:"location_confidence"` |
|
|
|
SeenWeight float64 `json:"seen_weight"` // 1.5 |
|
|
|
RSSIWeight float64 `json:"rssi_weight"` // 0.75 |
|
|
|
// ... |
|
|
|
} |
|
|
|
``` |
|
|
|
|
|
|
|
### 12. Inefficient JSON Marshaling |
|
|
|
**Location**: [cmd/server/main.go:206-207](cmd/server/main.go#L206) |
|
|
|
```go |
|
|
|
js, err := json.Marshal(list) |
|
|
|
if err != nil { |
|
|
|
js = []byte("error") // ❌ Invalid JSON! |
|
|
|
} |
|
|
|
``` |
|
|
|
|
|
|
|
**Fix**: Return proper error response: |
|
|
|
```go |
|
|
|
if err != nil { |
|
|
|
http.Error(w, "Failed to marshal trackers", http.StatusInternalServerError) |
|
|
|
return |
|
|
|
} |
|
|
|
``` |
|
|
|
|
|
|
|
--- |
|
|
|
|
|
|
|
## 🔵 Low Priority / Technical Debt |
|
|
|
|
|
|
|
### 13. Zero Test Coverage |
|
|
|
The codebase has **no test files**. This is critical for production systems. |
|
|
|
|
|
|
|
**Recommendation**: Add unit tests for: |
|
|
|
- All business logic in `service/` package |
|
|
|
- Controller handlers |
|
|
|
- Kafka message processing |
|
|
|
- Beacon parsing logic |
|
|
|
|
|
|
|
**Target**: 70%+ code coverage |
|
|
|
|
|
|
|
### 14. Missing Context Timeouts |
|
|
|
**Location**: [cmd/bridge/main.go:68](cmd/bridge/main.go#L68) |
|
|
|
```go |
|
|
|
err = writer.WriteMessages(context.Background(), msg) |
|
|
|
``` |
|
|
|
|
|
|
|
**Fix**: Use timeouts: |
|
|
|
```go |
|
|
|
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) |
|
|
|
defer cancel() |
|
|
|
err = writer.WriteMessages(ctx, msg) |
|
|
|
``` |
|
|
|
|
|
|
|
### 15. Inefficient String Concatenation |
|
|
|
**Location**: [cmd/bridge/main.go:182](cmd/bridge/main.go#L182) |
|
|
|
```go |
|
|
|
lMsg := fmt.Sprintf("Beacon added to lookup: %s", id) |
|
|
|
slog.Info(lMsg) |
|
|
|
``` |
|
|
|
|
|
|
|
**Fix**: Direct logging: |
|
|
|
```go |
|
|
|
slog.Info("beacon added to lookup", "id", id) |
|
|
|
``` |
|
|
|
|
|
|
|
### 16. Dead Code |
|
|
|
**Location**: [cmd/bridge/main.go:76-103](cmd/bridge/main.go#L76) |
|
|
|
|
|
|
|
Large block of commented code should be removed. |
|
|
|
|
|
|
|
### 17. Incomplete Graceful Shutdown |
|
|
|
**Location**: [cmd/bridge/main.go:212](cmd/bridge/main.go#L212) |
|
|
|
|
|
|
|
The MQTT client disconnects with timeout but doesn't wait for pending messages: |
|
|
|
```go |
|
|
|
client.Disconnect(250) // Only waits 250ms |
|
|
|
``` |
|
|
|
|
|
|
|
### 18. No Health Checks |
|
|
|
Services don't expose health endpoints for orchestration systems (Kubernetes, etc.). |
|
|
|
|
|
|
|
--- |
|
|
|
|
|
|
|
## 📊 Architecture Recommendations |
|
|
|
|
|
|
|
### 1. Implement Dependency Injection |
|
|
|
Instead of passing `db`, `writer`, `ctx` to controllers, create a service container: |
|
|
|
|
|
|
|
```go |
|
|
|
type Services struct { |
|
|
|
DB *gorm.DB |
|
|
|
KafkaWriter *kafka.Writer |
|
|
|
AppState *appcontext.AppState |
|
|
|
} |
|
|
|
|
|
|
|
func (s *Services) TrackerAddController() http.HandlerFunc { |
|
|
|
return func(w http.ResponseWriter, r *http.Request) { |
|
|
|
// Use s.DB, s.KafkaWriter |
|
|
|
} |
|
|
|
} |
|
|
|
``` |
|
|
|
|
|
|
|
### 2. Add Observability |
|
|
|
- **Structured logging** with request IDs |
|
|
|
- **Metrics** (Prometheus) for: |
|
|
|
- Kafka message throughput |
|
|
|
- Beacon processing latency |
|
|
|
- Database query performance |
|
|
|
- **Distributed tracing** (OpenTelemetry) |
|
|
|
|
|
|
|
### 3. Implement Circuit Breakers |
|
|
|
For external API calls in `apiclient` package to handle failures gracefully. |
|
|
|
|
|
|
|
### 4. Add Message Validation |
|
|
|
Validate Kafka messages before processing: |
|
|
|
```go |
|
|
|
func (adv *BeaconAdvertisement) Validate() error { |
|
|
|
if adv.ID == "" { |
|
|
|
return errors.New("beacon ID is required") |
|
|
|
} |
|
|
|
if adv.RSSI < -100 || adv.RSSI > 0 { |
|
|
|
return errors.New("invalid RSSI value") |
|
|
|
} |
|
|
|
return nil |
|
|
|
} |
|
|
|
``` |
|
|
|
|
|
|
|
--- |
|
|
|
|
|
|
|
## 🎯 Refactoring Priority Order |
|
|
|
|
|
|
|
| Priority | Category | Actions | |
|
|
|
|----------|----------|---------| |
|
|
|
| 1 | 🔴 Security | Remove hardcoded credentials from config | |
|
|
|
| 2 | 🔴 Stability | Fix unsafe map/array access | |
|
|
|
| 3 | 🔴 Testing | Add unit tests (aim for 70%+ coverage) | |
|
|
|
| 4 | 🟡 Error Handling | Implement structured error handling | |
|
|
|
| 5 | 🟡 Logging | Standardize to structured logging throughout | |
|
|
|
| 6 | 🟡 Code Quality | Extract duplicated code to shared packages | |
|
|
|
| 7 | 🟢 Architecture | Implement dependency injection gradually | |
|
|
|
| 8 | 🔵 Performance | Optimize hot paths (beacon processing) | |
|
|
|
|
|
|
|
--- |
|
|
|
|
|
|
|
## 📈 Metrics Summary |
|
|
|
|
|
|
|
| Category | Count | Status | |
|
|
|
|----------|-------|--------| |
|
|
|
| 🔴 Critical Issues | 4 | Fix Immediately | |
|
|
|
| 🟡 High Priority | 4 | Fix Soon | |
|
|
|
| 🟢 Medium Priority | 4 | Plan Refactor | |
|
|
|
| 🔵 Low Priority | 6 | Technical Debt | |
|
|
|
|
|
|
|
**Total Issues Identified**: 18 |
|
|
|
|
|
|
|
--- |
|
|
|
|
|
|
|
## System Architecture Overview |
|
|
|
|
|
|
|
The system consists of 4 microservices: |
|
|
|
|
|
|
|
1. **Bridge** ([cmd/bridge/main.go](cmd/bridge/main.go)) - MQTT to Kafka bridge |
|
|
|
2. **Decoder** ([cmd/decoder/main.go](cmd/decoder/main.go)) - BLE beacon decoder |
|
|
|
3. **Location** ([cmd/location/main.go](cmd/location/main.go)) - Location calculation service |
|
|
|
4. **Server** ([cmd/server/main.go](cmd/server/main.go)) - HTTP API & WebSocket server |
|
|
|
|
|
|
|
### Communication Flow |
|
|
|
``` |
|
|
|
MQTT Gateway → Bridge (Kafka) → Decoder (Kafka) → Location (Kafka) → Server (Kafka) |
|
|
|
↓ ↑ |
|
|
|
External API ←─────────────────────────────────────────────── |
|
|
|
``` |
|
|
|
|
|
|
|
### Technology Stack |
|
|
|
- **Language**: Go 1.24.0 |
|
|
|
- **Message Broker**: Apache Kafka |
|
|
|
- **Database**: PostgreSQL with GORM |
|
|
|
- **Cache**: Redis (valkey) |
|
|
|
- **MQTT**: Eclipse Paho |
|
|
|
- **HTTP**: Gorilla Mux + WebSocket |
|
|
|
- **Deployment**: Docker Compose |
|
|
|
|
|
|
|
--- |
|
|
|
|
|
|
|
## Conclusion |
|
|
|
|
|
|
|
The codebase demonstrates solid understanding of microservices architecture with good separation of concerns. The concurrent access patterns using `sync.RWMutex` are well-implemented. However, the system needs significant hardening before production deployment, particularly in areas of security, testing, and error handling. |
|
|
|
|
|
|
|
Focus on addressing critical security issues first, then build out test coverage to ensure reliability as you refactor other areas of the codebase. |