Normalize AnalyzedModule (#31345)

This PR removes the `MappedModule` struct which contains duplicate data from its parent, `AnalyzedModule`. This maintains the duplicated data in `ModuleMetadata` in the db, so the data is still denormalized when it is serialized.

GitOrigin-RevId: 37a34f22848f7e99e2a5ad37ed912c1dd0c94faa

Author: emmaling27

crates/application/src/lib.rs

@@ -876,10 +876,7 @@ impl<RT: Runtime> Application<RT> {
         let Some(analyze_result) = &metadata.analyze_result else {
             return Ok(None);
         };
-        let Some(source_mapped) = &analyze_result.source_mapped else {
-            return Ok(None);
-        };
-        let Some(source_index) = source_mapped.source_index else {
+        let Some(source_index) = analyze_result.source_index else {
             return Ok(None);
         };
         let Some(full_source) = self.module_cache.get_module(&mut tx, path).await? else {

crates/isolate/src/environment/analyze.rs

@@ -62,7 +62,6 @@ use model::{
             AnalyzedModule,
             AnalyzedSourcePosition,
             FullModuleSource,
-            MappedModule,
             Visibility,
         },
         user_error::ModuleNotFoundError,
@@ -418,16 +417,10 @@ impl AnalyzeEnvironment {
                 });
 
             let analyzed_module = AnalyzedModule {
-                functions: functions.clone(),
-                http_routes: http_routes.clone(),
-                cron_specs: cron_specs.clone(),
-                // source_mapped should be deprecated and migrated away from in the future
-                source_mapped: Some(MappedModule {
-                    source_index,
-                    functions,
-                    http_routes,
-                    cron_specs,
-                }),
+                functions,
+                http_routes,
+                cron_specs,
+                source_index,
             };
             result.insert(path, analyzed_module);
         }

crates/isolate/src/tests/analyze.rs

@@ -80,21 +80,14 @@ async fn test_analyze_module(rt: TestRuntime) -> anyhow::Result<()> {
         ("action_in_v8", UdfType::Action, 28),
     ];
     assert_eq!(module.functions.len(), expected.len());
-    let source_mapped = module.source_mapped.as_ref().unwrap();
-    assert_eq!(source_mapped.functions.len(), expected.len());
+    assert!(module.source_index.is_some());
 
     for (i, (name, expected_type, mapped_lineno)) in expected.iter().enumerate() {
         let function = &module.functions[i];
         assert_eq!(&function.name[..], *name);
         assert_eq!(&function.udf_type, expected_type);
 
-        let mapped_function = &source_mapped.functions[i];
-        assert_eq!(&mapped_function.name[..], *name);
-        assert_eq!(
-            mapped_function.pos.as_ref().unwrap().start_lineno,
-            *mapped_lineno
-        );
-        assert_eq!(&mapped_function.udf_type, expected_type);
+        assert_eq!(function.pos.as_ref().unwrap().start_lineno, *mapped_lineno);
     }
 
     let http_path = "http.js".parse()?;
@@ -139,20 +132,15 @@ async fn test_analyze_module(rt: TestRuntime) -> anyhow::Result<()> {
             .len(),
         expected_routes_unmapped.len()
     );
-    let source_mapped = module.source_mapped.as_ref().unwrap();
-    assert!(source_mapped.http_routes.is_some());
-    assert_eq!(
-        module.http_routes.as_ref().unwrap().len(),
-        source_mapped.http_routes.as_ref().unwrap().len()
-    );
+    assert!(module.source_index.is_some());
     for (i, (path, method)) in expected_routes_unmapped.iter().enumerate() {
         let route = &module.http_routes.as_ref().unwrap()[i];
         assert_eq!(&route.route.path, path);
         assert_eq!(&route.route.method, method);
     }
 
     for (i, (path, method, mapped_pos)) in expected_routes_mapped.iter().enumerate() {
-        let mapped_route = &source_mapped
+        let mapped_route = &module
             .http_routes
             .as_ref()
             .expect("no mapped http_routes found")[i];
@@ -262,46 +250,6 @@ async fn test_analyze_function(rt: TestRuntime) -> anyhow::Result<()> {
 
     assert_eq!(
         &Vec::from(analyzed_module.functions.clone()),
-        &[
-            AnalyzedFunction::new(
-                "throwsError".parse()?,
-                // Don't check line numbers since those change on every `convex/server`
-                // change.
-                Some(AnalyzedSourcePosition {
-                    path: "sourceMaps.js".parse()?,
-                    start_lineno: analyzed_module.functions[0]
-                        .pos
-                        .as_ref()
-                        .unwrap()
-                        .start_lineno,
-                    start_col: analyzed_module.functions[0].pos.as_ref().unwrap().start_col,
-                }),
-                UdfType::Query,
-                Some(Visibility::Public),
-                ArgsValidator::Unvalidated,
-                ReturnsValidator::Unvalidated,
-            )?,
-            AnalyzedFunction::new(
-                "throwsErrorInDep".parse()?,
-                Some(AnalyzedSourcePosition {
-                    path: "sourceMaps.js".parse()?,
-                    start_lineno: analyzed_module.functions[1]
-                        .pos
-                        .as_ref()
-                        .unwrap()
-                        .start_lineno,
-                    start_col: analyzed_module.functions[1].pos.as_ref().unwrap().start_col,
-                }),
-                UdfType::Query,
-                Some(Visibility::Public),
-                ArgsValidator::Unvalidated,
-                ReturnsValidator::Unvalidated,
-            )?,
-        ],
-    );
-    let source_mapped = analyzed_module.source_mapped.unwrap();
-    assert_eq!(
-        &Vec::from(source_mapped.functions),
         &[
             AnalyzedFunction::new(
                 "throwsError".parse()?,
@@ -329,6 +277,7 @@ async fn test_analyze_function(rt: TestRuntime) -> anyhow::Result<()> {
             )?,
         ],
     );
+    analyzed_module.source_index.unwrap();
     Ok(())
 }
 
@@ -357,50 +306,6 @@ async fn test_analyze_internal_function(rt: TestRuntime) -> anyhow::Result<()> {
                 "myInternalQuery".parse()?,
                 // Don't check line numbers since those change on every `convex/server`
                 // change.
-                analyzed_module.functions[0].pos.clone(),
-                UdfType::Query,
-                Some(Visibility::Internal),
-                ArgsValidator::Unvalidated,
-                ReturnsValidator::Unvalidated,
-            )?,
-            AnalyzedFunction::new(
-                "publicQuery".parse()?,
-                // Don't check line numbers since those change on every `convex/server`
-                // change.
-                analyzed_module.functions[1].pos.clone(),
-                UdfType::Query,
-                Some(Visibility::Public),
-                ArgsValidator::Unvalidated,
-                ReturnsValidator::Unvalidated,
-            )?,
-            AnalyzedFunction::new(
-                "myInternalMutation".parse()?,
-                // Don't check line numbers since those change on every `convex/server`
-                // change.
-                analyzed_module.functions[2].pos.clone(),
-                UdfType::Mutation,
-                Some(Visibility::Internal),
-                ArgsValidator::Unvalidated,
-                ReturnsValidator::Unvalidated,
-            )?,
-            AnalyzedFunction::new(
-                "publicMutation".parse()?,
-                // Don't check line numbers since those change on every `convex/server`
-                // change.
-                analyzed_module.functions[3].pos.clone(),
-                UdfType::Mutation,
-                Some(Visibility::Public),
-                ArgsValidator::Unvalidated,
-                ReturnsValidator::Unvalidated,
-            )?,
-        ],
-    );
-    let source_mapped = analyzed_module.source_mapped.unwrap();
-    assert_eq!(
-        &Vec::from(source_mapped.functions.clone()),
-        &[
-            AnalyzedFunction::new(
-                "myInternalQuery".parse()?,
                 Some(AnalyzedSourcePosition {
                     path: "internal.js".parse()?,
                     start_lineno: 16,
@@ -413,6 +318,8 @@ async fn test_analyze_internal_function(rt: TestRuntime) -> anyhow::Result<()> {
             )?,
             AnalyzedFunction::new(
                 "publicQuery".parse()?,
+                // Don't check line numbers since those change on every `convex/server`
+                // change.
                 Some(AnalyzedSourcePosition {
                     path: "internal.js".parse()?,
                     start_lineno: 19,
@@ -425,6 +332,8 @@ async fn test_analyze_internal_function(rt: TestRuntime) -> anyhow::Result<()> {
             )?,
             AnalyzedFunction::new(
                 "myInternalMutation".parse()?,
+                // Don't check line numbers since those change on every `convex/server`
+                // change.
                 Some(AnalyzedSourcePosition {
                     path: "internal.js".parse()?,
                     start_lineno: 23,
@@ -437,6 +346,8 @@ async fn test_analyze_internal_function(rt: TestRuntime) -> anyhow::Result<()> {
             )?,
             AnalyzedFunction::new(
                 "publicMutation".parse()?,
+                // Don't check line numbers since those change on every `convex/server`
+                // change.
                 Some(AnalyzedSourcePosition {
                     path: "internal.js".parse()?,
                     start_lineno: 26,

crates/model/src/config/tests.rs

@@ -92,13 +92,13 @@ async fn test_config(rt: TestRuntime) -> anyhow::Result<()> {
                     functions: WithHeapSize::default(),
                     http_routes: None,
                     cron_specs: None,
-                    source_mapped: None,
+                    source_index: None,
                 },
                 p2 =>  AnalyzedModule {
                     functions: WithHeapSize::default(),
                     http_routes: None,
                     cron_specs: None,
-                    source_mapped: None,
+                    source_index: None,
                 },
             },
             None,
@@ -148,7 +148,7 @@ async fn test_config_large_modules(rt: TestRuntime) -> anyhow::Result<()> {
                     functions: WithHeapSize::default(),
                     http_routes: None,
                     cron_specs: None,
-                    source_mapped: None,
+                    source_index: None,
                 },
             )
         })

crates/model/src/modules/module_versions.rs

@@ -82,15 +82,16 @@ pub struct AnalyzedModule {
         )
     )]
     pub cron_specs: Option<WithHeapSize<BTreeMap<CronIdentifier, CronSpec>>>,
-    pub source_mapped: Option<MappedModule>,
+    /// Index of the module's original source in the source map.
+    pub source_index: Option<u32>,
 }
 
 impl HeapSize for AnalyzedModule {
     fn heap_size(&self) -> usize {
         self.functions.heap_size()
             + self.http_routes.heap_size()
             + self.cron_specs.heap_size()
-            + self.source_mapped.heap_size()
+            + self.source_index.heap_size()
     }
 }
 
@@ -107,6 +108,11 @@ impl TryFrom<AnalyzedModule> for SerializedAnalyzedModule {
     type Error = anyhow::Error;
 
     fn try_from(m: AnalyzedModule) -> anyhow::Result<Self> {
+        let source_mapped = m
+            .source_index
+            .as_ref()
+            .map(|_source_mapped| SerializedMappedModule::try_from(m.clone()))
+            .transpose()?;
         Ok(Self {
             functions: m
                 .functions
@@ -121,7 +127,7 @@ impl TryFrom<AnalyzedModule> for SerializedAnalyzedModule {
                 .cron_specs
                 .map(|specs| specs.into_iter().map(TryFrom::try_from).try_collect())
                 .transpose()?,
-            source_mapped: m.source_mapped.map(TryFrom::try_from).transpose()?,
+            source_mapped,
         })
     }
 }
@@ -147,7 +153,9 @@ impl TryFrom<SerializedAnalyzedModule> for AnalyzedModule {
                 .cron_specs
                 .map(|specs| specs.into_iter().map(TryFrom::try_from).try_collect())
                 .transpose()?,
-            source_mapped: m.source_mapped.map(TryFrom::try_from).transpose()?,
+            source_index: m
+                .source_mapped
+                .and_then(|mapped_module| mapped_module.source_index),
         })
     }
 }
@@ -517,43 +525,10 @@ impl Deref for AnalyzedHttpRoutes {
     }
 }
 
-#[derive(Debug, Clone, PartialEq, Eq)]
-#[cfg_attr(any(test, feature = "testing"), derive(proptest_derive::Arbitrary))]
-pub struct MappedModule {
-    // Index of the module's original source in the source map.
-    // TODO: consider removing this or moving this out of MappedModule into AnalyzedModule and
-    //  instead just include source information. This requires a decent migration from Dashboard
-    //  schema.
-    //  See https://github.com/get-convex/convex/pull/14382/files#r1252372646 for further discussion.
-    pub source_index: Option<u32>,
-    #[cfg_attr(
-        any(test, feature = "testing"),
-        proptest(
-            strategy = "value::heap_size::of(prop::collection::vec(any::<AnalyzedFunction>(), \
-                        0..4))"
-        )
-    )]
-    pub functions: WithHeapSize<Vec<AnalyzedFunction>>,
-    pub http_routes: Option<AnalyzedHttpRoutes>,
-    #[cfg_attr(
-        any(test, feature = "testing"),
-        proptest(
-            strategy = "prop::option::of(value::heap_size::of(prop::collection::btree_map(any::<CronIdentifier>(), \
-                        any::<CronSpec>(), 0..4)))"
-        )
-    )]
-    pub cron_specs: Option<WithHeapSize<BTreeMap<CronIdentifier, CronSpec>>>,
-}
-
-impl HeapSize for MappedModule {
-    fn heap_size(&self) -> usize {
-        self.source_index.heap_size()
-            + self.functions.heap_size()
-            + self.http_routes.heap_size()
-            + self.cron_specs.heap_size()
-    }
-}
-
+// TODO: consider denormalizing SerializedMappedModule into
+// SerializedAnalyzedModule and  instead just include source information. This
+// requires a decent migration from Dashboard  schema.
+//  See https://github.com/get-convex/convex/pull/14382/files#r1252372646 for further discussion.
 #[derive(Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
 struct SerializedMappedModule {
@@ -563,10 +538,14 @@ struct SerializedMappedModule {
     cron_specs: Option<Vec<SerializedNamedCronSpec>>,
 }
 
-impl TryFrom<MappedModule> for SerializedMappedModule {
+impl TryFrom<AnalyzedModule> for SerializedMappedModule {
     type Error = anyhow::Error;
 
-    fn try_from(m: MappedModule) -> anyhow::Result<Self> {
+    fn try_from(m: AnalyzedModule) -> anyhow::Result<Self> {
+        anyhow::ensure!(
+            m.source_index.is_some(),
+            "source_index must be set to be serializing into SerializedMappedModule"
+        );
         Ok(Self {
             source_index: m.source_index,
             functions: m
@@ -586,32 +565,6 @@ impl TryFrom<MappedModule> for SerializedMappedModule {
     }
 }
 
-impl TryFrom<SerializedMappedModule> for MappedModule {
-    type Error = anyhow::Error;
-
-    fn try_from(m: SerializedMappedModule) -> anyhow::Result<Self> {
-        Ok(Self {
-            source_index: m.source_index,
-            functions: m
-                .functions
-                .into_iter()
-                .map(TryFrom::try_from)
-                .try_collect()?,
-            http_routes: m
-                .http_routes
-                .map(|routes| {
-                    let routes = routes.into_iter().map(TryFrom::try_from).try_collect()?;
-                    anyhow::Ok(AnalyzedHttpRoutes::new(routes))
-                })
-                .transpose()?,
-            cron_specs: m
-                .cron_specs
-                .map(|specs| specs.into_iter().map(TryFrom::try_from).try_collect())
-                .transpose()?,
-        })
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use value::{