NM-293: Nodes Schema Migration

[VishalDalwadi]

Describe your changes

Provide Issue ticket number if applicable/not in title

Provide testing steps

Checklist before requesting a review

• My changes affect only 10 files or less.
• I have performed a self-review of my code and tested it.
• If it is a new feature, I have added thorough tests, my code is <= 1450 lines.
• If it is a bugfix, my code is <= 200 lines.
• My functions are <= 80 lines.
• I have had my code reviewed by a peer.
• My unit tests pass locally.
• Netmaker is awesome.

[tenki-reviewer]

Tenki Code Review - Complete

Files Reviewed: 6
Findings: 4

By Severity:

• 🔴 High: 1
• 🟠 Medium: 1
• 🟡 Low: 2

---

This PR introduces new GORM schema models for Nodes, Gateways, and PostureCheckViolations and wires them into the v1.5.2 migration, but contains two correctness bugs that will cause runtime query failures and one migration stub that silently skips all node migration. These should be addressed before merging.

Files Reviewed (6 files)

database/database.go
migrate/migrate_v1_5_2.go
schema/gateways.go
schema/models.go
schema/nodes.go
schema/posture_check_violations.go

[tenki-reviewer]

Overview

The PR adds three new schema models (Gateway, Node, PostureCheckViolation), registers them in ListModels(), reorders NODES_TABLE_NAME in the legacy database.Tables slice so it is created last, and extends the v1.5.2 migration to call a new migrateNodes function.

Critical / High Issues

Wrong column name and field value in Node network queries

In schema/nodes.go, GetByHostAndNetwork (line 57) and ListByNetwork (line 85) both query WHERE network = ? and pass n.Network — a *Network struct pointer — as the bind value. The actual DB column is network_id (from the NetworkID string field), so both the column name and the bound value are wrong. Any call to these methods will generate invalid SQL and silently return empty results or panic.

migrateNodes is a no-op stub

migrate/migrate_v1_5_2.go calls migrateNodes, which deserialises every legacy node record but then hits a // TODO comment and does nothing. The function returns nil (success), so a production upgrade will report migration success while leaving the new nodes_v1 table completely empty.

Medium Issues

Malformed GORM column tags in PostureCheckViolation

schema/posture_check_violations.go lines 10–12 use gorm:"primaryKey;evaluation_cycle_id" — a bare value that GORM silently ignores. The correct syntax is gorm:"primaryKey;column:evaluation_cycle_id". Functionally harmless today because GORM's snake_case defaults produce the same column names, but the explicit overrides are never applied.

Low Issues

tx.Rollback() return values discarded in UpsertViolations

schema/nodes.go UpsertViolations calls tx.Rollback() twice without capturing the result. If a rollback fails silently, the data may be in an inconsistent state with no indication to the caller or operator.

[tenki-reviewer]

🔴 Wrong column name and wrong field used in Node network queries (bug)

In schema/nodes.go, both GetByHostAndNetwork (line 57) and ListByNetwork (line 85) use WHERE network = ? with the value n.Network — a *Network association pointer. The actual DB column storing the network identifier is network_id (from the NetworkID string field), so the column name is wrong AND the value being passed is a struct pointer rather than a string ID. Any call to these methods will produce a malformed SQL query that either silently returns no results or causes a runtime error.

💡 Suggestion: Replace network = ? with network_id = ? and replace n.Network with n.NetworkID in both GetByHostAndNetwork (line 57) and ListByNetwork (line 85).

Suggested change

	Where("host_id = ? AND network = ?", n.HostID, n.Network).
	Where("host_id = ? AND network_id = ?", n.HostID, n.NetworkID).

📋 Prompt for AI Agents

In schema/nodes.go, fix two incorrect GORM queries that reference the wrong column name and pass the wrong value:

1. Line 57, in GetByHostAndNetwork: change Where("host_id = ? AND network = ?", n.HostID, n.Network) to Where("host_id = ? AND network_id = ?", n.HostID, n.NetworkID). The DB column is network_id (from the NetworkID string field), not network, and n.Network is a *Network pointer, not the string ID.
2. Line 85, in ListByNetwork: change Where("network = ?", n.Network) to Where("network_id = ?", n.NetworkID) for the same reason.

[tenki-reviewer]

🟠 Malformed GORM column name tags in PostureCheckViolation (bug)

In schema/posture_check_violations.go (lines 10-12), the composite primary key fields use invalid GORM tag syntax: gorm:"primaryKey;evaluation_cycle_id". GORM does not parse bare values without a key:value prefix — the correct syntax is gorm:"primaryKey;column:evaluation_cycle_id". Currently the column names still default correctly via snake_case conversion, so the schema works — but the explicit column name overrides are never applied and any future field rename will silently use the wrong column.

💡 Suggestion: Use the correct GORM tag format with the column: key prefix for all three fields.

Suggested change

	EvaluationCycleID string `gorm:"primaryKey;evaluation_cycle_id"`
	CheckID string `gorm:"primaryKey;check_id"`
	NodeID string `gorm:"primaryKey;node_id"`
	EvaluationCycleID string `gorm:"primaryKey;column:evaluation_cycle_id"`
	CheckID string `gorm:"primaryKey;column:check_id"`
	NodeID string `gorm:"primaryKey;column:node_id"`

📋 Prompt for AI Agents

In schema/posture_check_violations.go, lines 10-12, the GORM struct tags use invalid syntax for specifying column names. Fix them to use the correct column: key prefix:

• Line 10: change gorm:"primaryKey;evaluation_cycle_id" to gorm:"primaryKey;column:evaluation_cycle_id"
• Line 11: change gorm:"primaryKey;check_id" to gorm:"primaryKey;column:check_id"
• Line 12: change gorm:"primaryKey;node_id" to gorm:"primaryKey;column:node_id"
  GORM silently ignores the bare values, so the column names currently default correctly via snake_case, but the explicit overrides are not applied.

[tenki-reviewer]

🟡 tx.Rollback() return values silently discarded in UpsertViolations (bug)

In schema/nodes.go, UpsertViolations calls tx.Rollback() on lines 109 and 116 but discards the returned *gorm.DB. If a rollback fails — e.g. due to a dropped connection — the data may be left in an inconsistent state with no error propagated to the caller or logged for the operator.

💡 Suggestion: Capture and log the rollback error on both rollback call sites. Consider using a deferred rollback pattern that is a no-op after a successful commit.

📋 Prompt for AI Agents

In schema/nodes.go, UpsertViolations function (lines 106-121): both tx.Rollback() calls (lines 109 and 116) silently discard errors. Capture and log the rollback error at each site, for example: if rbErr := tx.Rollback().Error; rbErr != nil { logger.Log(0, fmt.Sprintf("rollback failed: %v", rbErr)) }. This ensures rollback failures are visible to operators.