Skip to content

Comments

refactor(api): export shared PG query builders for cloud store#5859

Open
gustavosbarreto wants to merge 5 commits intomasterfrom
feature/cloud-pg-store
Open

refactor(api): export shared PG query builders for cloud store#5859
gustavosbarreto wants to merge 5 commits intomasterfrom
feature/cloud-pg-store

Conversation

@gustavosbarreto
Copy link
Member

Summary

  • Fix nil panic in DeviceToModel when Namespace relation is not loaded
  • Add Admin field to PG user entity and migration
  • Export SessionSelectQuery, UserSelectQuery, and ApplyOptions so the cloud store can reuse query logic with its own extended entities
  • Make TagTarget extensible via RegisterTagTarget so cloud can add custom targets without modifying core

Context

The cloud PG store needs to override UserResolve, UserList, SessionResolve, and SessionList because core's ColumnExpr("table.*") returns cloud-added columns (mfa_*, converted) that core entities can't scan. By extracting shared query builders, cloud can call them with its own entities that embed core entities + cloud fields.

Test plan

  • Existing core PG store tests pass
  • Cloud PG store tests pass (see companion PR in cloud repo)

@gustavosbarreto gustavosbarreto requested a review from a team as a code owner February 21, 2026 19:03
@gustavosbarreto
Copy link
Member Author

/review

@github-actions
Copy link

Code Review Summary - Reviewed 8 changed files across code quality, security, testing, Go patterns, and architecture. Posted 5 inline comments. Critical: migration 002 modification will not apply to existing databases. High: orphaned firewall_rules references and extensibility gap. Medium: RegisterTagTarget needs docs and dedup, stale comment in user.go, missing regression test for nil panic fix. Positives: clean refactoring of exported query builders, correct nil-safety fix, consistent Admin field addition, no injection or credential issues. Feedback provided as inline comments. Comment /review when ready for next iteration.

DeviceToModel panics when the Namespace relation is nil (e.g. when
queried without Relation("Namespace")). Initialize Namespace to empty
string and only set it when the relation is loaded.
Add Admin bool field to the PG user entity and the CREATE TABLE
migration so the column is available for cloud/enterprise features.
Export SessionSelectQuery, UserSelectQuery, UserResolverToString, and
ApplyOptions so the cloud store can reuse the same query logic with
its own extended entities instead of duplicating SQL.
Replace hardcoded TagTargetFirewallRule with RegisterTagTarget so
cloud/enterprise can register custom targets without modifying core.
Remove firewall_rules from mongo TagDelete since it belongs to cloud.
@otavio
Copy link
Member

otavio commented Feb 22, 2026

/review

@github-actions
Copy link

I reviewed this PR across code quality, security, testing, language patterns, and architecture and found no issues to report. The code looks good as-is. The feedback from the previous review round was addressed well - the RegisterTagTarget dedup and init-only documentation were applied, the stale comment was cleaned up, and the missing regression test for the nil panic fix is tracked in #5862.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants