cubemaster: fix integration tests panicking on unopened dao

[aptacc2421]

Fixes #1372

Summary

CubeMaster's integration test package panics at TestMain on current master: pkg/base/db.Init is now a thin wrapper over dao.Default(), which panics with "dao: Open has not been called yet" until the shared handle is established by dao.Open at app startup. Both the integration test bootstrap (TestMain) and mock-debug mode (cmd/cubemaster/main.go) call MockInit() before app.Run(), so mock_db() reached dao.Default() before any Open.

What Changed

Open the shared dao handle in mock_db() from the loaded DB config first. dao.Open is idempotent for the same identity, so the later open in app.Run()'s initDatabaseSchema is a no-op and schema migration still runs there.

Validation

• TestDemo passes against a reachable MySQL/Redis (PASS, ~120s).
• Without a database, the package now fails with a clean connection error instead of panicking.
• Single commit, single file (CubeMaster/integration/mock_init.go); gofmt clean.

Assisted-by: DeepSeek Harness:deepseek-v4-flash

[cubesandboxbot]

This is a field-for-field copy of the DBConfig → dao.Config mapping in app.initDatabaseSchema (cmd/cubemaster/app/main.go:242-255). The fix's correctness depends on dao.Open's identity-dedup (CubeDB/dao/dao.go:76-84), so these two copies must never drift: if a field is added to config.DBConfig/dao.Config and updated in only one place, the second dao.Open from app.Run stops being a no-op and instead fails with dao: already opened with ... (requested ...) — an opaque test-only failure far from the real cause.

Consider extracting the mapping into a single shared helper (e.g. dao.ConfigFromDBConfig(*config.DBConfig), or a ToDAOConfig() method on config.DBConfig) and calling it from both initDatabaseSchema and mock_db, so the idempotence guarantee holds by construction rather than by coincidence.

[cubesandboxbot]

Review: cubemaster: fix integration tests panicking on unopened dao (#1383)

Summary

This PR fixes #1372. CubeMaster/pkg/base/db.Init is now a thin wrapper over dao.Default(), which panics with dao: Open has not been called yet when invoked before dao.Open establishes the shared handle. Both the integration test bootstrap (integration.MockInit → mock_db) and mock-debug mode (cmd/cubemaster/main.go) call mock_db() before app.Run(), so the panic fired at TestMain/startup.

The fix:

1. Opens the shared dao handle in mock_db() from the loaded DB config before calling db.Init.
2. Extracts the config.DBConfig → dao.Config mapping into db.ConfigFromDBConfig, used by both mock_db() and app.initDatabaseSchema, so the two dao.Open identities cannot drift in the mapping.
3. Adds a unit test for the mapping.

Verdict

No blocking issues. The fix is correct and addresses the reported panic in both affected paths (integration tests and mock-debug mode). Verified against the base tree:

• Open ordering is safe: mock_db() runs on the main goroutine and completes before the app.Run() goroutine starts (TestMain) or before app.Run() (main.go), so every subsequent db.Init/dao.Default() sees the opened handle.
• dao.Open idempotence holds: the identity is computed over the normalized config (empty Driver defaults to mysql, Addr is normalized), and both call sites pass the same underlying config, so the later open in initDatabaseSchema is a no-op and dao.Migrate still runs there (including t_cube_host_type).
• Driver registration works in the integration binary: the mysql/postgres drivers are blank-imported by cmd/cubemaster/app, which the test imports, so resolveDriver succeeds at mock_db time.
• Field mapping matches the previous inline mapping in initDatabaseSchema exactly.
• Failure mode without a DB is now clean: dao.Open → GORM pings on Open (MySQL/Postgres dialects), so an unreachable DB surfaces as a dao open fail: ... bootstrap error rather than the ErrNotOpened panic.

Minor observations (non-blocking)

1. db.Init remains fragile for any future pre-dao.Open caller. Init still ignores its cfg and dao.Default() panics when the handle is unopened. This PR patches the two known call sites; a third bootstrap path (a new integration harness, a test helper) would hit the same panic. The pre-condition is documented on Init, but nothing enforces it. Consider whether Init should open the dao from the cfg it currently ignores as a safety net — weighing that against the existing "fail loudly on misconfiguration" intent.
2. ConfigFromDBConfig is a second copy of the dao.Config field list. The identity logic in CubeDB/dao already owns this list (configIdentity); if dao.Config gains a field (e.g. Extra), this mapping would silently omit it and the two bootstrap identities could drift — the exact problem this PR targets. A round-trip test or moving the mapping next to dao.Config would keep the list in one place.
3. Nil-config error message changed in initDatabaseSchema from dao: instance_db_config is not set to dao: db config is nil; the former named the offending config key. This path is effectively unreachable (guarded by coreInit), so impact is negligible.

Tests

TestConfigFromDBConfig covers the nil case and verifies the full field mapping. It runs in the same binary as TestInitReturnsDaoDefaultOnPostgreSQL (which dao.Close()s the global handle), but does not touch the global handle, so there is no interference. The integration scenario itself is covered by the existing integration suite.

---

This review was generated by an AI assistant.

[aptacc2421]

Addressed the review feedback: the DBConfig → dao.Config mapping is now extracted into db.ConfigFromDBConfig in pkg/base/db and used by both app startup (initDatabaseSchema) and the integration/mock-debug bootstrap (mock_db), so dao.Open's identity-dedup cannot drift between the two call sites. Added a focused unit test for the mapping; TestDemo still passes against a reachable MySQL/Redis.

[cubesandboxbot]

The error text instance_db_config is not set is specific to the app's config shape, but this helper lives in the generic pkg/base/db package and takes an arbitrary *config.DBConfig. Accurate for the two current call sites (both pass the instance DB config), but a neutral message such as errors.New("db config is nil") would keep the helper reusable without leaking a caller-specific field name. Non-blocking.

[cubesandboxbot]

Non-blocking: this refactor drops the actionable error text for a missing DB config. The old path reported dao: instance_db_config is not set — it names the exact yaml key an operator must add. The new path surfaces db config is nil, and because coreInit wraps initDatabaseSchema errors as dao migrate: %w, the final log reads dao migrate: dao: db config is nil — "dao" appears twice and the config key is gone.

No caller matches on the old string, and in the app this path is unreachable with a nil config because coreInit guards cfg.InstanceDBConfig == nil first. But mock-debug mode reaches ConfigFromDBConfig with whatever config.GetDbConfig() returns, so the message matters there. Keeping the field name in ConfigFromDBConfig's error (e.g. instance_db_config is not set / db config is nil) would preserve operator diagnostics.

[cubesandboxbot]

Non-blocking robustness note: this fix rests on dao.Open being idempotent for this config identity, and dao.Open keys identity on the full config — including Pwd, Addr, and every timeout/pool field (identityFromConfig in CubeDB/dao/dao.go). Both call sites read the same cfg.InstanceDBConfig global today, so they agree. But config.Init() starts a hotswap watcher (hotswap/file.go) that atomically swaps cfg on a config-file write; a write landing in the window between MockInit() and initDatabaseSchema would make the app's second dao.Open fail startup with dao: already opened with .... The window is tiny and unlikely in practice, but a one-line comment here (and in initDatabaseSchema) noting that the two opens must be built from a single config snapshot would make the coupling explicit and cheaper to debug.

[aptacc2421]

Addressed both new non-blocking notes:

1. ConfigFromDBConfig's nil error now reads "db config is nil: instance_db_config is not set" — keeps the neutral description while retaining the actionable YAML key for mock-debug mode (the app path is already guarded by coreInit).
2. Added comments in both mock_db and initDatabaseSchema noting the two dao.Open calls must be built from a single config snapshot, since a config hotswap between the two opens would trip dao.Open's identity check.

Build and the ConfigFromDBConfig unit test pass.

[cubesandboxbot]

Minor precision nit on the comment: the two call sites don't actually read the same snapshot. mock_db() reads the live global via config.GetDbConfig(), while this reads cfg.InstanceDBConfig from the cfg captured at Run() start. What ConfigFromDBConfig guarantees is the same mapping, not the same snapshot — if a config hot-reload landed in the window between MockInit() and app.Run(), the two identities would still differ and this second dao.Open would fail with already opened with ... (requested ...). The comment in mock_init.go already acknowledges that window honestly; consider rewording to "same mapping as" so the claim here doesn't overstate the protection.

[cubesandboxbot]

Trivial: the helper is generic over *config.DBConfig, but the nil error message hardcodes the instance-DB field name (instance_db_config is not set), and the wrap in initDatabaseSchema doubles the prefix (dao: db config is nil: instance_db_config is not set). A neutral message such as db config is nil would keep the helper accurate if it is ever reused for another DB config. Note this also changes the operator-visible error text from the previous dao: instance_db_config is not set.

[cubesandboxbot]

Minor diagnostic regression: the removed fmt.Errorf("dao: instance_db_config is not set") named the exact YAML key an operator must set, while this now surfaces the vaguer "dao: db config is nil" from ConfigFromDBConfig(nil). Consider preserving the key-specific wording — e.g., have ConfigFromDBConfig return an error that mentions instance_db_config, or keep a src == nil guard here so the message stays actionable.

[cubesandboxbot]

mocktest_Ctx is the mock-lifecycle context, cancelled at the end of TestMain. Opening the process-lifetime shared dao handle with it semantically ties the global handle to the mock's teardown; it's benign today because both drivers ignore ctx in Open, but context.Background() would express the handle's actual lifetime more accurately.

[fslongjin]

Thanks for the contribution! The overall behavioral change looks good to me. However, I think we can clean up the comments here a bit. We should focus on describing what the code does, rather than explaining the rationale behind this panic. Otherwise, it might confuse future readers and comes across as noise.

[cubesandboxbot]

The fix is correct for the two known bootstrap paths, but it leaves db.Init's contract fragile: Init still ignores its cfg argument and dao.Default() panics with ErrNotOpened if any future bootstrap path calls Init before dao.Open. The pre-condition is documented on Init, but nothing enforces it, so a third pre-dao.Open caller (a new integration harness, a test helper) would hit the same panic this PR fixes. Worth considering whether Init should fall back to opening the dao from the cfg it currently ignores when the handle is unopened — that would close the whole class of init-order bugs rather than this one site (balanced against the existing "fail loudly" intent).

[cubesandboxbot]

This helper is a second copy of the field list that dao.Config / identityFromConfig already owns. If dao.Config ever gains a field (e.g. the existing Extra), this mapping will silently omit it and the two bootstrap dao.Open identities could drift — the exact problem this PR sets out to prevent. Consider a round-trip test (config.DBConfig → dao.Config → identity) or moving the mapping next to dao.Config so the field list lives in one place.