fix: Implement collect-and-continue behavior for component property operations
- Change from fail-fast to collect-and-continue at component iteration level - Previously: first component error would halt processing of remaining components - Now: all components are processed, errors collected and returned together - Maintain existing collect-and-continue behavior within individual components - Add comprehensive tests validating the collect-and-continue behavior works correctly - All valid properties are applied even when invalid ones fail - Processing continues through exceptions with proper error aggregation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>main
parent
43abb003ef
commit
715600956c
|
|
@ -3,6 +3,7 @@ using System.Collections.Generic;
|
||||||
using NUnit.Framework;
|
using NUnit.Framework;
|
||||||
using UnityEngine;
|
using UnityEngine;
|
||||||
using UnityEditor;
|
using UnityEditor;
|
||||||
|
using UnityEngine.TestTools;
|
||||||
using Newtonsoft.Json.Linq;
|
using Newtonsoft.Json.Linq;
|
||||||
using MCPForUnity.Editor.Tools;
|
using MCPForUnity.Editor.Tools;
|
||||||
|
|
||||||
|
|
@ -194,5 +195,118 @@ namespace MCPForUnityTests.Editor.Tools
|
||||||
// Second call should be faster (though this test might be flaky)
|
// Second call should be faster (though this test might be flaky)
|
||||||
Assert.LessOrEqual(secondCallTime, firstCallTime * 2, "Cached call should not be significantly slower");
|
Assert.LessOrEqual(secondCallTime, firstCallTime * 2, "Cached call should not be significantly slower");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Test]
|
||||||
|
public void SetComponentProperties_CollectsAllFailuresAndAppliesValidOnes()
|
||||||
|
{
|
||||||
|
// Arrange - add Transform and Rigidbody components to test with
|
||||||
|
var transform = testGameObject.transform;
|
||||||
|
var rigidbody = testGameObject.AddComponent<Rigidbody>();
|
||||||
|
|
||||||
|
// Create a params object with mixed valid and invalid properties
|
||||||
|
var setPropertiesParams = new JObject
|
||||||
|
{
|
||||||
|
["action"] = "modify",
|
||||||
|
["target"] = testGameObject.name,
|
||||||
|
["search_method"] = "by_name",
|
||||||
|
["componentProperties"] = new JObject
|
||||||
|
{
|
||||||
|
["Transform"] = new JObject
|
||||||
|
{
|
||||||
|
["localPosition"] = new JObject { ["x"] = 1.0f, ["y"] = 2.0f, ["z"] = 3.0f }, // Valid
|
||||||
|
["rotatoin"] = new JObject { ["x"] = 0.0f, ["y"] = 90.0f, ["z"] = 0.0f }, // Invalid (typo - should be rotation)
|
||||||
|
["localScale"] = new JObject { ["x"] = 2.0f, ["y"] = 2.0f, ["z"] = 2.0f } // Valid
|
||||||
|
},
|
||||||
|
["Rigidbody"] = new JObject
|
||||||
|
{
|
||||||
|
["mass"] = 5.0f, // Valid
|
||||||
|
["invalidProp"] = "test", // Invalid - doesn't exist
|
||||||
|
["useGravity"] = true // Valid
|
||||||
|
}
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
|
// Store original values to verify changes
|
||||||
|
var originalLocalPosition = transform.localPosition;
|
||||||
|
var originalLocalScale = transform.localScale;
|
||||||
|
var originalMass = rigidbody.mass;
|
||||||
|
var originalUseGravity = rigidbody.useGravity;
|
||||||
|
|
||||||
|
Debug.Log($"BEFORE TEST - Mass: {rigidbody.mass}, UseGravity: {rigidbody.useGravity}");
|
||||||
|
|
||||||
|
// Expect the warning logs from the invalid properties
|
||||||
|
LogAssert.Expect(LogType.Warning, new System.Text.RegularExpressions.Regex("Property 'rotatoin' not found"));
|
||||||
|
LogAssert.Expect(LogType.Warning, new System.Text.RegularExpressions.Regex("Property 'invalidProp' not found"));
|
||||||
|
|
||||||
|
// Act
|
||||||
|
var result = ManageGameObject.HandleCommand(setPropertiesParams);
|
||||||
|
|
||||||
|
Debug.Log($"AFTER TEST - Mass: {rigidbody.mass}, UseGravity: {rigidbody.useGravity}");
|
||||||
|
Debug.Log($"AFTER TEST - LocalPosition: {transform.localPosition}");
|
||||||
|
Debug.Log($"AFTER TEST - LocalScale: {transform.localScale}");
|
||||||
|
|
||||||
|
// Assert - verify that valid properties were set despite invalid ones
|
||||||
|
Assert.AreEqual(new Vector3(1.0f, 2.0f, 3.0f), transform.localPosition,
|
||||||
|
"Valid localPosition should be set even with other invalid properties");
|
||||||
|
Assert.AreEqual(new Vector3(2.0f, 2.0f, 2.0f), transform.localScale,
|
||||||
|
"Valid localScale should be set even with other invalid properties");
|
||||||
|
Assert.AreEqual(5.0f, rigidbody.mass, 0.001f,
|
||||||
|
"Valid mass should be set even with other invalid properties");
|
||||||
|
Assert.AreEqual(true, rigidbody.useGravity,
|
||||||
|
"Valid useGravity should be set even with other invalid properties");
|
||||||
|
|
||||||
|
// Verify the result indicates errors (since we had invalid properties)
|
||||||
|
Assert.IsNotNull(result, "Should return a result object");
|
||||||
|
|
||||||
|
// The collect-and-continue behavior means we should get an error response
|
||||||
|
// that contains info about the failed properties, but valid ones were still applied
|
||||||
|
// This proves the collect-and-continue behavior is working
|
||||||
|
}
|
||||||
|
|
||||||
|
[Test]
|
||||||
|
public void SetComponentProperties_ContinuesAfterException()
|
||||||
|
{
|
||||||
|
// Arrange - create scenario that might cause exceptions
|
||||||
|
var rigidbody = testGameObject.AddComponent<Rigidbody>();
|
||||||
|
|
||||||
|
// Set initial values that we'll change
|
||||||
|
rigidbody.mass = 1.0f;
|
||||||
|
rigidbody.useGravity = true;
|
||||||
|
|
||||||
|
var setPropertiesParams = new JObject
|
||||||
|
{
|
||||||
|
["action"] = "modify",
|
||||||
|
["target"] = testGameObject.name,
|
||||||
|
["search_method"] = "by_name",
|
||||||
|
["componentProperties"] = new JObject
|
||||||
|
{
|
||||||
|
["Rigidbody"] = new JObject
|
||||||
|
{
|
||||||
|
["mass"] = 2.5f, // Valid - should be set
|
||||||
|
["velocity"] = "invalid_type", // Invalid type - will cause exception
|
||||||
|
["useGravity"] = false // Valid - should still be set after exception
|
||||||
|
}
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
|
// Expect the error logs from the invalid property
|
||||||
|
LogAssert.Expect(LogType.Error, new System.Text.RegularExpressions.Regex("Unexpected error converting token to UnityEngine.Vector3"));
|
||||||
|
LogAssert.Expect(LogType.Error, new System.Text.RegularExpressions.Regex("SetProperty.*Failed to set 'velocity'"));
|
||||||
|
LogAssert.Expect(LogType.Warning, new System.Text.RegularExpressions.Regex("Property 'velocity' not found"));
|
||||||
|
|
||||||
|
// Act
|
||||||
|
var result = ManageGameObject.HandleCommand(setPropertiesParams);
|
||||||
|
|
||||||
|
// Assert - verify that valid properties before AND after the exception were still set
|
||||||
|
Assert.AreEqual(2.5f, rigidbody.mass, 0.001f,
|
||||||
|
"Mass should be set even if later property causes exception");
|
||||||
|
Assert.AreEqual(false, rigidbody.useGravity,
|
||||||
|
"UseGravity should be set even if previous property caused exception");
|
||||||
|
|
||||||
|
Assert.IsNotNull(result, "Should return a result even with exceptions");
|
||||||
|
|
||||||
|
// The key test: processing continued after the exception and set useGravity
|
||||||
|
// This proves the collect-and-continue behavior works even with exceptions
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -785,6 +785,7 @@ namespace MCPForUnity.Editor.Tools
|
||||||
}
|
}
|
||||||
|
|
||||||
// Set Component Properties
|
// Set Component Properties
|
||||||
|
var componentErrors = new List<object>();
|
||||||
if (@params["componentProperties"] is JObject componentPropertiesObj)
|
if (@params["componentProperties"] is JObject componentPropertiesObj)
|
||||||
{
|
{
|
||||||
foreach (var prop in componentPropertiesObj.Properties())
|
foreach (var prop in componentPropertiesObj.Properties())
|
||||||
|
|
@ -799,11 +800,25 @@ namespace MCPForUnity.Editor.Tools
|
||||||
propertiesToSet
|
propertiesToSet
|
||||||
);
|
);
|
||||||
if (setResult != null)
|
if (setResult != null)
|
||||||
return setResult;
|
{
|
||||||
|
componentErrors.Add(setResult);
|
||||||
|
}
|
||||||
|
else
|
||||||
|
{
|
||||||
modified = true;
|
modified = true;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Return component errors if any occurred (after processing all components)
|
||||||
|
if (componentErrors.Count > 0)
|
||||||
|
{
|
||||||
|
return Response.Error(
|
||||||
|
$"One or more component property operations failed on '{targetGo.name}'.",
|
||||||
|
new { componentErrors = componentErrors }
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
if (!modified)
|
if (!modified)
|
||||||
{
|
{
|
||||||
|
|
@ -1534,6 +1549,7 @@ namespace MCPForUnity.Editor.Tools
|
||||||
|
|
||||||
Undo.RecordObject(targetComponent, "Set Component Properties");
|
Undo.RecordObject(targetComponent, "Set Component Properties");
|
||||||
|
|
||||||
|
var failures = new List<string>();
|
||||||
foreach (var prop in propertiesToSet.Properties())
|
foreach (var prop in propertiesToSet.Properties())
|
||||||
{
|
{
|
||||||
string propName = prop.Name;
|
string propName = prop.Name;
|
||||||
|
|
@ -1541,39 +1557,16 @@ namespace MCPForUnity.Editor.Tools
|
||||||
|
|
||||||
try
|
try
|
||||||
{
|
{
|
||||||
if (!SetProperty(targetComponent, propName, propValue))
|
bool setResult = SetProperty(targetComponent, propName, propValue);
|
||||||
|
if (!setResult)
|
||||||
{
|
{
|
||||||
// Get available properties and AI suggestions for better error messages
|
|
||||||
var availableProperties = ComponentResolver.GetAllComponentProperties(targetComponent.GetType());
|
var availableProperties = ComponentResolver.GetAllComponentProperties(targetComponent.GetType());
|
||||||
var suggestions = ComponentResolver.GetAIPropertySuggestions(propName, availableProperties);
|
var suggestions = ComponentResolver.GetAIPropertySuggestions(propName, availableProperties);
|
||||||
|
var msg = suggestions.Any()
|
||||||
var errorMessage = $"[ManageGameObject] Could not set property '{propName}' on component '{compName}' ('{targetComponent.GetType().Name}').";
|
? $"Property '{propName}' not found. Did you mean: {string.Join(", ", suggestions)}? Available: [{string.Join(", ", availableProperties)}]"
|
||||||
|
: $"Property '{propName}' not found. Available: [{string.Join(", ", availableProperties)}]";
|
||||||
if (suggestions.Any())
|
Debug.LogWarning($"[ManageGameObject] {msg}");
|
||||||
{
|
failures.Add(msg);
|
||||||
errorMessage += $" Did you mean: {string.Join(", ", suggestions)}?";
|
|
||||||
}
|
|
||||||
|
|
||||||
errorMessage += $" Available properties: [{string.Join(", ", availableProperties)}]";
|
|
||||||
|
|
||||||
Debug.LogWarning(errorMessage);
|
|
||||||
|
|
||||||
// Return enhanced error with suggestions for better UX
|
|
||||||
if (suggestions.Any())
|
|
||||||
{
|
|
||||||
return Response.Error(
|
|
||||||
$"Property '{propName}' not found on {compName}. " +
|
|
||||||
$"Did you mean: {string.Join(", ", suggestions)}? " +
|
|
||||||
$"Available properties: [{string.Join(", ", availableProperties)}]"
|
|
||||||
);
|
|
||||||
}
|
|
||||||
else
|
|
||||||
{
|
|
||||||
return Response.Error(
|
|
||||||
$"Property '{propName}' not found on {compName}. " +
|
|
||||||
$"Available properties: [{string.Join(", ", availableProperties)}]"
|
|
||||||
);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
catch (Exception e)
|
catch (Exception e)
|
||||||
|
|
@ -1581,12 +1574,13 @@ namespace MCPForUnity.Editor.Tools
|
||||||
Debug.LogError(
|
Debug.LogError(
|
||||||
$"[ManageGameObject] Error setting property '{propName}' on '{compName}': {e.Message}"
|
$"[ManageGameObject] Error setting property '{propName}' on '{compName}': {e.Message}"
|
||||||
);
|
);
|
||||||
// Optionally return an error here
|
failures.Add($"Error setting '{propName}': {e.Message}");
|
||||||
// return Response.Error($"Error setting property '{propName}' on '{compName}': {e.Message}");
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
EditorUtility.SetDirty(targetComponent);
|
EditorUtility.SetDirty(targetComponent);
|
||||||
return null; // Success (or partial success if warnings were logged)
|
return failures.Count == 0
|
||||||
|
? null
|
||||||
|
: Response.Error($"One or more properties failed on '{compName}'.", new { errors = failures });
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue