Simplify file mgmt by plugging workspace file abstraction leak (#235)

joshuadavidthomas · web-flow · commit 01e52a47a6f5 · 2025-09-16T16:01:07.000-05:00
diff --git a/crates/djls-server/src/db.rs b/crates/djls-server/src/db.rs
@@ -9,19 +9,16 @@ use std::sync::Mutex;
 
 use camino::Utf8Path;
 use camino::Utf8PathBuf;
-use dashmap::DashMap;
 use djls_project::Db as ProjectDb;
 use djls_project::InspectorPool;
 use djls_project::Interpreter;
 use djls_project::Project;
 use djls_semantic::Db as SemanticDb;
 use djls_semantic::TagSpecs;
 use djls_source::Db as SourceDb;
-use djls_source::File;
 use djls_templates::db::Db as TemplateDb;
 use djls_workspace::db::Db as WorkspaceDb;
 use djls_workspace::FileSystem;
-use salsa::Setter;
 
 /// Concrete Salsa database for the Django Language Server.
 ///
@@ -35,9 +32,6 @@ pub struct DjangoDatabase {
     /// File system for reading file content (checks buffers first, then disk).
     fs: Arc<dyn FileSystem>,
 
-    /// Maps paths to [`SourceFile`] entities for O(1) lookup.
-    files: Arc<DashMap<Utf8PathBuf, File>>,
-
     /// The single project for this database instance
     project: Arc<Mutex<Option<Project>>>,
 
@@ -60,7 +54,6 @@ impl Default for DjangoDatabase {
         let logs = <Arc<Mutex<Option<Vec<String>>>>>::default();
         Self {
             fs: Arc::new(InMemoryFileSystem::new()),
-            files: Arc::new(DashMap::new()),
             project: Arc::new(Mutex::new(None)),
             inspector_pool: Arc::new(InspectorPool::new()),
             storage: salsa::Storage::new(Some(Box::new({
@@ -95,64 +88,17 @@ impl DjangoDatabase {
 
         *self.project.lock().unwrap() = Some(project);
     }
-    /// Create a new [`DjangoDatabase`] with the given file system and file map.
-    pub fn new(file_system: Arc<dyn FileSystem>, files: Arc<DashMap<Utf8PathBuf, File>>) -> Self {
+    /// Create a new [`DjangoDatabase`] with the given file system handle.
+    pub fn new(file_system: Arc<dyn FileSystem>) -> Self {
         Self {
             fs: file_system,
-            files,
             project: Arc::new(Mutex::new(None)),
             inspector_pool: Arc::new(InspectorPool::new()),
             storage: salsa::Storage::new(None),
             #[cfg(test)]
             logs: Arc::new(Mutex::new(None)),
         }
     }
-
-    /// Get an existing [`SourceFile`] for the given path without creating it.
-    ///
-    /// Returns `Some(SourceFile)` if the file is already tracked, `None` otherwise.
-    pub fn get_file(&self, path: &Utf8Path) -> Option<File> {
-        self.files.get(path).map(|file_ref| *file_ref)
-    }
-
-    /// Get or create a [`SourceFile`] for the given path.
-    ///
-    /// Files are created with an initial revision of 0 and tracked in the database's
-    /// `DashMap`. The `Arc` ensures cheap cloning while maintaining thread safety.
-    pub fn get_or_create_file(&mut self, path: &Utf8PathBuf) -> File {
-        if let Some(file_ref) = self.files.get(path) {
-            return *file_ref;
-        }
-
-        let file = File::new(self, path.clone(), 0);
-
-        self.files.insert(path.clone(), file);
-        file
-    }
-
-    /// Check if a file is being tracked without creating it.
-    pub fn has_file(&self, path: &Utf8Path) -> bool {
-        self.files.contains_key(path)
-    }
-
-    /// Touch a file to mark it as modified, triggering re-evaluation of dependent queries.
-    ///
-    /// Updates the file's revision number to signal that cached query results
-    /// depending on this file should be invalidated.
-    pub fn touch_file(&mut self, path: &Utf8Path) {
-        let Some(file_ref) = self.files.get(path) else {
-            tracing::debug!("File {} not tracked, skipping touch", path);
-            return;
-        };
-        let file = *file_ref;
-        drop(file_ref); // Explicitly drop to release the lock
-
-        let current_rev = file.revision(self);
-        let new_rev = current_rev + 1;
-        file.set_revision(self).to(new_rev);
-
-        tracing::debug!("Touched {}: revision {} -> {}", path, current_rev, new_rev);
-    }
 }
 
 #[salsa::db]
diff --git a/crates/djls-server/src/server.rs b/crates/djls-server/src/server.rs
@@ -1,6 +1,7 @@
 use std::future::Future;
 use std::sync::Arc;
 
+use camino::Utf8PathBuf;
 use djls_project::Db as ProjectDb;
 use djls_semantic::Db as SemanticDb;
 use djls_source::FileKind;
@@ -406,13 +407,11 @@ impl LanguageServer for DjangoLanguageServer {
         }
 
         // Get diagnostics from the database
+        let path: Utf8PathBuf = url.path().into();
         let diagnostics: Vec<lsp_types::Diagnostic> = self
-            .with_session(|session| {
+            .with_session_mut(|session| {
+                let file = session.get_or_create_file(&path);
                 session.with_db(|db| {
-                    let Some(file) = db.get_file(url.path().into()) else {
-                        return vec![];
-                    };
-
                     let nodelist = djls_templates::parse_template(db, file);
                     djls_ide::collect_diagnostics(db, file, nodelist)
                 })
diff --git a/crates/djls-server/src/session.rs b/crates/djls-server/src/session.rs
@@ -3,10 +3,7 @@
 //! This module implements the LSP session abstraction that manages project-specific
 //! state and the Salsa database for incremental computation.
 
-use std::sync::Arc;
-
 use camino::Utf8PathBuf;
-use dashmap::DashMap;
 use djls_conf::Settings;
 use djls_project::Db as ProjectDb;
 use djls_project::Interpreter;
@@ -16,6 +13,7 @@ use djls_workspace::paths;
 use djls_workspace::PositionEncoding;
 use djls_workspace::TextDocument;
 use djls_workspace::Workspace;
+use djls_workspace::WorkspaceFileEvent;
 use salsa::Setter;
 use tower_lsp_server::lsp_types;
 use url::Url;
@@ -74,9 +72,7 @@ impl Session {
         };
 
         let workspace = Workspace::new();
-
-        let files = Arc::new(DashMap::new());
-        let mut db = DjangoDatabase::new(workspace.file_system(), files);
+        let mut db = DjangoDatabase::new(workspace.file_system());
 
         if let Some(root_path) = &project_path {
             db.set_project(root_path);
@@ -160,24 +156,8 @@ impl Session {
     /// the database or invalidates it if it already exists.
     /// For template files, immediately triggers parsing and validation.
     pub fn open_document(&mut self, url: &Url, document: TextDocument) {
-        // Add to workspace buffers
-        self.workspace.open_document(url, document);
-
-        if let Some(path) = paths::url_to_path(url) {
-            let already_exists = self.db.has_file(&path);
-            let file = self.db.get_or_create_file(&path);
-
-            if already_exists {
-                // File was already read - touch to invalidate cache
-                self.db.touch_file(&path);
-            }
-
-            if FileKind::from_path(&path) == FileKind::Template {
-                let nodelist = djls_templates::parse_template(&self.db, file);
-                if let Some(nodelist) = nodelist {
-                    djls_semantic::validate_nodelist(&self.db, nodelist);
-                }
-            }
+        if let Some(event) = self.workspace.open_document(&mut self.db, url, document) {
+            self.handle_file_event(&event);
         }
     }
 
@@ -191,39 +171,20 @@ impl Session {
         changes: Vec<lsp_types::TextDocumentContentChangeEvent>,
         version: i32,
     ) {
-        self.workspace
-            .update_document(url, changes, version, self.position_encoding);
-
-        if let Some(path) = paths::url_to_path(url) {
-            if self.db.has_file(&path) {
-                // Touch file in database to trigger invalidation
-                self.db.touch_file(&path);
-
-                if FileKind::from_path(&path) == FileKind::Template {
-                    let file = self.db.get_or_create_file(&path);
-                    let nodelist = djls_templates::parse_template(&self.db, file);
-                    if let Some(nodelist) = nodelist {
-                        djls_semantic::validate_nodelist(&self.db, nodelist);
-                    }
-                }
-            }
+        if let Some(event) = self.workspace.update_document(
+            &mut self.db,
+            url,
+            changes,
+            version,
+            self.position_encoding,
+        ) {
+            self.handle_file_event(&event);
         }
     }
 
     pub fn save_document(&mut self, url: &Url) {
-        if let Some(path) = paths::url_to_path(url) {
-            if self.db.has_file(&path) {
-                // Touch file in database to trigger invalidation
-                self.db.touch_file(&path);
-
-                if FileKind::from_path(&path) == FileKind::Template {
-                    let file = self.db.get_or_create_file(&path);
-                    let nodelist = djls_templates::parse_template(&self.db, file);
-                    if let Some(nodelist) = nodelist {
-                        djls_semantic::validate_nodelist(&self.db, nodelist);
-                    }
-                }
-            }
+        if let Some(event) = self.workspace.save_document(&mut self.db, url) {
+            self.handle_file_event(&event);
         }
     }
 
@@ -232,16 +193,7 @@ impl Session {
     /// Removes from workspace buffers and triggers database invalidation to fall back to disk.
     /// For template files, immediately re-parses from disk.
     pub fn close_document(&mut self, url: &Url) -> Option<TextDocument> {
-        let document = self.workspace.close_document(url);
-
-        if let Some(path) = paths::url_to_path(url) {
-            if self.db.has_file(&path) {
-                // Touch file in database to trigger re-read from disk
-                self.db.touch_file(&path);
-            }
-        }
-
-        document
+        self.workspace.close_document(&mut self.db, url)
     }
 
     /// Get a document from the buffer if it's open.
@@ -252,7 +204,18 @@ impl Session {
 
     /// Get or create a file in the database.
     pub fn get_or_create_file(&mut self, path: &Utf8PathBuf) -> File {
-        self.db.get_or_create_file(path)
+        self.workspace
+            .track_file(&mut self.db, path.as_path())
+            .file()
+    }
+
+    fn handle_file_event(&self, event: &WorkspaceFileEvent) {
+        if FileKind::from_path(event.path()) == FileKind::Template {
+            let nodelist = djls_templates::parse_template(&self.db, event.file());
+            if let Some(nodelist) = nodelist {
+                djls_semantic::validate_nodelist(&self.db, nodelist);
+            }
+        }
     }
 
     /// Check if the client supports pull diagnostics.
diff --git a/crates/djls-workspace/Cargo.toml b/crates/djls-workspace/Cargo.toml
@@ -4,7 +4,7 @@ version = "0.0.0"
 edition = "2021"
 
 [dependencies]
-djls-source = { workspace = true}
+djls-source = { workspace = true }
 
 anyhow = { workspace = true }
 camino = { workspace = true }
diff --git a/crates/djls-workspace/src/db.rs b/crates/djls-workspace/src/db.rs
@@ -22,11 +22,29 @@
 
 use std::sync::Arc;
 
+use djls_source::Db as SourceDb;
+use djls_source::File;
+use salsa::Setter;
+
 use crate::FileSystem;
 
 /// Base database trait that provides file system access for Salsa queries
 #[salsa::db]
-pub trait Db: salsa::Database {
+pub trait Db: SourceDb {
     /// Get the file system for reading files.
     fn fs(&self) -> Arc<dyn FileSystem>;
+
+    /// Bump the revision for a tracked file to invalidate dependent queries.
+    fn touch_file(&mut self, file: File) {
+        let current_rev = file.revision(self);
+        let new_rev = current_rev + 1;
+        file.set_revision(self).to(new_rev);
+
+        tracing::debug!(
+            "Touched {}: revision {} -> {}",
+            file.path(self),
+            current_rev,
+            new_rev
+        );
+    }
 }
diff --git a/crates/djls-workspace/src/lib.rs b/crates/djls-workspace/src/lib.rs
@@ -31,3 +31,4 @@ pub use fs::OsFileSystem;
 pub use fs::WorkspaceFileSystem;
 pub use language::LanguageId;
 pub use workspace::Workspace;
+pub use workspace::WorkspaceFileEvent;
diff --git a/crates/djls-workspace/src/workspace.rs b/crates/djls-workspace/src/workspace.rs
