Skip to content

Commit cb7d2de

Browse files
Mark Schareinaoliverbock
Mark Schareina
authored and
oliverbock
committed
Refactor dictionary access to use indexer
1 parent 2b83a94 commit cb7d2de

File tree

5 files changed

+144
-179
lines changed

5 files changed

+144
-179
lines changed

‎Source/Noesis.Javascript/JavascriptExternal.cpp

Lines changed: 67 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -131,30 +131,45 @@ JavascriptExternal::GetProperty(wstring iName, Handle<Value> &result)
131131
PropertyInfo^ propertyInfo = type->GetProperty(gcnew System::String(iName.c_str()));
132132

133133
v8::Isolate *isolate = JavascriptContext::GetCurrentIsolate();
134-
if (propertyInfo == nullptr)
135-
return false;
136-
else {
137-
try
134+
try
135+
{
136+
if (propertyInfo == nullptr)
138137
{
139-
if (!propertyInfo->CanRead)
138+
//may have an indexer
139+
PropertyInfo^ indexerInfo = type->GetProperty("Item", System::Object::typeid, gcnew cli::array<System::Type^> { System::String::typeid });
140+
if (indexerInfo == nullptr)
141+
{
142+
return false;
143+
}
144+
if (!indexerInfo->CanRead)
140145
{
141146
result = isolate->ThrowException(JavascriptInterop::ConvertToV8("Property " + gcnew System::String(iName.c_str()) + " may not be read."));
142147
}
143148
else
144149
{
145-
result = JavascriptInterop::ConvertToV8(propertyInfo->GetValue(self, nullptr));
150+
result = JavascriptInterop::ConvertToV8(indexerInfo->GetValue(self, gcnew cli::array<System::String^> { gcnew System::String(iName.c_str()) }));
146151
}
152+
return true;
147153
}
148-
catch(System::Reflection::TargetInvocationException^ exception)
154+
155+
if (!propertyInfo->CanRead)
149156
{
150-
result = JavascriptInterop::HandleTargetInvocationException(exception);
157+
result = isolate->ThrowException(JavascriptInterop::ConvertToV8("Property " + gcnew System::String(iName.c_str()) + " may not be read."));
151158
}
152-
catch(System::Exception^ exception)
159+
else
153160
{
154-
result = isolate->ThrowException(JavascriptInterop::ConvertToV8(exception));
161+
result = JavascriptInterop::ConvertToV8(propertyInfo->GetValue(self, nullptr));
155162
}
156-
return true;
157163
}
164+
catch (System::Reflection::TargetInvocationException^ exception)
165+
{
166+
result = JavascriptInterop::HandleTargetInvocationException(exception);
167+
}
168+
catch (System::Exception^ exception)
169+
{
170+
result = isolate->ThrowException(JavascriptInterop::ConvertToV8(exception));
171+
}
172+
return true;
158173
}
159174

160175
////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -212,48 +227,62 @@ JavascriptExternal::SetProperty(wstring iName, Handle<Value> iValue)
212227
PropertyInfo^ propertyInfo = type->GetProperty(gcnew System::String(iName.c_str()));
213228

214229
v8::Isolate *isolate = JavascriptContext::GetCurrentIsolate();
215-
if (propertyInfo == nullptr)
216-
{
217-
if ((mOptions & SetParameterOptions::RejectUnknownProperties) == SetParameterOptions::RejectUnknownProperties)
218-
return isolate->ThrowException(JavascriptInterop::ConvertToV8("Unknown member: " + gcnew System::String(iName.c_str())));
219-
}
220-
else
230+
231+
try
221232
{
222-
try
233+
if (propertyInfo == nullptr)
223234
{
224-
System::Object^ value = JavascriptInterop::ConvertFromV8(iValue);
225-
if (value != nullptr) {
226-
System::Type^ valueType = value->GetType();
227-
System::Type^ propertyType = propertyInfo->PropertyType;
228-
229-
// attempt conversion if assigned value is of wrong type
230-
if (propertyType != valueType && !propertyType->IsAssignableFrom(valueType))
231-
value = SystemInterop::ConvertToType(value, propertyType);
235+
//may have an indexer
236+
PropertyInfo^ indexerInfo = type->GetProperty("Item", System::Object::typeid, gcnew cli::array<System::Type^> { System::String::typeid });
237+
if (indexerInfo == nullptr)
238+
{
239+
if ((mOptions & SetParameterOptions::RejectUnknownProperties) == SetParameterOptions::RejectUnknownProperties)
240+
return isolate->ThrowException(JavascriptInterop::ConvertToV8("Unknown member: " + gcnew System::String(iName.c_str())));
241+
return Handle<Value>();
232242
}
233-
234-
if (!propertyInfo->CanWrite)
243+
if (!indexerInfo->CanWrite)
235244
{
236245
return isolate->ThrowException(JavascriptInterop::ConvertToV8("Property " + gcnew System::String(iName.c_str()) + " may not be set."));
237246
}
238247
else
239248
{
240-
propertyInfo->SetValue(self, value, nullptr);
241-
// We used to convert and return propertyInfo->GetValue() here.
242-
// I don't know why we did, but I stopped it because CanRead
243-
// might be false, which should not stop us _setting_.
244-
// Also it wastes precious CPU time.
245-
return iValue;
249+
indexerInfo->SetValue(self, JavascriptInterop::ConvertFromV8(iValue), gcnew cli::array<System::String^> { gcnew System::String(iName.c_str()) });
246250
}
251+
return iValue;
247252
}
248-
catch(System::Reflection::TargetInvocationException^ exception)
253+
254+
System::Object^ value = JavascriptInterop::ConvertFromV8(iValue);
255+
if (value != nullptr) {
256+
System::Type^ valueType = value->GetType();
257+
System::Type^ propertyType = propertyInfo->PropertyType;
258+
259+
// attempt conversion if assigned value is of wrong type
260+
if (propertyType != valueType && !propertyType->IsAssignableFrom(valueType))
261+
value = SystemInterop::ConvertToType(value, propertyType);
262+
}
263+
264+
if (!propertyInfo->CanWrite)
249265
{
250-
return JavascriptInterop::HandleTargetInvocationException(exception);
266+
return isolate->ThrowException(JavascriptInterop::ConvertToV8("Property " + gcnew System::String(iName.c_str()) + " may not be set."));
251267
}
252-
catch(System::Exception^ exception)
268+
else
253269
{
254-
return isolate->ThrowException(JavascriptInterop::ConvertToV8(exception));
270+
propertyInfo->SetValue(self, value, nullptr);
271+
// We used to convert and return propertyInfo->GetValue() here.
272+
// I don't know why we did, but I stopped it because CanRead
273+
// might be false, which should not stop us _setting_.
274+
// Also it wastes precious CPU time.
275+
return iValue;
255276
}
256277
}
278+
catch (System::Reflection::TargetInvocationException^ exception)
279+
{
280+
return JavascriptInterop::HandleTargetInvocationException(exception);
281+
}
282+
catch (System::Exception^ exception)
283+
{
284+
return isolate->ThrowException(JavascriptInterop::ConvertToV8(exception));
285+
}
257286

258287
return Handle<Value>();
259288
}

‎Source/Noesis.Javascript/JavascriptInterop.cpp

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -621,17 +621,6 @@ JavascriptInterop::Setter(Local<String> iName, Local<Value> iValue, const Proper
621621
Handle<External> external = Handle<External>::Cast(iInfo.Holder()->GetInternalField(0));
622622
JavascriptExternal* wrapper = (JavascriptExternal*) external->Value();
623623

624-
System::Object^ self = wrapper->GetObject();
625-
System::Type^ type = self->GetType();
626-
if (type->IsGenericType && type->GetInterface(System::Collections::Generic::IDictionary::typeid->Name) != nullptr)
627-
{
628-
// set property for c# dictionary object
629-
System::Collections::Generic::IDictionary<System::String^, System::Object^>^ dict = (System::Collections::Generic::IDictionary<System::String^, System::Object^>^)self;
630-
System::String^ key = gcnew System::String((wchar_t*)*String::Value(iName));
631-
System::Object^ value = ConvertFromV8(iValue);
632-
dict[key] = value;
633-
}
634-
635624
// set property
636625
iInfo.GetReturnValue().Set(wrapper->SetProperty(name, iValue));
637626
}

‎Tests/Noesis.Javascript.Tests/AccessorInterceptorTests.cs

Lines changed: 77 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
using FluentAssertions;
33
using System;
44
using System.Collections.Generic;
5-
using Noesis.Javascript.Tests.Proxy;
65

