Skip to content

Fix crash on operations requests when trace logging is turned on #1396

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ jobs:
- name: Setup PowerShell (Ubuntu)
if: matrix.os == 'ubuntu-latest'
run: |
dotnet tool install --global PowerShell
# Temporary version downgrade because .NET 8 is not installed on runner.
dotnet tool install --global PowerShell --version 7.3.10
- name: Find latest PowerShell version (Windows)
if: matrix.os == 'windows-latest'
shell: pwsh
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ public void AssertIsNotClearingAnyRequiredToOneRelationships(string resourceName
private static void AssertSameType(ResourceType resourceType, IIdentifiable resource)
{
Type declaredType = resourceType.ClrType;
Type instanceType = resource.GetType();
Type instanceType = resource.GetClrType();

if (instanceType != declaredType)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ private TableAccessorNode CreatePrimaryTableWithIdentityCondition(TableSourceNod
private TableAccessorNode? FindRelatedTable(TableAccessorNode leftTableAccessor, RelationshipAttribute relationship)
{
Dictionary<RelationshipAttribute, TableAccessorNode> rightTableAccessors = _queryState.RelatedTables[leftTableAccessor];
return rightTableAccessors.TryGetValue(relationship, out TableAccessorNode? rightTableAccessor) ? rightTableAccessor : null;
return rightTableAccessors.GetValueOrDefault(relationship);
}

private SelectNode ToSelect(bool isSubQuery, bool createAlias)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ private static bool IsMapped(PropertyInfo property)
return null;
}

PropertyInfo rightKeyProperty = rightResource.GetType().GetProperty(TableSourceNode.IdColumnName)!;
PropertyInfo rightKeyProperty = rightResource.GetClrType().GetProperty(TableSourceNode.IdColumnName)!;
return rightKeyProperty.GetValue(rightResource);
}

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

if (instanceType != declaredType)
{
Expand Down
4 changes: 2 additions & 2 deletions src/JsonApiDotNetCore/Configuration/ResourceGraph.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public ResourceType GetResourceType(string publicName)
{
ArgumentGuard.NotNull(publicName);

return _resourceTypesByPublicName.TryGetValue(publicName, out ResourceType? resourceType) ? resourceType : null;
return _resourceTypesByPublicName.GetValueOrDefault(publicName);
}

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

Type typeToFind = IsLazyLoadingProxyForResourceType(resourceClrType) ? resourceClrType.BaseType! : resourceClrType;
return _resourceTypesByClrType.TryGetValue(typeToFind, out ResourceType? resourceType) ? resourceType : null;
return _resourceTypesByClrType.GetValueOrDefault(typeToFind);
}

private bool IsLazyLoadingProxyForResourceType(Type resourceClrType)
Expand Down
141 changes: 109 additions & 32 deletions src/JsonApiDotNetCore/Middleware/TraceLogWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
using System.Text.Encodings.Web;
using System.Text.Json;
using System.Text.Json.Serialization;
using JsonApiDotNetCore.Configuration;
using JsonApiDotNetCore.Resources;
using JsonApiDotNetCore.Resources.Annotations;
using Microsoft.Extensions.Logging;

namespace JsonApiDotNetCore.Middleware;
Expand All @@ -14,8 +17,105 @@ internal abstract class TraceLogWriter
{
WriteIndented = true,
Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
ReferenceHandler = ReferenceHandler.Preserve
ReferenceHandler = ReferenceHandler.IgnoreCycles,
DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
Converters =
{
new JsonStringEnumConverter(),
new ResourceTypeInTraceJsonConverter(),
new ResourceFieldInTraceJsonConverterFactory(),
new AbstractResourceWrapperInTraceJsonConverterFactory(),
new IdentifiableInTraceJsonConverter()
}
};

private sealed class ResourceTypeInTraceJsonConverter : JsonConverter<ResourceType>
{
public override ResourceType Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
throw new NotSupportedException();
}

public override void Write(Utf8JsonWriter writer, ResourceType value, JsonSerializerOptions options)
{
writer.WriteStringValue(value.PublicName);
}
}

private sealed class ResourceFieldInTraceJsonConverterFactory : JsonConverterFactory
{
public override bool CanConvert(Type typeToConvert)
{
return typeToConvert.IsAssignableTo(typeof(ResourceFieldAttribute));
}

public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
{
Type converterType = typeof(ResourceFieldInTraceJsonConverter<>).MakeGenericType(typeToConvert);
return (JsonConverter)Activator.CreateInstance(converterType)!;
}

private sealed class ResourceFieldInTraceJsonConverter<TField> : JsonConverter<TField>
where TField : ResourceFieldAttribute
{
public override TField Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
throw new NotSupportedException();
}

public override void Write(Utf8JsonWriter writer, TField value, JsonSerializerOptions options)
{
writer.WriteStringValue(value.PublicName);
}
}
}

private sealed class IdentifiableInTraceJsonConverter : JsonConverter<IIdentifiable>
{
public override IIdentifiable Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
throw new NotSupportedException();
}

