Unify duplicate-name allowance & enforcement across sites, buildings, and racks
- Dominant language
- Go
- Stars
- 55
- Forks
- 16
- Avg merge
- 1d 22h
- Merged PRs (30d)
- 87
Description
**Priority: Low** (no active workstream; follow-up from #767 / PR #808)
## Summary
The three site-map entity types each enforce name uniqueness with a *different* scope, and the import layer's identity/classification logic silently assumes "name == identity". Where those two disagree — today, specifically **unassigned buildings** — an import can silently reuse or mutate an existing entity instead of creating a new one. Rather than patch the one symptom, we should decide and unify the duplicate-name model across sites, buildings, and racks at both the DB and import layers.
## Current state (DB constraints)
| Entity | Unique index | Scope | Duplicate names allowed? |
|---|---|---|---|
| Site | `uk_site_org_name` — `UNIQUE(org_id, name) WHERE deleted_at IS NULL` | org-wide | No |
| Rack | `uk_device_collection_org_type_label` — `UNIQUE(org_id, type, label) WHERE deleted_at IS NULL` | org-wide | No |
| Building | `uk_building_site_name` — `UNIQUE(site_id, name) WHERE site_id IS NOT NULL AND deleted_at IS NULL` | per-site, **NULL site_id excluded** | **Yes, when unassigned** |
Buildings are the odd one out on two axes: uniqueness is **site-scoped** (not org-wide), and **unassigned** buildings (`site_id IS NULL`) are intentionally *not* name-unique — a deliberate choice so cascade-unassign on site delete can't abort on a name collision (see comment in `000044_create_building_table.up.sql`).
## The motivating symptom (found by Codex on PR #808)
In `resolved.go`/`service.go`, `classifyTopologyActions` seeds a `"name:\x00"` alias for every existing entity, and `buildingRowIdentity` returns that same name key for a **blank-id** row. So a blank-id BUILDING row whose `(site, name)` matches an existing **unassigned** building is classified as `actionUpdate`/`actionNone` instead of `actionCreate`.
Effect (no server error — the classify and apply paths share the same key, so no crash / no `NotFound`):
- Fields identical → `actionNone`, the intended second unassigned building is silently **not created**.
- Fields differ → `actionUpdate`, the existing unassigned building is **mutated in place**, and any `NAME:` rack/miner refs bind to that existing record.
- If two same-named unassigned buildings already exist (the twin state the DB permits), the update targets a **nondeterministic** one (map last-writer-wins).
The alias behavior is *correct* for sites and racks (both genuinely org-wide name-unique) — buildings are the only place the assumption breaks.
## Why bigger-picture
The single-cell reference model from #767 already leans on identity semantics (`blank = create`, `id = existing`, `NAME:x = same-import create`). That contract is only coherent if "name is a stable identity" holds uniformly. Today it holds for sites/racks and half-holds for buildings. Options worth deciding *together* rather than per-entity:
1. **Make the rules uniform** — e.g. org-wide name uniqueness for all three (would require reconciling the unassigned-building cascade-unassign requirement), or
2. **Make the import layer honor the DB reality** — blank-id rows always classify as creates; surface unassigned-name collisions as the "UX warning" the migration comment already anticipates, instead of silently name-matching; and define deterministic behavior for the twin case.
## Acceptance criteria (to refine)
- [ ] A written, documented duplicate-name policy covering site / building / rack, at both DB-constraint and import-identity layers.
- [ ] Import classification no longer silently converts a blank-id row into an update of an existing entity where the DB permits duplicates.
- [ ] Deterministic, tested behavior for pre-existing duplicate (twin) unassigned buildings.
- [ ] Regression tests for: create-second-unassigned-building, `NAME:` ref resolution under duplicates, round-trip export/import.
## References
- PR #808 review thread (Codex P2): https://github.com/block/proto-fleet/pull/808#discussion_r3659038184
- `server/migrations/000044_create_building_table.up.sql` (unassigned-building rationale)
- `server/migrations/000043_create_site_table.up.sql`, `000012_create_device_collection_tables.up.sql`
- `classifyTopologyActions`, `rowNodeAction`, `buildingRowIdentity` in `server/internal/domain/sitemap/`
Contributor guide
Research direction
Start with classifyTopologyActions, rowNodeAction, and buildingRowIdentity in server/internal/domain/sitemap/, then read the referenced migration files, especially server/migrations/000044_create_building_table.up.sql. Trace the import classification and apply paths for blank-id buildings and NAME: references, and inspect PR #808's review context. Done means a decided policy, documented DB/import semantics, deterministic duplicate handling, and regression coverage for the listed scenarios.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, sql
- Domain
- backend, databases
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100