76
namespace Noesis.Javascript.Tests
87
{
@@ -57,26 +56,90 @@ public void AccessingByIndexAPropertyInAManagedObject()
5756

5857
class ClassWithDictionary
5958
{
60-
public JavaScriptDictionary<string, object> prop { get; set; }
59+
public DictionaryLike prop { get; set; }
6160
}
6261

63-
[TestMethod]
62+
63+
class DictionaryLike
64+
{
65+
public Dictionary<string, object> internalDict { get; set; }
66+
67+
public DictionaryLike(Dictionary<string, object> internalDict = null)
68+
{
69+
this.internalDict = internalDict ?? new Dictionary<string, object>();
70+
}
71+
72+
public object this[string key]
73+
{
74+
get => internalDict[key];
75+
set => internalDict[key] = value;
76+
}
77+
}
78+
79+
[TestMethod]
6480
public void AccessingDictionaryInManagedObject()
6581
{
6682
var dict = new Dictionary<string, object> { { "bar", "33" }, { "baz", true } };
67-
ClassWithDictionary testObj = new ClassWithDictionary() { prop = new JavaScriptDictionary<string, object>(dict) };
83+
ClassWithDictionary testObj = new ClassWithDictionary() { prop = new DictionaryLike(dict) };
6884

6985
_context.SetParameter("test", testObj);
70-
var result = _context.Run(@"test.prop.foo = 42; test.prop.baz = false;");
71-
var testObjResult = (ClassWithDictionary)_context.GetParameter("test");
72-
73-
testObjResult.prop.Count.Should().Be(3);
74-
testObjResult.prop["foo"].Should().Be(42);
75-
testObjResult.prop["bar"].Should().Be("33");
76-
testObjResult.prop["baz"].Should().Be(false);
77-
}
78-
79-
class ClassWithProperty
86+
var result = _context.Run(@"test.prop.foo = 42;
87+
test.prop.baz = false;
88+
var complex = {};
89+
complex.v0 = test.prop.foo
90+
test.prop.complex = complex;");
91+
92+
testObj.prop.internalDict.Count.Should().Be(4);
93+
testObj.prop.internalDict["foo"].Should().Be(42);
94+
testObj.prop.internalDict["bar"].Should().Be("33");
95+
testObj.prop.internalDict["baz"].Should().Be(false);
96+
testObj.prop.internalDict["complex"].Should().BeOfType<Dictionary<string, object>>();
97+
((Dictionary<string,object>)testObj.prop.internalDict["complex"])["v0"].Should().Be(42);
98+
}
99+
100+
[TestMethod]
101+
public void AccessingDictionaryDirectlyInManagedObject()
102+
{
103+
var dict = new Dictionary<string, object>() { { "bar", "33" }, { "baz", true } };
104+
105+
_context.SetParameter("test", dict);
106+
var result = _context.Run(@"test.foo = 42; test.baz = false;");
107+
var testObjResult = (Dictionary<string, object>)_context.GetParameter("test");
108+
109+
testObjResult.Count.Should().Be(3);
110+
testObjResult["foo"].Should().Be(42);
111+
testObjResult["bar"].Should().Be("33");
112+
testObjResult["baz"].Should().Be(false);
113+
}
114+
115+
116+
[TestMethod]
117+
public void AccessingDictionaryOverObjectInManagedObject()
118+
{
119+
DictionaryLike testObj = new DictionaryLike();
120+
testObj.internalDict["foo"] = 42;
121+
122+
_context.SetParameter("test", testObj);
123+
var result = _context.Run(@"test.foo;");
124+
125+
result.Should().Be(42);
126+
testObj.internalDict.Count.Should().Be(1);
127+
testObj.internalDict["foo"].Should().Be(42);
128+
}
129+
130+
[TestMethod]
131+
public void AccessingDictionaryOverObjectInManagedObject2()
132+
{
133+
DictionaryLike testObj = new DictionaryLike();
134+
135+
_context.SetParameter("test", testObj);
136+
var result = _context.Run(@"test.foo = 'bar';");
137+
138+
testObj.internalDict.Count.Should().Be(1);
139+
testObj.internalDict["foo"].Should().Be("bar");
140+
}
141+
142+
class ClassWithProperty
80143
{
81144
public string MyProperty { get; set; }
82145
}

‎Tests/Noesis.Javascript.Tests/Noesis.Javascript.Tests.csproj

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,6 @@
9292
<Compile Include="Properties\AssemblyInfo.cs" />
9393
<Compile Include="MultipleAppDomainsTest.cs" />
9494
<Compile Include="DateTest.cs" />
95-
<Compile Include="Proxy\JavaScriptDictionary.cs" />
9695
<Compile Include="VersionStringTests.cs" />
9796
</ItemGroup>
9897
<ItemGroup>

0 commit comments

Comments
 (0)