public override void Write(Utf8JsonWriter writer, IIdentifiable value, JsonSerializerOptions options)
{
// Intentionally *not* calling GetClrType() because we need delegation to the wrapper converter.
Type runtimeType = value.GetType();

JsonSerializer.Serialize(writer, value, runtimeType, options);
}
}

private sealed class AbstractResourceWrapperInTraceJsonConverterFactory : JsonConverterFactory
{
public override bool CanConvert(Type typeToConvert)
{
return typeToConvert.IsAssignableTo(typeof(IAbstractResourceWrapper));
}

public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
{
Type converterType = typeof(AbstractResourceWrapperInTraceJsonConverter<>).MakeGenericType(typeToConvert);
return (JsonConverter)Activator.CreateInstance(converterType)!;
}

private sealed class AbstractResourceWrapperInTraceJsonConverter<TWrapper> : JsonConverter<TWrapper>
where TWrapper : IAbstractResourceWrapper
{
public override TWrapper Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
throw new NotSupportedException();
}

public override void Write(Utf8JsonWriter writer, TWrapper value, JsonSerializerOptions options)
{
writer.WriteStartObject();
writer.WriteString("ClrType", value.AbstractType.FullName);
writer.WriteString("StringId", value.StringId);
writer.WriteEndObject();
}
}
}
}

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

object? value = property.GetValue(instance);

if (value == null)
{
builder.Append("null");
}
else if (value is string stringValue)
{
builder.Append('"');
builder.Append(stringValue);
builder.Append('"');
}
else
{
WriteObject(builder, value);
}
WriteObject(builder, value);
}

private static void WriteObject(StringBuilder builder, object value)
private static void WriteObject(StringBuilder builder, object? value)
{
if (HasToStringOverload(value.GetType()))
if (value != null && value is not string && HasToStringOverload(value.GetType()))
{
builder.Append(value);
}
Expand All @@ -118,28 +204,19 @@ private static void WriteObject(StringBuilder builder, object value)
}
}

private static bool HasToStringOverload(Type? type)
private static bool HasToStringOverload(Type type)
{
if (type != null)
{
MethodInfo? toStringMethod = type.GetMethod("ToString", Array.Empty<Type>());

if (toStringMethod != null && toStringMethod.DeclaringType != typeof(object))
{
return true;
}
}

return false;
MethodInfo? toStringMethod = type.GetMethod("ToString", Array.Empty<Type>());
return toStringMethod != null && toStringMethod.DeclaringType != typeof(object);
}

private static string SerializeObject(object value)
private static string SerializeObject(object? value)
{
try
{
return JsonSerializer.Serialize(value, SerializerOptions);
}
catch (JsonException)
catch (Exception exception) when (exception is JsonException or NotSupportedException)
{
// Never crash as a result of logging, this is best-effort only.
return "object";
Expand Down
2 changes: 1 addition & 1 deletion src/JsonApiDotNetCore/Queries/QueryLayer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ internal void WriteLayer(IndentingStringWriter writer, string? prefix)

using (writer.Indent())
{
if (Include != null)
if (Include != null && Include.Elements.Any())
{
writer.WriteLine($"{nameof(Include)}: {Include}");
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
using System.Reflection;
using System.Text.Json;
using System.Text.Json.Serialization;
using JetBrains.Annotations;
Expand All @@ -24,7 +23,7 @@ public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializer
Type objectType = typeToConvert.GetGenericArguments()[0];
Type converterType = typeof(SingleOrManyDataConverter<>).MakeGenericType(objectType);

return (JsonConverter)Activator.CreateInstance(converterType, BindingFlags.Instance | BindingFlags.Public, null, null, null)!;
return (JsonConverter)Activator.CreateInstance(converterType)!;
}

private sealed class SingleOrManyDataConverter<T> : JsonObjectConverter<SingleOrManyData<T>>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,10 @@ public async Task Logs_at_error_level_on_unhandled_exception()
error.Source.ShouldNotBeNull();
error.Source.Pointer.Should().Be("/atomic:operations[0]");

loggerFactory.Logger.Messages.ShouldNotBeEmpty();
IReadOnlyList<FakeLogMessage> logMessages = loggerFactory.Logger.GetMessages();
logMessages.ShouldNotBeEmpty();

loggerFactory.Logger.Messages.Should().ContainSingle(message => message.LogLevel == LogLevel.Error &&
logMessages.Should().ContainSingle(message => message.LogLevel == LogLevel.Error &&
message.Text.Contains("Simulated failure.", StringComparison.Ordinal));
}

Expand Down Expand Up @@ -117,9 +118,10 @@ public async Task Logs_at_info_level_on_invalid_request_body()

responseDocument.Errors.ShouldHaveCount(1);

loggerFactory.Logger.Messages.ShouldNotBeEmpty();
IReadOnlyList<FakeLogMessage> logMessages = loggerFactory.Logger.GetMessages();
logMessages.ShouldNotBeEmpty();

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

Expand Down
Loading