Skip to content

Add JSON Serialization Support for OLE#11527

Merged
lonitra merged 36 commits intodotnet:mainfrom
lonitra:jsonclipboard
Dec 16, 2024
Merged

Add JSON Serialization Support for OLE#11527
lonitra merged 36 commits intodotnet:mainfrom
lonitra:jsonclipboard

Conversation

@lonitra
Copy link
Member

@lonitra lonitra commented Jun 13, 2024

Related: #11368

This is a start to support JSON serialization in OLE scenarios.
JsonData: This is our wrapper class that holds the JSON serialized data along with the original type information. Note that the data that is being JSON serialized uses default JsonSerializer behavior. Users can follow the remarks of the public APIs if custom JSON serialization behavior is needed.

Public APIs:

  • DataObject.SetDataAsJson() - Allows users to add data to DataObject that will be JSON serialized. This is how we support JSON serialization for drag/drop scenario.
  • Clipboard.SetDataAsJson() - Allows users to conveniently JSON serialize a type onto the clipboard, but note that this does not accept DataObject being passed in. If users want to place a DataObject on the clipboard, users should ensure data they want to be JSON serialized is already in the DataObject by utilizing DataObject.SetDataAsJson and then use Clipboard.SetDataObject().
  • Control.DoDragDropAsJson() - Allows users to conveniently JSON serialize a type for drag operation, but note that this does not accept DataObject being passed in. If users wants to start a drag operation with a DataObject, user should ensure data they want to be JSON serialized is already in the DataObject by utilizing DataObject.SetDataAsJson and then use Control.DoDragDrop()

Tests:

  • Unit tests added for new APIs, some of the same tests also added in ComDisabledTests to ensure same behavior if built in com support is turned off.
  • Manually tested cross process scenario -- drag/drop and copy/paste from 2 WinForms apps and confirmed able to retrieve the original data without exposing JsonData<T>. If using legacy GetData APIs, user should change to use TryGetData APIs, otherwise follow recommendations to manually deserialize the data.
  • Manually tested copying data onto clipboard from app running on main branch and pasting into .NET 8 app for informational purposes. This scenario does not work as one may expect likely due to JsonData not existing in .NET 8. In order to properly deserialize, users will need to follow recommendations to manually deserialize the stream.
  • Test behavior in trimmed app: JsonSerializer uses reflection to serialize by default, which will not be available when trimming is turned on. In the future, we will need to either use source generator APIs to support trimming or configure JsonSerializerOptions.TypeInfoResolver
    to support trim scenarios.
Microsoft Reviewers: Open in CodeFlow

@codecov
Copy link

codecov bot commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 90.61224% with 69 lines in your changes missing coverage. Please review.

Project coverage is 76.03659%. Comparing base (a5281d9) to head (3d32e84).
Report is 2 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                  @@
##                main      #11527          +/-   ##
====================================================
- Coverage   97.03492%   76.03659%   -20.99833%     
====================================================
  Files           1182        3178        +1996     
  Lines         354561      639600      +285039     
  Branches        5411       47215       +41804     
====================================================
+ Hits          344048      486330      +142282     
- Misses          9724      149750      +140026     
- Partials         789        3520        +2731     
Flag Coverage Δ
Debug 76.03659% <90.61224%> (-20.99833%) ⬇️
integration 18.17640% <38.66667%> (?)
production 49.83146% <80.66667%> (?)
test 97.03102% <93.16239%> (-0.00391%) ⬇️
unit 47.05355% <77.33333%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Generally looks good, some minor comments.

@elachlan
Copy link
Contributor

@willibrandon you might be interested in the clipboard work here.

@lonitra lonitra changed the base branch from feature/clipboard to feature/10.0 July 30, 2024 20:13
@lonitra lonitra changed the base branch from feature/10.0 to main August 15, 2024 00:59
Tanya-Solyanik added a commit to Tanya-Solyanik/winforms that referenced this pull request Sep 4, 2024
…ot forcing preview language version which is not supported by VB

