Skip to content

Commit 93ef1e5

Browse files
mathe00mathe00
committed
fix: Clear cached settings definitions on discovery failure
Previously, if settings discovery failed for a script (e.g., timeout, non-zero exit code, invalid JSON output, script not handling discovery mode), any previously cached settings definitions for that script would persist in the plugin's settings UI and cache. This commit modifies `updateScriptSettingsCache` in `src/python_executor.ts` to correctly handle discovery failures: - If `discoverScriptSettings` returns `null` (indicating failure), any existing cached definitions for that script are now removed. - Associated settings values (`scriptSettingsValues`) are also cleared for scripts where discovery fails after having been previously successful. This ensures that the settings UI accurately reflects only the scripts that currently provide valid settings definitions, preventing stale data from persisting after a script is modified or becomes incompatible with the discovery process.
1 parent d2cb8ba commit 93ef1e5

File tree

1 file changed

+152
-29
lines changed

1 file changed

+152
-29
lines changed

‎src/python_executor.ts‎

Lines changed: 152 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -122,54 +122,177 @@ export async function discoverScriptSettings(plugin: ObsidianPythonBridge, scrip
122122

123123
/**
124124
* Scans the scripts folder, discovers settings for each script, and updates the cache.
125+
* Handles clearing cached definitions if discovery fails for a script.
125126
* @param plugin The ObsidianPythonBridge plugin instance.
126127
* @param scriptsFolder Absolute path to the Python scripts folder.
127128
*/
128-
export async function updateScriptSettingsCache(plugin: ObsidianPythonBridge, scriptsFolder: string): Promise<void> {
129+
export async function updateScriptSettingsCache(
130+
plugin: ObsidianPythonBridge,
131+
scriptsFolder: string,
132+
): Promise<void> {
129133
plugin.logInfo("Updating script settings definitions cache...");
130-
if (!plugin.pythonExecutable) { plugin.logError("Cannot update script settings cache: Python executable not found."); return; }
131-
if (!scriptsFolder || !fs.existsSync(scriptsFolder)) { plugin.logWarn("Cannot update script settings cache: Scripts folder path is invalid or not found."); return; }
134+
if (!plugin.pythonExecutable) {
135+
plugin.logError(
136+
"Cannot update script settings cache: Python executable not found.",
137+
);
138+
return;
139+
}
140+
if (!scriptsFolder || !fs.existsSync(scriptsFolder)) {
141+
plugin.logWarn(
142+
"Cannot update script settings cache: Scripts folder path is invalid or not found.",
143+
);
144+
return;
145+
}
146+
132147
let pythonFiles: string[];
133-
try { pythonFiles = fs.readdirSync(scriptsFolder).filter((f) => f.toLowerCase().endsWith(".py") && !f.startsWith(".") && f !== PYTHON_LIBRARY_FILENAME); }
134-
catch (err) { plugin.logError(`Error reading scripts folder for settings discovery ${scriptsFolder}:`, err); return; }
148+
try {
149+
// Read directory and filter for .py files, excluding hidden files and the library itself
150+
pythonFiles = fs
151+
.readdirSync(scriptsFolder)
152+
.filter(
153+
(f) =>
154+
f.toLowerCase().endsWith(".py") &&
155+
!f.startsWith(".") &&
156+
f !== PYTHON_LIBRARY_FILENAME,
157+
);
158+
} catch (err) {
159+
plugin.logError(
160+
`Error reading scripts folder for settings discovery ${scriptsFolder}:`,
161+
err,
162+
);
163+
return;
164+
}
165+
166+
// Store newly discovered definitions here
135167
const newDefinitions: Record<string, ScriptSettingDefinition[]> = {};
136-
let changesMade = false;
137-
const currentDefinitionKeys = new Set<string>();
168+
let changesMade = false; // Track if any updates require saving settings
169+
const currentScriptPaths = new Set<string>(); // Keep track of scripts found in the folder
170+
138171
for (const file of pythonFiles) {
139172
const scriptAbsolutePath = path.join(scriptsFolder, file);
140-
const relativePath = normalizePath(path.relative(scriptsFolder, scriptAbsolutePath));
141-
currentDefinitionKeys.add(relativePath);
173+
// Use normalized relative path as the key for settings objects
174+
const relativePath = normalizePath(
175+
path.relative(scriptsFolder, scriptAbsolutePath),
176+
);
177+
currentScriptPaths.add(relativePath);
178+
let discoveryFailed = false; // Flag to track failure for the current script
179+
142180
try {
143-
const definitions = await discoverScriptSettings(plugin, scriptAbsolutePath);
144-
if (definitions !== null) {
145-
newDefinitions[relativePath] = definitions;
146-
if (JSON.stringify(definitions) !== JSON.stringify(plugin.settings.scriptSettingsDefinitions[relativePath])) { changesMade = true; plugin.logDebug(`Definitions updated for ${relativePath}`); }
181+
// Attempt to discover settings by running the script with --get-settings-json
182+
const definitions = await discoverScriptSettings(
183+
plugin,
184+
scriptAbsolutePath,
185+
);
186+
187+
// discoverScriptSettings returns null on failure (timeout, non-zero exit, parse error, etc.)
188+
if (definitions === null) {
189+
discoveryFailed = true;
190+
plugin.logWarn(
191+
`Settings discovery failed for script: ${relativePath}.`,
192+
);
147193
} else {
148-
if (plugin.settings.scriptSettingsDefinitions.hasOwnProperty(relativePath)) { newDefinitions[relativePath] = plugin.settings.scriptSettingsDefinitions[relativePath]; plugin.logWarn(`Failed to discover settings for ${relativePath}, keeping cached version.`); }
149-
else plugin.logWarn(`Failed to discover settings for ${relativePath}, no cached version available.`);
194+
// Discovery succeeded (definitions can be an empty array if no settings are defined)
195+
newDefinitions[relativePath] = definitions;
196+
// Check if the discovered definitions differ from the cached ones
197+
if (
198+
JSON.stringify(definitions) !==
199+
JSON.stringify(
200+
plugin.settings.scriptSettingsDefinitions[relativePath],
201+
)
202+
) {
203+
changesMade = true;
204+
plugin.logDebug(
205+
`Definitions updated or added for ${relativePath}`,
206+
);
207+
}
150208
}
151209
} catch (error) {
152-
plugin.logError(`Unexpected error during settings discovery for ${file}:`, error);
153-
if (plugin.settings.scriptSettingsDefinitions.hasOwnProperty(relativePath)) newDefinitions[relativePath] = plugin.settings.scriptSettingsDefinitions[relativePath];
210+
// Catch unexpected errors during the discoverScriptSettings call itself
211+
plugin.logError(
212+
`Unexpected error during settings discovery call for ${file}:`,
213+
error,
214+
);
215+
discoveryFailed = true;
154216
}
155-
}
156-
const cachedPaths = Object.keys(plugin.settings.scriptSettingsDefinitions);
157-
for (const cachedPath of cachedPaths) {
158-
if (!currentDefinitionKeys.has(cachedPath)) {
217+
218+
// --- Handle Cache Clearing on Discovery Failure ---
219+
if (discoveryFailed) {
220+
// If discovery failed, check if there were old definitions cached
221+
if (
222+
plugin.settings.scriptSettingsDefinitions.hasOwnProperty(
223+
relativePath,
224+
)
225+
) {
226+
plugin.logInfo(
227+
`Removing cached settings definitions for ${relativePath} due to discovery failure.`,
228+
);
229+
// By *not* adding this script to newDefinitions, we effectively remove it.
230+
changesMade = true; // Mark that a change (removal) occurred
231+
}
232+
// If no old cache existed, we just logged the failure above.
233+
}
234+
} // --- End of script file loop ---
235+
236+
// --- Final Cleanup: Remove data for scripts no longer present and orphaned values ---
237+
const previouslyCachedPaths = Object.keys(
238+
plugin.settings.scriptSettingsDefinitions,
239+
);
240+
for (const cachedPath of previouslyCachedPaths) {
241+
if (!currentScriptPaths.has(cachedPath)) {
242+
// Script file was deleted from the folder
159243
changesMade = true;
160-
plugin.logInfo(`Script ${cachedPath} removed, clearing its settings definitions and values.`);
161-
delete plugin.settings.scriptSettingsDefinitions[cachedPath];
162-
if (plugin.settings.scriptSettingsValues.hasOwnProperty(cachedPath)) delete plugin.settings.scriptSettingsValues[cachedPath];
163-
if (plugin.settings.scriptActivationStatus.hasOwnProperty(cachedPath)) delete plugin.settings.scriptActivationStatus[cachedPath];
164-
if (plugin.settings.scriptAutoStartStatus.hasOwnProperty(cachedPath)) delete plugin.settings.scriptAutoStartStatus[cachedPath];
165-
if (plugin.settings.scriptAutoStartDelay.hasOwnProperty(cachedPath)) delete plugin.settings.scriptAutoStartDelay[cachedPath];
244+
plugin.logInfo(
245+
`Script ${cachedPath} removed, clearing its settings definitions and values.`,
246+
);
247+
// Definition is already absent from newDefinitions because it wasn't in currentScriptPaths.
248+
// Clean up associated values and statuses.
249+
if (plugin.settings.scriptSettingsValues.hasOwnProperty(cachedPath)) {
250+
delete plugin.settings.scriptSettingsValues[cachedPath];
251+
}
252+
if (
253+
plugin.settings.scriptActivationStatus.hasOwnProperty(cachedPath)
254+
) {
255+
delete plugin.settings.scriptActivationStatus[cachedPath];
256+
}
257+
if (
258+
plugin.settings.scriptAutoStartStatus.hasOwnProperty(cachedPath)
259+
) {
260+
delete plugin.settings.scriptAutoStartStatus[cachedPath];
261+
}
262+
if (
263+
plugin.settings.scriptAutoStartDelay.hasOwnProperty(cachedPath)
264+
) {
265+
delete plugin.settings.scriptAutoStartDelay[cachedPath];
266+
}
267+
} else if (!newDefinitions.hasOwnProperty(cachedPath)) {
268+
// Script file exists, but discovery failed (and it was previously cached)
269+
// The definition was already omitted from newDefinitions above.
270+
// Also remove any stored values for this script.
271+
if (plugin.settings.scriptSettingsValues.hasOwnProperty(cachedPath)) {
272+
plugin.logInfo(
273+
`Clearing settings values for ${cachedPath} due to discovery failure.`,
274+
);
275+
delete plugin.settings.scriptSettingsValues[cachedPath];
276+
changesMade = true; // Ensure change is marked
277+
}
278+
// Keep activation/autostart status even if settings discovery fails? Yes, seems reasonable.
166279
}
167280
}
168-
if (cachedPaths.length !== currentDefinitionKeys.size) changesMade = true;
281+
282+
// Check if the overall structure of definitions changed (covers additions/removals)
283+
if (
284+
JSON.stringify(newDefinitions) !==
285+
JSON.stringify(plugin.settings.scriptSettingsDefinitions)
286+
) {
287+
changesMade = true;
288+
}
289+
290+
// Save settings only if there were actual changes to definitions or values
169291
if (changesMade) {
170292
plugin.logInfo("Script settings definitions cache updated.");
171293
plugin.settings.scriptSettingsDefinitions = newDefinitions;
172-
await plugin.saveSettings(); // Save updated definitions and potentially cleared values
294+
// Note: scriptSettingsValues and statuses might have been modified directly above
295+
await plugin.saveSettings();
173296
} else {
174297
plugin.logInfo("Script settings definitions cache is up to date.");
175298
}

0 commit comments

Comments
 (0)