Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 28 additions & 10 deletions MCPForUnity/Editor/Helpers/ComponentOps.cs
Original file line number Diff line number Diff line change
Expand Up @@ -174,15 +174,37 @@ public static bool SetProperty(Component component, string propertyName, JToken
return SetViaSerializedProperty(component, propertyName, normalizedName, value, out error);
}

// Try property first - check both original and normalized names for backwards compatibility
PropertyInfo propInfo = type.GetProperty(propertyName, flags)
// Try reflection first (property, field, then non-public serialized field)
if (TrySetViaReflection(component, type, propertyName, normalizedName, flags, value, out error))
return true;

// Reflection failed — fall back to SerializedProperty which handles arrays,
// custom serialization (e.g. UdonSharp), and types reflection can't convert.
string reflectionError = error;
if (SetViaSerializedProperty(component, propertyName, normalizedName, value, out error))
return true;

// Both paths failed. If reflection found the member but couldn't convert,
// report that (more useful than the SerializedProperty error).
// If reflection didn't find it at all, report the SerializedProperty error.
if (reflectionError != null && !reflectionError.Contains("not found"))
error = reflectionError;

return false;
}

private static bool TrySetViaReflection(object component, Type type, string propertyName, string normalizedName, BindingFlags flags, JToken value, out string error)
{
error = null;

// Try property first
PropertyInfo propInfo = type.GetProperty(propertyName, flags)
?? type.GetProperty(normalizedName, flags);
if (propInfo != null && propInfo.CanWrite)
{
try
{
object convertedValue = PropertyConversion.ConvertToType(value, propInfo.PropertyType);
// Detect conversion failure: null result when input wasn't null
if (convertedValue == null && value.Type != JTokenType.Null)
{
error = $"Failed to convert value for property '{propertyName}' to type '{propInfo.PropertyType.Name}'.";
Expand All @@ -198,15 +220,14 @@ public static bool SetProperty(Component component, string propertyName, JToken
}
}

// Try field - check both original and normalized names for backwards compatibility
FieldInfo fieldInfo = type.GetField(propertyName, flags)
// Try field
FieldInfo fieldInfo = type.GetField(propertyName, flags)
?? type.GetField(normalizedName, flags);
if (fieldInfo != null && !fieldInfo.IsInitOnly)
{
try
{
object convertedValue = PropertyConversion.ConvertToType(value, fieldInfo.FieldType);
// Detect conversion failure: null result when input wasn't null
if (convertedValue == null && value.Type != JTokenType.Null)
{
error = $"Failed to convert value for field '{propertyName}' to type '{fieldInfo.FieldType.Name}'.";
Expand All @@ -222,17 +243,14 @@ public static bool SetProperty(Component component, string propertyName, JToken
}
}

// Try non-public serialized fields - traverse inheritance hierarchy
// Type.GetField() with NonPublic only finds fields declared directly on that type,
// so we need to walk up the inheritance chain manually
// Try non-public serialized fields — traverse inheritance hierarchy
fieldInfo = FindSerializedFieldInHierarchy(type, propertyName)
?? FindSerializedFieldInHierarchy(type, normalizedName);
if (fieldInfo != null)
{
try
{
object convertedValue = PropertyConversion.ConvertToType(value, fieldInfo.FieldType);
// Detect conversion failure: null result when input wasn't null
if (convertedValue == null && value.Type != JTokenType.Null)
{
error = $"Failed to convert value for serialized field '{propertyName}' to type '{fieldInfo.FieldType.Name}'.";
Expand Down
76 changes: 47 additions & 29 deletions MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -172,14 +172,26 @@ internal static object SetComponentPropertiesInternal(GameObject targetGo, strin

try
{
bool setResult = SetProperty(targetComponent, propName, propValue);
bool setResult = SetProperty(targetComponent, propName, propValue, out string setError);
if (!setResult)
{
var availableProperties = ComponentResolver.GetAllComponentProperties(targetComponent.GetType());
var suggestions = ComponentResolver.GetFuzzyPropertySuggestions(propName, availableProperties);
var msg = suggestions.Any()
? $"Property '{propName}' not found. Did you mean: {string.Join(", ", suggestions)}? Available: [{string.Join(", ", availableProperties)}]"
: $"Property '{propName}' not found. Available: [{string.Join(", ", availableProperties)}]";
// Local reflection failed — fall back to ComponentOps which
// tries SerializedProperty (handles arrays, UdonSharp, etc.)
if (ComponentOps.SetProperty(targetComponent, propName, propValue, out string opsError))
{
continue;
}

// Both paths failed. Prefer the more specific error.
string msg = setError ?? opsError;
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
Outdated
if (msg == null || msg.Contains("not found"))
{
var availableProperties = ComponentResolver.GetAllComponentProperties(targetComponent.GetType());
var suggestions = ComponentResolver.GetFuzzyPropertySuggestions(propName, availableProperties);
msg = suggestions.Any()
? $"Property '{propName}' not found. Did you mean: {string.Join(", ", suggestions)}? Available: [{string.Join(", ", availableProperties)}]"
: $"Property '{propName}' not found. Available: [{string.Join(", ", availableProperties)}]";
}
McpLog.Warn($"[ManageGameObject] {msg}");
failures.Add(msg);
}
Expand All @@ -199,8 +211,9 @@ internal static object SetComponentPropertiesInternal(GameObject targetGo, strin

private static JsonSerializer InputSerializer => UnityJsonSerializer.Instance;

private static bool SetProperty(object target, string memberName, JToken value)
private static bool SetProperty(object target, string memberName, JToken value, out string error)
{
error = null;
Type type = target.GetType();
BindingFlags flags = BindingFlags.Public | BindingFlags.Instance | BindingFlags.IgnoreCase;

Expand All @@ -223,39 +236,44 @@ private static bool SetProperty(object target, string memberName, JToken value)
propInfo.SetValue(target, convertedValue);
return true;
}
error = $"Failed to convert value for property '{memberName}' to type '{propInfo.PropertyType.Name}'.";
return false;
}
else

FieldInfo fieldInfo = type.GetField(memberName, flags) ?? type.GetField(normalizedName, flags);
if (fieldInfo != null)
{
FieldInfo fieldInfo = type.GetField(memberName, flags) ?? type.GetField(normalizedName, flags);
if (fieldInfo != null)
object convertedValue = ConvertJTokenToType(value, fieldInfo.FieldType, inputSerializer);
if (convertedValue != null || value.Type == JTokenType.Null)
{
object convertedValue = ConvertJTokenToType(value, fieldInfo.FieldType, inputSerializer);
if (convertedValue != null || value.Type == JTokenType.Null)
{
fieldInfo.SetValue(target, convertedValue);
return true;
}
fieldInfo.SetValue(target, convertedValue);
return true;
}
else
error = $"Failed to convert value for field '{memberName}' to type '{fieldInfo.FieldType.Name}'.";
return false;
}

var npField = type.GetField(memberName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase)
?? type.GetField(normalizedName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase);
if (npField != null && npField.GetCustomAttribute<SerializeField>() != null)
{
object convertedValue = ConvertJTokenToType(value, npField.FieldType, inputSerializer);
if (convertedValue != null || value.Type == JTokenType.Null)
{
var npField = type.GetField(memberName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase)
?? type.GetField(normalizedName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase);
if (npField != null && npField.GetCustomAttribute<SerializeField>() != null)
{
object convertedValue = ConvertJTokenToType(value, npField.FieldType, inputSerializer);
if (convertedValue != null || value.Type == JTokenType.Null)
{
npField.SetValue(target, convertedValue);
return true;
}
}
npField.SetValue(target, convertedValue);
return true;
}
error = $"Failed to convert value for serialized field '{memberName}' to type '{npField.FieldType.Name}'.";
return false;
}
}
catch (Exception ex)
{
McpLog.Error($"[SetProperty] Failed to set '{memberName}' on {type.Name}: {ex.Message}\nToken: {value.ToString(Formatting.None)}");
error = $"Failed to set '{memberName}' on {type.Name}: {ex.Message}\nToken: {value.ToString(Formatting.None)}";
return false;
}

// Property/field not found — caller will generate "not found" with suggestions
return false;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
using System.Text.RegularExpressions;
using NUnit.Framework;
using UnityEngine;
using UnityEngine.TestTools;
using Newtonsoft.Json.Linq;
using MCPForUnity.Editor.Helpers;
using MCPForUnity.Editor.Tools;
using MCPForUnity.Editor.Tools.GameObjects;

namespace MCPForUnityTests.Editor.Tools
{
/// <summary>
/// Tests for GameObjectComponentHelpers.SetComponentPropertiesInternal error reporting.
/// Reproduces issue #765: conversion failures incorrectly reported as "Property not found".
/// </summary>
public class GameObjectComponentHelpersErrorTests
{
private GameObject testGo;

[SetUp]
public void SetUp()
{
testGo = new GameObject("ErrorTestGO");
CommandRegistry.Initialize();
}

[TearDown]
public void TearDown()
{
if (testGo != null)
Object.DestroyImmediate(testGo);
}

/// <summary>
/// When a property exists but conversion fails, the error should say
/// "Failed to convert" rather than "Property not found. Did you mean: X?"
/// </summary>
[Test]
public void SetComponentProperties_ConversionFailure_ReportsConversionError_NotPropertyNotFound()
{
// Expect conversion error logs from PropertyConversion (once from local SetProperty,
// once from ComponentOps fallback trying reflection before SerializedProperty)
LogAssert.Expect(LogType.Error, new Regex("Error converting token"));
LogAssert.Expect(LogType.Error, new Regex("Error converting token"));
// Expect the warning log from SetComponentPropertiesInternal
LogAssert.Expect(LogType.Warning, new Regex("Failed to set"));

var audioSource = testGo.AddComponent<AudioSource>();

// spatialBlend is a float property — passing an array triggers conversion failure
var props = new JObject { ["spatialBlend"] = JArray.Parse("[1, 2, 3]") };

var result = GameObjectComponentHelpers.SetComponentPropertiesInternal(
testGo, "AudioSource", props, audioSource);

Assert.IsNotNull(result, "Should return an error response");
Assert.IsInstanceOf<ErrorResponse>(result);

var errorResponse = (ErrorResponse)result;

// The error message must NOT say "not found" for a property that exists
Assert.IsFalse(
errorResponse.Error.Contains("not found"),
$"Error should report conversion failure, not 'not found'. Got: {errorResponse.Error}");
Comment on lines +54 to +62

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Find the ErrorResponse class definition
fd -e cs -x grep -l "class ErrorResponse" {}

Repository: CoplayDev/unity-mcp

Length of output: 103


🏁 Script executed:

# Search for ErrorResponse class definition with context
rg -n "class ErrorResponse" --type=cs -A 20

Repository: CoplayDev/unity-mcp

Length of output: 1574


🏁 Script executed:

# Also search for any properties or methods that might concatenate error messages
rg -n "ErrorResponse" --type=cs -B 2 -A 10 | head -100

Repository: CoplayDev/unity-mcp

Length of output: 8341


🏁 Script executed:

# Get the full ErrorResponse class definition
sed -n '35,80p' MCPForUnity/Editor/Helpers/Response.cs

Repository: CoplayDev/unity-mcp

Length of output: 1334


🏁 Script executed:

# Look for the ErrorResponse constructor
rg -n "public ErrorResponse" MCPForUnity/Editor/Helpers/Response.cs -A 15

Repository: CoplayDev/unity-mcp

Length of output: 554


Test assertions do not validate the intended behavior — they check the wrong error property.

ErrorResponse.Error contains only the top-level message passed to the constructor:

new ErrorResponse($"One or more properties failed on '{componentTypeName}'.", new { errors = failures })

Results in errorResponse.Error = "One or more properties failed on 'AudioSource'." The individual error messages live in Data.errors, not in the Error property.

Consequently:

  • Test 1 (line 62): !errorResponse.Error.Contains("not found") always passes because the top-level message never contains "not found" — regardless of whether conversion errors are actually reported correctly.
  • Test 2 (line 88): errorResponse.Error.Contains("failed") always passes because the top-level message always contains "failed" — failing to distinguish between conversion failures and "not found" errors.

Both assertions should inspect the individual failure messages in Data.errors to properly verify that conversion failures report correctly and do not report "not found".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs`
around lines 56 - 64, The tests are asserting against ErrorResponse.Error (the
top-level message) instead of the detailed failure messages stored in
ErrorResponse.Data.errors; update the assertions in
GameObjectComponentHelpersErrorTests.cs to inspect errorResponse.Data.errors
(cast/unpack it to the expected collection or dynamic) and assert that the
individual error strings contain "failed" for conversion failures and do NOT
contain "not found" for properties that exist, referencing the ErrorResponse
type and the Data.errors payload when locating where to change the checks.

}

/// <summary>
/// When a property genuinely doesn't exist, the error should still say "not found" with suggestions.
/// </summary>
[Test]
public void SetComponentProperties_NonexistentProperty_ReportsNotFound()
{
// Expect the "not found" warning
LogAssert.Expect(LogType.Warning, new Regex("not found"));

var audioSource = testGo.AddComponent<AudioSource>();

var props = new JObject { ["totallyFakeProperty"] = 42 };

var result = GameObjectComponentHelpers.SetComponentPropertiesInternal(
testGo, "AudioSource", props, audioSource);

Assert.IsNotNull(result);
Assert.IsInstanceOf<ErrorResponse>(result);

var errorResponse = (ErrorResponse)result;

Assert.IsTrue(
errorResponse.Error.Contains("not found") || errorResponse.Error.Contains("failed"),
$"Error for nonexistent property should say 'not found'. Got: {errorResponse.Error}");
}

/// <summary>
/// Valid property setting should still succeed.
/// </summary>
[Test]
public void SetComponentProperties_ValidProperty_Succeeds()
{
var audioSource = testGo.AddComponent<AudioSource>();

var props = new JObject { ["volume"] = 0.42f };

var result = GameObjectComponentHelpers.SetComponentPropertiesInternal(
testGo, "AudioSource", props, audioSource);

Assert.IsNull(result, "Should return null on success (no errors)");
Assert.AreEqual(0.42f, audioSource.volume, 0.001f);
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.