feat(bulk-import): adding sorting based on column in /imports #178

its-mitesh-kumar · 2024-12-13T11:18:32Z

Resolves : https://issues.redhat.com/browse/RHIDP-5295

rhdh-gh-app · 2024-12-13T11:18:52Z

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@red-hat-developer-hub/backstage-plugin-bulk-import-backend	workspaces/bulk-import/plugins/bulk-import-backend	patch	`v5.2.1`
@red-hat-developer-hub/backstage-plugin-bulk-import	workspaces/bulk-import/plugins/bulk-import	patch	`v1.10.7`

christoph-jerolimov

Hi @its-mitesh-kumar, thanks for picking up also the backend work.

I added some comments, that IMHO we should remove the sorting in our backend, and add sorting params to the request calls for GitHub / Catalog API calls.

Let me know if I miss something and its not possible for some reason. And let the backend team and me know if you want continue with this.

christoph-jerolimov · 2024-12-16T11:24:20Z

workspaces/bulk-import/plugins/bulk-import/src/types/types.ts

+export enum AddedRepositoryColumnNameEnum {
+  repoName = 'repository.name',
+  organizationUrl = 'repository.organization',
+  repoUrl = 'repository.url',
+  catalogInfoYamlLastUpdated = 'lastUpdate',
+  catalogInfoYamlStatus = 'status',
+}
+export enum SortingOrderEnum {
+  ASC = 'asc',
+  DESC = 'desc',
+}


Please respect the convention in this file

and add empty lines between each type definition

and it looks like enum keys here are all UPPER_CASE

christoph-jerolimov · 2024-12-16T11:27:26Z

workspaces/bulk-import/plugins/bulk-import-backend/src/service/handlers/import/bulkImports.ts

  // sorting the output to make it deterministic and easy to navigate in the UI
-  sortImports(imports);
-
+  if (sortColumn) {
+    sortImports(imports, sortColumn, sortOrder);
+  }


Are we always fetching ALL elements in the backend?

And guess that we are and should not do that. So this sort function will only sort the current window (fetched page).

Instead of that we should pass the sorting to the backend calls like GitHub and the catalog API and remove the sorting from our backend.

Yes, it would be the best if we could sort and paginate at the Github side. Unfortunately, for /imports endpoint, I believe we are fetching all Catalog locations and then filtering those reachable with GH integrations. @rm3l might have valuable insights into the reasoning behind this solution and whether we can approach this more effectively. Maybe we can adjust filterLocationsAccessibleFromIntegrations here.

When fetching Catalog locations, pageNumber and pageSize are passed as params but not really used there, since we don't know which ones are actually reachable GH integrations. Maybe the best approach would be to switch stuff around and first fetch GH integrations, maybe it would help performance.

Indeed, IIRC, the /imports endpoint fetches Catalog locations first and only keeps those reachable from the configured GH integrations. Initially, it was querying GH first, but this was causing major performance issues, as reported in https://issues.redhat.com/browse/RHIDP-3680

Also, note that the imports may be coming from different sources (app-config, repos registered manually in Backstage, auto-discovery, ...), and the user may have configured several GH integrations (like different GH apps installed in multiple orgs).
That's essentially why the backend needed to aggregate such various sources to properly implement not only sorting, but also search and pagination, and return what the user is requesting.
But I guess pagination should help reduce the number of data fetched and iterated over.

That being said, open to better suggestions.

christoph-jerolimov · 2024-12-16T11:28:07Z

workspaces/bulk-import/plugins/bulk-import-backend/src/service/handlers/import/bulkImports.ts

+function sortImports(
+  imports: Components.Schemas.Import[],
+  sortColumn: string = 'repository.name',
+  sortOrder: string = SortingOrderEnum.ASC,
+) {
  imports.sort((a, b) => {
-    if (a.repository?.name === undefined && b.repository?.name === undefined) {
-      return 0;
-    }
-    if (a.repository?.name === undefined) {
-      return -1;
-    }
-    if (b.repository?.name === undefined) {
-      return 1;
-    }
-    return a.repository.name.localeCompare(b.repository.name);
+    const value1 = getNestedValue(a, sortColumn);
+    const value2 = getNestedValue(b, sortColumn);
+    // Handle cases where values are undefined
+    if (value1 === undefined && value2 === undefined) return 0;
+    if (value1 === undefined) return SortingOrderEnum.ASC ? -1 : 1;
+    if (value2 === undefined) return SortingOrderEnum.ASC ? 1 : -1;
+    // Compare values based on sort order
+    return SortingOrderEnum.ASC === sortOrder
+      ? value1.localeCompare(value2)
+      : value2.localeCompare(value1);
  });
 }


We should eliminate this and add the sorting parameter to the GitHub / Catalog API calls.

Github APIs not supporting sorting on these params .

christoph-jerolimov · 2024-12-16T11:28:26Z

workspaces/bulk-import/plugins/bulk-import-backend/src/helpers/utils.ts

+export function getNestedValue<T>(obj: T, path: string): any {
+  return path
+    .split('.')
+    .reduce(
+      (acc, key) =>
+        acc && (acc as Record<string, any>)[key]
+          ? (acc as Record<string, any>)[key]
+          : undefined,
+      obj,
+    );
+}
+export enum SortingOrderEnum {
+  ASC = 'asc',
+  DESC = 'desc',
+}


We should eliminate this and add the sorting parameter to the GitHub / Catalog API calls.

Github APIs not supporting sorting on these params .

dzemanov · 2024-12-19T08:43:36Z

workspaces/bulk-import/plugins/bulk-import-backend/src/schema/openapi.yaml

+    sortColumnQueryParam:
+      in: query
+      name: sortColumn
+      description: The name of the column to sort by
+      required: false
+      schema:
+        type: string


It would be amazing if you could add for sortColumnQueryParam enum values with description of what each value means and then reuse the generated types.

dzemanov · 2024-12-19T08:56:25Z

workspaces/bulk-import/plugins/bulk-import-backend/src/helpers/utils.ts

+export enum SortingOrderEnum {
+  ASC = 'asc',
+  DESC = 'desc',
+}


You have defined SortingOrderEnum also in types, ultimately it would be the best to reuse types generated in openapi.d.ts so we are not out of sync.

its-mitesh-kumar · 2024-12-23T15:00:44Z

@christoph-jerolimov @dzemanov I have resolved the comments , Please re-review it

dzemanov

Tested with all sort values, on the backend and frontend. Works as expected except for when sortColumn is not specified. Please run prettier to fix failing tests :).

dzemanov · 2025-01-03T11:03:23Z

workspaces/bulk-import/plugins/bulk-import/src/types/types.ts

+export enum AddedRepositoryColumnNameEnum {
+  repoName = 'repository.name', // because of property name
+  repositoryName = 'repository.name',
+  organizationUrl = 'repository.organization',
+  repoUrl = 'repository.url',
+  catalogInfoYamlLastUpdated = 'lastUpdate',
+  catalogInfoYamlStatus = 'status',
+}


Maybe it would be good to unify this and not repeat values. If repoName is needed, you can remove repositoryName and instead always use repoName.

Suggested change

export enum AddedRepositoryColumnNameEnum {

repoName = 'repository.name', // because of property name

repositoryName = 'repository.name',

organizationUrl = 'repository.organization',

repoUrl = 'repository.url',

catalogInfoYamlLastUpdated = 'lastUpdate',

catalogInfoYamlStatus = 'status',

}

export enum AddedRepositoryColumnNameEnum {

repoName = 'repository.name',

organizationUrl = 'repository.organization',

repoUrl = 'repository.url',

catalogInfoYamlLastUpdated = 'lastUpdate',

catalogInfoYamlStatus = 'status',

}

dzemanov · 2025-01-03T11:26:25Z

workspaces/bulk-import/plugins/bulk-import-backend/src/service/handlers/import/bulkImports.ts

+  const sortColumn = queryParams?.sortColumn;
+  const sortOrder = queryParams?.sortOrder;


You have defined in openapi.yaml both sortOrderQueryParam and sortColumnQueryParam to have default values if user does not provide them. But they are not used (they are not set automatically to default if undefined).
So either remove default values - in that case, we need to handle invalid option when user does not provide both of these parameters. Or I would suggest adding them here and then remove the change with if (sortColumn)

workspaces/bulk-import/plugins/bulk-import-backend/src/schema/openapi.yaml

dzemanov · 2025-01-03T11:53:32Z

Also, please add changeset.

rm3l · 2025-01-04T21:33:36Z

workspaces/bulk-import/plugins/bulk-import-backend/src/service/handlers/import/bulkImports.ts

@@ -45,18 +49,31 @@ type FindAllImportsResponse =
  | Components.Schemas.Import[]
  | Components.Schemas.ImportJobListV2;

-function sortImports(imports: Components.Schemas.Import[]) {
+function sortImports(


Could you add some tests in the backend for this feature? At least a few unit tests for this function. Or, better, some more complete tests in https://github.com/redhat-developer/rhdh-plugins/blob/main/workspaces/bulk-import/plugins/bulk-import-backend/src/service/handlers/import/bulkImports.test.ts and/or https://github.com/redhat-developer/rhdh-plugins/blob/main/workspaces/bulk-import/plugins/bulk-import-backend/src/service/handlers/import/imports.test.ts

…openapi.yaml Co-authored-by: Dominika Zemanovicova <[email protected]>

its-mitesh-kumar requested review from christoph-jerolimov, debsmita1 and rm3l as code owners December 13, 2024 11:18

its-mitesh-kumar force-pushed the feat/bulk-import-import-job-sorting branch from 54ca224 to ba86c46 Compare December 13, 2024 13:45

christoph-jerolimov requested changes Dec 16, 2024

View reviewed changes

christoph-jerolimov assigned christoph-jerolimov, debsmita1, its-mitesh-kumar, rm3l and schultzp2020 Dec 16, 2024

christoph-jerolimov requested a review from schultzp2020 December 16, 2024 11:31

dzemanov reviewed Dec 19, 2024

View reviewed changes

its-mitesh-kumar force-pushed the feat/bulk-import-import-job-sorting branch from ba86c46 to ea7ffa3 Compare December 23, 2024 14:59

its-mitesh-kumar requested review from christoph-jerolimov and dzemanov December 23, 2024 15:00

dzemanov suggested changes Jan 3, 2025

View reviewed changes

rm3l reviewed Jan 4, 2025

View reviewed changes

its-mitesh-kumar added 4 commits January 6, 2025 14:16

feat(bulk-import): adding sorting based on column in /imports

155d7c0

feat(bul-import): fixing typescript error

6d75dcc

feat(bulk-import) : resolving comments

cc837e9

feat(bulk-import): adding changeset

c2cff41

its-mitesh-kumar force-pushed the feat/bulk-import-import-job-sorting branch from e19fab3 to c2cff41 Compare January 6, 2025 08:46

Update workspaces/bulk-import/plugins/bulk-import-backend/src/schema/…

49b27f9

…openapi.yaml Co-authored-by: Dominika Zemanovicova <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bulk-import): adding sorting based on column in /imports #178

feat(bulk-import): adding sorting based on column in /imports #178

its-mitesh-kumar commented Dec 13, 2024

rhdh-gh-app bot commented Dec 13, 2024 •

edited

Loading

christoph-jerolimov left a comment

christoph-jerolimov Dec 16, 2024

christoph-jerolimov Dec 16, 2024

dzemanov Dec 19, 2024

rm3l Jan 4, 2025

christoph-jerolimov Dec 16, 2024

its-mitesh-kumar Dec 23, 2024 •

edited

Loading

christoph-jerolimov Dec 16, 2024

its-mitesh-kumar Dec 23, 2024

dzemanov Dec 19, 2024

dzemanov Dec 19, 2024

its-mitesh-kumar commented Dec 23, 2024

dzemanov left a comment

dzemanov Jan 3, 2025

dzemanov Jan 3, 2025

dzemanov commented Jan 3, 2025

rm3l Jan 4, 2025

		const sortColumn = queryParams?.sortColumn;
		const sortOrder = queryParams?.sortOrder;

feat(bulk-import): adding sorting based on column in /imports #178

Are you sure you want to change the base?

feat(bulk-import): adding sorting based on column in /imports #178

Conversation

its-mitesh-kumar commented Dec 13, 2024

rhdh-gh-app bot commented Dec 13, 2024 • edited Loading

Changed Packages

christoph-jerolimov 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

its-mitesh-kumar Dec 23, 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

its-mitesh-kumar commented Dec 23, 2024

dzemanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzemanov commented Jan 3, 2025

Choose a reason for hiding this comment

rhdh-gh-app bot commented Dec 13, 2024 •

edited

Loading

its-mitesh-kumar Dec 23, 2024 •

edited

Loading