Use fluent assertions in ClipboardProxyTests  - copied from Loni's PR dotnet#11527 for future merging.
@Tanya-Solyanik Tanya-Solyanik mentioned this pull request Sep 4, 2024
Tanya-Solyanik added a commit that referenced this pull request Sep 5, 2024
Enable simple creation of VB test projects in this repo solution by not forcing preview language version which is not  supported by VB
Use fluent assertions in ClipboardProxyTests  - copied from Loni's PR #11527 for future merging.
@willibrandon
Copy link
Contributor

Thanks for working on these new APIs 🤤 I plan on testing your branch this evening, and will post any feedback that I have here.

@willibrandon
Copy link
Contributor

Looks good! I am able to drag and drop JSON cats now 😸

var data = new DataObject();
var cat = new NyanCat()
{
    Ascii = _nyanCatAscii
};

data.SetDataAsJson(nameof(_nyanCatAscii), cat);
toolStripItem.DoDragDrop(
    data: data,
    allowedEffects: DragDropEffects.All,
    dragImage: _nyanCatAscii301Bmp,
    cursorOffset: new Point(0, 111),
    useDefaultDragImage: false);

outData.Should().BeSameAs(data);
}

public static IEnumerable<object[]> DataObjectWithJsonMockRoundTripData()
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider replacing with TheoryData time permitting

{
object? result = _innerData.GetData(format, autoConvert);
// Avoid exposing our internal JsonData<T>
return result is IJsonData ? null : result;
Copy link
Contributor

Choose a reason for hiding this comment

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

We would need the same change in Clipboard.GetData, or alternatively we could block this in the BinaryFormatUtilities reader code. This is the case of legacyMode == true and we can test if the stream contains BD-ed JsonData struct

Copy link
Member Author

@lonitra lonitra Dec 16, 2024

Choose a reason for hiding this comment

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

Clipboard.GetData will call to DataObject.GetData so I don't think JsonData will still not be exposed in that scenario, but I'll also write a test for this

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, but still user applications that have no access to JsonData type will throw a first chance SerializationException from BInaryFormatter. If this scenario is blocked before the binary formatter, then there is no exception.

Copy link
Member Author

@lonitra lonitra Dec 16, 2024

Choose a reason for hiding this comment

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

Wouldn't the exception be a good indicator for user the data has been JSON serialized and binary formatted - if they want to deserialize this with BinaryFormatter to retrieve the data, they need to follow recommended steps? Otherwise, user will just get null and that gives very little information to user what went wrong. We also cannot avoid this exception from happening if user is trying to retrieve data in older .NET versions. We check and return null here because this is for in proc scenarios where users doing SetDataAsJson should use TryGetData instead of GetData.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with argument about exception stirring the user in the right direction. But are you sure that result is IJsonData can happen outside an internalsvisible to assembly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I had tested this in our scratch project which does not have InternalsVisibleTo and this is the following results:

        TestData testData = new() { x = 1 };
        DataObject dataObject = new DataObject();
        dataObject.SetDataAsJson("test", testData);

        // This is null.
        var test1 = dataObject.GetData("test");


        Clipboard.SetDataObject(dataObject, copy: false);
        // This is null.
        var test2 = Clipboard.GetData("test");

        Clipboard.SetDataObject(dataObject, copy: true);
        // Empty memory stream. If first chance exception on, throws SerializationException when path to use BinaryFormatter enabled
        var test3 = Clipboard.GetData("test");

Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik Dec 16, 2024

Choose a reason for hiding this comment

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

But are you hitting a breakpoint on line 243 with result not null?

@lonitra lonitra marked this pull request as ready for review December 16, 2024 19:15
@lonitra lonitra requested a review from a team as a code owner December 16, 2024 19:15
@lonitra lonitra removed the draft draft PR label Dec 16, 2024
@lonitra lonitra merged commit b3ff4e9 into dotnet:main Dec 16, 2024
@lonitra lonitra deleted the jsonclipboard branch December 16, 2024 23:56
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0 Preview1 milestone Dec 16, 2024
@elachlan
Copy link
Contributor

Congratulations on getting this merged!

@github-actions github-actions bot locked and limited conversation to collaborators Jan 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants