Move FunctionName to sync_types (#25401)

Sujay Jayakar · Convex, Inc. · commit 2a88609e8eb0 · 2024-05-07T22:28:49.000Z
I'm going to start adding function paths that are namespaced by component, so I wanted to be able to reuse `FunctionName`. This also makes our function name handling more typesafe.

GitOrigin-RevId: c309d9e310d9a171b2872de687efe9bc5dec6eef
diff --git a/crates/application/src/application_function_runner/mod.rs b/crates/application/src/application_function_runner/mod.rs
@@ -1650,10 +1650,8 @@ impl<RT: Runtime> ApplicationFunctionRunner<RT> {
                     ))));
                 };
                 let name = cron_spec.udf_path.function_name();
-                let Some(scheduled_function) = scheduled_module
-                    .functions
-                    .iter()
-                    .find(|f| f.name.as_ref() == name)
+                let Some(scheduled_function) =
+                    scheduled_module.functions.iter().find(|f| &f.name == name)
                 else {
                     return Ok(Err(JsError::from_message(format!(
                         "The cron job '{identifier}' schedules a function that does not exist: {}",
diff --git a/crates/application/src/lib.rs b/crates/application/src/lib.rs
@@ -236,6 +236,7 @@ use sync_types::{
     AuthenticationToken,
     CanonicalizedModulePath,
     CanonicalizedUdfPath,
+    FunctionName,
     UdfPath,
 };
 use table_summary_worker::{
@@ -1831,7 +1832,7 @@ impl<RT: Runtime> Application<RT> {
         // 2. get the function type
         let mut analyzed_function = None;
         for function in &analyzed_module.functions {
-            if function.name.as_ref() == "default" {
+            if function.name.is_default_export() {
                 analyzed_function = Some(function.clone());
             } else {
                 anyhow::bail!("Only `export default` is supported.");
@@ -1852,7 +1853,7 @@ impl<RT: Runtime> Application<RT> {
             .await?;
 
         // 4. run the function within the transaction
-        let path = CanonicalizedUdfPath::new(module_path, "default".to_owned());
+        let path = CanonicalizedUdfPath::new(module_path, FunctionName::default_export());
         let arguments = parse_udf_args(&path.clone().into(), args)?;
         let (result, log_lines) = match analyzed_function.udf_type {
             UdfType::Query => self
diff --git a/crates/application/src/tests/analyze.rs b/crates/application/src/tests/analyze.rs
@@ -79,13 +79,13 @@ async fn test_analyze(rt: ProdRuntime) -> anyhow::Result<()> {
     assert_eq!(modules[&"a.js".parse()?].functions.len(), 1);
     let module = &modules[&"a.js".parse()?].functions[0];
     assert_eq!(module.udf_type, UdfType::Query);
-    assert_eq!(module.name.as_ref(), "isolateFunction");
+    assert_eq!(&module.name[..], "isolateFunction");
     assert!(module.pos.is_none());
 
     assert_eq!(modules[&"b.js".parse()?].functions.len(), 1);
     let module = &modules[&"b.js".parse()?].functions[0];
     assert_eq!(module.udf_type, UdfType::Action);
-    assert_eq!(module.name.as_ref(), "nodeFunction");
+    assert_eq!(&module.name[..], "nodeFunction");
     assert!(module.pos.is_none());
 
     Ok(())
@@ -178,19 +178,19 @@ export { hello, internalHello };
 
     assert_eq!(modules[&"isolate_source.js".parse()?].functions.len(), 2);
     let module = &modules[&"isolate_source.js".parse()?];
-    assert_eq!(module.functions[0].name.as_ref(), "hello");
+    assert_eq!(&module.functions[0].name[..], "hello");
     assert_eq!(module.functions[0].udf_type, UdfType::Action);
     assert_eq!(module.functions[0].pos.as_ref().unwrap().start_lineno, 28);
-    assert_eq!(module.functions[1].name.as_ref(), "internalHello");
+    assert_eq!(&module.functions[1].name[..], "internalHello");
     assert_eq!(module.functions[1].udf_type, UdfType::Action);
     assert_eq!(module.functions[1].pos.as_ref().unwrap().start_lineno, 31);
 
     assert_eq!(modules[&"node_source.js".parse()?].functions.len(), 2);
     let module = &modules[&"node_source.js".parse()?];
-    assert_eq!(module.functions[0].name.as_ref(), "hello");
+    assert_eq!(&module.functions[0].name[..], "hello");
     assert_eq!(module.functions[0].udf_type, UdfType::Action);
     assert_eq!(module.functions[0].pos.as_ref().unwrap().start_lineno, 28);
-    assert_eq!(module.functions[1].name.as_ref(), "internalHello");
+    assert_eq!(&module.functions[1].name[..], "internalHello");
     assert_eq!(module.functions[1].udf_type, UdfType::Action);
     assert_eq!(module.functions[1].pos.as_ref().unwrap().start_lineno, 31);
 
@@ -231,13 +231,13 @@ async fn test_analyze_crons(rt: ProdRuntime) -> anyhow::Result<()> {
     assert_eq!(modules[&"a.js".parse()?].functions.len(), 1);
     let module = &modules[&"a.js".parse()?].functions[0];
     assert_eq!(module.udf_type, UdfType::Query);
-    assert_eq!(module.name.as_ref(), "isolateFunction");
+    assert_eq!(&module.name[..], "isolateFunction");
     assert!(module.pos.is_none());
 
     assert_eq!(modules[&"b.js".parse()?].functions.len(), 1);
     let module = &modules[&"b.js".parse()?].functions[0];
     assert_eq!(module.udf_type, UdfType::Action);
-    assert_eq!(module.name.as_ref(), "nodeFunction");
+    assert_eq!(&module.name[..], "nodeFunction");
     assert!(module.pos.is_none());
 
     let application = Application::new_for_tests(&rt).await?;
diff --git a/crates/convex/sync_types/src/function_name.rs b/crates/convex/sync_types/src/function_name.rs
@@ -0,0 +1,59 @@
+use std::{
+    ops::Deref,
+    str::FromStr,
+};
+
+use crate::identifier::check_valid_identifier;
+
+#[derive(Debug, Clone, PartialEq, Eq, Ord, PartialOrd, Hash, derive_more::Display)]
+pub struct FunctionName(String);
+
+impl FromStr for FunctionName {
+    type Err = anyhow::Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        check_valid_identifier(s)?;
+        Ok(FunctionName(s.to_string()))
+    }
+}
+
+impl Deref for FunctionName {
+    type Target = str;
+
+    fn deref(&self) -> &str {
+        &self.0[..]
+    }
+}
+
+impl From<FunctionName> for String {
+    fn from(function_name: FunctionName) -> Self {
+        function_name.0
+    }
+}
+
+impl FunctionName {
+    pub fn default_export() -> Self {
+        Self("default".to_string())
+    }
+
+    pub fn is_default_export(&self) -> bool {
+        &self.0 == "default"
+    }
+}
+
+#[cfg(any(test, feature = "testing"))]
+impl proptest::arbitrary::Arbitrary for FunctionName {
+    type Parameters = ();
+    type Strategy = proptest::strategy::BoxedStrategy<Self>;
+
+    fn arbitrary_with(_: Self::Parameters) -> Self::Strategy {
+        use proptest::prelude::*;
+
+        use crate::identifier::arbitrary_regexes::IDENTIFIER_REGEX;
+        IDENTIFIER_REGEX
+            .prop_filter_map("Invalid IdentifierFieldName", |s| {
+                FunctionName::from_str(&s).ok()
+            })
+            .boxed()
+    }
+}
diff --git a/crates/convex/sync_types/src/lib.rs b/crates/convex/sync_types/src/lib.rs
@@ -1,4 +1,5 @@
 pub mod backoff;
+pub mod function_name;
 pub mod headers;
 pub mod identifier;
 pub mod json;
@@ -10,6 +11,7 @@ pub mod types;
 pub mod udf_path;
 
 pub use crate::{
+    function_name::FunctionName,
     module_path::{
         CanonicalizedModulePath,
         ModulePath,
diff --git a/crates/convex/sync_types/src/udf_path.rs b/crates/convex/sync_types/src/udf_path.rs
@@ -10,15 +10,15 @@ use super::module_path::{
     CanonicalizedModulePath,
     ModulePath,
 };
-use crate::identifier::check_valid_identifier;
+use crate::function_name::FunctionName;
 
 /// User-specified path to a function, consisting of a module path and an
 /// optional function name, separated by a colon. If a function name isn't
 /// provided, the UDF loader uses the default export from the module.
 #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct UdfPath {
     module: ModulePath,
-    function: Option<String>,
+    function: Option<FunctionName>,
 }
 
 impl UdfPath {
@@ -33,8 +33,8 @@ impl UdfPath {
     }
 
     /// What is the function name for this UDF?
-    pub fn function_name(&self) -> &str {
-        self.function.as_ref().map(|s| &s[..]).unwrap_or("default")
+    pub fn function_name(&self) -> Option<&FunctionName> {
+        self.function.as_ref()
     }
 
     pub fn assume_canonicalized(self) -> anyhow::Result<CanonicalizedUdfPath> {
@@ -47,7 +47,7 @@ impl UdfPath {
 
     pub fn canonicalize(self) -> CanonicalizedUdfPath {
         let module = self.module.canonicalize();
-        let function = self.function.unwrap_or_else(|| "default".to_string());
+        let function = self.function.unwrap_or_else(FunctionName::default_export);
         CanonicalizedUdfPath { module, function }
     }
 }
@@ -57,10 +57,7 @@ impl FromStr for UdfPath {
 
     fn from_str(p: &str) -> Result<Self, Self::Err> {
         let (module, function) = match p.rsplit_once(':') {
-            Some((module, function)) => {
-                check_valid_identifier(function)?;
-                (module.parse()?, Some(function.to_owned()))
-            },
+            Some((module, function)) => (module.parse()?, Some(function.parse()?)),
             None => (p.parse()?, None),
         };
         Ok(Self { module, function })
@@ -133,11 +130,11 @@ impl proptest::arbitrary::Arbitrary for UdfPath {
 #[derive(Clone, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct CanonicalizedUdfPath {
     module: CanonicalizedModulePath,
-    function: String,
+    function: FunctionName,
 }
 
 impl CanonicalizedUdfPath {
-    pub fn new(module: CanonicalizedModulePath, function: String) -> Self {
+    pub fn new(module: CanonicalizedModulePath, function: FunctionName) -> Self {
         Self { module, function }
     }
 
@@ -149,12 +146,12 @@ impl CanonicalizedUdfPath {
         &self.module
     }
 
-    pub fn function_name(&self) -> &str {
+    pub fn function_name(&self) -> &FunctionName {
         &self.function
     }
 
     pub fn strip(self) -> UdfPath {
-        let function = if self.function == "default" {
+        let function = if self.function.is_default_export() {
             None
         } else {
             Some(self.function)
diff --git a/crates/isolate/src/environment/analyze.rs b/crates/isolate/src/environment/analyze.rs
@@ -49,11 +49,11 @@ use model::{
     modules::{
         args_validator::ArgsValidator,
         module_versions::{
+            invalid_function_name_error,
             AnalyzedFunction,
             AnalyzedHttpRoute,
             AnalyzedModule,
             AnalyzedSourcePosition,
-            FunctionName,
             MappedModule,
             ModuleSource,
             SourceMap,
@@ -67,6 +67,7 @@ use rand_chacha::ChaCha12Rng;
 use serde_json::Value as JsonValue;
 use sync_types::{
     CanonicalizedModulePath,
+    FunctionName,
     ModulePath,
 };
 use value::{
@@ -564,7 +565,9 @@ fn udf_analyze<RT: Runtime>(
             )
         };
 
-        let canonicalized_name = FunctionName::from_untrusted(&property_name)?;
+        let canonicalized_name: FunctionName = property_name
+            .parse()
+            .map_err(|e| invalid_function_name_error(&e))?;
         if let Some(Some(token)) = fn_source_map.as_ref().map(|sm| sm.lookup_token(lineno, linecol))
             // This condition is in place so that we don't have to jump to source in source mappings
             // to get back to the original source. This logic gets complicated and is not strictly necessary now
diff --git a/crates/isolate/src/environment/helpers/module_loader.rs b/crates/isolate/src/environment/helpers/module_loader.rs
@@ -75,7 +75,7 @@ pub trait ModuleLoader<RT: Runtime>: Sync + Send + 'static {
                 .ok_or_else(|| anyhow::anyhow!("Expected analyze result for {udf_path:?}"))?;
 
             for function in &analyzed_module.functions {
-                if function.name.as_ref() == udf_path.function_name() {
+                if &function.name == udf_path.function_name() {
                     return Ok(Ok(function.clone()));
                 }
             }
diff --git a/crates/isolate/src/environment/helpers/validation.rs b/crates/isolate/src/environment/helpers/validation.rs
@@ -108,7 +108,7 @@ pub async fn validate_schedule_args<RT: Runtime>(
         let found = analyze_result
             .functions
             .iter()
-            .any(|f| f.name.as_ref() == function_name);
+            .any(|f| &f.name == function_name);
         if !found {
             anyhow::bail!(ErrorMetadata::bad_request(
                 "InvalidScheduledFunction",
diff --git a/crates/isolate/src/tests/analyze.rs b/crates/isolate/src/tests/analyze.rs
@@ -75,11 +75,11 @@ async fn test_analyze_module(rt: TestRuntime) -> anyhow::Result<()> {
 
     for (i, (name, expected_type, mapped_lineno)) in expected.iter().enumerate() {
         let function = &module.functions[i];
-        assert_eq!(&function.name.as_ref(), name);
+        assert_eq!(&function.name[..], *name);
         assert_eq!(&function.udf_type, expected_type);
 
         let mapped_function = &source_mapped.functions[i];
-        assert_eq!(&mapped_function.name.as_ref(), name);
+        assert_eq!(&mapped_function.name[..], *name);
         assert_eq!(
             mapped_function.pos.as_ref().unwrap().start_lineno,
             *mapped_lineno
diff --git a/crates/model/src/modules/module_versions.rs b/crates/model/src/modules/module_versions.rs
diff --git a/crates/node_executor/src/executor.rs b/crates/node_executor/src/executor.rs
diff --git a/crates/value/src/heap_size.rs b/crates/value/src/heap_size.rs

Original file line number	Diff line number	Diff line change
`@@ -75,7 +75,7 @@ pub trait ModuleLoader<RT: Runtime>: Sync + Send + 'static {`
`75`	`75`	`.ok_or_else(\|\| anyhow::anyhow!("Expected analyze result for {udf_path:?}"))?;`
`76`	`76`
`77`	`77`	`for function in &analyzed_module.functions {`
`78`		`- if function.name.as_ref() == udf_path.function_name() {`
	`78`	`+ if &function.name == udf_path.function_name() {`
`79`	`79`	`return Ok(Ok(function.clone()));`
`80`	`80`	`}`
`81`	`81`	`}`