Merge pull request #1593 from json-api-dotnet/fix-empty-id

Fixed: Produce error on invalid ID in request body

Author: bkoelman

src/Examples/DapperExample/Repositories/ResultSetMapper.cs

@@ -18,9 +18,6 @@ internal sealed class ResultSetMapper<TResource, TId>
     // Note we don't do full bidirectional relationship fix-up; this just avoids duplicate instances.
     private readonly Dictionary<Type, Dictionary<object, object>> _resourceByTypeCache = [];
 
-    // Optimization to avoid unneeded calls to expensive Activator.CreateInstance() method, which is needed multiple times per row.
-    private readonly Dictionary<Type, object?> _defaultValueByTypeCache = [];
-
     // Used to determine where in the tree of included relationships a join object belongs to.
     private readonly Dictionary<IncludeElementExpression, int> _includeElementToJoinObjectArrayIndexLookup = new(ReferenceEqualityComparer.Instance);
 
@@ -114,22 +111,10 @@ public ResultSetMapper(IncludeExpression? include)
 
     private bool HasDefaultValue(object value)
     {
-        object? defaultValue = GetDefaultValueCached(value.GetType());
+        object? defaultValue = RuntimeTypeConverter.GetDefaultValue(value.GetType());
         return Equals(defaultValue, value);
     }
 
-    private object? GetDefaultValueCached(Type type)
-    {
-        if (_defaultValueByTypeCache.TryGetValue(type, out object? defaultValue))
-        {
-            return defaultValue;
-        }
-
-        defaultValue = RuntimeTypeConverter.GetDefaultValue(type);
-        _defaultValueByTypeCache[type] = defaultValue;
-        return defaultValue;
-    }
-
     private void RecursiveSetRelationships(object leftResource, IEnumerable<IncludeElementExpression> includeElements, object?[] joinObjects)
     {
         foreach (IncludeElementExpression includeElement in includeElements)

src/JsonApiDotNetCore.Annotations/Resources/RuntimeTypeConverter.cs

@@ -1,3 +1,4 @@
+using System.Collections.Concurrent;
 using System.Globalization;
 using JetBrains.Annotations;
 
@@ -13,6 +14,8 @@ public static class RuntimeTypeConverter
 {
     private const string ParseQueryStringsUsingCurrentCultureSwitchName = "JsonApiDotNetCore.ParseQueryStringsUsingCurrentCulture";
 
+    private static readonly ConcurrentDictionary<Type, object?> DefaultTypeCache = new();
+
     /// <summary>
     /// Converts the specified value to the specified type.
     /// </summary>
@@ -137,6 +140,8 @@ public static class RuntimeTypeConverter
     /// </summary>
     public static bool CanContainNull(Type type)
     {
+        ArgumentGuard.NotNull(type);
+
         return !type.IsValueType || Nullable.GetUnderlyingType(type) != null;
     }
 
@@ -148,6 +153,8 @@ public static bool CanContainNull(Type type)
     /// </returns>
     public static object? GetDefaultValue(Type type)
     {
-        return type.IsValueType ? Activator.CreateInstance(type) : null;
+        ArgumentGuard.NotNull(type);
+
+        return type.IsValueType ? DefaultTypeCache.GetOrAdd(type, Activator.CreateInstance) : null;
     }
 }

src/JsonApiDotNetCore/Controllers/JsonApiController.cs

@@ -51,15 +51,15 @@ public override async Task<IActionResult> GetAsync(CancellationToken cancellatio
     /// <inheritdoc />
     [HttpGet("{id}")]
     [HttpHead("{id}")]
-    public override async Task<IActionResult> GetAsync([Required] [DisallowNull] TId id, CancellationToken cancellationToken)
+    public override async Task<IActionResult> GetAsync([Required(AllowEmptyStrings = true)] [DisallowNull] TId id, CancellationToken cancellationToken)
     {
         return await base.GetAsync(id, cancellationToken);
     }
 
     /// <inheritdoc />
     [HttpGet("{id}/{relationshipName}")]
     [HttpHead("{id}/{relationshipName}")]
-    public override async Task<IActionResult> GetSecondaryAsync([Required] [DisallowNull] TId id, [Required] string relationshipName,
+    public override async Task<IActionResult> GetSecondaryAsync([Required(AllowEmptyStrings = true)] [DisallowNull] TId id, [Required] string relationshipName,
         CancellationToken cancellationToken)
     {
         return await base.GetSecondaryAsync(id, relationshipName, cancellationToken);
@@ -68,8 +68,8 @@ public override async Task<IActionResult> GetSecondaryAsync([Required] [Disallow
     /// <inheritdoc />
     [HttpGet("{id}/relationships/{relationshipName}")]
     [HttpHead("{id}/relationships/{relationshipName}")]
-    public override async Task<IActionResult> GetRelationshipAsync([Required] [DisallowNull] TId id, [Required] string relationshipName,
-        CancellationToken cancellationToken)
+    public override async Task<IActionResult> GetRelationshipAsync([Required(AllowEmptyStrings = true)] [DisallowNull] TId id,
+        [Required] string relationshipName, CancellationToken cancellationToken)
     {
         return await base.GetRelationshipAsync(id, relationshipName, cancellationToken);
     }
@@ -83,39 +83,41 @@ public override async Task<IActionResult> PostAsync([Required] TResource resourc
 
     /// <inheritdoc />
     [HttpPost("{id}/relationships/{relationshipName}")]
-    public override async Task<IActionResult> PostRelationshipAsync([Required] [DisallowNull] TId id, [Required] string relationshipName,
-        [Required] ISet<IIdentifiable> rightResourceIds, CancellationToken cancellationToken)
+    public override async Task<IActionResult> PostRelationshipAsync([Required(AllowEmptyStrings = true)] [DisallowNull] TId id,
+        [Required] string relationshipName, [Required] ISet<IIdentifiable> rightResourceIds, CancellationToken cancellationToken)
     {
         return await base.PostRelationshipAsync(id, relationshipName, rightResourceIds, cancellationToken);
     }
 
     /// <inheritdoc />
     [HttpPatch("{id}")]
-    public override async Task<IActionResult> PatchAsync([Required] [DisallowNull] TId id, [Required] TResource resource, CancellationToken cancellationToken)
+    public override async Task<IActionResult> PatchAsync([Required(AllowEmptyStrings = true)] [DisallowNull] TId id, [Required] TResource resource,
+        CancellationToken cancellationToken)
     {
         return await base.PatchAsync(id, resource, cancellationToken);
     }
 
     /// <inheritdoc />
     [HttpPatch("{id}/relationships/{relationshipName}")]
+    // `AllowEmptyStrings = true` in `[Required]` prevents the model binder from producing a validation error on whitespace when TId is string.
     // Parameter `[Required] object? rightValue` makes Swashbuckle generate the OpenAPI request body as required. We don't actually validate ModelState, so it doesn't hurt.
-    public override async Task<IActionResult> PatchRelationshipAsync([Required] [DisallowNull] TId id, [Required] string relationshipName,
-        [Required] object? rightValue, CancellationToken cancellationToken)
+    public override async Task<IActionResult> PatchRelationshipAsync([Required(AllowEmptyStrings = true)] [DisallowNull] TId id,
+        [Required] string relationshipName, [Required] object? rightValue, CancellationToken cancellationToken)
     {
         return await base.PatchRelationshipAsync(id, relationshipName, rightValue, cancellationToken);
     }
 
     /// <inheritdoc />
     [HttpDelete("{id}")]
-    public override async Task<IActionResult> DeleteAsync([Required] [DisallowNull] TId id, CancellationToken cancellationToken)
+    public override async Task<IActionResult> DeleteAsync([Required(AllowEmptyStrings = true)] [DisallowNull] TId id, CancellationToken cancellationToken)
     {
         return await base.DeleteAsync(id, cancellationToken);
     }
 
     /// <inheritdoc />
     [HttpDelete("{id}/relationships/{relationshipName}")]
-    public override async Task<IActionResult> DeleteRelationshipAsync([Required] [DisallowNull] TId id, [Required] string relationshipName,
-        [Required] ISet<IIdentifiable> rightResourceIds, CancellationToken cancellationToken)
+    public override async Task<IActionResult> DeleteRelationshipAsync([Required(AllowEmptyStrings = true)] [DisallowNull] TId id,
+        [Required] string relationshipName, [Required] ISet<IIdentifiable> rightResourceIds, CancellationToken cancellationToken)
     {
         return await base.DeleteRelationshipAsync(id, relationshipName, rightResourceIds, cancellationToken);
     }

src/JsonApiDotNetCore/Resources/IdentifiableExtensions.cs

@@ -18,17 +18,12 @@ public static object GetTypedId(this IIdentifiable identifiable)
         }
 
         object? propertyValue = property.GetValue(identifiable);
+        object? defaultValue = RuntimeTypeConverter.GetDefaultValue(property.PropertyType);
 
-        // PERF: We want to throw when 'Id' is unassigned without doing an expensive reflection call, unless this is likely the case.
-        if (identifiable.StringId == null)
+        if (Equals(propertyValue, defaultValue))
         {
-            object? defaultValue = RuntimeTypeConverter.GetDefaultValue(property.PropertyType);
-
-            if (Equals(propertyValue, defaultValue))
-            {
-                throw new InvalidOperationException($"Property '{identifiable.GetClrType().Name}.{IdPropertyName}' should " +
-                    $"have been assigned at this point, but it contains its default {property.PropertyType.Name} value '{propertyValue}'.");
-            }
+            throw new InvalidOperationException($"Property '{identifiable.GetClrType().Name}.{IdPropertyName}' should " +
+                $"have been assigned at this point, but it contains its default {property.PropertyType.Name} value '{propertyValue}'.");
         }
 
         return propertyValue!;

src/JsonApiDotNetCore/Serialization/Request/Adapters/ResourceIdentityAdapter.cs

@@ -116,6 +116,7 @@ private IIdentifiable CreateResource(ResourceIdentity identity, ResourceIdentity
             AssertHasNoId(identity, state);
         }
 
+        AssertNoBrokenId(identity, resourceType.IdentityClrType, state);
         AssertSameIdValue(identity, requirements.IdValue, state);
         AssertSameLidValue(identity, requirements.LidValue, state);
 
@@ -177,6 +178,25 @@ private static void AssertHasNoId(ResourceIdentity identity, RequestAdapterState
         }
     }
 
+    private static void AssertNoBrokenId(ResourceIdentity identity, Type resourceIdClrType, RequestAdapterState state)
+    {
+        if (identity.Id != null)
+        {
+            if (resourceIdClrType == typeof(string))
+            {
+                // Empty and whitespace strings are valid when TId is string.
+                return;
+            }
+
+            string? defaultIdValue = RuntimeTypeConverter.GetDefaultValue(resourceIdClrType)?.ToString();
+
+            if (string.IsNullOrWhiteSpace(identity.Id) || identity.Id == defaultIdValue)
+            {
+                throw new ModelConversionException(state.Position, "The 'id' element is invalid.", null);
+            }
+        }
+    }
+
     private static void AssertSameIdValue(ResourceIdentity identity, string? expected, RequestAdapterState state)
     {
         if (expected != null && identity.Id != expected)

test/JsonApiDotNetCoreTests/IntegrationTests/AtomicOperations/Creating/AssignIdToTextLanguageDefinition.cs

@@ -0,0 +1,20 @@
+using JetBrains.Annotations;
+using JsonApiDotNetCore.Configuration;
+using JsonApiDotNetCore.Middleware;
+
+namespace JsonApiDotNetCoreTests.IntegrationTests.AtomicOperations.Creating;
+
+[UsedImplicitly(ImplicitUseKindFlags.InstantiatedNoFixedConstructorSignature)]
+public sealed class AssignIdToTextLanguageDefinition(IResourceGraph resourceGraph, ResourceDefinitionHitCounter hitCounter, OperationsDbContext dbContext)
+    : ImplicitlyChangingTextLanguageDefinition(resourceGraph, hitCounter, dbContext)
+{
+    public override Task OnWritingAsync(TextLanguage resource, WriteOperationKind writeOperation, CancellationToken cancellationToken)
+    {
+        if (writeOperation == WriteOperationKind.CreateResource && resource.Id == Guid.Empty)
+        {
+            resource.Id = Guid.NewGuid();
+        }
+
+        return Task.CompletedTask;
+    }
+}