[BUG] Deserializing a Base with an Organization property throws a NullReferencException

[tshowers-bt]

ECS integration/library project(s) (e.g. Elastic.CommonSchema.Serilog): Elastic.CommonSchema

ECS schema version (e.g. 1.4.0):

ECS .NET assembly version (e.g. 1.4.2): 1.5.3

.NET framework / OS: Windows

Description of the problem, including expected versus actual behavior:

Deserializing a serialized Elastic.CommonSchema.Base that has a non-null value for Organization throws a NullReferenceException. I would expect to return an instance of Elastic.CommonSchema.Base with the same values as the the instance that was originally serialized.

edit: The same thing happens for non-null values for Agent and Client but not for Labels.

Stacktrace:

   at System.Text.Json.JsonSerializer.LookupProperty(Object obj, ReadOnlySpan`1 unescapedPropertyName, ReadStack& state, JsonSerializerOptions options, Boolean& useExtensionProperty, Boolean createExtensionProperty)
   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonResumableConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options)
   at Elastic.CommonSchema.Serialization.EcsJsonConverterBase`1.ReadProp[TValue](Utf8JsonReader& reader, String key)
   at Elastic.CommonSchema.Serialization.EcsJsonConverterBase`1.ReadProp[TValue](Utf8JsonReader& reader, String key, T b, Action`2 set)
   at Elastic.CommonSchema.Serialization.BaseJsonConverter`1.ReadProperties(Utf8JsonReader& reader, TBase ecsEvent, Nullable`1& timestamp, String& loglevel)
   at Elastic.CommonSchema.Serialization.BaseJsonConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 utf8Json, JsonTypeInfo jsonTypeInfo, Nullable`1 actualByteCount)
   at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 json, JsonTypeInfo jsonTypeInfo)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
   at Elastic.CommonSchema.Serialization.EcsSerializerFactory`1.Deserialize(String json)
   at Elastic.CommonSchema.Base.Deserialize(String json)
   at Program.<Main>$(String[] args) in C:\Users\tshowers\src\EcsDeserBug\EcsDeserBug\Program.cs:line 14

Steps to reproduce:

using Elastic.CommonSchema;

var x = new Base
{
    Timestamp = DateTime.UtcNow,
    Organization = new Organization
    {
        Id = Guid.NewGuid().ToString(),
    }
};

var ser = x.Serialize();

var y = Base.Deserialize(ser);

[Insomniak47]

FYI I bisected the repo and found that it is fixed on main. The commit that fixes it is: 91771540280c473580a2b5161d977620ea7a7e3f

It seems the dotnet 5 update fixed it though it likely needs a fix in the 1.5.x branch

Seems like the difference that matters is:

diff --git a/src/Elastic.CommonSchema/Serialization/EcsJsonConverterBase.cs b/src/Elastic.CommonSchema/Serialization/EcsJsonConverterBase.cs
index 2227f90..0d61972 100644
--- a/src/Elastic.CommonSchema/Serialization/EcsJsonConverterBase.cs
+++ b/src/Elastic.CommonSchema/Serialization/EcsJsonConverterBase.cs
@@ -54,15 +54,13 @@ internal static object ReadPropDeserialize(ref Utf8JsonReader reader, Type type)
 			return JsonSerializer.Deserialize(ref reader, type, options);
 		}
 
-		protected static TValue ReadProp<TValue>(ref Utf8JsonReader reader, string key)  where TValue : class
+		protected static TValue ReadProp<TValue>(ref Utf8JsonReader reader, string _)  where TValue : class
 		{
-			if (reader.TokenType == JsonTokenType.Null) return null;
+			if (reader.TokenType == JsonTokenType.Null)
+				return null;
 
-			var t = typeof(T);
 			var options = JsonConfiguration.SerializerOptions;
 
-			if (typeof(T) != typeof(object) && (options?.GetConverter(typeof(TValue)) is JsonConverter<TValue> keyConverter))
-				return keyConverter.Read(ref reader, t, options);
 			return JsonSerializer.Deserialize<TValue>(ref reader, options);
 		}
 

I might be missing the intent of that line - is it intended as a perf improvement? None of the tests fail locally if it's removed and JsonSerializer.Deserialize<TValue> successfully deserializes it as I'd expect it to.

Edit 2:

runtime deps: Not sure if this matters since removing the lines above makes it succeed though might be interesting.

{
  "runtimeTarget": {
    "name": ".NETCoreApp,Version=v6.0",
    "signature": ""
  },
  "compilationOptions": {},
  "targets": {
    ".NETCoreApp,Version=v6.0": {
      "TestApp/1.0.0": {
        "dependencies": {
          "Elastic.CommonSchema": "1.0.0"
        },
        "runtime": {
          "TestApp.dll": {}
        }
      },
      "System.Text.Json/4.7.0": {},
      "Elastic.CommonSchema/1.0.0": {
        "dependencies": {
          "System.Text.Json": "4.7.0"
        },
        "runtime": {
          "Elastic.CommonSchema.dll": {}
        }
      }
    }
  },
  "libraries": {
    "TestApp/1.0.0": {
      "type": "project",
      "serviceable": false,
      "sha512": ""
    },
    "System.Text.Json/4.7.0": {
      "type": "package",
      "serviceable": true,
      "sha512": "sha512-IPq/x/d5nAcnD3vIyM3AbPOaTgcqrh0AqPSx7U53UFu3M6k1TH1u/eXc9/h4jm/3mpP1WRUpevlPY4PACd7AWw==",
      "path": "system.text.json/4.7.0",
      "hashPath": "system.text.json.4.7.0.nupkg.sha512"
    },
    "Elastic.CommonSchema/1.0.0": {
      "type": "project",
      "serviceable": false,
      "sha512": ""
    }
  }
}

looks like the original commit that added the fix was in the branch within that PR. Specifically the commit was: 44e6a6b0412d6ebe653d6a95f1a2d131802bab01. @Mpdreamz made the fix but there's no note about it in the commit message so I'm assuming** that it's a safe patch to toss in.

[Mpdreamz]

I added an explicit unit test for this but I can not reproduce this on main

https://github.com/elastic/ecs-dotnet/pull/253

Thank you for reporting this @tshowers-bt, a new version should go up on NuGet soon.