Improve Import Error Handling for Database Constraint Violations

[v-shahzad]

Description

This change introduces a pre-flight validation step within the $import data pipeline to proactively detect and report database constraint violations on a per-resource basis.

Related issues

Addresses AB# 151709

Testing

For now it just contains sample change for one constraint to get approval for the approach

FHIR Team Checklist

• Update the title of the PR to be succinct and less than 65 characters
• Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
• Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
• Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
• Tag the PR with Schema Version backward compatible or Schema Version backward incompatible or Schema Version unchanged if this adds or updates Sql script which is/is not backward compatible with the code.
• When changing or adding behavior, if your code modifies the system design or changes design assumptions, please create and include an ADR.
• CI is green before merge
• Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

[SergeyGaluzo]

Adding checks before calling merge adds extra load on app compute for each normal merge call. I suggest running this check only if merge fails with constraint violation. This way there should be zero effect on performance for case when there are not constraint errors.

I do not like having any code duplicated (C# constraint code and the database). Can we get away with reporting some details of SQL error and attributing it to the resource lines in the input file for the batch we processed? This might not be that precise, but we will not have any code duplication.

We also should abort import on first constraint error and should not spend app compute on analyzing all the input resources.

Please add a test for this functionality.

[v-shahzad]

Hi @SergeyGaluzo , Thanks for the detailed suggestions. I’ll start implementing these changes accordingly.

[SergeyGaluzo]

Hi @SergeyGaluzo , Thanks for the detailed suggestions. I’ll start implementing these changes accordingly.

@v-shahzad I would consider "no code duplication" approach if possible. This sounds very attractive,

[SergeyGaluzo]

@v-shahzad Please remove any constraint validation from C#.

[v-shahzad]

Hi @SergeyGaluzo , I have added the constraint details to the first resource of the batch with the format

Error on batch with offset 0 rows from 1 to 3. Insert failed due to the constraint 'CHK_TokenSearchParam_CodeOverflow' on 'TokenSearchParam'. Details: (len([Code])=(256) OR [CodeOverflow] IS NULL).

Please suggest me if it matches your expectation or if you would like any changes.
Thank you

[SergeyGaluzo]

Hi @SergeyGaluzo , I have added the constraint details to the first resource of the batch with the format

Error on batch with offset 0 rows from 1 to 3. Insert failed due to the constraint 'CHK_TokenSearchParam_CodeOverflow' on 'TokenSearchParam'. Details: (len([Code])=(256) OR [CodeOverflow] IS NULL).

Please suggest me if it matches your expectation or if you would like any changes. Thank you

@v-shahzad This is much better.

1. The problem I see in adding to the first resource in a batch is a potential confusion for the customer. They might think that error is on that specific resource. Do we need to add any resource level errors at all? Can't we just raise exception and not record errors in the import error log?
2. We should avoid putting constraint and table names into the error message. In this case we know that it is a search parameter of type token, so something like this "Insert failed due to the constraint violation for token search parameter. Details: (len(Code]=(256) OR CodeOverflow IS NULL)". You can remove [] to simplify reading.

[v-shahzad]

Hi @SergeyGaluzo , I have added the constraint details to the first resource of the batch with the format
Error on batch with offset 0 rows from 1 to 3. Insert failed due to the constraint 'CHK_TokenSearchParam_CodeOverflow' on 'TokenSearchParam'. Details: (len([Code])=(256) OR [CodeOverflow] IS NULL).
Please suggest me if it matches your expectation or if you would like any changes. Thank you

@v-shahzad This is much better.

1. The problem I see in adding to the first resource in a batch is a potential confusion for the customer. They might think that error is on that specific resource. Do we need to add any resource level errors at all? Can't we just raise exception and not record errors in the import error log?
2. We should avoid putting constraint and table names into the error message. In this case we know that it is a search parameter of type token, so something like this "Insert failed due to the constraint violation for token search parameter. Details: (len(Code]=(256) OR CodeOverflow IS NULL)". You can remove [] to simplify reading.

Thanks for the feedback, @SergeyGaluzo . I’ve made the suggested changes. Please let me know if any further updates are needed.

[SergeyGaluzo]

Please make sure that only added lines are marked as changed in diff.

[SergeyGaluzo]

Please add import tests for all expected types of constraint violations.

[SergeyGaluzo]

We do not plan to have foreign keys, please remove this section from SQL query.

[SergeyGaluzo]

Please evaluate whether violation of PK/Unique constraints are possible in import. If not, remove corresponding section from SQL query.

[v-shahzad]

Hi @SergeyGaluzo , I’ve removed the checks for primary key and foreign key constraints since we’re only catching exceptions related to overflow constraints. I’ll create a separate user story and PR for the test cases.