feat(GIFT-3546): add endpoint to fetch customer data #1127

ChristianMcc · 2025-01-03T16:05:24Z

Introduction ✏️

Create an endpoint and new ODS service/controller/module to query ODS for the customer.
Added types and functionality for new endpoints for other ODS entities to be fetched in the future.

All entities from ODS will be fetched in a similar way using the same stored procedure hence lumping it all together in a single ODS service, we could look at splitting up controllers and services in the future if we feel they are getting too bloated.

No full payload validation has been done of the response from the ODS stored procedures - only validation of the fields that is required for the endpoint.

An API test will be added separately to this PR in a following PR

… and update .env.sample

…ure calls

ChristianMcc · 2025-01-06T14:59:14Z

.github/workflows/test.yml

@@ -10,7 +10,7 @@ run-name: Executing test QA on ${{ github.repository }} 🚀

 on:
  pull_request:
-    branches: [main]
+    branches: [main, feat/FN-3543/integrate-with-ods]


temporarily leaving this in while the feature branch is open to run tests, will remove before feature branch is merged to main

ChristianMcc · 2025-01-06T15:00:09Z

src/modules/ods/dto/get-ods-customer-query.dto.ts

+    pattern: regexToString(UKEFID.PARTY_ID.REGEX),
+  })
+  @Matches(UKEFID.PARTY_ID.REGEX)
+  public customerUrn: string;


The endpoint and ODS refer to the entity as customer so I've used customer URN instead of party URN here but I'm happy to change back to partyUrn if people think that is more logical/consistent

Opted to just call it urn while in the namespace of customer

ChristianMcc · 2025-01-06T15:00:22Z

src/modules/ods/dto/get-ods-customer-response.dto.ts

+  @ApiProperty({
+    description: 'The unique UKEF id of the customer',
+  })
+  readonly customerUrn: string;


ditto on customerUrn comment on the other dto

ChristianMcc · 2025-01-06T15:01:26Z

src/modules/ods/ods.controller.test.ts

+import { OdsController } from './ods.controller';
+import { OdsService } from './ods.service';
+
+describe('OdsController', () => {


I wanted a test to check the validation of the dto (e.g. valid when a urn matching the regex is passed through) but that would require setting up a validationPipe for this test which no other test in the codebase has done. For now I'm going to leave out to stay consistent with the other test suites but one to look at across the board

At the moment DTO validations are tested as part of api/e2e tests (root/tests). Most likely these tests are not part of github pipelines yet.

Would be nice to create api/e2e test for osd.

Yep will add this in a new PR straight after this one (don't have a tonne of time today/tomorrow but keen to get my feature branch tested in dev so have split up the work)

ChristianMcc · 2025-01-06T15:03:15Z

src/modules/ods/ods.service.ts

+@Injectable()
+export class OdsService {
+  constructor(
+    @InjectDataSource(DATABASE.ODS)


This black box magic of adding the name of the data source to pick the right one is amazing, I don't fully understand it but I like it

ChristianMcc · 2025-01-06T15:05:02Z

src/modules/ods/ods.service.ts

+        customerName: storedProcedureJson.results[0]?.customer_name,
+      };
+    } catch (err) {
+      if (err instanceof NotFoundException) {


the catching and rethrowing feels a little clunky here but is the best way to get better logging for debugging which I think is worth it. Any recommendations on refactoring this are welcome

Its not ideal but to be honest, I can see reminisense of this pattern elsewhere in the codebase. Ideally an exceptional class will be clean but might creep the scope of this PR.

ChristianMcc · 2025-01-06T15:06:34Z

src/modules/ods/ods.service.ts

+    const queryRunner = this.odsDataSource.createQueryRunner();
+    try {
+      // Use the query runner to call a stored procedure
+      const result = await queryRunner.query(


Unfortuantely typeorm doesn't support any calling of stored procedures outside of just a SQL query so I've written a parameterised SQL query to stop SQL injection but has to be hard coded like this

.github/workflows/deployment.yml

src/modules/ods/dto/get-ods-customer-response.dto.ts

src/modules/ods/dto/ods-payloads.dto.ts

src/modules/ods/ods.controller.test.ts

src/modules/ods/ods.module.ts

src/modules/ods/ods.service.test.ts

src/modules/ods/ods.service.ts

…cumentation and small fixes

sonarqubecloud · 2025-01-15T15:49:30Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ChristianMcc · 2025-01-15T15:50:09Z

src/modules/ods/ods.service.test.ts

+
+      expect(mockDataSource.createQueryRunner).toHaveBeenCalled();
+      expect(mockQueryRunner.query).toHaveBeenCalledWith(
+        expect.stringMatching(


I didn't want to have to match indentation in the string matching so there is a regex match instead. Thoughts @toddym42 ?

Not sure I can think of anything better if we want to match the whole string, but I'm wondering if there's much value asserting on the whole thing, as it's just a hard-coded value that we're repeating. Could just expect any string, or something more minimal like expect.stringContaining('EXEC t_apim.sp_ODS_query')?

toddym42 · 2025-01-16T14:18:40Z

src/modules/ods/ods.controller.ts

+  @ApiResponse({
+    status: 200,
+    description: 'Customers matching search parameters',
+    type: GetOdsCustomerResponse,
+  })


Not requesting to change, but just an fyi that there is a more specific @ApiOkResponse that we could use here, like with the error responses.

toddym42 · 2025-01-16T14:30:15Z

src/modules/ods/ods.service.test.ts

+
+      expect(mockDataSource.createQueryRunner).toHaveBeenCalled();
+      expect(mockQueryRunner.query).toHaveBeenCalledWith(
+        expect.stringMatching(


Not sure I can think of anything better if we want to match the whole string, but I'm wondering if there's much value asserting on the whole thing, as it's just a hard-coded value that we're repeating. Could just expect any string, or something more minimal like expect.stringContaining('EXEC t_apim.sp_ODS_query')?

toddym42 · 2025-01-16T14:31:19Z

src/modules/ods/ods.service.test.ts

+  });
+
+  describe('findCustomer', () => {
+    it('findCustomer should return a customer the customer urn and name when findCustomer is called', async () => {


Suggested change

it('findCustomer should return a customer the customer urn and name when findCustomer is called', async () => {

it('should return a customer, the customer urn, and name when findCustomer is called', async () => {

Missed this one.

toddym42 · 2025-01-16T14:33:15Z

src/modules/ods/ods.service.ts

+
+      const storedProcedureJson: OdsStoredProcedureOuputBody = JSON.parse(storedProcedureResult[0]?.output_body);
+
+      if (storedProcedureJson === undefined || storedProcedureJson?.status != 'SUCCESS') {


Suggested change

if (storedProcedureJson === undefined || storedProcedureJson?.status != 'SUCCESS') {

if (storedProcedureJson === undefined || storedProcedureJson?.status !== 'SUCCESS') {

(sorry I missed this first time round 😅)

Can it not just be storedProcedureJson?.status !== 'SUCCESS', is there a reason for an explicity undefined comparison?

It was there for code readability showing that it can be undefined rather than any required programmatical check, I'm happy to remove

abhi-markan

Nice work, few minor and medium comments to address please.

abhi-markan · 2025-01-20T08:47:06Z

src/modules/ods/dto/get-ods-customer-param.dto.ts

+  @ApiProperty({
+    required: true,
+    example: CUSTOMERS.EXAMPLES.PARTYURN,
+    description: 'The unique UKEF id of the customer to search for.',


Suggested change

description: 'The unique UKEF id of the customer to search for.',

description: 'The unique UKEF ID of the customer to search for.',

I know this exists else where, will raise a seprate PR to rectify this minor typo.

abhi-markan · 2025-01-20T08:47:25Z

src/modules/ods/dto/get-ods-customer-param.dto.ts

+    pattern: regexToString(UKEFID.PARTY_ID.REGEX),
+  })
+  @Matches(UKEFID.PARTY_ID.REGEX)
+  public urn: string;


Should this be a number?

Because a lot of urns start with 00 we don't want to cast this to number, has to stay a string

abhi-markan · 2025-01-20T08:47:37Z

src/modules/ods/dto/get-ods-customer-response.dto.ts

+
+export class GetOdsCustomerResponse {
+  @ApiProperty({
+    description: 'The unique UKEF id of the customer',


Suggested change

description: 'The unique UKEF id of the customer',

description: 'The unique UKEF ID of the customer',

abhi-markan · 2025-01-20T08:48:03Z

src/modules/ods/dto/get-ods-customer-response.dto.ts

+    description: 'The unique UKEF id of the customer',
+    example: '00312345',
+  })
+  readonly urn: string;


ditto other comment: Because a lot of urns start with 00 we don't want to cast this to number, has to stay a string

abhi-markan · 2025-01-20T08:49:41Z

src/modules/ods/dto/ods-payloads.dto.ts

+};
+
+export const ODS_ENTITIES = {
+  CUSTOMER: 'customer',


Is there a constant for this? If not then don't worry will have to be a separate PR.

This is a constant I've defined to have all the different entities that can be queried in ODS

abhi-markan · 2025-01-20T09:02:27Z

src/modules/ods/ods.service.ts

+        customerName: storedProcedureJson.results[0]?.customer_name,
+      };
+    } catch (err) {
+      if (err instanceof NotFoundException) {


Its not ideal but to be honest, I can see reminisense of this pattern elsewhere in the codebase. Ideally an exceptional class will be clean but might creep the scope of this PR.

abhi-markan · 2025-01-20T09:08:02Z

src/modules/ods/ods.service.ts

+   */
+  async callOdsStoredProcedure(storedProcedureInput: OdsStoredProcedureInput): Promise<OdsStoredProcedureOutput> {
+    const queryRunner = this.odsDataSource.createQueryRunner();
+    try {


Do you not need await queryRunner.connect();?

good catch!

abhi-markan · 2025-01-20T09:08:54Z

src/modules/ods/ods.service.ts

+        [JSON.stringify(storedProcedureInput)],
+      );
+
+      return result;


Suggested change

return result;

// Return the data or falsy

return result[0]?.output_body || null;

abhi-markan · 2025-01-20T09:09:06Z

src/modules/ods/ods.service.ts

+      );
+
+      return result;
+    } finally {


Missed catch block.

I didnt' have a catch block here as it'll be caught in the function above - I can add one which will just be:

catch (error) { throw error }

It won't add or do anything but if it's a pattern we want to stick to will add it

abhi-markan · 2025-01-20T09:11:00Z

src/modules/ods/ods.service.ts

+        EXEC t_apim.sp_ODS_query @input_body=@0, @output_body=@output_body OUTPUT;
+        SELECT @output_body as output_body
+      `,
+        [JSON.stringify(storedProcedureInput)],


Is storedProcedureInput validated before passing it to the query?

Does SP has enought validation to ensure no malcious code wil be executed.

Does SP has correct ACL i.e. role management?

Error handling should also be sanitized to ensure error messages do not expose anything sensitive back to the client.

we create storedProcedureInput from code and from a validated input to the endpoint (urn).

we use parameterisation to stop SQL injection

The user (service principal) calling this stored procedure has a very tight ACL, it can only run this stored procedure and query one logs table, it can't do anything else

None of our error handling uses the output of the stored procedure, we only log that to Azure server logs

Christian McCaffery added 8 commits December 17, 2024 10:32

feat(FN-6804): setup new ODS repo

b968c06

feat(FN-6804): add env variables to deployment script

dfbadbc

feat(FN-6804): remove unnecessary comments

1c2dd48

feat(FN-6804): remove FF reference

1b92da1

feat(GIFT-6804): align module naming capitalisation, include ODS port…

b684f32

… and update .env.sample

feat(GIFT-3546): add ODS module service and controller

994a243

feat(GIFT-3546): add types and refactor code for future stored proced…

59f584d

…ure calls

feat(GIFT-3546): WIP add tests

4fc4b0c

ChristianMcc had a problem deploying to qa January 3, 2025 16:05 — with GitHub Actions Failure

feat(GIFT-3546): update ods.service unit tests

efd22ed

ChristianMcc had a problem deploying to qa January 3, 2025 17:45 — with GitHub Actions Failure

feat(GIFT-3546): remove test that uses validation pipe

44331c3

ChristianMcc temporarily deployed to qa January 6, 2025 14:49 — with GitHub Actions Inactive

ChristianMcc commented Jan 6, 2025

View reviewed changes

feat(GIFT-3546): update test

78446fb

ChristianMcc temporarily deployed to qa January 6, 2025 15:23 — with GitHub Actions Inactive

feat(GIFT-3546): add some documentation and update param names

27fcf9d

ChristianMcc had a problem deploying to qa January 8, 2025 10:08 — with GitHub Actions Failure

feat(GIFT-3546): update documentation

1f34c07

ChristianMcc had a problem deploying to qa January 8, 2025 13:38 — with GitHub Actions Failure

feat(GIFT-3546): update tests for new response shape

388e3bb

ChristianMcc had a problem deploying to qa January 8, 2025 13:45 — with GitHub Actions Failure

feat(GIFT-3546): fix tests

9d57205

ChristianMcc temporarily deployed to qa January 8, 2025 13:49 — with GitHub Actions Inactive

toddym42 reviewed Jan 10, 2025

View reviewed changes

feat(GIFT-3546): update types, refer to data engineering types and do…

6857a12

…cumentation and small fixes

ChristianMcc had a problem deploying to qa January 15, 2025 11:12 — with GitHub Actions Failure

feat(GIFT-3546): update tests

dc437ae

ChristianMcc temporarily deployed to qa January 15, 2025 15:49 — with GitHub Actions Inactive

ChristianMcc commented Jan 15, 2025

View reviewed changes

toddym42 approved these changes Jan 16, 2025

View reviewed changes

abhi-markan requested changes Jan 20, 2025

View reviewed changes

	it('findCustomer should return a customer the customer urn and name when findCustomer is called', async () => {
	it('should return a customer, the customer urn, and name when findCustomer is called', async () => {


		const storedProcedureJson: OdsStoredProcedureOuputBody = JSON.parse(storedProcedureResult[0]?.output_body);

		if (storedProcedureJson === undefined \|\| storedProcedureJson?.status != 'SUCCESS') {

	description: 'The unique UKEF id of the customer to search for.',
	description: 'The unique UKEF ID of the customer to search for.',

	return result;
	// Return the data or falsy
	return result[0]?.output_body \|\| null;

feat(GIFT-3546): add endpoint to fetch customer data #1127

Are you sure you want to change the base?

feat(GIFT-3546): add endpoint to fetch customer data #1127

Conversation

ChristianMcc commented Jan 3, 2025 • edited Loading

Introduction ✏️

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

sonarqubecloud bot commented Jan 15, 2025

Quality Gate passed

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

abhi-markan left a 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

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

ChristianMcc commented Jan 3, 2025 •

edited

Loading