Initial serialized member name mapping

[heaths]

This is the prototype implementation for serialized member name mappings we discussed. The other commits will be squashed out if/when merged into master.

[tg-msft]

Looks like a good start. We should definitely get the names/locations of projects right before merging to avoid any unnecessary confusion for customers who pay attention to what's happening in the repo.

[tg-msft]

Let's start a thread, but my understanding is the name for this package should be Microsoft.[Feature].[Dependency] and would expect it to be more like Microsoft.ObjectSerializer.NewtonsoftJson. We might also include the version if we want to avoid breaks. +@KrzysztofCwalina

[tg-msft]

I also think this should probably go in the /sdk/extensions/ folder instead of /sdk/core/

[pakrym]

I disagree, we would need a place to put GeoJSON converter implementation for newtonsoft too. Not sure if it's wise to ship a package for every tiny feature. Or we can say the "feature" here is Azure.Core integration.

And I would keep it next to core for the same reason.

[heaths]

I started a thread, but unless we reach agreement soon will keep this as-is for now.

[tg-msft]

I think the GeoJSON converter belongs somewhere else. We don't want everyone using Newtonsoft to have to pull in a reference to Microsoft.Spatial. I'd prefer Microsoft.ObjectSerializer.NewtonsoftJson and Microsoft.ObjectSerializer.MicrosoftSpatial as separate packages.

[tg-msft]

Ah, sorry, I thought you were talking about support for #13165 as well. We'll need System.Text.Json and Newtonsoft.Json converters for those spatial types.

I think Microsoft.ObjectSerializer.NewtonsoftJson could hold both an implementation of ObjectSerializer and Newtonsoft converters for any Azure.Core types. Maybe ObjectSerializer isn't a broad enough feature name and we should use something more like Microsoft.Serialization.NewtonsoftJson?

[pakrym]

What if we do some JsonPatch support later that would require Newtonsoft adapters.

I would like the understand benefits of having feature-scoped packages instead of something more generic and keeping a leeway for the future.

[heaths]

I agree with @pakrym. I don't understand what benefit not having a 1:1 relationship with Azure.Core with an assembly for Newtonsoft.Json. Think of it this way: that 1 assembly is the Newtonsoft.Json implementations for Azure.Core. If it weren't already taboo, I'd actually think something like Azure.Core.Newtonsoft(.)Json would be appropriate, just for comparison.

[tg-msft]

I would consider ObjectSerializer a feature and Azure.Core a library. We have approval for Microsoft.[Feature].[Dependency]. Microsoft.Azure.Core.[Dependency] feels confusing given all the Microsoft.Azure.* Track 1 libraries.
We should discuss it with the Krzysztof and the Arch Board.

[heaths]

@AlexGhiondea also agrees with the current plane. @KrzysztofCwalina, what do you think?

[tg-msft]

(+@ahsonkhan if he has time for a sanity check)

[pakrym]

Pending comments.

[ahsonkhan]

Follow up to the above comment #13527 (comment)

The following code snippet won't work when the stream starts with a UTF-8 BOM.

azure-sdk-for-net/sdk/core/Azure.Core.Experimental/src/Serialization/JsonObjectSerializer.cs

Lines 52 to 54 in 6124178

	using var memoryStream = new MemoryStream();
	stream.CopyTo(memoryStream);
	return JsonSerializer.Deserialize(memoryStream.ToArray(), returnType, _options);

The synchronous, JsonSerializer.Deserialize method that accepts a ReadOnlySpan<byte> expects only valid JSON data (without any trivia or artifacts). The BOM is considered invalid JSON and will fail.

The async, stream accepting API explicitly skips the BOM for you (to support things like file stream). For this workaround, you would have to explicitly skip the BOM yourself.

https://github.com/dotnet/runtime/blob/e87fbd1030b472b4bc44c375434da65611aa4df0/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs#L159-L164

var utf8 = new byte[5] {0xEF, 0xBB, 0xBF, (byte)'{', (byte)'}'};
object obj = JsonSerializer.Deserialize<object>(utf8);

/*
Unhandled exception. System.Text.Json.JsonException: '0xEF' is an invalid start of a value. Path: $ | LineNumber: 0 | BytePositionInLine: 0.
 ---> System.Text.Json.JsonReaderException: '0xEF' is an invalid start of a value. LineNumber: 0 | BytePositionInLine: 0.
   at System.Text.Json.ThrowHelper.ThrowJsonReaderException(Utf8JsonReader& json, ExceptionResource resource, Byte nextByte, ReadOnlySpan`1 bytes)
*/

This is why I asked if accepting input streams that could contain a BOM is important to support here.

There is an existing request to add the sync stream-accepting APIs to S.T.J, but until then, you'd need to address it yourself as a workaround:
dotnet/runtime#1574

As an aside, we can avoid the extra allocation and .ToArray() call in that method, if performance is important, if the stream is seek-able and we have access to ArrayBufferWriter<T>, i.e. running on core (which should be true in most cases):
https://docs.microsoft.com/en-us/dotnet/api/system.buffers.arraybufferwriter-1?view=netcore-3.1

public static object Deserialize(Stream stream, Type returnType)
{
  if (stream.CanSeek)
  {
    int dataLength = (int)stream.Length;
    var abw = new ArrayBufferWriter<byte>(dataLength);
    Span<byte> span = abw.GetSpan(dataLength);
    
    while(true)
    {
      int bytesWritten = stream.Read(span);
      if (bytesWritten == 0) break;
      abw.Advance(bytesWritten);
      span = span.Slice(bytesWritten);
    }

    ReadOnlySpan<byte> written = abw.WrittenSpan;
    int start = 0;
    // TODO: if first byte is start of utf-8 bom, check the next 3 bytes, and then skip them.
    written = written.Slice(start);

    return JsonSerializer.Deserialize(written, returnType);
  }

  using var memoryStream = new MemoryStream();
  stream.CopyTo(memoryStream);
   // TODO: if first byte is start of utf-8 bom, check the next 3 bytes, and then skip them.
  return JsonSerializer.Deserialize(memoryStream.ToArray(), returnType);
}

[heaths]

@ahsonkhan I opened #13698 to track since it's out of scope for this PR.

[heaths]

ObjectSerializer and friends are being moved to Azure.Core in PR #13619 and this interface isn't yet board-approved. Will need to retarget/rebase once approved.

[pakrym]

This might need to be updated to include the new package: https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/ci.yml#L38

[heaths]

This might need to be updated to include the new package: https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/ci.yml#L38

Are we planning on shipping it just yet? I didn't expect so. If not, I'll open an issue to track releasing it.