Dictionary Migration - Get migration endpoints by leoraba · Pull Request #200 · overture-stack/lyric

leoraba · 2026-04-16T17:25:08Z

Description

This PR creates 3 new GET endpoints to retrieve migrations information.

GET /migration/category/{categoryId}:
GET /migration/{migrationId}
GET /migration/{migrationId}/records

Related tickets:

This PR depends on:

…mentation

…on_db

…mentation

…on_db

…mentation

…endpoints

…mentation

…endpoints

joneubank

Minor changes here. Good work.

joneubank · 2026-04-23T11:59:56Z

+        retries:
+          type: number
+          description: Number of retries of the migration


Do we explain "retries" anywhere? this is a confusing data point.

I've updated the description for this field "Number of times the migration has run. When a migration fails, it can be retried by registering again the same dictionary."

However, a proper documentation is added in the following PR, where the "retry" functionality is implemented

joneubank · 2026-04-23T12:02:07Z

+	const defaultPage = 1;
+	const defaultPageSize = 20;


Maybe should be global constants defined with the pagination types?

moved this constants to a new shared file.

joneubank · 2026-04-23T12:04:03Z

+			try {
+				const migrationId = Number(req.params.migrationId);
+
+				logger.info(LOG_MODULE, `Request Migration id '${migrationId}'`);


should probably be debug level

This comment applies this to all similar request detail logging in this controller.

we add a info log for every request in every controller because we're not using express request logger.

joneubank · 2026-04-23T12:04:03Z

+				if (!submission) {
+					throw new NotFound('Submission not found');


should we be logging that the migration was not found?

👍 code changed as suggested

joneubank · 2026-04-23T12:09:04Z

+				if (submissions.result.length === 0) {
+					throw new NotFound('Submissions not found');
+				}


If the category ID exists, this should return an empty list.

If the category ID does not exist, then it should return NotFound.

code changed to return NotFound when category does not exists, also changed to return same error in other endpoints of this controller

joneubank · 2026-04-23T14:03:04Z

 		}
 	};

+	const getMigrationById = async (migrationId: number): Promise<MigrationRecordWithRelations | undefined> => {


Similar comment as previous... this needs to explain why it sometimes returns undefined instead of the returned type.

While I prefer using undefined for missing values, the rest of the Service functions return null instead. It is confusing to have both values used in an ad-hoc way. Let's consistently use either null or undefined

for consistency, will return null when migration is not found, and will update tsdocs

joneubank · 2026-04-23T14:03:39Z

+	const getMigrationsByCategoryId = async (
+		categoryId: number,
+		paginationOptions: PaginationOptions,
+	): Promise<{ metadata: { totalRecords: number; errorMessage?: string }; result: MigrationRecordWithRelations[] }> => {


Might be useful to create a type alias for this return and export it for use where this function is consumed.

added a type PaginatedResult<T> to be used in this and other functions

joneubank · 2026-04-23T14:07:50Z

+		} catch (error) {
+			logger.error(LOG_MODULE, `Error retrieving migrations for categoryId '${categoryId}'`, error);
+			throw error;
+		}


This can be a troubling pattern to fall into, where every function that could throw an error just logs the error then throws it. You can get a long log where every function in the call stack logs the same error with no new information added. Generally, we want to log errors where we catch and handle them.

We could instead make a design decision here, or in any function within this call stack, that the caught error can be handled here instead of thrown again. We can replace our return value either with void return type if there is exactly only one expected failure condition, or replace the return type with a Result that has a list of known failure conditions.

Removed the try-catch here as the error is unhandled in this function, it should be handled by the upper function (controller) that uses this function, repeated this logic in other functions in this Service.

joneubank · 2026-04-23T14:08:14Z

+			organizations?: string | string[];
+			isInvalid?: boolean;
+		},
+	): Promise<{ result: AuditDataResponse[]; metadata: { totalRecords: number; errorMessage?: string } }> => {


Another place for a helpful type alias.

joneubank · 2026-04-23T14:21:57Z

+ * @param migration
+ * @returns
+ */
+export const formatMigrationSummary = (migration: MigrationSummary): MigrationSummary => ({


I assume this is to make the return type from the route nicely formatted for presentation. If this is important to enforce, there should be a unit test for this on the route.

Don't spend much time on this next comment, but: this may be a good use for branded types. You can create a Branded type MigrationSummaryFormatted and have the controller require that it returns this branded type, that will enforce that this function is called before returning. This will enforce the rule through the type system, but since its only used for the route return type there's not a lot gained for the amount of effort here, so its not worth doin. Writing a test is the best way to enforce this.

for the purpose of this PR, I added unit test for this function to verify it, I would considerate using branded types as suggested in future PRs

…tation

…endpoints

…oints

leoraba · 2026-04-30T18:40:16Z

unit tests added for migration formatter functions

leoraba · 2026-05-04T12:58:11Z

PR has been updated based on the feedback received, it is now ready for code review.

JamesTLopez · 2026-05-04T17:16:00Z

+					records: migrationsResult.result.map(formatMigrationSummary),
+				};
+
+				return res.send(response);


We should send a status 200

added .status(200) in the response

JamesTLopez · 2026-05-04T17:25:59Z

-		 * @param {string} filterOptions.systemId
+		 * and additional filters
+		 * @param {number} categoryId Category ID to filter the Audit records
+		 * @param {object} filterOptions Additional filters and pagination options


Instead of {object} AuditFilterOptions might be better for the tsDocs

Responding to James' comment:

Instead of {object} AuditFilterOptions might be better for the tsDocs

Alternately, TS captures our function interfaces. It is entirely acceptable to remove the param annotations, or just the type information, from the TSDoc. These properties are useful if there is something about the parameter that needs to be described - like maybe it is used in a confusing way by the function logic and the caller needs to know that.

I just removed the type in the tsdocs to avoid unnecessary information in the docs

JamesTLopez · 2026-05-04T17:31:43Z

 		status: Extract<MigrationStatus, 'COMPLETED' | 'FAILED'>;
 		userName: string;
-	}): Promise<Result<null, string>> => {
+	}): AsyncResult<null, string> => {


Should we use the AsyncResult pattern for the remainder of the services in this file? Or is this only used for create/update actions?

Long term, I would like to. Not required for scope for this PR.

for simplicity just using the AsyncResult pattern in the actionables functions: initiate, perform and finalize migrations.

JamesTLopez · 2026-05-04T17:36:46Z

+
+/**
+ * Order the properties of the Migration Summary Record
+ * @param migration


Suggested change

* @param migration

* @param {MigrationSummary} migration - The migration summary to format.

docs updated as suggested

joneubank

This looks very good. Thanks!

joneubank · 2026-05-11T14:06:31Z

+    Pagination:
+      type: object
+      properties:
+        currentPage:
+          type: number
+        pageSize:
+          type: number
+        totalPages:
+          type: number
+        totalRecords:
+          type: number


joneubank · 2026-05-11T14:10:37Z

+        status:
+          type: string
+          description: Status of the Migration
+          enum: ['IN-PROGRESS', 'COMPLETED', 'FAILED']


status reports that the migration completed... is there anythign that indicates if any records have errors after the migration?

there is invalidRecords I missed updating the openapi docs, now it reflects the right response

joneubank · 2026-05-11T14:15:03Z

+					records: migrationsResult.result.map(formatMigrationSummary),
+				};
+
+				return res.send(response);


Really minor point, but we should be using res.json(response) instead of .send.

They will be almost identical, or even exactly identical, in most cases. The point though is to indicate that we INTEND to be responding with JSON. If our response is not an object (like a string) then .send(someString) will happily send a response that does not have Content-Type application/json, and TS will not prevent us from returning this value.

There's also some weird cases where if you do send(400) it will set the 400 as the http status and send an empty body. Its best to avoid the "smart" logic in .send and use .json when you intend to return json.

code change to respond with .json(response) as suggested.

joneubank · 2026-05-11T14:16:07Z

+		if (organization && !isEmptyString(organization)) {
+			filterArray.push(eq(auditSubmittedData.organization, organization));
+		}
+
+		if (submissionId) {
+			filterArray.push(eq(auditSubmittedData.submissionId, submissionId));
+		}
+
+		if (newIsValid !== undefined) {
+			filterArray.push(eq(auditSubmittedData.newDataIsValid, newIsValid));
+		}


Responding to:

The integration test is broken due to a previous change in the worker pool, it requires additional work out of the scope of this PR and should be fixed in a separate PR and extended to cover this function

Can you create an issue and link it?

joneubank · 2026-05-11T14:17:51Z

-		 * @param {string} filterOptions.systemId
+		 * and additional filters
+		 * @param {number} categoryId Category ID to filter the Audit records
+		 * @param {object} filterOptions Additional filters and pagination options


Responding to James' comment:

Instead of {object} AuditFilterOptions might be better for the tsDocs

Alternately, TS captures our function interfaces. It is entirely acceptable to remove the param annotations, or just the type information, from the TSDoc. These properties are useful if there is something about the parameter that needs to be described - like maybe it is used in a confusing way by the function logic and the caller needs to know that.

joneubank · 2026-05-11T14:19:31Z

+					};
+				}
+
+				const newIsValid = options.isInvalid !== undefined ? !options.isInvalid : undefined;


I forgot this. Thanks for clarification.

joneubank · 2026-05-11T14:21:12Z

 		status: Extract<MigrationStatus, 'COMPLETED' | 'FAILED'>;
 		userName: string;
-	}): Promise<Result<null, string>> => {
+	}): AsyncResult<null, string> => {


Long term, I would like to. Not required for scope for this PR.

leoraba added 23 commits January 9, 2026 11:10

database changes

74d088a

update dbml

28f683a

migration services

e1ada63

updates migration repository

c1f87dd

Merge branch 'feat/dictionary_migration_db' into feat/migration_imple…

118fc42

…mentation

update migration service

0a26540

data validation

1ec4dc4

refactor migration process

afc4f87

block commit submission

58d4ad8

fix sort imports

b081dd9

Merge branch 'feat/dictionary_migration' into feat/dictionary_migrati…

e6dc14d

…on_db

Merge branch 'feat/dictionary_migration_db' into feat/migration_imple…

608c096

…mentation

Merge branch 'feat/dictionary_migration' into feat/dictionary_migrati…

b23d648

…on_db

update docs

8688240

migration table index

2487bbe

Merge branch 'feat/dictionary_migration_db' into feat/migration_imple…

96b6c63

…mentation

fix typos and logs

d351b2e

migration audit and logs

fb05263

migration on worker thread

b5d6756

GET migration endpoints

f5af236

fix unit tests

d7d17d0

Merge branch 'feat/migration_implementation' into feat/get_migration_…

2d05348

…endpoints

GET migration records

2758202

leoraba changed the base branch from main to feat/migration_implementation April 16, 2026 17:26

leoraba linked an issue Apr 16, 2026 that may be closed by this pull request

[Part 2 - Data migration] Get Migrations endpoints #176

Open

leoraba requested review from JamesTLopez and joneubank April 16, 2026 17:32

leoraba added 3 commits April 17, 2026 14:34

adding ts config noUncheckedIndexedAccess

b8cd961

rename migration enum status IN_PROGRESS

cdf32cc

Merge branch 'feat/dictionary_migration_db' into feat/migration_imple…

2c889d1

…mentation

Merge branch 'feat/migration_implementation' into feat/get_migration_…

430ea27

…endpoints

joneubank requested changes Apr 23, 2026

View reviewed changes

leoraba added 8 commits April 27, 2026 10:19

Merge branch 'feat/dictionary_migration' into feat/migration_implemen…

c512349

…tation

refactoring migration function with Result

f5994eb

Merge branch 'feat/migration_implementation' into feat/get_migration_…

280395f

…endpoints

default constants pagination

c682a47

change logs and response NotFound

b41d1ad

fix swagger docs

6f231ce

include errors or migration changes

4f7a01b

type paginated result

57cf05b

Base automatically changed from feat/migration_implementation to feat/dictionary_migration April 30, 2026 04:22

leoraba added 3 commits April 30, 2026 00:30

Merge branch 'feat/dictionary_migration' into feat/get_migration_endp…

8f69160

…oints

using type paginated result

a1c4d8f

unit test migration formatter functions

e588d0b

leoraba commented Apr 30, 2026

View reviewed changes

remove unhandled error thrown

d8b930d

leoraba requested a review from joneubank May 4, 2026 13:43

leoraba mentioned this pull request May 4, 2026

retry dictionary registration #201

Open

JamesTLopez reviewed May 4, 2026

View reviewed changes

leoraba mentioned this pull request May 4, 2026

Dictionary Migration - Integration tests #202

Draft

joneubank approved these changes May 11, 2026

View reviewed changes

leoraba added 3 commits May 12, 2026 09:12

return 200 json response

16c510d

tsdocs updates

720e248

swagger docs update

0128889

leoraba merged commit 23a3f96 into feat/dictionary_migration May 12, 2026

leoraba deleted the feat/get_migration_endpoints branch May 12, 2026 16:56

		if (!submission) {
		throw new NotFound('Submission not found');

	* @param migration
	* @param {MigrationSummary} migration - The migration summary to format.

Conversation

leoraba commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related tickets:

This PR depends on:

Uh oh!

joneubank left a comment

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!

leoraba commented May 4, 2026

Uh oh!

JamesTLopez May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

joneubank May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

joneubank left a comment

Choose a reason for hiding this comment

leoraba commented Apr 16, 2026 •

edited

Loading

JamesTLopez May 4, 2026 •

edited

Loading

joneubank May 11, 2026 •

edited

Loading

joneubank May 11, 2026 •

edited

Loading

joneubank May 11, 2026 •

edited

Loading