Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IDictionary doesn't work with Fsharp dictionary implementation #1322

Closed
1 of 4 tasks
Lanayx opened this issue Mar 9, 2020 · 3 comments · Fixed by #1655
Closed
1 of 4 tasks

IDictionary doesn't work with Fsharp dictionary implementation #1322

Lanayx opened this issue Mar 9, 2020 · 3 comments · Fixed by #1655
Assignees
Labels
bug This issue is a bug. effort/medium Medium work item – a couple days of effort in-progress Issue is being actively worked on. language/dotnet Related to .NET bindings (C#, F#, ...) p2

Comments

@Lanayx
Copy link

Lanayx commented Mar 9, 2020

🐛 Bug Report

Affected Languages

  • TypeScript or Javascript
  • Python
  • Java
  • .NET (F#)

General Information

  • JSII Version: 0.22.0
  • Platform: Windows 10

What is the problem?

Properties whose type is IDictionary (for example Environment property in different templates) can't take F# dictionary like
Environment = dict [("alertEmails", alertEmails)]
to make it work I have to convert it into Dictionary like
Environment = Dictionary(dict [("alertEmails", alertEmails)])
which is undesirable.

The problem is that on this line valueType.GetProperty("Keys") returns null, while if I check ((IDictionary<string, string>) value).Keys it works.

Verbose Log

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at Amazon.JSII.Runtime.Services.Converters.FrameworkToJsiiConverter.TryConvertMap(IReferenceMap referenceMap, TypeReference elementType, Object value, Object& result)
   at Amazon.JSII.Runtime.Services.Converters.ValueConverter.TryConvertCollection(IReferenceMap referenceMap, Object value, Boolean isOptional, CollectionTypeReference collectionType, Object& result)
   at Amazon.JSII.Runtime.Services.Converters.ValueConverter.TryConvert(IOptionalValue optionalValue, Type type, IReferenceMap referenceMap, Object value, Object& result)
   at Amazon.JSII.Runtime.Services.Converters.FrameworkToJsiiConverter.TryConvertClass(Type type, IReferenceMap referenceMap, Object value, Object& result)
   at Amazon.JSII.Runtime.Services.Converters.ValueConverter.TryConvertCustomType(Type type, IReferenceMap referenceMap, Object value, Boolean isOptional, String fullyQualifiedName, Object& result)
   at Amazon.JSII.Runtime.Services.Converters.ValueConverter.TryConvert(IOptionalValue optionalValue, Type type, IReferenceMap referenceMap, Object value, Object& result)
   at Amazon.JSII.Runtime.Services.Converters.FrameworkToJsiiConverter.TryConvert(IOptionalValue optionalValue, IReferenceMap referenceMap, Object value, Object& result)
   at Amazon.JSII.Runtime.Deputy.DeputyBase.<>c__DisplayClass20_0.<ConvertArguments>b__0(Parameter parameter, Object frameworkArgument)
   at System.Linq.Enumerable.ZipIterator[TFirst,TSecond,TResult](IEnumerable`1 first, IEnumerable`1 second, Func`3 resultSelector)+MoveNext()
@Lanayx Lanayx added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 9, 2020
@Lanayx
Copy link
Author

Lanayx commented Mar 9, 2020

This is an example of the fix for the similar issue in another library
https://github.com/Cysharp/MagicOnion/pull/207/files

@SomayaB SomayaB added the language/dotnet Related to .NET bindings (C#, F#, ...) label Mar 10, 2020
@MrArnoldPalmer MrArnoldPalmer added p2 effort/medium Medium work item – a couple days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Mar 10, 2020
@MrArnoldPalmer
Copy link
Contributor

@Lanayx thanks for pointing out a possible fix. This seems relatively simple to implement.

@Lanayx
Copy link
Author

Lanayx commented Mar 11, 2020

@MrArnoldPalmer even easier fix will be to cast value to IDictionary instead of using reflection since you are trying to access properties of this interface anyway

RomainMuller added a commit that referenced this issue May 12, 2020
Simplified handling of the code that transforms an `IDictionary`
instance into the relevant `JObject` by performing the necessary cast
(which has been type checked previously) instead of using reflection.

Fixes #1322
RomainMuller added a commit that referenced this issue May 12, 2020
Perform reflection calls against the resolved `IDictionary` interface
instead of on the raw value type, guaranteeing the expected accessors
are returned.

Fixes #1322
@SomayaB SomayaB added the in-progress Issue is being actively worked on. label May 12, 2020
@mergify mergify bot closed this as completed in #1655 May 14, 2020
mergify bot pushed a commit that referenced this issue May 14, 2020
Perform reflection calls against the resolved `IDictionary` interface
instead of on the raw value type, guaranteeing the expected accessors
are returned.

Fixes #1322
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. effort/medium Medium work item – a couple days of effort in-progress Issue is being actively worked on. language/dotnet Related to .NET bindings (C#, F#, ...) p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants