Users/clintwar/handletypeinfo #5960
Conversation
ghost
added
the
|
Hi @clintwar. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along. |
|
Hi @clintwar; Thanks for taking action on your previously stale pull request. Resetting staleness. |
|
The checks seems to keep failing on the Android-CI check. Is there anything that can be done to correct this? @paulcam206 is there something that needs to be done to be sure that all the checks complete? |
|
Hi @clintwar. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along. |
|
Hi @clintwar; Thanks for taking action on your previously stale pull request. Resetting staleness. |
|
Hi @clintwar. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along. |
|
@RebeccaAnne - can you take a quick look and see if this looks good to you too? I think this PR hit our Android CI issue 2 weeks ago. if it can be merged, the Android CI should be unblocked |
|
Hi @clintwar. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along. |
|
Hi @clintwar; Thanks for taking action on your previously stale pull request. Resetting staleness. |
|
Hi @clintwar. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along. |
|
Hi @clintwar; Thanks for taking action on your previously stale pull request. Resetting staleness. |
|
@jonmill and @RebeccaAnne It seems that the checks continue to fail and that is what is holding up merging of this PR. Looks like now it is IOS instead of Android. |
|
@clintwar checks are all passed! |
|
@golddove - Thank you! Can you also give this a final review so we have 2 folks with eyes on it? |
| // now bring it back | ||
| AdaptiveCard card2 = cardObject.ToObject<AdaptiveCard>(); | ||
|
|
||
| // card2 will now have AdditionalProperties because $type is not known and it seems $type is not ignored by Newtonsoft JsonExtensionData |
There was a problem hiding this comment.
@clintwar should this be considered a bug we should look into?
There was a problem hiding this comment.
@golddove not sure I would call it a bug, since AdaptiveTypeElement has AdditionalProperties with JsonExtensionData attribute anything not defined gets thrown into it. $type is included by Newtonsoft when you serialize but I do not see any good means to ignore since there is not a good way to differentiate things included by the serialization vs things introduced in the json directly.
* Fixing issue 2148 * Adding test to validate type handling Co-authored-by: Clint Warriner <clintwa@microsoft.com> Co-authored-by: Paul Campbell <paulcam@microsoft.com> (cherry picked from commit 1ba5635)
* Fixing issue 2148 * Adding test to validate type handling Co-authored-by: Clint Warriner <clintwa@microsoft.com> Co-authored-by: Paul Campbell <paulcam@microsoft.com> (cherry picked from commit 1ba5635)
fixes #2148
Description
If $type information is included in serialized content the reader will be at start of an object. Instead of loading into JArray load into JToken and test to see if it is JObject. If it is set token to $values token. if there isn't one it will handle correctly and leave for instances when it is null.
How Verified
Added new test Test_TypeHandling
Since $type is included and AdditionalProperties is decorated with JsonExtensionData it gets stored there. Test removes this form expected elements and compares to ensure that the structure of the card is the same.
Microsoft Reviewers: Open in CodeFlow