275 lines
11 KiB
Markdown
275 lines
11 KiB
Markdown
# Custom Ecto Types Refactor - Progress Tracker
|
|
|
|
## Overview
|
|
|
|
Implementing custom Ecto types to avoid "primitive obsession" by creating rich domain objects for IP addresses, email addresses, and MAC addresses. This follows the pattern from https://www.elixirstreams.com/tips/richer-domain-types-with-ecto-custom-types.
|
|
|
|
**Current Phase**: Phase 1 - IP Address Custom Type
|
|
**Status**: In Progress (1 of 7 schemas migrated)
|
|
**Started**: 2026-01-31
|
|
|
|
---
|
|
|
|
## Phase 1: IP Address Custom Type
|
|
|
|
### Completed ✅
|
|
|
|
1. **IPAddress Custom Type Implementation** (`lib/towerops/ecto_types/ip_address.ex`)
|
|
- Implements `Ecto.Type` behavior with all 4 callbacks
|
|
- Validates IPv4 and IPv6 addresses using `:inet.parse_address/1`
|
|
- Provides helpers: `new/1`, `new!/1`, `ipv4?/1`, `ipv6?/1`, `to_string/1`, `to_tuple/1`
|
|
- Handles edge cases:
|
|
- nil values
|
|
- IPv6 zone IDs (strips before validation)
|
|
- CIDR notation (rejects)
|
|
- BSD-style IP notation (accepts, matches Erlang behavior)
|
|
- **JSON Encoding**: Added `@derive {Jason.Encoder, only: [:address]}` for GDPR data export compatibility
|
|
- Database type: `:string` (no migrations needed)
|
|
|
|
2. **Comprehensive Unit Tests** (`test/towerops/ecto_types/ip_address_test.exs`)
|
|
- 44 tests covering all callbacks and edge cases
|
|
- Tests for: `type/0`, `cast/1`, `load/1`, `dump/1`, roundtrips, helpers
|
|
- 100% coverage of custom type module
|
|
- All tests passing ✅
|
|
|
|
3. **Schema Migration #1: Towerops.Admin.AuditLog**
|
|
- File: `lib/towerops/admin/audit_log.ex`
|
|
- Changed: `field :ip_address, :string` → `field :ip_address, Towerops.EctoTypes.IpAddress`
|
|
- Risk Level: **Lowest** (append-only audit logs)
|
|
- No validation code to remove (didn't have any)
|
|
- Tests: All account data controller tests passing ✅
|
|
- JSON serialization verified working for GDPR data export
|
|
|
|
4. **Documentation Updated**
|
|
- Added "Custom Ecto Types" section to `CLAUDE.md` (after "Development Environment")
|
|
- Includes: overview, available types, usage examples, implementation pattern, testing pattern
|
|
- Plan file created at `/Users/graham/.claude/plans/swift-riding-mountain.md`
|
|
|
|
### In Progress 🔄
|
|
|
|
**Current Task**: Document remaining work before continuing
|
|
|
|
### Remaining Schemas to Migrate (6 schemas)
|
|
|
|
#### Low Risk (migrate next)
|
|
|
|
1. **Towerops.Accounts.BrowserSession** (`lib/towerops/accounts/browser_session.ex`)
|
|
- Change: `field :ip_address, :string` → `field :ip_address, Towerops.EctoTypes.IpAddress`
|
|
- Risk: Low (session tracking)
|
|
- Notes: Keep `validate_length(:ip_address, max: 45)` if present (optional, harmless)
|
|
- Tests to update: `test/towerops/accounts/browser_session_test.exs` (if exists)
|
|
|
|
2. **Towerops.Accounts.LoginAttempt** (`lib/towerops/accounts/login_attempt.ex`)
|
|
- Change: `field :ip_address, :string` → `field :ip_address, Towerops.EctoTypes.IpAddress`
|
|
- Risk: Low (logging only)
|
|
- Tests to update: Check for related tests
|
|
|
|
#### Medium Risk (migrate after low risk)
|
|
|
|
3. **Towerops.Snmp.ArpEntry** (`lib/towerops/snmp/schemas/arp_entry.ex`)
|
|
- Change: `field :ip_address, :string` → `field :ip_address, Towerops.EctoTypes.IpAddress`
|
|
- Risk: Medium (SNMP discovery populates this)
|
|
- Testing: Run SNMP discovery on test device after migration
|
|
- Tests to update: `test/towerops/snmp/*_test.exs`
|
|
|
|
4. **Towerops.Snmp.Neighbor** (`lib/towerops/snmp/schemas/neighbor.ex`)
|
|
- Change: `field :remote_address, :string` → `field :remote_address, Towerops.EctoTypes.IpAddress`
|
|
- Risk: Medium (optional field, may be nil)
|
|
- Note: Field name is `remote_address`, not `ip_address`
|
|
- Tests to update: SNMP neighbor discovery tests
|
|
|
|
#### High Risk (migrate last)
|
|
|
|
5. **Towerops.Devices.Device** (`lib/towerops/devices/device.ex`) ⚠️ **HIGH RISK**
|
|
- Change: `field :ip_address, :string` → `field :ip_address, Towerops.EctoTypes.IpAddress`
|
|
- **Remove validation code** (lines 121-141 in current version):
|
|
- `validate_ip_address/1` function
|
|
- `parse_ip/1` function
|
|
- Call to `|> validate_ip_address()` in changeset
|
|
- Risk: High (core Device schema, heavily used)
|
|
- Tests to update extensively: `test/towerops/devices_test.exs`
|
|
- Add assertions: `assert %Towerops.EctoTypes.IpAddress{} = device.ip_address`
|
|
- Verify: `assert device.ip_address.version == :ipv4`
|
|
- Test invalid IP still produces changeset error
|
|
- Test IPv6 addresses (lines 95-99 already exist)
|
|
|
|
6. **Towerops.Snmp.IpAddress** (`lib/towerops/snmp/schemas/ip_address.ex`) ⚠️ **COMPLEX**
|
|
- Change: `field :ip_address, :string` → `field :ip_address, Towerops.EctoTypes.IpAddress`
|
|
- **Keep**: `validate_prefix_length/1` (validates against `ip_type` field)
|
|
- **Add**: Helper to auto-populate `ip_type` from `IPAddress.version`
|
|
- Related fields: `ip_type`, `prefix_length`, `subnet_mask`
|
|
- Future consideration: Could derive `ip_type` from `IPAddress.version` and remove redundant field
|
|
- Risk: Complex (multiple related fields, SNMP-specific)
|
|
|
|
---
|
|
|
|
## Testing Strategy
|
|
|
|
### Per-Schema Testing Checklist
|
|
|
|
For each schema migration:
|
|
- [ ] Update schema field type
|
|
- [ ] Remove manual IP validation (if exists)
|
|
- [ ] Update related tests to assert on `%IpAddress{}` struct
|
|
- [ ] Run schema-specific tests: `mix test test/path/to/schema_test.exs`
|
|
- [ ] Run related integration tests
|
|
- [ ] Commit with message: `"Migrate [Schema] to IPAddress custom type"`
|
|
|
|
### Final Testing (after all 7 schemas)
|
|
|
|
- [ ] Run full test suite: `mix test`
|
|
- [ ] Check coverage: `mix test --cover` (target >90%)
|
|
- [ ] Run precommit checks: `mix precommit`
|
|
- [ ] Run Dialyzer: `mix dialyzer`
|
|
- [ ] Manual testing:
|
|
- [ ] Device CRUD operations
|
|
- [ ] SNMP discovery on test device
|
|
- [ ] Audit log creation
|
|
- [ ] GDPR data export
|
|
|
|
---
|
|
|
|
## Known Issues / Edge Cases Handled
|
|
|
|
1. **JSON Encoding**: Fixed by adding `@derive {Jason.Encoder, only: [:address]}` to IPAddress struct
|
|
- Required for GDPR data export API (`/api/v1/account/data`)
|
|
- Serializes as just the IP address string, not the full struct
|
|
|
|
2. **BSD-Style IP Notation**: `:inet.parse_address/1` accepts "192.168.1" as valid (interprets as "192.168.0.1")
|
|
- This is expected Erlang behavior
|
|
- Updated tests to document this, not reject it
|
|
|
|
3. **Database Compatibility**: No migrations needed
|
|
- Column type stays as `varchar(45)`
|
|
- Existing data valid on first load
|
|
- Ecto handles conversion transparently via `load/1` callback
|
|
|
|
4. **IPv6 Zone IDs**: Stripped before validation (`fe80::1%eth0` → `fe80::1`)
|
|
- Zone IDs are interface-specific and not stored
|
|
|
|
5. **CIDR Notation**: Rejected by custom type
|
|
- Use separate `prefix_length` field instead
|
|
- Prevents confusion between host addresses and network addresses
|
|
|
|
---
|
|
|
|
## Performance Considerations
|
|
|
|
- **Parsing Overhead**: ~1-5 microseconds per IP address (negligible)
|
|
- **Memory**: ~40 bytes per struct vs ~15-45 for string (~25 byte increase)
|
|
- **Query Performance**: Unchanged (database still stores strings)
|
|
- **JSON Serialization**: Efficient (only `address` field encoded)
|
|
|
|
---
|
|
|
|
## Rollback Plan
|
|
|
|
If issues arise after deployment:
|
|
|
|
1. Revert schema field changes to `:string` (git revert)
|
|
2. Restore `validate_ip_address/1` functions (git revert)
|
|
3. No database rollback needed (data unchanged, still stored as strings)
|
|
4. Redeploy previous version
|
|
|
|
---
|
|
|
|
## Future Phases (Not Started)
|
|
|
|
### Phase 2: Email Address Custom Type
|
|
- **Impact**: 4 schemas affected
|
|
- **Risk**: Medium (user-facing data)
|
|
- **Complexity**: Medium (normalization + case-insensitive handling)
|
|
- **Features**:
|
|
- Normalize to lowercase
|
|
- Split into local + domain parts
|
|
- Consistent validation across schemas
|
|
- **Schemas**:
|
|
- `Towerops.Accounts.User` (email field)
|
|
- Others TBD (need to search codebase)
|
|
|
|
### Phase 3: MAC Address Custom Type
|
|
- **Impact**: 3 schemas affected
|
|
- **Risk**: Low (SNMP-specific)
|
|
- **Complexity**: Medium (multiple format handling)
|
|
- **Features**:
|
|
- Handle multiple input formats (colon, hyphen, dot-separated)
|
|
- Normalize to colon-separated lowercase
|
|
- SNMP binary conversion support
|
|
- **Schemas**: TBD (need to search SNMP schemas)
|
|
|
|
### Phase 4: Optional Enhancements
|
|
- Derive `ip_type` from `IPAddress.version` in `Towerops.Snmp.IpAddress`
|
|
- Create `SubnetMask` custom type
|
|
- Create `IPNetwork` custom type (CIDR support)
|
|
|
|
---
|
|
|
|
## Commit History
|
|
|
|
1. `feat: add IPAddress custom Ecto type with comprehensive tests`
|
|
- Created `lib/towerops/ecto_types/ip_address.ex`
|
|
- Created `test/towerops/ecto_types/ip_address_test.exs`
|
|
- Added Jason.Encoder derivation for JSON compatibility
|
|
- All 44 tests passing
|
|
|
|
2. `docs: add Custom Ecto Types section to CLAUDE.md`
|
|
- Added comprehensive documentation after "Development Environment" section
|
|
- Includes pattern, usage examples, testing guidelines
|
|
|
|
3. `refactor: migrate AuditLog schema to IPAddress custom type`
|
|
- Changed `field :ip_address, :string` → `field :ip_address, Towerops.EctoTypes.IpAddress`
|
|
- No validation changes needed (schema didn't have IP validation)
|
|
- All account data controller tests passing
|
|
|
|
---
|
|
|
|
## Next Steps
|
|
|
|
**When resuming this work:**
|
|
|
|
1. Continue with low-risk schemas (BrowserSession, LoginAttempt)
|
|
2. Move to medium-risk SNMP schemas (ArpEntry, Neighbor)
|
|
3. Carefully migrate high-risk Device schema with extensive testing
|
|
4. Complete complex IpAddress schema migration
|
|
5. Run full test suite and coverage check
|
|
6. Deploy to staging and monitor
|
|
7. Deploy to production
|
|
|
|
**Estimated Time Remaining**: 4-8 hours for Phase 1 completion
|
|
|
|
---
|
|
|
|
## Reference Files
|
|
|
|
**Pattern to Follow**:
|
|
- `lib/towerops/ecto_types/json_any.ex` - Existing custom type
|
|
- `test/towerops/ecto_types/json_any_test.exs` - Test pattern (184 lines)
|
|
|
|
**Modified Files** (so far):
|
|
- `lib/towerops/ecto_types/ip_address.ex` (NEW)
|
|
- `test/towerops/ecto_types/ip_address_test.exs` (NEW)
|
|
- `lib/towerops/admin/audit_log.ex` (MODIFIED)
|
|
- `CLAUDE.md` (MODIFIED - added Custom Ecto Types section)
|
|
|
|
**Files to Modify** (remaining):
|
|
- `lib/towerops/accounts/browser_session.ex`
|
|
- `lib/towerops/accounts/login_attempt.ex`
|
|
- `lib/towerops/snmp/schemas/arp_entry.ex`
|
|
- `lib/towerops/snmp/schemas/neighbor.ex`
|
|
- `lib/towerops/devices/device.ex` (HIGH RISK)
|
|
- `lib/towerops/snmp/schemas/ip_address.ex` (COMPLEX)
|
|
|
|
---
|
|
|
|
## Success Criteria for Phase 1 Completion
|
|
|
|
- [x] IPAddress custom type implemented with all 4 callbacks
|
|
- [x] >90% test coverage on custom type (100% achieved)
|
|
- [ ] All 7 schemas migrated successfully (1/7 complete)
|
|
- [ ] Duplicate validation code removed from changesets
|
|
- [ ] All existing tests pass
|
|
- [ ] No database migrations required (verified ✅)
|
|
- [x] Documentation updated in CLAUDE.md
|
|
- [ ] Code reviewed and deployed to production
|
|
|
|
**Progress**: 30% complete (foundation laid, 1 schema migrated, 6 remaining)
|