Skip to content

Conversation

@SamAinsworth-NHS
Copy link
Contributor

@SamAinsworth-NHS SamAinsworth-NHS commented Dec 1, 2025

Description

Introduce robust key conversion for data service Delete/GetById/Put operations so GUID primary keys are handled correctly.

  • Update RequestHandler.CreateGetByKeyExpression(...) to:
    • Derive the key’s underlying type (handles nullable keys).
    • Convert the incoming key string to the correct type via a new ConvertKeyString(...) helper.
    • Special-case Guid, Enum, DateTime, and string; fall back to Convert.ChangeType with invariant culture.
    • Compare using the underlying type for nullable keys.
  • Add using System.Globalization; for invariant conversions.

File changed:

  • application/CohortManager/src/Functions/Shared/DataServices.Core/RequestHandler.cs

Context

After the ServiceNow cases table switched to a GUID primary key (Id), Playwright cleanup started failing on DELETE. The generic handler used Convert.ChangeType for key conversion, which does not support Guid, causing DELETE to return non‑2xx and expect(deleteResponse.ok()).toBeTruthy() to fail. This change parses GUIDs properly, restoring DELETE behavior for ServiceNowCasesDataService and any other data service that may use GUID keys in future.

Supports the change in https://nhsd-jira.digital.nhs.uk/browse/DTOSS-11698

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Refactoring (non-breaking change)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I am familiar with the contributing guidelines
  • I have followed the code style of the project
  • I have added tests to cover my changes
    • Rationale: Existing Playwright smoke tests exercise the cleanup path that was failing; they serve as coverage for this change. Happy to add a focused unit test for CreateGetByKeyExpression with a GUID key as a follow-up.
  • I have updated the documentation accordingly
    • Not applicable — internal handler behavior change only.
  • This PR is a result of pair or mob programming

Sensitive Information Declaration

To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.

  • I confirm that neither PII/PID nor sensitive data are included in this PR and the codebase changes.

@github-actions
Copy link

github-actions bot commented Dec 1, 2025

Unit Test Results

✔️ Tests 887 / 887 - passed in 71.3s
📝 Coverage 46.78%
📏 4528 / 9921 lines covered 🌿 1095 / 2100 branches covered
🔍 click here for more details

✏️ updated for commit 0e4f12a

@SamAinsworth-NHS SamAinsworth-NHS changed the title fix: updated the shared data service handler so DELETE works with GUI… fix: updated the shared data service handler so DELETE works with GUID keys Dec 1, 2025
@SamAinsworth-NHS SamAinsworth-NHS marked this pull request as ready for review December 1, 2025 17:20
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 1, 2025

@SamAinsworth-NHS SamAinsworth-NHS changed the title fix: updated the shared data service handler so DELETE works with GUID keys fix: DTOSS-11698 updated the shared data service handler so DELETE works with GUID keys Dec 2, 2025
@SamAinsworth-NHS SamAinsworth-NHS added this pull request to the merge queue Dec 2, 2025
Merged via the queue into main with commit 0d89590 Dec 2, 2025
67 checks passed
@SamAinsworth-NHS SamAinsworth-NHS deleted the Fix/playwright-tests-service-now-cases-guid branch December 2, 2025 09:52
@stephhou stephhou added the Deployment 1.0.4 All PRs assigned for deployment 1.0.4 - planned for 18/12/2025 label Dec 12, 2025
@rfk-nc rfk-nc added Deployment 1.0.3 and removed Deployment 1.0.4 All PRs assigned for deployment 1.0.4 - planned for 18/12/2025 labels Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants