Skip to content

Commit fe2aa73

Browse files
bkoelmanbkoelman
authored
Merge pull request #1396 from json-api-dotnet/fix-trace-logging-in-operations
Fix crash on operations requests when trace logging is turned on
2 parents 24b9546 + 4d2ffd8 commit fe2aa73

File tree

23 files changed

+836
-101
lines changed

23 files changed

+836
-101
lines changed

‎.github/workflows/build.yml

+2-1
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ jobs:
5151
- name: Setup PowerShell (Ubuntu)
5252
if: matrix.os == 'ubuntu-latest'
5353
run: |
54-
dotnet tool install --global PowerShell
54+
# Temporary version downgrade because .NET 8 is not installed on runner.
55+
dotnet tool install --global PowerShell --version 7.3.10
5556
- name: Find latest PowerShell version (Windows)
5657
if: matrix.os == 'windows-latest'
5758
shell: pwsh

‎src/Examples/DapperExample/Repositories/ResourceChangeDetector.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ public void AssertIsNotClearingAnyRequiredToOneRelationships(string resourceName
209209
private static void AssertSameType(ResourceType resourceType, IIdentifiable resource)
210210
{
211211
Type declaredType = resourceType.ClrType;
212-
Type instanceType = resource.GetType();
212+
Type instanceType = resource.GetClrType();
213213

214214
if (instanceType != declaredType)
215215
{

‎src/Examples/DapperExample/TranslationToSql/Builders/SelectStatementBuilder.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -436,7 +436,7 @@ private TableAccessorNode CreatePrimaryTableWithIdentityCondition(TableSourceNod
436436
private TableAccessorNode? FindRelatedTable(TableAccessorNode leftTableAccessor, RelationshipAttribute relationship)
437437
{
438438
Dictionary<RelationshipAttribute, TableAccessorNode> rightTableAccessors = _queryState.RelatedTables[leftTableAccessor];
439-
return rightTableAccessors.TryGetValue(relationship, out TableAccessorNode? rightTableAccessor) ? rightTableAccessor : null;
439+
return rightTableAccessors.GetValueOrDefault(relationship);
440440
}
441441

442442
private SelectNode ToSelect(bool isSubQuery, bool createAlias)

‎src/Examples/DapperExample/TranslationToSql/DataModel/BaseDataModelService.cs

+2-2
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ private static bool IsMapped(PropertyInfo property)
139139
return null;
140140
}
141141

142-
PropertyInfo rightKeyProperty = rightResource.GetType().GetProperty(TableSourceNode.IdColumnName)!;
142+
PropertyInfo rightKeyProperty = rightResource.GetClrType().GetProperty(TableSourceNode.IdColumnName)!;
143143
return rightKeyProperty.GetValue(rightResource);
144144
}
145145

@@ -150,7 +150,7 @@ private static bool IsMapped(PropertyInfo property)
150150
private static void AssertSameType(ResourceType resourceType, IIdentifiable resource)
151151
{
152152
Type declaredType = resourceType.ClrType;
153-
Type instanceType = resource.GetType();
153+
Type instanceType = resource.GetClrType();
154154

155155
if (instanceType != declaredType)
156156
{

‎src/JsonApiDotNetCore/Configuration/ResourceGraph.cs

+2-2
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ public ResourceType GetResourceType(string publicName)
5353
{
5454
ArgumentGuard.NotNull(publicName);
5555

56-
return _resourceTypesByPublicName.TryGetValue(publicName, out ResourceType? resourceType) ? resourceType : null;
56+
return _resourceTypesByPublicName.GetValueOrDefault(publicName);
5757
}
5858

5959
/// <inheritdoc />
@@ -75,7 +75,7 @@ public ResourceType GetResourceType(Type resourceClrType)
7575
ArgumentGuard.NotNull(resourceClrType);
7676

7777
Type typeToFind = IsLazyLoadingProxyForResourceType(resourceClrType) ? resourceClrType.BaseType! : resourceClrType;
78-
return _resourceTypesByClrType.TryGetValue(typeToFind, out ResourceType? resourceType) ? resourceType : null;
78+
return _resourceTypesByClrType.GetValueOrDefault(typeToFind);
7979
}
8080

8181
private bool IsLazyLoadingProxyForResourceType(Type resourceClrType)

‎src/JsonApiDotNetCore/Middleware/TraceLogWriter.cs

+109-32
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@
44
using System.Text.Encodings.Web;
55
using System.Text.Json;
66
using System.Text.Json.Serialization;
7+
using JsonApiDotNetCore.Configuration;
8+
using JsonApiDotNetCore.Resources;
9+
using JsonApiDotNetCore.Resources.Annotations;
710
using Microsoft.Extensions.Logging;
811

912
namespace JsonApiDotNetCore.Middleware;
@@ -14,8 +17,105 @@ internal abstract class TraceLogWriter
1417
{
1518
WriteIndented = true,
1619
Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
17-
ReferenceHandler = ReferenceHandler.Preserve
20+
ReferenceHandler = ReferenceHandler.IgnoreCycles,
21+
DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
22+
Converters =
23+
{
24+
new JsonStringEnumConverter(),
25+
new ResourceTypeInTraceJsonConverter(),
26+
new ResourceFieldInTraceJsonConverterFactory(),
27+
new AbstractResourceWrapperInTraceJsonConverterFactory(),
28+
new IdentifiableInTraceJsonConverter()
29+
}
1830
};
31+
32+
private sealed class ResourceTypeInTraceJsonConverter : JsonConverter<ResourceType>
33+
{
34+
public override ResourceType Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
35+
{
36+
throw new NotSupportedException();
37+
}
38+
39+
public override void Write(Utf8JsonWriter writer, ResourceType value, JsonSerializerOptions options)
40+
{
41+
writer.WriteStringValue(value.PublicName);
42+
}
43+
}
44+
45+
private sealed class ResourceFieldInTraceJsonConverterFactory : JsonConverterFactory
46+
{
47+
public override bool CanConvert(Type typeToConvert)
48+
{
49+
return typeToConvert.IsAssignableTo(typeof(ResourceFieldAttribute));
50+
}
51+
52+
public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
53+
{
54+
Type converterType = typeof(ResourceFieldInTraceJsonConverter<>).MakeGenericType(typeToConvert);
55+
return (JsonConverter)Activator.CreateInstance(converterType)!;
56+
}
57+
58+
private sealed class ResourceFieldInTraceJsonConverter<TField> : JsonConverter<TField>
59+
where TField : ResourceFieldAttribute
60+
{
61+
public override TField Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
62+
{
63+
throw new NotSupportedException();
64+
}
65+
66+
public override void Write(Utf8JsonWriter writer, TField value, JsonSerializerOptions options)
67+
{
68+
writer.WriteStringValue(value.PublicName);
69+
}
70+
}
71+
}
72+
73+
private sealed class IdentifiableInTraceJsonConverter : JsonConverter<IIdentifiable>
74+
{
75+
public override IIdentifiable Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
76+
{
77+
throw new NotSupportedException();
78+
}
79+
80+
public override void Write(Utf8JsonWriter writer, IIdentifiable value, JsonSerializerOptions options)
81+
{
82+
// Intentionally *not* calling GetClrType() because we need delegation to the wrapper converter.
83+
Type runtimeType = value.GetType();
84+
85+
JsonSerializer.Serialize(writer, value, runtimeType, options);
86+
}
87+
}
88+
89+
private sealed class AbstractResourceWrapperInTraceJsonConverterFactory : JsonConverterFactory
90+
{
91+
public override bool CanConvert(Type typeToConvert)
92+
{
93+
return typeToConvert.IsAssignableTo(typeof(IAbstractResourceWrapper));
94+
}
95+
96+
public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
97+
{
98+
Type converterType = typeof(AbstractResourceWrapperInTraceJsonConverter<>).MakeGenericType(typeToConvert);
99+
return (JsonConverter)Activator.CreateInstance(converterType)!;
100+
}
101+
102+
private sealed class AbstractResourceWrapperInTraceJsonConverter<TWrapper> : JsonConverter<TWrapper>
103+
where TWrapper : IAbstractResourceWrapper
104+
{
105+
public override TWrapper Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
106+
{
107+
throw new NotSupportedException();
108+
}
109+
110+
public override void Write(Utf8JsonWriter writer, TWrapper value, JsonSerializerOptions options)
111+
{
112+
writer.WriteStartObject();
113+
writer.WriteString("ClrType", value.AbstractType.FullName);
114+
writer.WriteString("StringId", value.StringId);
115+
writer.WriteEndObject();
116+
}
117+
}
118+
}
19119
}
20120

21121
internal sealed class TraceLogWriter<T> : TraceLogWriter
@@ -88,26 +188,12 @@ private static void WriteProperty(StringBuilder builder, PropertyInfo property,
88188
builder.Append(": ");
89189

90190
object? value = property.GetValue(instance);
91-
92-
if (value == null)
93-
{
94-
builder.Append("null");
95-
}
96-
else if (value is string stringValue)
97-
{
98-
builder.Append('"');
99-
builder.Append(stringValue);
100-
builder.Append('"');
101-
}
102-
else
103-
{
104-
WriteObject(builder, value);
105-
}
191+
WriteObject(builder, value);
106192
}
107193

108-
private static void WriteObject(StringBuilder builder, object value)
194+
private static void WriteObject(StringBuilder builder, object? value)
109195
{
110-
if (HasToStringOverload(value.GetType()))
196+
if (value != null && value is not string && HasToStringOverload(value.GetType()))
111197
{
112198
builder.Append(value);
113199
}
@@ -118,28 +204,19 @@ private static void WriteObject(StringBuilder builder, object value)
118204
}
119205
}
120206

121-
private static bool HasToStringOverload(Type? type)
207+
private static bool HasToStringOverload(Type type)
122208
{
123-
if (type != null)
124-
{
125-
MethodInfo? toStringMethod = type.GetMethod("ToString", Array.Empty<Type>());
126-
127-
if (toStringMethod != null && toStringMethod.DeclaringType != typeof(object))
128-
{
129-
return true;
130-
}
131-
}
132-
133-
return false;
209+
MethodInfo? toStringMethod = type.GetMethod("ToString", Array.Empty<Type>());
210+
return toStringMethod != null && toStringMethod.DeclaringType != typeof(object);
134211
}
135212

136-
private static string SerializeObject(object value)
213+
private static string SerializeObject(object? value)
137214
{
138215
try
139216
{
140217
return JsonSerializer.Serialize(value, SerializerOptions);
141218
}
142-
catch (JsonException)
219+
catch (Exception exception) when (exception is JsonException or NotSupportedException)
143220
{
144221
// Never crash as a result of logging, this is best-effort only.
145222
return "object";

‎src/JsonApiDotNetCore/Queries/QueryLayer.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ internal void WriteLayer(IndentingStringWriter writer, string? prefix)
4242

4343
using (writer.Indent())
4444
{
45-
if (Include != null)
45+
if (Include != null && Include.Elements.Any())
4646
{
4747
writer.WriteLine($"{nameof(Include)}: {Include}");
4848
}

‎src/JsonApiDotNetCore/Serialization/JsonConverters/SingleOrManyDataConverterFactory.cs

+1-2
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
using System.Reflection;
21
using System.Text.Json;
32
using System.Text.Json.Serialization;
43
using JetBrains.Annotations;
@@ -24,7 +23,7 @@ public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializer
2423
Type objectType = typeToConvert.GetGenericArguments()[0];
2524
Type converterType = typeof(SingleOrManyDataConverter<>).MakeGenericType(objectType);
2625

27-
return (JsonConverter)Activator.CreateInstance(converterType, BindingFlags.Instance | BindingFlags.Public, null, null, null)!;
26+
return (JsonConverter)Activator.CreateInstance(converterType)!;
2827
}
2928

3029
private sealed class SingleOrManyDataConverter<T> : JsonObjectConverter<SingleOrManyData<T>>

‎test/JsonApiDotNetCoreTests/IntegrationTests/AtomicOperations/Mixed/AtomicLoggingTests.cs

+6-4
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,10 @@ public async Task Logs_at_error_level_on_unhandled_exception()
8080
error.Source.ShouldNotBeNull();
8181
error.Source.Pointer.Should().Be("/atomic:operations[0]");
8282

83-
loggerFactory.Logger.Messages.ShouldNotBeEmpty();
83+
IReadOnlyList<FakeLogMessage> logMessages = loggerFactory.Logger.GetMessages();
84+
logMessages.ShouldNotBeEmpty();
8485

85-
loggerFactory.Logger.Messages.Should().ContainSingle(message => message.LogLevel == LogLevel.Error &&
86+
logMessages.Should().ContainSingle(message => message.LogLevel == LogLevel.Error &&
8687
message.Text.Contains("Simulated failure.", StringComparison.Ordinal));
8788
}
8889

@@ -117,9 +118,10 @@ public async Task Logs_at_info_level_on_invalid_request_body()
117118

118119
responseDocument.Errors.ShouldHaveCount(1);
119120

120-
loggerFactory.Logger.Messages.ShouldNotBeEmpty();
121+
IReadOnlyList<FakeLogMessage> logMessages = loggerFactory.Logger.GetMessages();
122+
logMessages.ShouldNotBeEmpty();
121123

122-
loggerFactory.Logger.Messages.Should().ContainSingle(message => message.LogLevel == LogLevel.Information &&
124+
logMessages.Should().ContainSingle(message => message.LogLevel == LogLevel.Information &&
123125
message.Text.Contains("Failed to deserialize request body", StringComparison.Ordinal));
124126
}
125127

0 commit comments

Comments
 (0)