Reduced fetch of data elements after change to maintain readstatus/la…

…stchanged* when changing data elements (#345)

* Added includeDataelements as parameter to GetInstancesFromQuery. Should be false from sbl message box search.

* - Made parameter includeElements in IInstanceRepository.GetOne mandatory
- Changed GetMessageBoxInstance to not get elements
- Avoid updating read status if status is unchanged
- Removed SetStatues (previous PR ensure statuses maintained for data element create/write/delete)
- Reduced number of warnings and info messages

* Completed rename of parameter

* - Added unit test for continuation token
- Added more logging functionality to TelemetryTracker
- Added logging to GetInstancesFromQuery

* Make sonarcloud happy

* Added null-check in GetSBLStatusForCurrentTask

* Introduced local variable

---------

Co-authored-by: Henning Normann <h.normann@accenture.com>

src/Storage/Controllers/CleanupController.cs

@@ -90,7 +90,7 @@ public async Task<ActionResult> CleanupInstancesForApp(string appId)
             Stopwatch stopwatch = Stopwatch.StartNew();
             do
             {
-                instancesResponse = await instanceRepository.GetInstancesFromQuery(options, instancesResponse.ContinuationToken, 5000);
+                instancesResponse = await instanceRepository.GetInstancesFromQuery(options, instancesResponse.ContinuationToken, 5000, true);
                 successfullyDeleted += await CleanupInstancesInternal(instancesResponse.Instances, []);
                 processed += (int)instancesResponse.Count;
             }

src/Storage/Controllers/InstancesController.cs

@@ -202,7 +202,7 @@ public async Task<ActionResult<QueryResponse<Instance>>> GetInstances(
 
             try
             {
-                InstanceQueryResponse result = await _instanceRepository.GetInstancesFromQuery(queryParams, continuationToken, pageSize);
+                InstanceQueryResponse result = await _instanceRepository.GetInstancesFromQuery(queryParams, continuationToken, pageSize, true);
 
                 if (!string.IsNullOrEmpty(result.Exception))
                 {
@@ -536,14 +536,19 @@ public async Task<ActionResult<Instance>> UpdateReadStatus(
             Instance updatedInstance;
             try
             {
+                ReadStatus? oldStatus = null;
                 if (instance.Status == null)
                 {
                     instance.Status = new InstanceStatus();
                 }
+                else
+                {
+                    oldStatus = instance.Status.ReadStatus;
+                }
 
                 instance.Status.ReadStatus = newStatus;
 
-                updatedInstance = await _instanceRepository.Update(instance);
+                updatedInstance = (oldStatus == null || oldStatus != newStatus) ? await _instanceRepository.Update(instance) : instance;
                 updatedInstance.SetPlatformSelfLinks(_storageBaseAndHost);
             }
             catch (Exception e)

src/Storage/Controllers/MessageboxInstancesController.cs

@@ -96,7 +96,7 @@ public async Task<ActionResult> SearchMessageBoxInstances([FromBody] MessageBoxQ
                 queryParams.Remove("searchString");
             }
 
-            InstanceQueryResponse queryResponse = await _instanceRepository.GetInstancesFromQuery(queryParams, null, 100);
+            InstanceQueryResponse queryResponse = await _instanceRepository.GetInstancesFromQuery(queryParams, null, 100, false);
 
             AddQueryModelToTelemetry(queryModel);
 
@@ -130,7 +130,7 @@ public async Task<ActionResult> GetMessageBoxInstance(
                 languageId = language;
             }
 
-            (Instance instance, _) = await _instanceRepository.GetOne(instanceGuid, true);
+            (Instance instance, _) = await _instanceRepository.GetOne(instanceGuid, false);
 
             if (instance == null)
             {

src/Storage/Helpers/InstanceHelper.cs

@@ -24,7 +24,7 @@ public static MessageBoxInstance ConvertToMessageBoxInstance(Instance instance)
 
             DateTime createdDateTime = visibleAfter != null && visibleAfter > instance.Created ? (DateTime)visibleAfter : instance.Created.Value;
 
-            MessageBoxInstance messageBoxInstance = new MessageBoxInstance
+            MessageBoxInstance messageBoxInstance = new()
             {
                 CreatedDateTime = createdDateTime,
                 DueDateTime = instance.DueBefore,
@@ -73,7 +73,7 @@ public static string GetAppId(MessageBoxInstance instance)
         /// <param name="instanceEvents">List of instance events to convert.</param>
         public static List<SblInstanceEvent> ConvertToSBLInstanceEvent(List<InstanceEvent> instanceEvents)
         {
-            List<SblInstanceEvent> simpleEvents = new List<SblInstanceEvent>();
+            List<SblInstanceEvent> simpleEvents = [];
             foreach (InstanceEvent instanceEvent in instanceEvents)
             {
                 simpleEvents.Add(
@@ -98,6 +98,7 @@ public static string GetSBLStatusForCurrentTask(Instance instance)
         {
             if (instance.Process != null)
             {
+                string taskType;
                 if (instance.Process.Ended != null && instance.Status?.Archived == null)
                 {
                     return "Submit";
@@ -106,22 +107,23 @@ public static string GetSBLStatusForCurrentTask(Instance instance)
                 {
                     return "Archived";
                 }
-                else if (instance.Process.CurrentTask.AltinnTaskType.Equals("confirmation", StringComparison.OrdinalIgnoreCase))
+                else if ((taskType = instance.Process?.CurrentTask?.AltinnTaskType) != null)
                 {
-                    return "Confirmation";
-                }
-                else if (instance.Process.CurrentTask.AltinnTaskType.Equals("feedback", StringComparison.OrdinalIgnoreCase))
-                {
-                    return "Feedback";
-                }
-                else if (instance.Process.CurrentTask.AltinnTaskType.Equals("signing", StringComparison.OrdinalIgnoreCase))
-                {
-                    return "Signing";
-                }
-                else
-                {
-                    return "FormFilling";
+                    if (taskType.Equals("confirmation", StringComparison.OrdinalIgnoreCase))
+                    {
+                        return "Confirmation";
+                    }
+                    else if (taskType.Equals("feedback", StringComparison.OrdinalIgnoreCase))
+                    {
+                        return "Feedback";
+                    }
+                    else if (taskType.Equals("signing", StringComparison.OrdinalIgnoreCase))
+                    {
+                        return "Signing";
+                    }
                 }
+
+                return "FormFilling";
             }
             else
             {
@@ -210,7 +212,7 @@ public static List<MessageBoxInstance> ReplaceTextKeys(List<MessageBoxInstance>
         /// <param name="instances">The list of applications to process.</param>
         public static void RemoveHiddenInstances(Dictionary<string, Application> applications, List<Instance> instances)
         {
-            List<Instance> instancesToRemove = new List<Instance>();
+            List<Instance> instancesToRemove = [];
 
             foreach (Instance instance in instances)
             {

src/Storage/Migration/v0.03/01-setup-functions.sql

@@ -0,0 +1,117 @@
+-- instances ---------------------------------------------------
+CREATE OR REPLACE FUNCTION storage.readinstancefromquery_v2(
+    _appId TEXT DEFAULT NULL,
+    _appIds TEXT[] DEFAULT NULL,
+    _archiveReference TEXT DEFAULT NULL,
+    _continue_idx BIGINT DEFAULT 0,
+    _created_eq TIMESTAMPTZ DEFAULT NULL,
+    _created_gt TIMESTAMPTZ DEFAULT NULL,
+    _created_gte TIMESTAMPTZ DEFAULT NULL,
+    _created_lt TIMESTAMPTZ DEFAULT NULL,
+    _created_lte TIMESTAMPTZ DEFAULT NULL,
+    _dueBefore_eq TEXT DEFAULT NULL,
+    _dueBefore_gt TEXT DEFAULT NULL,
+    _dueBefore_gte TEXT DEFAULT NULL,
+    _dueBefore_lt TEXT DEFAULT NULL,
+    _dueBefore_lte TEXT DEFAULT NULL,
+    _excludeConfirmedBy JSONB[] DEFAULT NULL,
+    _includeElements BOOL DEFAULT TRUE,
+    _instanceOwner_partyId INTEGER DEFAULT NULL,
+    _instanceOwner_partyIds INTEGER[] DEFAULT NULL,
+    _lastChanged_eq TIMESTAMPTZ DEFAULT NULL,
+    _lastChanged_gt TIMESTAMPTZ DEFAULT NULL,
+    _lastChanged_gte TIMESTAMPTZ DEFAULT NULL,
+    _lastChanged_idx TIMESTAMPTZ DEFAULT NULL,
+    _lastChanged_lt TIMESTAMPTZ DEFAULT NULL,
+    _lastChanged_lte TIMESTAMPTZ DEFAULT NULL,
+    _org TEXT DEFAULT NULL,
+    _process_currentTask TEXT DEFAULT NULL,
+    _process_ended_eq TEXT DEFAULT NULL,
+    _process_ended_gt TEXT DEFAULT NULL,
+    _process_ended_gte TEXT DEFAULT NULL,
+    _process_ended_lt TEXT DEFAULT NULL,
+    _process_ended_lte TEXT DEFAULT NULL,
+    _process_isComplete BOOLEAN DEFAULT NULL,
+    _size INTEGER DEFAULT 100,
+    _sort_ascending BOOLEAN DEFAULT FALSE,
+    _status_isActiveOrSoftDeleted BOOLEAN  DEFAULT NULL,
+    _status_isArchived BOOLEAN DEFAULT NULL,
+    _status_isArchivedOrSoftDeleted BOOLEAN DEFAULT NULL,
+    _status_isHardDeleted BOOLEAN DEFAULT NULL,
+    _status_isSoftDeleted BOOLEAN DEFAULT NULL,
+    _visibleAfter_eq TEXT DEFAULT NULL,
+    _visibleAfter_gt TEXT DEFAULT NULL,
+    _visibleAfter_gte TEXT DEFAULT NULL,
+    _visibleAfter_lt TEXT DEFAULT NULL,
+    _visibleAfter_lte TEXT DEFAULT NULL
+    )
+    RETURNS TABLE (id BIGINT, instance JSONB, element JSONB)
+    LANGUAGE 'plpgsql'
+
+AS $BODY$
+BEGIN
+    IF _sort_ascending IS NULL THEN
+        _sort_ascending := false;
+    END IF;
+
+    RETURN QUERY
+    WITH filteredInstances AS
+    (
+        SELECT i.id, i.instance, i.lastchanged FROM storage.instances i
+        WHERE 1 = 1
+            AND (_continue_idx <= 0 OR
+                (_continue_idx > 0 AND _sort_ascending = true  AND (i.lastchanged > _lastChanged_idx OR (i.lastchanged = _lastChanged_idx AND i.id > _continue_idx))) OR
+                (_continue_idx > 0 AND _sort_ascending = false AND (i.lastchanged < _lastChanged_idx OR (i.lastchanged = _lastChanged_idx AND i.id < _continue_idx))))
+            AND (_appId IS NULL OR i.appid = _appId)
+            AND (_appIds IS NULL OR i.appid = ANY(_appIds))
+            AND (_archiveReference IS NULL OR i.instance ->> 'Id' like '%' || _archiveReference)		
+            AND (_created_gte IS NULL OR i.created >= _created_gte)
+            AND (_created_gt  IS NULL OR i.created >  _created_gt)
+            AND (_created_lte IS NULL OR i.created <= _created_lte)
+            AND (_created_lt  IS NULL OR i.created <  _created_lt)
+            AND (_created_eq  IS NULL OR i.created =  _created_eq)
+            AND (_dueBefore_gte IS NULL OR i.instance ->> 'DueBefore' >= _dueBefore_gte)
+            AND (_dueBefore_gt  IS NULL OR i.instance ->> 'DueBefore' >  _dueBefore_gt)
+            AND (_dueBefore_lte IS NULL OR i.instance ->> 'DueBefore' <= _dueBefore_lte)
+            AND (_dueBefore_lt  IS NULL OR i.instance ->> 'DueBefore' <  _dueBefore_lt)
+            AND (_dueBefore_eq   IS NULL OR i.instance ->> 'DueBefore' =  _dueBefore_eq)
+            AND (_excludeConfirmedBy IS NULL OR i.instance -> 'CompleteConfirmations' IS NULL OR NOT i.instance -> 'CompleteConfirmations' @> ANY (_excludeConfirmedBy))
+            AND (_instanceOwner_partyId IS NULL OR partyId = _instanceOwner_partyId)
+            AND (_instanceOwner_partyIds IS NULL OR partyId = ANY(_instanceOwner_partyIds))
+            AND (_lastChanged_gte IS NULL OR i.lastchanged >= _lastChanged_gte)
+            AND (_lastChanged_gt  IS NULL OR i.lastchanged >  _lastChanged_gt)
+            AND (_lastChanged_lte IS NULL OR i.lastchanged <= _lastChanged_lte)
+            AND (_lastChanged_lt  IS NULL OR i.lastchanged <  _lastChanged_lt)
+            AND (_lastChanged_eq  IS NULL OR i.lastchanged =  _lastChanged_eq)
+            AND (_org IS NULL OR i.org = _org)
+            AND (_process_currentTask IS NULL OR i.instance -> 'Process' -> 'CurrentTask' ->> 'ElementId' = _process_currentTask)
+            AND (_process_ended_gte IS NULL OR i.instance -> 'Process' ->> 'Ended' >= _process_ended_gte)
+            AND (_process_ended_gt  IS NULL OR i.instance -> 'Process' ->> 'Ended' >  _process_ended_gt)
+            AND (_process_ended_lte IS NULL OR i.instance -> 'Process' ->> 'Ended' <= _process_ended_lte)
+            AND (_process_ended_lt  IS NULL OR i.instance -> 'Process' ->> 'Ended' <  _process_ended_lt)
+            AND (_process_ended_eq  IS NULL OR i.instance -> 'Process' ->> 'Ended' =  _process_ended_eq)
+            AND (_process_isComplete IS NULL OR (_process_isComplete = TRUE AND i.instance -> 'Process' -> 'Ended' IS NOT NULL) OR (_process_isComplete = FALSE AND i.instance -> 'Process' -> 'CurrentTask' IS NOT NULL))
+            AND ((_status_isActiveOrSoftDeleted IS NULL OR _status_isActiveOrSoftDeleted = false) OR ((i.instance -> 'Status' -> 'IsArchived')::boolean = false OR (i.instance -> 'Status' -> 'IsSoftDeleted')::boolean = true))           
+            AND (_status_isArchived IS NULL OR  (i.instance -> 'Status' -> 'IsArchived')::boolean = _status_isArchived)
+            AND ((_status_isArchivedOrSoftDeleted IS NULL OR _status_isArchivedOrSoftDeleted = false) OR ((i.instance -> 'Status' -> 'IsArchived')::boolean = true OR (i.instance -> 'Status' -> 'IsSoftDeleted')::boolean = true))                
+            AND (_status_isHardDeleted IS NULL OR  (i.instance -> 'Status' -> 'IsHardDeleted')::boolean = _status_isHardDeleted)
+            AND (_status_isSoftDeleted IS NULL OR  (i.instance -> 'Status' -> 'IsSoftDeleted')::boolean = _status_isSoftDeleted)                     
+            AND (_visibleAfter_gte IS NULL OR i.instance ->> 'VisibleAfter' >= _visibleAfter_gte)
+            AND (_visibleAfter_gt  IS NULL OR i.instance ->> 'VisibleAfter' >  _visibleAfter_gt)
+            AND (_visibleAfter_lte IS NULL OR i.instance ->> 'VisibleAfter' <= _visibleAfter_lte)
+            AND (_visibleAfter_lt  IS NULL OR i.instance ->> 'VisibleAfter' <  _visibleAfter_lt)
+            AND (_visibleAfter_eq  IS NULL OR i.instance ->> 'VisibleAfter' =  _visibleAfter_eq)
+        ORDER BY
+            (CASE WHEN _sort_ascending = true  THEN i.lastChanged END) ASC,
+            (CASE WHEN _sort_ascending = false THEN i.lastChanged END) DESC,
+            i.id
+        FETCH FIRST _size ROWS ONLY
+    )
+        SELECT filteredInstances.id, filteredInstances.instance, d.element FROM filteredInstances
+            LEFT JOIN storage.dataelements d ON filteredInstances.id = d.instanceInternalId AND _includeElements = TRUE
+        ORDER BY
+            (CASE WHEN _sort_ascending = true  THEN filteredInstances.lastChanged END) ASC,
+            (CASE WHEN _sort_ascending = false THEN filteredInstances.lastChanged END) DESC,
+            filteredInstances.id;
+END;
+$BODY$;

src/Storage/Repository/IInstanceRepository.cs

@@ -17,16 +17,17 @@ public interface IInstanceRepository
         /// <param name="queryParams">the query params</param>
         /// <param name="continuationToken">a token to get the next page, more performant than using page</param>
         /// <param name="size">The number of items per page</param>
+        /// <param name="includeDataelements">Whether to include data elements</param>
         /// <returns>The query response including the list of instances</returns>
-        Task<InstanceQueryResponse> GetInstancesFromQuery(Dictionary<string, StringValues> queryParams, string continuationToken, int size);
+        Task<InstanceQueryResponse> GetInstancesFromQuery(Dictionary<string, StringValues> queryParams, string continuationToken, int size, bool includeDataelements);
 
         /// <summary>
         /// Get an instance for a given instance id
         /// </summary>
         /// <param name="instanceGuid">the instance guid</param>
         /// <param name="includeElements">whether to include data elements</param>
         /// <returns>The instance for the given parameters</returns>
-        Task<(Instance Instance, long InternalId)> GetOne(Guid instanceGuid, bool includeElements = true);
+        Task<(Instance Instance, long InternalId)> GetOne(Guid instanceGuid, bool includeElements);
 
         /// <summary>
         /// insert new instance into collection

src/Storage/Repository/PgDataRepository.cs

@@ -17,33 +17,26 @@ namespace Altinn.Platform.Storage.Repository
     /// <summary>
     /// Represents an implementation of <see cref="IDataRepository"/>.
     /// </summary>
-    public class PgDataRepository : IDataRepository
+    /// <remarks>
+    /// Initializes a new instance of the <see cref="PgDataRepository"/> class.
+    /// </remarks>
+    /// <param name="logger">The logger to use when writing to logs.</param>
+    /// <param name="dataSource">The npgsql data source.</param>
+    /// <param name="telemetryClient">Telemetry client</param>
+    public class PgDataRepository(
+        ILogger<PgDataRepository> logger,
+        NpgsqlDataSource dataSource,
+        TelemetryClient telemetryClient = null) : IDataRepository
     {
         private readonly string _insertSql = "call storage.insertdataelement ($1, $2, $3, $4)";
         private readonly string _readSql = "select * from storage.readdataelement($1)";
         private readonly string _deleteSql = "select * from storage.deletedataelement_v2 ($1, $2, $3)";
         private readonly string _deleteForInstanceSql = "select * from storage.deletedataelements ($1)";
         private readonly string _updateSql = "select * from storage.updatedataelement_v2 ($1, $2, $3, $4, $5, $6)";
 
-        private readonly ILogger<PgDataRepository> _logger;
-        private readonly NpgsqlDataSource _dataSource;
-        private readonly TelemetryClient _telemetryClient;
-
-        /// <summary>
-        /// Initializes a new instance of the <see cref="PgDataRepository"/> class.
-        /// </summary>
-        /// <param name="logger">The logger to use when writing to logs.</param>
-        /// <param name="dataSource">The npgsql data source.</param>
-        /// <param name="telemetryClient">Telemetry client</param>
-        public PgDataRepository(
-            ILogger<PgDataRepository> logger,
-            NpgsqlDataSource dataSource,
-            TelemetryClient telemetryClient = null)
-        {
-            _logger = logger;
-            _dataSource = dataSource;
-            _telemetryClient = telemetryClient;
-        }
+        private readonly ILogger<PgDataRepository> _logger = logger;
+        private readonly NpgsqlDataSource _dataSource = dataSource;
+        private readonly TelemetryClient _telemetryClient = telemetryClient;
 
         /// <inheritdoc/>
         public async Task<DataElement> Create(DataElement dataElement, long instanceInternalId = 0)
@@ -122,8 +115,8 @@ public async Task<DataElement> Update(Guid instanceGuid, Guid dataElementId, Dic
                 throw new ArgumentOutOfRangeException(nameof(propertylist), "PropertyList can contain at most 12 entries.");
             }
 
-            List<string> elementProperties = new();
-            List<string> instanceProperties = new();
+            List<string> elementProperties = [];
+            List<string> instanceProperties = [];
             DataElement element = new();
             Instance lastChangedWrapper = new();
             bool isReadChangedToFalse = false;

src/Storage/Repository/PgInstanceEventRepository.cs

@@ -18,28 +18,22 @@ namespace Altinn.Platform.Storage.Repository
     /// <summary>
     /// Represents an implementation of <see cref="IInstanceEventRepository"/>.
     /// </summary>
-    public class PgInstanceEventRepository: IInstanceEventRepository
+    /// <remarks>
+    /// Initializes a new instance of the <see cref="PgInstanceEventRepository"/> class.
+    /// </remarks>
+    /// <param name="dataSource">The npgsql data source.</param>
+    /// <param name="telemetryClient">Telemetry client</param>
+    public class PgInstanceEventRepository(
+        NpgsqlDataSource dataSource,
+        TelemetryClient telemetryClient = null) : IInstanceEventRepository
     {
         private readonly string _readSql = "select * from storage.readinstanceevent($1)";
         private readonly string _deleteSql = "select * from storage.deleteInstanceevent($1)";
         private readonly string _insertSql = "call storage.insertInstanceevent($1, $2, $3)";
         private readonly string _filterSql = "select * from storage.filterinstanceevent($1, $2, $3, $4)";
 
-        private readonly NpgsqlDataSource _dataSource;
-        private readonly TelemetryClient _telemetryClient;
-
-        /// <summary>
-        /// Initializes a new instance of the <see cref="PgInstanceEventRepository"/> class.
-        /// </summary>
-        /// <param name="dataSource">The npgsql data source.</param>
-        /// <param name="telemetryClient">Telemetry client</param>
-        public PgInstanceEventRepository(
-            NpgsqlDataSource dataSource,
-            TelemetryClient telemetryClient = null)
-        {
-            _dataSource = dataSource;
-            _telemetryClient = telemetryClient;
-        }
+        private readonly NpgsqlDataSource _dataSource = dataSource;
+        private readonly TelemetryClient _telemetryClient = telemetryClient;
 
         /// <inheritdoc/>
         public async Task<InstanceEvent> InsertInstanceEvent(InstanceEvent instanceEvent)