block / block/proto-fleet

Unify duplicate-name allowance & enforcement across sites, buildings, and racks

Open
#814 0 comments 0 reactions 0 assignees View on GitHub
enhancement server
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.