ToCloudEventInternalAsync fails when using ContentMode.Binary in SystemTextJson and NewtonsoftJson

[squanbeck]

Following this sample of code from the guide,

sdk-csharp/docs/guide.md

Lines 293 to 303 in 3ce4aa0

	public class GameResult
	{
	[JsonProperty("playerId")]
	public string PlayerId { get; set; }
	[JsonProperty("gameId")]
	public string GameId { get; set; }
	[JsonProperty("score")]
	public int Score { get; set; }
	}

sdk-csharp/docs/guide.md

Lines 314 to 334 in 3ce4aa0

	var result = new GameResult
	{
	PlayerId = "player1",
	GameId = "game1",
	Score = 200
	};
	var cloudEvent = new CloudEvent
	{
	Id = "result-1",
	Type = "game.played.v1",
	Source = new Uri("/game", UriKind.Relative),
	Time = DateTimeOffset.UtcNow,
	DataContentType = "application/json",
	Data = result
	};
	var formatter = new JsonEventFormatter();
	var request = new HttpRequestMessage
	{
	Method = HttpMethod.Post,
	Content = cloudEvent.ToHttpContent(ContentMode.Binary, formatter)
	};

sdk-csharp/docs/guide.md

Lines 366 to 368 in 3ce4aa0

	CloudEventFormatter formatter = new JsonEventFormatter<GameResult>();
	CloudEvent cloudEvent = await request.ToCloudEventAsync(formatter);
	GameResult result = (GameResult) cloudEvent.Data;

I end up with this error (line 151):

sdk-csharp/src/CloudNative.CloudEvents/Http/HttpClientExtensions.cs

Lines 139 to 152 in 3ce4aa0

	if (HasCloudEventsContentType(content))
	{
	var stream = await content.ReadAsStreamAsync().ConfigureAwait(false);
	return await formatter.DecodeStructuredModeMessageAsync(stream, MimeUtilities.ToContentType(content.Headers.ContentType), extensionAttributes).ConfigureAwait(false);
	}
	else
	{
	string? versionId = headers.Contains(HttpUtilities.SpecVersionHttpHeader)
	? headers.GetValues(HttpUtilities.SpecVersionHttpHeader).First()
	: null;
	if (versionId is null)
	{
	throw new ArgumentException($"Request does not represent a CloudEvent. It has neither a {HttpUtilities.SpecVersionHttpHeader} header, nor a suitable content type.", nameof(paramName));
	}

When I looked further into it, it seems that the variable, "headers", is empty when using ContentMode.Binary. This is the case when using both SystemTextJson and NewtonsoftJson (likely when using the other packages as well, but these are the only ones I tested). However, I did notice that "content.headers" does contain the ce-specversion. Now, I am unsure if I am reading the CloudEvents specification correctly, but I think that this is in conformance, based off of my understanding that in binary mode, the event data need not be in the HTTP headers themselves, but rather in the message (in this case, the "content") metadata.

https://github.com/cloudevents/spec/blob/604fef3eea26801d1249bd7ee796e947d38cb5a1/cloudevents/spec.md?plain=1#L125-L134

If my understanding is accurate, does an extra check for "content.headers" containing ce-specversion need to exist here, somewhere around line 147?

[jskeet]

I'll have a look at this in detail next week - although I'm travelling, which may delay things.

[squanbeck]

Sounds good, thank you!

[jskeet]

(Looking now briefly as I have some time.)

What's missing from this issue is how you're receiving the request, and ideally a repro of the problem. I've never been 100% clear how .NET decides which headers in an inbound request should be counted as content headers vs which are just request headers. Are you encountering the problem when receiving an actual HTTP request on a server, or is this for an HttpMessage you've constructed in code?

It's entirely possible that we should indeed look at all headers together - but I'll need to look in more detail, and if you can provide more context of where you're seeing this, it would help.

[jskeet]

Note to self: RFC-2616 never talks about content headers, but it does include entity headers, which appear to include "everything not explicitly declared as a general header or request header".

[squanbeck]

As you have guessed, this is just the contrived example from the guide; it does not involve any actual HTTP request on a server. To be perfectly honest, I don't know that I understand the HTTP protocol well enough to know how the request headers and content/entity headers differ, particularly in their relation to .NET's implementation.

[jskeet]

Ah, I see what you mean - I hadn't seen why you'd included all the guide stuff before. Right, that makes sense. I'll investigate further next week and see whether we need a code change, a guide change, or both...

[squanbeck]

I have been working on a more complete sample to verify if this is still an issue when sending through a server, but haven't completed it yet. I will let you know the results when I get that finished up. It may be a few days, though.

[jskeet]

Thanks - it's going to be a few days before I can put adequate time into it too, to be honest :)

[squanbeck]

In trying to set up a server example, I can't seem to have access to the HttpRequestMessage in my controller. Is there a normal way in which to receive that so that I can call .ToCloudEventAsync() on it? All of my searches haven't seemed to turn up anything on this.

[jskeet]

Will look into that when I look at the whole thing, hopefully early next week :)

[squanbeck]

Thank you, I appreciate all of the help!

[squanbeck]

So I came back to this for a little bit of a further look today. Seems I only ran into this issue as I misunderstood how the HttpRequestMessage and HttpRequest were being used. When using System.Web.Http (I think this only works with .NET Framework), and using a controller inheriting from ApiController, the HttpRequestMessage is available from the inherited member "Request". When using the newer(?) Microsoft.AspNetCore.Mvc and controllers inheriting from ControllerBase, the HttpRequest is available from the inherited member "Request". This indicated that I needed to use the CloudNative.CloudEvents.AspNetCore library to access the HttpRequest.ToCloudEventAsync() method. I got confused within the guide, as I had assumed that the "request" in line 367 was an HttpRequestMessage instead of an HttpRequest and I was using .NET Core to test this all out. Hope that helps!

[jskeet]

@squanbeck: Regardless of your confusion, I believe you did discover a real but, which hopefully #222 fixes correctly. But it sounds like I should probably update the guide too.

[squanbeck]

Glad to have been of help!