Skip to content

Commit 22fcd3d

Browse files
authored
ENG-1160: fix default inframap overriding db name on first migration (#2955)
<!-- CURSOR_SUMMARY --> > [!NOTE] > Use project-derived `InfrastructureMap::empty_from_project` instead of defaults to keep custom DB names on first migration; update call sites and add tests. > > - **Core/Infra Map**: > - Add `InfrastructureMap::empty_from_project(project)` to initialize an empty map using project `clickhouse_config.db_name`. > - **Refactors to use `empty_from_project` (preserve DB name)**: > - `local_webserver.rs`: admin reality-check, integrate-changes, and admin reconciliation paths load empty map from project when state is missing. > - `routines.rs`: serverless remote inframap load fallback updated. > - `routines/migrate.rs`: migration state load fallback updated. > - `framework/core/plan.rs`: replace manual/default fallbacks with `empty_from_project`. > - **Planning/Tests**: > - Add tests ensuring custom database name persists across proto round-trips and reconciliation, and when loading old protos without `default_database`. > - Minor import cleanups and test utilities updates. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 93cf1c1. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent bfdae6c commit 22fcd3d

File tree

5 files changed

+212
-23
lines changed

5 files changed

+212
-23
lines changed

apps/framework-cli/src/cli/local_webserver.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -982,7 +982,7 @@ async fn admin_reality_check_route(
982982
// Load infrastructure map from Redis
983983
let infra_map = match InfrastructureMap::load_from_redis(redis_client).await {
984984
Ok(Some(map)) => map,
985-
Ok(None) => InfrastructureMap::default(),
985+
Ok(None) => InfrastructureMap::empty_from_project(project),
986986
Err(e) => {
987987
return Response::builder()
988988
.status(StatusCode::INTERNAL_SERVER_ERROR)
@@ -3119,7 +3119,7 @@ async fn admin_integrate_changes_route(
31193119

31203120
let mut infra_map = match InfrastructureMap::load_from_redis(redis_client).await {
31213121
Ok(Some(infra_map)) => infra_map,
3122-
Ok(None) => InfrastructureMap::default(),
3122+
Ok(None) => InfrastructureMap::empty_from_project(project),
31233123
Err(e) => {
31243124
return IntegrationError::InternalError(format!(
31253125
"Failed to load infrastructure map: {e}"
@@ -3236,7 +3236,7 @@ async fn get_admin_reconciled_inframap(
32363236
// Load current map from state storage (these are the tables under Moose management)
32373237
let current_map = match state_storage.load_infrastructure_map().await {
32383238
Ok(Some(infra_map)) => infra_map,
3239-
Ok(None) => InfrastructureMap::default(),
3239+
Ok(None) => InfrastructureMap::empty_from_project(project),
32403240
Err(e) => {
32413241
return Err(crate::framework::core::plan::PlanningError::Other(
32423242
anyhow::anyhow!("Failed to load infrastructure map from state storage: {e}"),

apps/framework-cli/src/cli/routines.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1204,7 +1204,7 @@ async fn get_remote_inframap_serverless(
12041204
let remote_infra_map = state_storage
12051205
.load_infrastructure_map()
12061206
.await?
1207-
.unwrap_or_default();
1207+
.unwrap_or_else(|| InfrastructureMap::empty_from_project(project));
12081208

12091209
// Reconcile with actual database state to detect manual changes
12101210
let reconciled_infra_map = if project.features.olap {

apps/framework-cli/src/cli/routines/migrate.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,7 @@ pub async fn execute_migration(
465465
e,
466466
)
467467
})?
468-
.unwrap_or_default();
468+
.unwrap_or_else(|| InfrastructureMap::empty_from_project(project));
469469

470470
let current_infra_map = if project.features.olap {
471471
use crate::framework::core::plan::reconcile_with_reality;

apps/framework-cli/src/framework/core/infrastructure_map.rs

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -550,6 +550,42 @@ pub struct InfrastructureMap {
550550
}
551551

552552
impl InfrastructureMap {
553+
/// Creates an empty infrastructure map from project configuration.
554+
///
555+
/// This is used when loading state from storage returns None (first migration scenario).
556+
/// All fields are initialized to empty/default values except `default_database`,
557+
/// which is extracted from the project's ClickHouse configuration.
558+
///
559+
/// # Why explicit field listing?
560+
/// This method explicitly lists all fields instead of using `..Default::default()`
561+
/// to force compiler errors when new fields are added to InfrastructureMap.
562+
/// This ensures developers must consciously decide: does this new field need
563+
/// project configuration (like default_database), or can it safely default to empty?
564+
///
565+
/// # Arguments
566+
/// * `project` - The project context containing configuration
567+
///
568+
/// # Returns
569+
/// An empty infrastructure map with the database name from project configuration
570+
pub fn empty_from_project(project: &Project) -> Self {
571+
Self {
572+
default_database: project.clickhouse_config.db_name.clone(),
573+
topics: Default::default(),
574+
api_endpoints: Default::default(),
575+
tables: Default::default(),
576+
views: Default::default(),
577+
topic_to_table_sync_processes: Default::default(),
578+
topic_to_topic_sync_processes: Default::default(),
579+
function_processes: Default::default(),
580+
block_db_processes: OlapProcess {},
581+
consumption_api_web_server: ConsumptionApiWebServer {},
582+
orchestration_workers: Default::default(),
583+
sql_resources: Default::default(),
584+
workflows: Default::default(),
585+
web_apps: Default::default(),
586+
}
587+
}
588+
553589
/// Creates a new infrastructure map from a project and primitive map
554590
///
555591
/// This is the primary constructor for creating an infrastructure map. It transforms

apps/framework-cli/src/framework/core/plan.rs

Lines changed: 171 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,6 @@
1313
///
1414
/// The resulting plan is then used by the execution module to apply the changes.
1515
use crate::framework::core::infra_reality_checker::{InfraRealityChecker, RealityCheckError};
16-
use crate::framework::core::infrastructure::consumption_webserver::ConsumptionApiWebServer;
17-
use crate::framework::core::infrastructure::olap_process::OlapProcess;
1816
use crate::framework::core::infrastructure_map::{
1917
InfraChanges, InfrastructureMap, OlapChange, TableChange,
2018
};
@@ -302,22 +300,8 @@ pub async fn plan_changes(
302300
.unwrap_or("Could not serialize current infrastructure map".to_string())
303301
);
304302

305-
let current_map_or_empty = current_infra_map.unwrap_or_else(|| InfrastructureMap {
306-
default_database: project.clickhouse_config.db_name.clone(),
307-
topics: Default::default(),
308-
api_endpoints: Default::default(),
309-
tables: Default::default(),
310-
views: Default::default(),
311-
topic_to_table_sync_processes: Default::default(),
312-
topic_to_topic_sync_processes: Default::default(),
313-
function_processes: Default::default(),
314-
block_db_processes: OlapProcess {},
315-
consumption_api_web_server: ConsumptionApiWebServer {},
316-
orchestration_workers: Default::default(),
317-
sql_resources: Default::default(),
318-
workflows: Default::default(),
319-
web_apps: Default::default(),
320-
});
303+
let current_map_or_empty =
304+
current_infra_map.unwrap_or_else(|| InfrastructureMap::empty_from_project(project));
321305

322306
// Reconcile the current map with reality before diffing, but only if OLAP is enabled
323307
let reconciled_map = if project.features.olap {
@@ -403,6 +387,7 @@ mod tests {
403387
use crate::infrastructure::olap::OlapChangesError;
404388
use crate::infrastructure::olap::OlapOperations;
405389
use async_trait::async_trait;
390+
use protobuf::Message;
406391

407392
// Mock OLAP client for testing
408393
struct MockOlapClient {
@@ -744,4 +729,172 @@ mod tests {
744729
// Compare the tables to ensure they are identical
745730
assert_eq!(reconciled.tables.values().next().unwrap(), &table);
746731
}
732+
733+
#[tokio::test]
734+
async fn test_custom_database_name_preserved_on_first_migration() {
735+
// This test reproduces ENG-1160: custom database name should be preserved
736+
// on first migration when no prior state exists
737+
738+
const CUSTOM_DB_NAME: &str = "my_custom_database";
739+
740+
// Create a project with a CUSTOM database name (not "local")
741+
let mut project = create_test_project();
742+
project.clickhouse_config.db_name = CUSTOM_DB_NAME.to_string();
743+
744+
// Create an infrastructure map as if it's the target map
745+
// (this simulates what InfrastructureMap::new would create)
746+
let mut target_map = InfrastructureMap {
747+
default_database: CUSTOM_DB_NAME.to_string(),
748+
..Default::default()
749+
};
750+
751+
// Add a test table to make it realistic
752+
let table = create_test_table("test_table");
753+
target_map.tables.insert(table.id(CUSTOM_DB_NAME), table);
754+
755+
// Simulate storing to Redis (serialize to protobuf)
756+
let proto_bytes = target_map.to_proto().write_to_bytes().unwrap();
757+
758+
// Simulate loading from Redis (deserialize from protobuf)
759+
let loaded_map = InfrastructureMap::from_proto(proto_bytes).unwrap();
760+
761+
// ASSERTION: The custom database name should be preserved after round-trip
762+
assert_eq!(
763+
loaded_map.default_database, CUSTOM_DB_NAME,
764+
"Custom database name '{}' was not preserved after serialization round-trip. Got: '{}'",
765+
CUSTOM_DB_NAME, loaded_map.default_database
766+
);
767+
768+
// Also verify that reconciliation preserves the database name
769+
let mock_client = MockOlapClient { tables: vec![] };
770+
771+
let target_table_names = HashSet::new();
772+
let reconciled =
773+
reconcile_with_reality(&project, &loaded_map, &target_table_names, mock_client)
774+
.await
775+
.unwrap();
776+
777+
assert_eq!(
778+
reconciled.default_database, CUSTOM_DB_NAME,
779+
"Custom database name '{}' was not preserved after reconciliation. Got: '{}'",
780+
CUSTOM_DB_NAME, reconciled.default_database
781+
);
782+
}
783+
784+
#[tokio::test]
785+
async fn test_loading_old_proto_without_default_database_field() {
786+
// This test simulates loading an infrastructure map from an old proto
787+
// that was serialized before the default_database field was added (field #15)
788+
789+
const CUSTOM_DB_NAME: &str = "my_custom_database";
790+
791+
// Create a project with a CUSTOM database name
792+
let mut project = create_test_project();
793+
project.clickhouse_config.db_name = CUSTOM_DB_NAME.to_string();
794+
795+
// Manually create a proto WITHOUT the default_database field
796+
// by creating an empty proto (which won't have default_database set)
797+
use crate::proto::infrastructure_map::InfrastructureMap as ProtoInfrastructureMap;
798+
let old_proto = ProtoInfrastructureMap::new();
799+
// Note: NOT setting old_proto.default_database - simulates old proto
800+
801+
let proto_bytes = old_proto.write_to_bytes().unwrap();
802+
803+
// Load it back
804+
let loaded_map = InfrastructureMap::from_proto(proto_bytes).unwrap();
805+
806+
// BUG: When loading an old proto, the default_database will be empty string ""
807+
// This should fail if the bug exists
808+
println!(
809+
"Loaded map default_database: '{}'",
810+
loaded_map.default_database
811+
);
812+
813+
// The bug manifests here: loading an old proto results in empty string for default_database
814+
// which might get replaced with DEFAULT_DATABASE_NAME ("local") somewhere
815+
assert_eq!(
816+
loaded_map.default_database, "",
817+
"Old proto should have empty default_database, got: '{}'",
818+
loaded_map.default_database
819+
);
820+
821+
// Now test reconciliation - this is where the fix should be applied
822+
let mock_client = MockOlapClient { tables: vec![] };
823+
824+
let target_table_names = HashSet::new();
825+
let reconciled =
826+
reconcile_with_reality(&project, &loaded_map, &target_table_names, mock_client)
827+
.await
828+
.unwrap();
829+
830+
// After reconciliation, the database name should be set from the project config
831+
assert_eq!(
832+
reconciled.default_database, CUSTOM_DB_NAME,
833+
"After reconciliation, custom database name should be set from project. Got: '{}'",
834+
reconciled.default_database
835+
);
836+
}
837+
838+
#[tokio::test]
839+
#[allow(clippy::unnecessary_literal_unwrap)] // Test intentionally demonstrates buggy pattern
840+
async fn test_bug_eng_1160_default_overwrites_custom_db_name() {
841+
// This test demonstrates the actual bug pattern found in local_webserver.rs
842+
// where `Ok(None) => InfrastructureMap::default()` is used instead of
843+
// creating an InfrastructureMap with the project's db_name.
844+
845+
const CUSTOM_DB_NAME: &str = "my_custom_database";
846+
let mut project = create_test_project();
847+
project.clickhouse_config.db_name = CUSTOM_DB_NAME.to_string();
848+
849+
// Simulate the buggy pattern: when no state exists, use default()
850+
let loaded_map_buggy: Option<InfrastructureMap> = None;
851+
let buggy_map = loaded_map_buggy.unwrap_or_default();
852+
853+
// BUG: This will use "local" instead of "my_custom_database"
854+
assert_eq!(
855+
buggy_map.default_database, "local",
856+
"BUG REPRODUCED: default() returns 'local' instead of project's db_name"
857+
);
858+
assert_ne!(
859+
buggy_map.default_database, CUSTOM_DB_NAME,
860+
"Bug confirmed: custom database name is lost"
861+
);
862+
863+
// CORRECT PATTERN: Create InfrastructureMap with project's config
864+
let loaded_map_correct: Option<InfrastructureMap> = None;
865+
let correct_map =
866+
loaded_map_correct.unwrap_or_else(|| InfrastructureMap::empty_from_project(&project));
867+
868+
assert_eq!(
869+
correct_map.default_database, CUSTOM_DB_NAME,
870+
"Correct pattern: InfrastructureMap uses project's db_name"
871+
);
872+
}
873+
874+
#[test]
875+
fn test_only_default_database_field_is_config_driven() {
876+
// Verify that default_database is the ONLY field in InfrastructureMap
877+
// that comes directly from project clickhouse_config.db_name.
878+
// This is the critical field for ENG-1160: when InfrastructureMap::default()
879+
// is used instead of InfrastructureMap::new(), default_database gets "local"
880+
// instead of the project's configured database name.
881+
882+
const CUSTOM_DB_NAME: &str = "custom_db";
883+
let mut project = create_test_project();
884+
project.clickhouse_config.db_name = CUSTOM_DB_NAME.to_string();
885+
886+
let primitive_map = PrimitiveMap::default();
887+
let infra_map = InfrastructureMap::new(&project, primitive_map);
888+
889+
// Critical: default_database must be set from project config
890+
assert_eq!(
891+
infra_map.default_database, CUSTOM_DB_NAME,
892+
"default_database must use project's clickhouse_config.db_name, not hardcoded 'local'"
893+
);
894+
895+
// Note: Other fields may be populated based on project properties
896+
// (e.g., orchestration_workers is created based on project.language)
897+
// but they don't directly use clickhouse_config.db_name.
898+
// The bug in ENG-1160 is specifically about default_database being hardcoded to "local".
899+
}
747900
}

0 commit comments

Comments
 (0)