Appt xxx/add point read slide cache site by pata9 · Pull Request #1633 · NHSDigital/nbs-appointments-management-service

pata9 · 2026-04-21T16:42:47Z

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Checklist:

My work is behind a feature toggle (if appropriate)
If my work is behind a feature toggle, I've added a full suite of tests for both the ON and OFF state
The ticket number is in the Pull Request title, with format "APPT-XXX: My Title Here"
I have ran npm tsc / lint (in the future these will be ran automatically)
My code generates no new .NET warnings (in the future these will be treated as errors)
If I've added a new Function, it is disabled in all but one of the terraform groups (e.g. http_functions)
If I've added a new Function, it has both unit and integration tests. Any request body validators have unit tests also
If I've made UI changes, I've added appropriate Playwright and Jest tests
If I've added/updated an end-point, I've added the appropriate annotations and tested the Swagger documentation reflects the change

pata9 · 2026-04-23T10:30:57Z

    protected override async Task<ApiResult<Site>> HandleRequest(SiteBasedResourceRequest request, ILogger logger)
    {
-        var site = await siteService.GetSiteByIdAsync(request.Site, request.Scope);
+        var site = await siteService.GetSiteByIdAsync(request.Site, request.IgnoreCache, request.Scope);


IgnoreCache should be last param for this method

pata9 · 2026-04-23T10:31:42Z

    {
        const string scope = "site_details";
-        var site = await siteService.GetSiteByIdAsync(request.Site, scope);
+        var site = await siteService.GetSiteByIdAsync(request.Site, false, scope);


I think this function is not used? Can we just delete this?

pata9 · 2026-04-23T10:32:48Z

        {
            var site = req.Query["site"];
-            return Task.FromResult((ErrorMessageResponseItem.None, new SiteBasedResourceRequest(site, requestedScope)));
+            return Task.FromResult((ErrorMessageResponseItem.None, new SiteBasedResourceRequest(site, requestedIgnoreCache, requestedScope)));


IgnoreCache should be the last param here

pata9 · 2026-04-23T10:33:37Z

 namespace Nhs.Appointments.Api.Models;

-public record SiteBasedResourceRequest(string Site, string Scope);
+public record SiteBasedResourceRequest(string Site, bool IgnoreCache, string Scope);


Move to last param and maybe default to false?

pata9 · 2026-04-23T10:35:22Z

+                $"site:referenceNumberGroup:{siteId}", 
+                new CacheOptions<int>(
+                    async () => await GetSitesAssignedReferenceGroup(siteId), 
+                    TimeSpan.FromHours(24)));


Configure expiration in app settings

pata9 · 2026-04-23T10:35:38Z

+    {
+        var referenceNumberGroup =
+            await _cacheService.GetCacheValue(
+                $"site:referenceNumberGroup:{siteId}", 


Resolve cache key from a factory

pata9 · 2026-04-23T10:36:46Z

-        var sequence = await _referenceNumberDocumentStore.GetNextSequenceNumber(referenceGroup);
+        var sequence = await _referenceNumberDocumentStore.GetNextSequenceNumber(referenceNumberGroup);
        var now = _timeProvider.GetUtcNow();
        var rng = now.Day + now.Second;


Another level of randomness here (90-99 are not being used currently)

pata9 · 2026-04-23T10:43:09Z

+        var referenceNumberGroup =
+            await _cacheService.GetCacheValue(
+                $"site:referenceNumberGroup:{siteId}", 
+                new CacheOptions<int>(


We thought about this and decided there's no real benefit to sliding cache here. use the default long lived cache for less complexity

pata9 · 2026-04-23T10:46:03Z


 public interface ISiteService
 {
-    Task<Site> GetSiteByIdAsync(string siteId, string scope = "*");


Ignore cache should be the last param and it should be a enum.

enum should be called something like - DataSource
enum values should follow something like - "UseDatabase", "UseCache"

pata9 · 2026-04-23T10:47:06Z

+        }
+        else 
+        {
+            site = await cacheService.GetLazySlidingCacheValue($"site:{siteId}",


cache key factory here

pata9 · 2026-04-23T10:48:24Z

    {
-        var site = await siteStore.GetSiteById(siteId);
+        Site site;
+        if (options.Value.DisableSiteCache || ignoreCache)


Invert if and move and introduce a private method for use cache

pata9 · 2026-04-23T10:51:02Z

+    /// <summary>
+    ///     The duration, in minutes, to cache site for.
+    /// </summary>
+    public int SiteCacheDurationMinutes { get; set; }


Add getter to expose these int's as timespan that can then be unit tested

pata9 · 2026-04-23T10:53:59Z

+[CosmosDocumentType("reference_group")]
+public class BookingReferenceGroupDocument : BookingReferenceDataCosmosDocument
+{
+    public int SiteCount { get; set; }


Debate should prefix be explicit or use Id as this implementation does

Maybe just expose it here?

pata9 · 2026-04-23T10:56:42Z

+
+            referenceGroupDocuments = (await _cosmosStore.RunQueryAsync<BookingReferenceGroupDocument>(x => x.DocumentType == docType)).ToArray();
+
+            if (!referenceGroupDocuments.Any())


Count vs any

pata9 · 2026-04-23T10:57:28Z

+            }
+
+            var target = referenceGroupDocuments.Where(g => g.Id != 0.ToString()).OrderBy(g => g.SiteCount).ThenBy(g => g.Id).First();
+            var siteCountIncrement = PatchOperation.Increment($"/SiteCount", 1);


Magic strings to static class

Object orientated? ReferenceGroupOperation.SiteIncrement() OR ReferenceGroupOperation.UpdateSiteCount()

Maybe should live on the BookingReferenceGroupDocument

pata9 · 2026-04-23T10:57:37Z

+
+        public async Task<int> GetNextSequenceNumber(int prefix)
+        {
+            var incrementSequencePatch = PatchOperation.Increment($"/Sequence", 1);


Magic string!

pata9 · 2026-04-23T13:09:51Z


 export const fetchSite = async (
  siteId: string,
+  ignoreCache: boolean,


Use the enum like in the API layor

pata9 · 2026-04-23T13:28:02Z

            isDeleted: dataTable.GetBoolRowValueOrDefault(row, "IsDeleted"),
-            Type: dataTable.GetRowValueOrDefault(row, "Type")
+            Type: dataTable.GetRowValueOrDefault(row, "Type"),
+            ReferenceNumberGroup: 0


Can we do this once somewhere please?

Maybe use -1 by default

pata9 added 17 commits April 21, 2026 11:36

introduce sliding cache for point read of sites

cc65176

fix tests

35afcea

refactor reference group to go through the service layer and use caching

f34a991

refactor to referenceGroup Cache

3115861

add prop to ignore cache on get site and update ui

608bbd3

add prop to ignore cache on get site and update ui

e5231d5

add new store with migration to new container

3ee288b

setup infra

1bc23fd

increase site caching durations

d6ecdb6

fix web app test

3c3d4a6

add container and fix tests

737eca8

fix where GetNextSequenceNumber happens before AssignReferenceGroup

8f733b9

rename document

5be0658

make reference documents individual documents

5536a72

fix up the logic for copy old

62f0916

fix up migration for assign ref group

841ca2f

refactoring

b9e9d80

pata9 commented Apr 23, 2026

View reviewed changes

change reference group to not be lastUpdatedBy

297d524

pata9 commented Apr 23, 2026

View reviewed changes

pata9 added 5 commits April 23, 2026 15:20

donut

05d4107

make point reads instead of query on db

4a02a3c

business logic corrected

4ade28c

business logic corrected

fb05571

business logic corrected

db34751


		referenceGroupDocuments = (await _cosmosStore.RunQueryAsync<BookingReferenceGroupDocument>(x => x.DocumentType == docType)).ToArray();

		if (!referenceGroupDocuments.Any())

Conversation

pata9 commented Apr 21, 2026

Description

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant