Refactor and write tests for JsonHelper in DataController (#116)

* Refactor and write tests for JsonHelper in DataController

* Ensure that JsonHelper.FindChangedFields does not return BigInteger

Big integer is not supported in json becuase it serialized on the usless form
{"IsPowerOfTwo":true,"IsZero":false,"IsOne":false,"IsEven":true,"Sign":1}

Co-authored-by: Ivar <ivar.nesje@finanstilsynet.no>

src/Altinn.App.Api/Controllers/DataController.cs

@@ -550,8 +550,7 @@ private async Task<ActionResult> PutFormData(string org, string app, Instance in
                 return BadRequest("No data found in content");
             }
 
-            string serviceModelJsonString = JsonSerializer.Serialize(serviceModel);
-            bool changedByCalculation = await _dataProcessor.ProcessDataWrite(instance, dataGuid, serviceModel);
+            Dictionary<string, object> changedFields = await JsonHelper.ProcessDataWriteWithDiff(instance, dataGuid, serviceModel, _dataProcessor, _logger);
 
             await UpdatePresentationTextsOnInstance(instance, dataType, serviceModel);
             await UpdateDataValuesOnInstance(instance, dataType, serviceModel);
@@ -569,21 +568,10 @@ private async Task<ActionResult> PutFormData(string org, string app, Instance in
             SelfLinkHelper.SetDataAppSelfLinks(instanceOwnerPartyId, instanceGuid, updatedDataElement, Request);
 
             string dataUrl = updatedDataElement.SelfLinks.Apps;
-
-            if (changedByCalculation)
+            if (changedFields is not null)
             {
-                string updatedServiceModelString = JsonSerializer.Serialize(serviceModel);
                 CalculationResult calculationResult = new CalculationResult(updatedDataElement);
-                try
-                {
-                    Dictionary<string, object> changedFields = JsonHelper.FindChangedFields(serviceModelJsonString, updatedServiceModelString);
-                    calculationResult.ChangedFields = changedFields;
-                }
-                catch (Exception e)
-                {
-                    _logger.LogError(e, "Unable to determine changed fields");
-                }
-
+                calculationResult.ChangedFields = changedFields;
                 return StatusCode((int)HttpStatusCode.SeeOther, calculationResult);
             }
 

src/Altinn.App.Core/Helpers/JsonHelper.cs

@@ -1,5 +1,9 @@
 #nullable enable
 
+using System.Numerics;
+using Altinn.App.Core.Features;
+using Altinn.Platform.Storage.Interface.Models;
+using Microsoft.Extensions.Logging;
 using Newtonsoft.Json.Linq;
 
 namespace Altinn.App.Core.Helpers
@@ -9,6 +13,38 @@ namespace Altinn.App.Core.Helpers
     /// </summary>
     public static class JsonHelper
     {
+        /// <summary>
+        /// Run DataProcessWrite returning the dictionary of the changed fields.
+        /// </summary>
+        public static async Task<Dictionary<string, object?>?> ProcessDataWriteWithDiff(Instance instance, Guid dataGuid, object serviceModel, IDataProcessor dataProcessor, ILogger logger)
+        {
+            string serviceModelJsonString = System.Text.Json.JsonSerializer.Serialize(serviceModel);
+
+            bool changedByCalculation = await dataProcessor.ProcessDataWrite(instance, dataGuid, serviceModel);
+
+            Dictionary<string, object?>? changedFields = null;
+            if (changedByCalculation)
+            {
+                string updatedServiceModelString = System.Text.Json.JsonSerializer.Serialize(serviceModel);
+                try
+                {
+                    changedFields = FindChangedFields(serviceModelJsonString, updatedServiceModelString);
+                }
+                catch (Exception e)
+                {
+                    logger.LogError(e, "Unable to determine changed fields");
+                }
+            }
+
+            // TODO: Consider not bothering frontend with an empty changes list
+            // if(changedFields?.Count == 0)
+            // {
+            //     return null;
+            // }
+
+            return changedFields;
+        }
+
         /// <summary>
         /// Find changed fields between old and new json objects
         /// </summary>
@@ -147,7 +183,13 @@ private static void FindDiff(Dictionary<string, object?> dict, JToken? old, JTok
                     break;
 
                 default:
-                    dict.Add(prefix, current == null ? null : ((JValue)current).Value);
+                    var convertedValue = (current as JValue)?.Value switch
+                    {
+                        // BigInteger is not supported in json, so try to reduce to decimal, if possible, or string if too big
+                        BigInteger bigInt => bigInt <= new BigInteger(decimal.MaxValue) ? (decimal)bigInt : bigInt.ToString(System.Globalization.NumberFormatInfo.InvariantInfo),
+                        _ => (current as JValue)?.Value
+                    };
+                    dict.Add(prefix, convertedValue);
                     break;
             }
         }

test/Altinn.App.Core.Tests/Helpers/JsonHelperTests.cs

@@ -0,0 +1,273 @@
+#nullable enable
+
+using System.Text.Json.Serialization;
+using Altinn.App.Core.Features;
+using Altinn.App.Core.Helpers;
+using Altinn.Platform.Storage.Interface.Models;
+using FluentAssertions;
+using Microsoft.Extensions.Logging;
+using Moq;
+using Xunit;
+
+namespace Altinn.App.Core.Tests.Helpers;
+
+public class JsonHelperTests
+{
+    /// <summary>
+    /// Helper method to setup and get the dictionary of the diffs
+    /// </summary>
+    public async Task<Dictionary<string, object?>?> DoTest<TModel>(TModel model, Func<TModel, bool> processDataWriteImpl)
+                where TModel : class
+    {
+        var instance = new Instance();
+        var logger = new Mock<ILogger>().Object;
+        var guid = Guid.Empty;
+        var dataProcessorMock = new Mock<IDataProcessor>();
+        Func<Instance, Guid, object, Task<bool>> dataProcessWrite = (instance, guid, model) => Task.FromResult(processDataWriteImpl((TModel)model));
+        dataProcessorMock
+            .Setup((d) => d.ProcessDataWrite(It.IsAny<Instance>(), It.IsAny<Guid>(), It.IsAny<object>()))
+            .Returns(dataProcessWrite);
+
+        return await JsonHelper.ProcessDataWriteWithDiff(instance, guid, model, dataProcessorMock.Object, logger);
+    }
+
+    public class TestModel
+    {
+        public int IntegerTest { get; set; }
+
+        public int? NullableIntTest { get; set; }
+
+        public decimal DecimalTest { get; set; }
+
+        public decimal? NullableDecimalTest { get; set; }
+
+        public string StringTest { get; set; } = null!;
+
+        public string? NullableStringTest { get; set; }
+
+        // [Newtonsoft.Json.JsonProperty("jsonPropertyName")]
+        [JsonPropertyName("jsonPropertyName")]
+        public string? NotJsonPropertyNameTest { get; set; }
+
+        // [JsonPropertyName("newtonsoftString")]
+        [Newtonsoft.Json.JsonProperty("newtonsoftString")]
+        public string? NewtonsoftAttributeWorks { get; set; }
+
+        public TestModel? RecursiveTest { get; set; }
+
+        public TestModel NonNullableRecursiveTest { get; set; } = default!;
+
+        public List<int> PrimitiveList { get; set; } = default!;
+
+        public List<TestModel> ListTest { get; set; } = default!;
+
+        public List<TestModel>? NullableListTest { get; set; }
+    }
+
+    [Fact]
+    public async Task SimpleNoChangeTest()
+    {
+        var data = new TestModel();
+        var diff = await DoTest(data, (model) => false);
+        diff.Should().BeNull();
+    }
+
+    [Fact(Skip = "Curently empty diffs are sendt to frontend, so the result here is an empty dictionary")]
+    public async Task SimpleNoChangeTestTrueReturn()
+    {
+        var data = new TestModel();
+        var diff = await DoTest(data, (model) => true);
+        diff.Should().BeNull();
+    }
+
+    [Fact]
+    public async Task InitializingPropertiesLeadsToNoDiff()
+    {
+        var data = new TestModel();
+        var diff = await DoTest(data, (model) =>
+        {
+            model.ListTest = new();
+            model.PrimitiveList = new();
+            model.NullableListTest = new();
+            return true;
+        });
+
+        // Might be null in the future
+        diff.Should().BeEmpty();
+    }
+
+    [Fact]
+    public async Task InitializingNonNullablePropertiesCreatesDiff()
+    {
+        var data = new TestModel();
+        var diff = await DoTest(data, (model) =>
+        {
+            model.RecursiveTest = new();
+            return true;
+        });
+
+        // Not sure if RecursiveTest should be null here, but apparently it does not hurt
+        diff.Should().Equal(new Dictionary<string, object?>()
+        {
+            {"RecursiveTest.IntegerTest", 0},
+            {"RecursiveTest.DecimalTest", 0M},
+            {"RecursiveTest", null},
+        });
+    }
+
+    [Fact]
+    public async Task NullIsNotZero()
+    {
+        var data = new TestModel()
+        {
+            NullableIntTest = null,
+        };
+        var diff = await DoTest(data, (model) =>
+        {
+            model.NullableIntTest = 0;
+            return true;
+        });
+
+        diff.Should().Contain("NullableIntTest", 0);
+        diff.Should().HaveCount(1);
+    }
+
+    [Fact]
+    public async Task ZeroIsNotNull()
+    {
+        var data = new TestModel()
+        {
+            NullableIntTest = 0,
+        };
+        var diff = await DoTest(data, (model) =>
+        {
+            model.NullableIntTest = null;
+            return true;
+        });
+
+        diff.Should().Contain("NullableIntTest", null);
+        diff.Should().HaveCount(1);
+    }
+
+    [Fact]
+    public async Task TestSystemTextJsonAnnotation()
+    {
+        var data = new TestModel()
+        {
+            NotJsonPropertyNameTest = "Original Value",
+        };
+        var diff = await DoTest(data, (model) =>
+        {
+            model.NotJsonPropertyNameTest = "New Value";
+            return true;
+        });
+
+        diff.Should().Equal(new Dictionary<string, object?>
+        {
+            {"jsonPropertyName", "New Value"},
+        });
+    }
+
+    [Fact(Skip = "System.Text.Json annotation is required")]
+    public async Task NewtonsoftJsonAnnotation()
+    {
+        var data = new TestModel()
+        {
+            NewtonsoftAttributeWorks = "Original Value",
+        };
+        var diff = await DoTest(data, (model) =>
+        {
+            model.NewtonsoftAttributeWorks = "New Value";
+            return true;
+        });
+
+        diff.Should().Equal(new Dictionary<string, object?>
+        {
+            {"newtonsoftString", "New Value"},
+        });
+    }
+
+    [Theory]
+    [InlineData(-1)]
+    [InlineData(10)]
+    [InlineData(100)]
+    [InlineData(int.MinValue)]
+    [InlineData(int.MaxValue)]
+    public async Task ChangeInteger(int value)
+    {
+        var data = new TestModel()
+        {
+            RecursiveTest = new(),
+            PrimitiveList = new(),
+        };
+
+        var diff = await DoTest(data, (model) =>
+        {
+            model.IntegerTest = value;
+            model.NullableIntTest = value;
+            model.RecursiveTest ??= new();
+            model.RecursiveTest.IntegerTest = value;
+            model.PrimitiveList ??= new();
+            model.PrimitiveList.Add(value);
+            return true;
+        });
+
+        diff.Should().BeEquivalentTo(new Dictionary<string, object?>()
+        {
+             { "IntegerTest", value },
+             { "NullableIntTest", value },
+             { "RecursiveTest.IntegerTest", value },
+             { "PrimitiveList[0]", value },
+        });
+    }
+
+    [Theory]
+    [InlineData("-1")]
+    [InlineData("10")]
+    [InlineData("100")]
+    [InlineData(int.MinValue)]
+    [InlineData(int.MaxValue)]
+    [InlineData(long.MinValue)]
+    [InlineData(long.MaxValue)]
+    [InlineData("9223372036854775808")]
+    [InlineData("79228162514264337593543950335")]
+    [InlineData("-79228162514264337593543950334")]
+    public async Task ChangeDecimal(object rawValue)
+    {
+        var value = rawValue switch
+        {
+            string stringValue => decimal.Parse(stringValue),
+            int intValue => (decimal)intValue,
+            long longValue => (decimal)longValue,
+            _ => throw new NotImplementedException(),
+        };
+        var data = new TestModel()
+        {
+            RecursiveTest = new(),
+            ListTest = new() { new() },
+            NullableListTest = new() { new() },
+        };
+        var diff = await DoTest(data, (model) =>
+        {
+            model.DecimalTest = value;
+            model.NullableDecimalTest = value;
+            model.RecursiveTest ??= new();
+            model.RecursiveTest.DecimalTest = value;
+            model.NullableListTest ??= new();
+            model.NullableListTest[0].DecimalTest = value;
+            model.ListTest ??= new();
+            model.ListTest[0].DecimalTest = value;
+            return true;
+        });
+
+        // casting is weird (the current implementation of diff returns System.Numerics.BigInteger for large numbers)
+        Func<object, bool> isMatch = x => (decimal?)(dynamic?)x == value;
+
+        diff.Should().ContainKey("DecimalTest").WhoseValue.Should().Match(x => isMatch(x));
+        diff.Should().ContainKey("NullableDecimalTest").WhoseValue.Should().Match(x => isMatch(x));
+        diff.Should().ContainKey("RecursiveTest.DecimalTest").WhoseValue.Should().Match(x => isMatch(x));
+        diff.Should().ContainKey("NullableListTest[0].DecimalTest").WhoseValue.Should().Match(x => isMatch(x));
+        diff.Should().ContainKey("ListTest[0].DecimalTest").WhoseValue.Should().Match(x => isMatch(x));
+        diff.Should().HaveCount(5);
+    }
+}