Remove remaining Component.*TODOs (#30708)

GitOrigin-RevId: 203aaaf997f7cbaddba01dbdbb7f843cead85b02

Author: Sujay Jayakar

crates/application/src/application_function_runner/mod.rs

@@ -27,6 +27,7 @@ use common::{
         ComponentDefinitionPath,
         ComponentId,
         ComponentName,
+        ComponentPath,
         PublicFunctionPath,
         Resource,
     },
@@ -1952,23 +1953,28 @@ impl<RT: Runtime> ActionCallbacks for ApplicationFunctionRunner<RT> {
         identity: Identity,
         component: ComponentId,
         storage_id: FileStorageId,
-    ) -> anyhow::Result<Option<FileStorageEntry>> {
+    ) -> anyhow::Result<Option<(ComponentPath, FileStorageEntry)>> {
         let mut tx = self.database.begin(identity).await?;
         self.bail_if_backend_not_running(&mut tx).await?;
-        self.file_storage
+        let Some(component_path) = tx.get_component_path(component) else {
+            return Ok(None);
+        };
+        let entry = self
+            .file_storage
             .get_file_entry(&mut tx, component.into(), storage_id)
-            .await
+            .await?;
+        Ok(entry.map(|e| (component_path, e)))
     }
 
     async fn storage_store_file_entry(
         &self,
         identity: Identity,
         component: ComponentId,
         entry: FileStorageEntry,
-    ) -> anyhow::Result<DeveloperDocumentId> {
+    ) -> anyhow::Result<(ComponentPath, DeveloperDocumentId)> {
         let mut tx = self.database.begin(identity.clone()).await?;
         self.bail_if_backend_not_running(&mut tx).await?;
-        let (_ts, id, _stats) = self
+        let (_ts, r, _stats) = self
             .database
             .execute_with_occ_retries(
                 identity,
@@ -1977,17 +1983,20 @@ impl<RT: Runtime> ActionCallbacks for ApplicationFunctionRunner<RT> {
                 "app_funrun_storage_store_file_entry",
                 |tx| {
                     async {
+                        let component_path = tx
+                            .get_component_path(component)
+                            .context(format!("Component {component:?} not found"))?;
                         let id = self
                             .file_storage
                             .store_file_entry(tx, component.into(), entry.clone())
                             .await?;
-                        Ok(id)
+                        Ok((component_path, id))
                     }
                     .into()
                 },
             )
             .await?;
-        Ok(id)
+        Ok(r)
     }
 
     async fn storage_delete(

crates/application/src/function_log.rs

@@ -171,7 +171,8 @@ impl FunctionExecution {
         };
         let component_path = match &self.params {
             UdfParams::Function { identifier, .. } => identifier.component.clone(),
-            UdfParams::Http { .. } => ComponentPath::TODO(),
+            // TODO(ENG-7612): Support HTTP actions in components.
+            UdfParams::Http { .. } => ComponentPath::root(),
         };
 
         FunctionEventSource {
@@ -977,7 +978,8 @@ impl<RT: Runtime> FunctionExecutionLog<RT> {
         module_environment: ModuleEnvironment,
     ) {
         let event_source = FunctionEventSource {
-            component_path: ComponentPath::TODO(),
+            // TODO(ENG-7612): Support HTTP actions in components.
+            component_path: ComponentPath::root(),
             udf_path: identifier.to_string(),
             udf_type: UdfType::HttpAction,
             module_environment,

crates/application/src/lib.rs

@@ -2470,12 +2470,32 @@ impl<RT: Runtime> Application<RT> {
         storage_id: FileStorageId,
     ) -> anyhow::Result<FileStream> {
         self.bail_if_not_running().await?;
-        let file_entry = self.get_file_entry(component, storage_id).await?;
+        let mut file_storage_tx = self.begin(Identity::system()).await?;
+        let Some(file_entry) = self
+            .file_storage
+            .transactional_file_storage
+            // The transaction is not part of UDF so use the global usage counters.
+            .get_file_entry(&mut file_storage_tx, component.into(), storage_id.clone())
+            .await?
+        else {
+            return Err(ErrorMetadata::transient_not_found(
+                "FileNotFound",
+                format!("File {storage_id} not found"),
+            )
+            .into());
+        };
+        let Some(component_path) = file_storage_tx.get_component_path(component) else {
+            return Err(ErrorMetadata::transient_not_found(
+                "FileNotFound",
+                format!("Component {component:?} not found"),
+            )
+            .into());
+        };
         self
             .file_storage
             .transactional_file_storage
             // The transaction is not part of UDF so use the global usage counters.
-            .get_file_stream(file_entry, self.usage_tracking.clone())
+            .get_file_stream(component_path, file_entry, self.usage_tracking.clone())
             .await
     }
 
@@ -2501,12 +2521,24 @@ impl<RT: Runtime> Application<RT> {
             )
             .into());
         };
+        let Some(component_path) = file_storage_tx.get_component_path(component) else {
+            return Err(ErrorMetadata::transient_not_found(
+                "FileNotFound",
+                format!("Component {component:?} not found"),
+            )
+            .into());
+        };
 
         self
             .file_storage
             .transactional_file_storage
             // The transaction is not part of UDF so use the global usage counters.
-            .get_file_range_stream(file_entry, bytes_range, self.usage_tracking.clone())
+            .get_file_range_stream(
+                component_path,
+                file_entry,
+                bytes_range,
+                self.usage_tracking.clone(),
+            )
             .await
     }
 

crates/common/src/components/component_path.rs

@@ -89,13 +89,6 @@ impl ComponentPath {
         self.path.is_empty()
     }
 
-    /// Use ComponentPath::TODO() when the path should be passed down from a
-    /// higher layer.
-    #[allow(non_snake_case)]
-    pub fn TODO() -> Self {
-        Self::root()
-    }
-
     /// Component path to use in tests, representing a user-space component.
     /// Ideally this could be changed to an arbitrary path and the tests would
     /// still pass.

crates/common/src/components/mod.rs

@@ -46,13 +46,6 @@ impl ComponentId {
         matches!(self, ComponentId::Root)
     }
 
-    /// This component should be passed in somehow -- it's not always going to
-    /// be Root.
-    #[allow(non_snake_case)]
-    pub const fn TODO() -> Self {
-        ComponentId::Root
-    }
-
     /// Component for tests where we need a user component.
     /// Ideally we could switch this to some other component with no test
     /// breakage.

crates/file_storage/src/core.rs

@@ -185,13 +185,15 @@ impl<RT: Runtime> TransactionalFileStorage<RT> {
 
     pub async fn get_file_stream(
         &self,
+        component_path: ComponentPath,
         file: FileStorageEntry,
         usage_tracker: impl StorageUsageTracker + Clone + 'static,
     ) -> anyhow::Result<FileStream> {
         let sha256 = file.sha256.clone();
 
         let result = self
             .file_stream(
+                component_path,
                 file,
                 (Bound::Included(0), Bound::Unbounded),
                 usage_tracker,
@@ -209,16 +211,24 @@ impl<RT: Runtime> TransactionalFileStorage<RT> {
 
     pub async fn get_file_range_stream(
         &self,
+        component_path: ComponentPath,
         file: FileStorageEntry,
         bytes_range: (Bound<u64>, Bound<u64>),
         usage_tracker: impl StorageUsageTracker + Clone + 'static,
     ) -> anyhow::Result<FileRangeStream> {
-        self.file_stream(file, bytes_range, usage_tracker, GetFileType::Range)
-            .await
+        self.file_stream(
+            component_path,
+            file,
+            bytes_range,
+            usage_tracker,
+            GetFileType::Range,
+        )
+        .await
     }
 
     async fn file_stream(
         &self,
+        component_path: ComponentPath,
         file: FileStorageEntry,
         bytes_range: (Bound<u64>, Bound<u64>),
         usage_tracker: impl StorageUsageTracker + Clone + 'static,
@@ -243,7 +253,6 @@ impl<RT: Runtime> TransactionalFileStorage<RT> {
         let stream = storage_get_stream.stream;
         let content_length = ContentLength(storage_get_stream.content_length as u64);
 
-        let component_path = ComponentPath::TODO();
         let call_tracker = usage_tracker.track_storage_call(
             component_path.clone(),
             "get range",

crates/isolate/src/client.rs

@@ -30,6 +30,7 @@ use common::{
         ComponentDefinitionPath,
         ComponentId,
         ComponentName,
+        ComponentPath,
         Resource,
     },
     errors::{
@@ -336,15 +337,15 @@ pub trait ActionCallbacks: Send + Sync {
         identity: Identity,
         component: ComponentId,
         storage_id: FileStorageId,
-    ) -> anyhow::Result<Option<FileStorageEntry>>;
+    ) -> anyhow::Result<Option<(ComponentPath, FileStorageEntry)>>;
 
     // Used to store an already uploaded file from an action running in v8.
     async fn storage_store_file_entry(
         &self,
         identity: Identity,
         component: ComponentId,
         entry: FileStorageEntry,
-    ) -> anyhow::Result<DeveloperDocumentId>;
+    ) -> anyhow::Result<(ComponentPath, DeveloperDocumentId)>;
 
     // Scheduler
     async fn schedule_job(
@@ -1831,10 +1832,10 @@ impl<RT: Runtime> IsolateWorker<RT> for BackendIsolateWorker<RT> {
             } => {
                 drop(queue_timer);
                 let timer = metrics::service_request_timer(&UdfType::Action);
-                let component_path = request.params.path_and_args.path().component;
+                let component_id = request.params.path_and_args.path().component;
                 let environment = ActionEnvironment::new(
                     self.rt.clone(),
-                    component_path,
+                    component_id,
                     environment_data,
                     request.identity,
                     request.transaction,

crates/isolate/src/environment/action/async_syscall.rs

@@ -21,10 +21,7 @@ use model::{
         auth::propagate_component_auth,
         handles::function_handle_not_found,
     },
-    file_storage::{
-        types::FileStorageEntry,
-        FileStorageId,
-    },
+    file_storage::FileStorageId,
 };
 use serde::{
     Deserialize,
@@ -397,23 +394,16 @@ impl<RT: Runtime> TaskExecutor<RT> {
             .action_callbacks
             .storage_get_file_entry(self.identity.clone(), self.component_id(), storage_id)
             .await?
-            .map(
-                |FileStorageEntry {
-                     storage_id,
-                     storage_key: _, // internal field that we shouldn't return in syscalls
-                     sha256,
-                     size,
-                     content_type,
-                 }| {
-                    FileMetadataJson {
-                        storage_id: storage_id.to_string(),
-                        // TODO(CX-5533) use base64 for consistency.
-                        sha256: sha256.as_hex(),
-                        size,
-                        content_type,
-                    }
-                },
-            );
+            .map(|(_, entry)| {
+                // NB: `storage_key is an internal field that we shouldn't return in syscalls.
+                FileMetadataJson {
+                    storage_id: entry.storage_id.to_string(),
+                    // TODO(CX-5533) use base64 for consistency.
+                    sha256: entry.sha256.as_hex(),
+                    size: entry.size,
+                    content_type: entry.content_type,
+                }
+            });
         Ok(serde_json::to_value(file_metadata)?)
     }
 

crates/isolate/src/environment/action/storage.rs

@@ -2,7 +2,6 @@ use std::str::FromStr;
 
 use anyhow::Context;
 use common::{
-    components::ComponentPath,
     runtime::Runtime,
     sha256::{
         DigestHeader,
@@ -80,12 +79,11 @@ impl<RT: Runtime> TaskExecutor<RT> {
         let storage_id = entry.storage_id.clone();
         let size = entry.size;
         let sha256 = entry.sha256.clone();
-        let storage_doc_id = self
+        let (component_path, storage_doc_id) = self
             .action_callbacks
             .storage_store_file_entry(self.identity.clone(), self.component_id(), entry)
             .await?;
 
-        let component_path = ComponentPath::TODO();
         self.usage_tracker
             .track_storage_call(
                 component_path.clone(),
@@ -147,14 +145,13 @@ impl<RT: Runtime> TaskExecutor<RT> {
             .action_callbacks
             .storage_get_file_entry(self.identity.clone(), self.component_id(), storage_id)
             .await?;
-        let file_entry = match file_entry {
+        let (component_path, file_entry) = match file_entry {
             None => return Ok(None),
             Some(f) => f,
         };
-
         let file_stream = self
             .file_storage
-            .get_file_stream(file_entry, self.usage_tracker.clone())
+            .get_file_stream(component_path, file_entry, self.usage_tracker.clone())
             .await?;
 
         let stream = file_stream.stream.map_err(|e| e.into()).boxed();

crates/isolate/src/isolate2/entered_context.rs

@@ -323,9 +323,9 @@ impl<'enter, 'scope: 'enter> EnteredContext<'enter, 'scope> {
         self.execute_user_code(|s| s.perform_microtask_checkpoint())?;
 
         let path = ResolvedComponentFunctionPath {
-            component: ComponentId::TODO(),
+            component: ComponentId::Root,
             udf_path: udf_path.clone(),
-            component_path: Some(ComponentPath::TODO()),
+            component_path: Some(ComponentPath::root()),
         };
         let evaluate_result = self.check_promise_result(&path, &promise)?;
         Ok((v8::Global::new(self.scope, promise), evaluate_result))
@@ -431,9 +431,9 @@ impl<'enter, 'scope: 'enter> EnteredContext<'enter, 'scope> {
 
         let promise = v8::Local::new(self.scope, &pending_function.promise);
         let path = ResolvedComponentFunctionPath {
-            component: ComponentId::TODO(),
+            component: ComponentId::Root,
             udf_path: pending_function.udf_path.clone(),
-            component_path: Some(ComponentPath::TODO()),
+            component_path: Some(ComponentPath::root()),
         };
         self.check_promise_result(&path, &promise)
     }