[DO NOT MERGE] POC for getting/updating sessions in different states. #275

chris-cooksley-gds · 2024-12-02T16:04:46Z

Rationale

For the async work we'll soon need to work with async sessions in multiple states, for which it will pay to have a pattern that's type safe and easy to extend/modify.

Some objectives:

specific session types for each state, with the fields in the type limited to those available to a session in that state
the ability to fetch a session in a specified state, and get back an object as above
the ability to update a session to be in a particular new state, along with any new fields to be added, and including any conditional checks on a required initial state

A lot of this is adapted from patterns used in the STS codebase, where they've seen some success.

Where to start

I'd recommend:

Start by looking at (and perhaps running) the demo test. EDIT: I've also now added an example handler to show how the methods might be used.
Then look at the Session types and update operations
Then the new adapter methods
Finally the convertor functions

Overview of new code

getSessionWithState attempts to get a session from dynamo that's expected to be in a particular state.

It's overloaded - this turned out to be the easiest way to make it work type-safely for different states (TS conditionals/generics never quite worked as expected, at least when we tried it on MPT).
It'll either give you back a Success with a session object in that state (with all and only the fields we'd expect to be on a session in this state), or a failure with a reason why
It'll fall back to a strongly consistent read if the first attempt can't find a session in this state, as the default (cheaper) eventually consistent read may not reflect recent writes/updates
It makes use of state-specific convertor functions to convert from the Dynamo output into a particular type/shape of session, with validation applied to make sure the expected fields are present.

updateSession performs a particular update operation to a pre-existing session

the update operations are polymorphic. They implement a common interface, and can all produce the necessary dynamo expressions etc to perform the update they represent. This makes them individually easy to manage/maintain/test, while also making it simpler to test the (agnostic) update method.
Some updates come with a conditional check on a specific current session state. Others don't, on the basis that we'll already have retrieved the session and checked its state before making the update (and don't want the extra read charge from dynamo to check again). This could also be a boolean on the updateMethod, I suppose, if we didn't want to manage it at the operation level.

Stray comments

I've put (placeholder) logging in just using the console, to be replaced by real logging. Couple differences to our usual logging pattern here that might be interesting to chat about (but not the focus of this):
- I log in the adapter itself rather than propagating the error up to be logged elsewhere. The idea is to capture and log specific error details as close to the source as possible, and simplify the information that then needs to be passed around elsewhere in the system.
- I log the attempt and success of the operation as well as just failures. I've found this to be very useful for debugging external calls - e.g. we can see that we made an attempt that never returned a success or failure, or that we got a successful result (or failure) a full 2 seconds after starting the attempt.
I've updated our use of the Result pattern to make it easier to capture 'empty' successes/failures (when we don't need any information back aside from the success/failure of an op), and to return different error information on a failure. Again not the focus of this change, just made it easier to adapt some of the MPT functions. I've tried to make it backwards compatible with our current usage but there may be some TS compiler errors that need massaging away.
I've not covered every session state for this POC - notably ASYNC_AUTH_SESSION_ABORTED is absent
EDIT: as part of adding an example lambda handler I've also introduced a new pattern for getting config from the environment that avoids a bit of repetition. Again, not the focus here, but might be something to think about.

jmooney-dd · 2024-12-03T12:46:08Z

backend-api/src/functions/common/session/sessionsDemo.test.ts

+import { BiometricSessionFinished } from "./updateOperations/BiometricSessionFinished";
+import { BiometricTokenIssued } from "./updateOperations/BiometricTokenIssued";
+
+describe("Sessions", () => {


Very clear console.logs produced here, thanks! Wish I had read line 11 a little harder before running the test a few times 😆

jmooney-dd · 2024-12-10T11:15:26Z

backend-api/src/functions/adapters/dynamoDbAdapter.ts

+    if (session.sessionState === SessionState.BIOMETRIC_TOKEN_ISSUED) {
+      return successResult(session);
+    }
+    if (session.sessionState === SessionState.BIOMETRIC_SESSION_FINISHED) {
+      return successResult(session);
+    }
+    if (session.sessionState === SessionState.RESULT_SENT) {
+      return successResult(session);
+    }
+    return successResult(session);


We say if it's one of these 3 states return a successResult(session), but if not we still return the same thing. Am I missing a reason why we do this or is this just because it's POC?

This is needed to appease TypeScript, annoyingly, try removing the conditions and you'll see what I mean. I can't remember the exact reason it happens, but I think essentially if you have a function that returns one (specific) type from several options, every exit point from that function needs to be returning a specific type, rather than returning an object that could be any one of the valid types.

jmooney-dd · 2024-12-10T11:18:20Z

backend-api/src/functions/adapters/dynamoDbAdapter.ts

+  ): Promise<Result<Session, ReadSessionError>> {
+    let output: GetItemCommandOutput;
+    try {
+      console.log("Get session attempt"); // replace with proper logging


In the past we only log by exception (apart from started, completed logs), do you think here there's a good reason to change this?

See the PR description (stray comments) for the reasoning here. tl;dr, yes, I think it's very useful for working with external calls. But not crucial to this POC, definitely something to discuss in more detail elsewhere.

Sorry should have re read that first, thanks for confirming!

backend-api/src/functions/common/session/updateOperations/BiometricSessionFinished.ts

jmooney-dd · 2024-12-10T11:36:49Z

backend-api/src/functions/common/session/updateOperations/BiometricSessionFinished.ts

+        S: SessionState.BIOMETRIC_SESSION_FINISHED,
+      },
+      ":biometricTokenIssued": { S: SessionState.BIOMETRIC_TOKEN_ISSUED },
+    } as const;


Is casting as a const to do with strongly typing here? Or to make it readonly? Or something else entirely?

Honest answer is, I can't remember why we did it on MPT. Doesn't seem to cause any TS issues if it's removed, so I assume it's just to keep it readonly, but I can't see that it's really needed here.

jmooney-dd · 2024-12-10T11:39:38Z

backend-api/src/functions/exampleLambda/exampleLambdaConfig.ts

+} from "../common/utils/environment";
+import { Result } from "../utils/result";
+
+const REQUIRED_ENVIRONMENT_VARIABLES = ["SESSIONS_TABLE"] as const;


What is the value in asking for the envVars you want like you do here vs having envVars defined for each lambda and just getting all of them?

Not sure I understand the question - this would be where we define all the env vars for the lambda, it just so happens that this example lambda only has one.

Sorry had misunderstood the getConfigFromEnvironment. This will actually be a function callable by all lambdas and we just give it the envVars we want. Makes sense

jmooney-dd · 2024-12-10T11:42:44Z

backend-api/src/functions/common/utils/environment.ts

+  [key in T]: string;
+};
+
+export const getRequiredEnvironmentVariables = <T extends string>(


Made another comment re envVars, would be good to chat about the general approach here. Looking at this func it feels a bit weird but unsure if it's just because of the generics and typings of stuff look a bit different to me. Not against at all but would be useful for me to chat through

Happy to chat through it any time (and see if it can be improved, too!). The goal was to have a generic function that could take a list of strings for the env vars we want to retrieve, and give back either a type-safe map with all these strings as keys, or an error. This way, the config functions for the individual lambdas only need to specify once which env vars they need. The pursuit of type-safety is why we've had to use all the generics etc - but there may well be a simpler way to do it.

sandymay-dd · 2024-12-11T13:36:21Z

backend-api/src/functions/common/session/updateOperations/BiometricSessionFinished.ts

General Q: What's the thinking behind some filenames being capitalised and some not? If it's intensional that is!

The thinking was supposed to be - PascalCase for files that export one primary class or interface, with the name matching the name of said class/interface (i.e. BiometricSessionFinished.ts is where BiometricSessionFinished comes from). And camelCase for files that export a function (name matching, e.g. exampleHandler), or for files that export several different entities (e.g. dynamoDBItemToSessionConvertors).

I don't think I've been consistent here, though - sessionRegistry should probably be capitalised, for example.

sandymay-dd · 2024-12-11T13:39:08Z

backend-api/src/functions/common/session/Session.ts

+  redirectUri?: string;
+};
+
+export type AuthSessionCreatedSession = CommonSessionKeys & {


This pattern is nice

sandymay-dd · 2024-12-11T13:39:50Z

backend-api/src/functions/common/session/Session.ts

+export type BiometricTokenIssuedSession = CommonSessionKeys & {
+  sessionState: SessionState.BIOMETRIC_TOKEN_ISSUED;
+  documentType: string;
+  accessToken: string;


More for the back of your mind and not related to this spike - we shouldn't save the accessToken in the database - it's a short-lived token and I can't think why we'd ever need it!

Good point, I hadn't stopped to consider where it would actually be used again. One to update on the TD where it came from I suppose - perhaps as part of a more general review of the session schema?

Yes! It's on my list to update that TD having spoken to Darren

sandymay-dd · 2024-12-11T13:43:08Z

backend-api/src/functions/common/session/updateOperations/ResultSent.ts

+  }
+
+  getDynamoDbConditionExpression(): undefined {
+    return undefined;


I assume this is undefined, given this is just a spike?

Or is this because we've already fetched the session record earlier in the lambda and we know in the lambda itself whether we want to make an update operation or not?

The second one! Saves a redundant read-before-write.

sandymay-dd · 2024-12-11T13:52:33Z

backend-api/src/functions/common/session/sessionRegistry.ts

+    sessionState: SessionState,
+  ): Promise<Result<Session, ReadSessionError>>;
+
+  // Ideally the return value of this would be more dynamo-agnostic, to keep it in this interface


Yes - I agree, and I'd prefer this. This interface would ideally return the AuthSessionCreatedSession I think?

Yep that's what I was thinking

sandymay-dd · 2024-12-11T14:01:21Z

backend-api/src/functions/common/session/Session.ts

+  | BiometricSessionFinishedSession
+  | ResultSentSession;
+
+export interface IUpdateSessionOperation {


I'm just mulling (or marinading as Elishka would say) on this one. I like how the lambda decides what implementation of the operations to follow.

Perhaps it's just the location of this interface! It would be good to think about partitioning (even if it's by folder structure) as much of the Dynamo implementation we can from the business logic/interfaces you've written that are so clean.

Definitely happy to move the interface elsewhere.

Something I considered was having the update operations just know about (and be able to tell you about) their valid 'FROM' session states and target session state, as well as what other fields need adding, in a dynamo-agnostic way. I just couldn't think of a neat way to work that list bit in in the time I'd taken on it. But will keep marinading.

It's possible that avenue might add a bit more abstraction than we really need, but yes if you can think of an improvement by all means go for it.

If the lambda directly referenced this interface, I'd be much more hesitant about the solution you've proposed.

sandymay-dd · 2024-12-11T16:30:14Z

backend-api/src/functions/adapters/dynamoDbAdapter.ts

    this.tableName = tableName;
  }

+  async getSessionWithState(


I've tried to play with a mapped type for this but probably hit similar pitfalls to what you've tried historically! If it works, it works.

chris-cooksley-gds added 12 commits November 28, 2024 15:25

Define state machine

39e503d

Define state machine properly

933ef4e

Define session types

f8f9367

Working example (no tests, no AUTH_SESSION_ABORTED)

1c7ffd1

Fix omission

226fd2f

Introduce update operations and adapter method

d26efbe

Add test to demonstrate functionality

79f51b6

Merge remote-tracking branch 'origin/main' into cc-session-states

8fb7912

Run formatter

a1084e2

Introduce SessionRegistry interface

6b4ff69

Add example lambda handler

e111ce0

Merge remote-tracking branch 'origin/main' into cc-session-states

eea6212

jmooney-dd reviewed Dec 3, 2024

View reviewed changes

jmooney-dd reviewed Dec 10, 2024

View reviewed changes

backend-api/src/functions/common/session/updateOperations/BiometricSessionFinished.ts Show resolved Hide resolved

jmooney-dd reviewed Dec 10, 2024

View reviewed changes

sandymay-dd reviewed Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NOT MERGE] POC for getting/updating sessions in different states. #275

[DO NOT MERGE] POC for getting/updating sessions in different states. #275

chris-cooksley-gds commented Dec 2, 2024 •

edited

Loading

jmooney-dd Dec 3, 2024

jmooney-dd Dec 10, 2024

chris-cooksley-gds Dec 10, 2024

jmooney-dd Dec 10, 2024

chris-cooksley-gds Dec 10, 2024

jmooney-dd Dec 10, 2024

jmooney-dd Dec 10, 2024

chris-cooksley-gds Dec 10, 2024

jmooney-dd Dec 10, 2024

chris-cooksley-gds Dec 10, 2024

jmooney-dd Dec 10, 2024

jmooney-dd Dec 10, 2024 •

edited

Loading

chris-cooksley-gds Dec 10, 2024

sandymay-dd Dec 11, 2024

chris-cooksley-gds Dec 11, 2024

sandymay-dd Dec 11, 2024

sandymay-dd Dec 11, 2024

chris-cooksley-gds Dec 11, 2024

sandymay-dd Dec 11, 2024

sandymay-dd Dec 11, 2024

sandymay-dd Dec 11, 2024

chris-cooksley-gds Dec 11, 2024 •

edited

Loading

sandymay-dd Dec 11, 2024

chris-cooksley-gds Dec 11, 2024

sandymay-dd Dec 11, 2024 •

edited

Loading

chris-cooksley-gds Dec 11, 2024

sandymay-dd Dec 11, 2024

sandymay-dd Dec 11, 2024

[DO NOT MERGE] POC for getting/updating sessions in different states. #275

Are you sure you want to change the base?

[DO NOT MERGE] POC for getting/updating sessions in different states. #275

Conversation

chris-cooksley-gds commented Dec 2, 2024 • edited Loading

Rationale

Where to start

Overview of new code

Stray comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmooney-dd Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-cooksley-gds Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandymay-dd Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-cooksley-gds commented Dec 2, 2024 •

edited

Loading

jmooney-dd Dec 10, 2024 •

edited

Loading

chris-cooksley-gds Dec 11, 2024 •

edited

Loading

sandymay-dd Dec 11, 2024 •

edited

Loading