Refactor XREF to use Sasha plane file storage instead of Dali IPT #153

Copilot · 2025-10-30T12:51:29Z

XREF Refactoring - Fixed File Storage Implementation

This PR refactors how saxref saves XREF branches to move from Dali IPT (in-memory property tree) to file-based storage on the Sasha plane.

Latest Changes (Addressing Review Feedback):

CXRefFilesNode Changes

Added xrefPath support: CXRefFilesNode now tracks xrefPath and branchName
Smart queryData(): Loads from file if xrefPath set, otherwise from "data" attribute in Dali
Smart Deserialize(): Saves to file if xrefPath set, otherwise to "data" attribute in Dali
No Dali pollution: File-based data never written to Dali's "data" attribute

XRefNodeManager Changes

Removed loadBranchFromPath: Previous approach incorrectly loaded file data into Dali
Updated accessor methods: getLostFiles, getFoundFiles, getOrphanFiles now call setXRefPath on CXRefFilesNode
File-based serialize/deserialize: serializeMessages/Directories and deserializeMessages/Directories handle files directly
No commits for file storage: File-based branches don't get committed to Dali

Storage Path Structure

<sasha_prefix>/xref/<cluster>/<YYYY-MM-DD>/<branch>.xml

How It Works

File-based storage (xrefPath exists):

CXRefFilesNode.queryData() reads from <xrefPath>/<branchName>.xml
CXRefFilesNode.Deserialize() writes to <xrefPath>/<branchName>.xml
Data stays on filesystem, never stored in Dali

Dali-based storage (no xrefPath):

CXRefFilesNode.queryData() reads from branch's "data" attribute
CXRefFilesNode.Deserialize() writes to branch's "data" attribute
Traditional behavior preserved

Configuration

<DfuXRef useSashaPlane="true"/>  <!-- Expert option, defaults to isContainerized() -->

Original prompt

Refactor the way saxref saves it's branches to Dali (via committing them to a IPT in Dali), and instead lookup the Sasha plane and save them there. A new named directory should be created for the edition under the sasha prefix, and under "xref", for the cluster it is dealing with, with a datestamp subdir under that.

Then update the Dali XREF cluster tree with reference info so a client can find this path.

Then look at dali/dfuXRefLib/XRefNodeManager.cpp, and how it currently fetch "data" in the CXRefNode ctor. Instead use the newly recorded file path to read from instead.
NB: the path saved by saxref.cpp should be in the form of a hostname based url if it is a non local path.
NB2: the XREFNodeManager.cpp changes should be backward compatible, IOW, if pathing meta data is there, it should use it, but it should fall back to reading "data" if not.

Avoid making extensive changes and unrelated improvements where at all possible.

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

- Modified saxref.cpp to save XREF branches to Sasha plane instead of Dali IPT - Created directory structure: <sasha_prefix>/xref/<cluster>/<datestamp>/ - Store path reference in Dali cluster tree instead of full data - Updated XRefNodeManager.cpp to read from path-based files - Maintained backward compatibility by falling back to "data" attribute Co-authored-by: jakesmith <[email protected]>

Co-authored-by: jakesmith <[email protected]>

jakesmith

@copilot - see comments.

jakesmith · 2025-10-30T13:26:24Z

dali/sasha/saxref.cpp

+            {
+                StringBuffer planeName;
+                // Look for sasha plane
+                if (getDefaultPlane(planeName, nullptr, "sasha"))


@copilot - There should always be a sasha plane categeory defined.
This should throw an error if one cannot be found.

Fixed in c7c4bbb - Now throws error if Sasha plane category not found instead of falling back.

jakesmith · 2025-10-30T13:27:54Z

dali/sasha/saxref.cpp

+                    if (sashaPlane)
+                    {
+                        StringBuffer prefix;
+                        if (sashaPlane->getProp("@prefix", prefix))


@copilot - the above calls could be simplified by using getDefaultPlaneDirectory to get the prefix directly (and throwing an error if fails)

Fixed in c7c4bbb - Simplified to use getDefaultPlane + getStoragePlaneConfig. Note: getDefaultPlaneDirectory is a static function not available in the header.

jakesmith · 2025-10-30T13:29:05Z

dali/sasha/saxref.cpp

+                            addPathSepChar(sashaDir).append(datestamp);
+
+                            // Create the directory
+                            Owned<IFile> dir = createIFile(sashaDir.str());


@copilot - use recursiveCreateDirectoryForFile instead and fire an error if it fails.

We should not failover to write to dali.

Fixed in c7c4bbb - Now uses recursiveCreateDirectoryForFile and throws error on failure (no fallback to Dali).

jakesmith · 2025-10-30T13:31:58Z

dali/sasha/saxref.cpp

+            saveSuccess = saveBranchToSashaPlane(sashaDir.str(), "Orphans", orphansbranch) && saveSuccess;
+            saveSuccess = saveBranchToSashaPlane(sashaDir.str(), "Lost", lostbranch) && saveSuccess;
+            saveSuccess = saveBranchToSashaPlane(sashaDir.str(), "Found", foundbranch) && saveSuccess;
+            saveSuccess = saveBranchToSashaPlane(sashaDir.str(), "Directories", dirbranch) && saveSuccess;


@copilot - use 'warn' in saveBranchToSashaPlane to preserve and publish the warning, but don't bother with 'saveSuccess' and a return result.

Fixed in c7c4bbb - saveBranchToSashaPlane now uses warn() and has no return value or saveSuccess tracking.

jakesmith · 2025-10-30T13:32:15Z

dali/sasha/saxref.cpp

+            }
+            saveSuccess = saveBranchToSashaPlane(sashaDir.str(), "Messages", message) && saveSuccess;
+
+            if (saveSuccess)


@copilot - see previous comment. Get rid of 'saveSuccess'

Fixed in c7c4bbb - Removed saveSuccess tracking completely.

jakesmith · 2025-10-30T13:34:33Z

dali/sasha/saxref.cpp

+                // Store path reference in Dali instead of full data
+                // Convert to hostname-based URL if not a local path
+                StringBuffer pathUrl;
+                if (!isAbsolutePath(sashaDir.str()))


@copilot - this in fact is not necessary, simply store the sashaDir in all cases.

Fixed in c7c4bbb - Now stores plain sashaDir path directly (no URL conversion).

jakesmith · 2025-10-30T13:35:22Z

dali/sasha/saxref.cpp

+            }
+        }
+
+        if (!useSashaPlane)


given we are not going to failover to this. Just make this the else branch of if (useSashaPlane)

Fixed in c7c4bbb - Made it a simple else branch with no fallback logic.

jakesmith · 2025-10-30T13:35:48Z

dali/dfuXRefLib/XRefNodeManager.cpp

+            {
+                StringBuffer basePath(xrefPath);
+                // Handle file:// URLs
+                if (hasPrefix(basePath, "file://", false))


@copilot - this is not necessary, remove this block.

Fixed in c7c4bbb - Removed file:// URL handling block entirely.

jakesmith · 2025-10-30T13:36:52Z

dali/dfuXRefLib/XRefNodeManager.cpp

+        }
+
+        // Fall back to loading from "data" attribute if path not available or failed
+        if (!m_XRefTree->hasProp("Orphans") && !m_XRefTree->hasProp("Lost") && 


@copilot - don't failover like this, simply make this the else branch if xrefpath is missing.

Fixed in c7c4bbb - Made it a simple else branch instead of fallback logic.

jakesmith · 2025-10-30T13:58:20Z

dali/dfuXRefLib/XRefNodeManager.cpp


 }

 CXRefNode::CXRefNode(IPropertyTree* pTreeRoot)


@copilot - in fact , all changes to this version of the ctor should be undone. This ctor is unused, leave it untouched for now.

The correct place to handle xrefPath is in places like CXRefNode::getOrphanFiles(), getFoundFiles, getLostFiles, getDataTree , and CXRefFilesNode (e.g. queryData()), which currently looks at "data" and then uses that MemoryBuffer to create an IPT (in queryDataTree), or Serialize that returns it into a StringBuffer.

These methods need to cope with the new and old approach.

Fixed in c7c4bbb - Reverted all changes to CXRefNode(IPropertyTree*) constructor. Added loadBranchFromPath helper and modified getLostFiles, getFoundFiles, getOrphanFiles, serializeMessages, and serializeDirectories to handle xrefPath properly.

- Made useSashaPlane configurable via expert option (default to isContainerized) - Simplified sasha plane setup to throw errors if not configured - Use recursiveCreateDirectoryForFile for directory creation - Removed saveSuccess tracking, use warn() in saveBranchToSashaPlane - Store plain sashaDir path instead of URL - Reverted all changes to CXRefNode(IPropertyTree*) constructor - Added loadBranchFromPath helper to load from xrefPath - Modified getLostFiles, getFoundFiles, getOrphanFiles to load from path - Modified serializeMessages and serializeDirectories to load from path - Removed fallback to Dali storage (throw error instead) Co-authored-by: jakesmith <[email protected]>

jakesmith

@copilot Several problems:

loadBranchFromPath is currently conditional on whether the individual branch nodea re present or not, but it should be based purely on whether xrefPath was present.
it's loading the branch into the m_XRefTree, which is a tree in Dali, and it then commits it, pushing the loaded trees into Dali. That is not the goal/wanted. We want to keep the external file base trees out of Dali.
The trees are saved in various places (see places that do setPropBin("data" ..). This works if the data is to be stored in Dali, but our goal is to save them back to disk. NB: if the original was in dali, after save, xrefPath can be set, so future handling can use the diskbased versions
CXRefFilesNode needs to cope with the old way and new way, paying careful attention to the way the old way read/set "data", and how the new way should load the tree from disk and if request to, serialize it back to disk.

jakesmith

@copilot - there are Several problems:

loadBranchFromPath is currently conditional on whether the individual branch nodea re present or not, but it should be based purely on whether xrefPath was present.
it's loading the branch into the m_XRefTree, which is a tree in Dali, and it then commits it, pushing the loaded trees into Dali. That is not the goal/wanted. We want to keep the external file base trees out of Dali.
The trees are saved in various places (see places that do setPropBin("data" ..). This works if the data is to be stored in Dali, but our goal is to save them back to disk. NB: if the original was in dali, after save, xrefPath can be set, so future handling can use the diskbased versions
CXRefFilesNode needs to cope with the old way and new way, paying careful attention to the way the old way read/set "data", and how the new way should load the tree from disk and if request to, serialize it back to disk.

@copilot - please fix these issues.

- Modified CXRefFilesNode to handle both file-based and Dali-based storage - Added setXRefPath() method to CXRefFilesNode - queryData() loads from file when xrefPath is set, otherwise from "data" attribute - Deserialize() saves to file when xrefPath is set, otherwise to "data" attribute - Updated getLostFiles, getFoundFiles, getOrphanFiles to set xrefPath on CXRefFilesNode - Don't commit file-loaded branches to Dali - Updated serializeMessages/Directories and deserializeMessages/Directories to handle file-based storage - Removed loadBranchFromPath helper that was loading into Dali Co-authored-by: jakesmith <[email protected]>

Copilot · 2025-10-30T15:25:38Z

@copilot - there are Several problems:
...

Fixed in ab4d5fa:

loadBranchFromPath removed - No longer conditional on branch presence, now properly based on xrefPath
No data in Dali - File-based branches never committed to m_XRefTree. CXRefFilesNode handles file I/O directly
File saves work correctly - Deserialize() saves to disk when xrefPath set, not to "data" attribute
CXRefFilesNode handles both - queryData() loads from file or "data" attribute; Deserialize() saves to file or "data" attribute based on xrefPath presence

Initial plan

bfb8933

Copilot AI assigned Copilot and jakesmith Oct 30, 2025

Copilot started work on behalf of jakesmith October 30, 2025 12:51 View session

Copilot AI and others added 3 commits October 30, 2025 13:03

Add jsocket.hpp include for hostname support

e8678f8

Co-authored-by: jakesmith <[email protected]>

Fix comment: date format is YYYY-MM-DD not YYYYMMDD

0b600da

Co-authored-by: jakesmith <[email protected]>

Copilot AI changed the title ~~[WIP] Refactor saxref branch saving to use Sasha plane~~ Refactor XREF to use Sasha plane file storage instead of Dali IPT Oct 30, 2025

Copilot finished work on behalf of jakesmith October 30, 2025 13:12

Copilot AI requested a review from jakesmith October 30, 2025 13:12

jakesmith requested changes Oct 30, 2025

View reviewed changes

Copilot started work on behalf of jakesmith October 30, 2025 13:59 View session

Copilot finished work on behalf of jakesmith October 30, 2025 14:09

Copilot AI requested a review from jakesmith October 30, 2025 14:09

jakesmith requested changes Oct 30, 2025

View reviewed changes

Copilot started work on behalf of jakesmith October 30, 2025 15:19 View session

Copilot AI requested a review from jakesmith October 30, 2025 15:26

Copilot finished work on behalf of jakesmith October 30, 2025 15:26

Refactor XREF to use Sasha plane file storage instead of Dali IPT #153

Are you sure you want to change the base?

Refactor XREF to use Sasha plane file storage instead of Dali IPT #153

Uh oh!

Conversation

Copilot AI commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

XREF Refactoring - Fixed File Storage Implementation

Latest Changes (Addressing Review Feedback):

CXRefFilesNode Changes

XRefNodeManager Changes

Storage Path Structure

How It Works

Configuration

Uh oh!

jakesmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

jakesmith left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakesmith left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Oct 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Copilot AI commented Oct 30, 2025 •

edited

Loading

jakesmith left a comment •

edited

Loading