[PM-14360] Import Batching #703
+534
−131
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| import { DirectoryType } from "@/src/enums/directoryType"; | ||
| import { IDirectoryService } from "@/src/services/directory.service"; | ||
|
|
||
| export abstract class DirectoryFactoryService { | ||
| abstract createService(type: DirectoryType): IDirectoryService; | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| import { OrganizationImportRequest } from "@/jslib/common/src/models/request/organizationImportRequest"; | ||
|
|
||
| import { GroupEntry } from "@/src/models/groupEntry"; | ||
| import { UserEntry } from "@/src/models/userEntry"; | ||
|
|
||
| export abstract class RequestBuilderAbstratction { | ||
| buildRequest: ( | ||
| groups: GroupEntry[], | ||
| users: UserEntry[], | ||
| removeDisabled: boolean, | ||
| overwriteExisting: boolean, | ||
| ) => OrganizationImportRequest[]; | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| import { OrganizationImportRequest } from "@/jslib/common/src/models/request/organizationImportRequest"; | ||
|
|
||
| import { GroupEntry } from "@/src/models/groupEntry"; | ||
| import { UserEntry } from "@/src/models/userEntry"; | ||
|
|
||
| import { RequestBuilderAbstratction } from "../abstractions/request-builder.service"; | ||
|
|
||
| import { batchSize } from "./sync.service"; | ||
|
|
||
| /** | ||
| * This class is responsible for batching large sync requests (>2k users) into multiple smaller | ||
| * requests to the /import REST endpoint. This is done to ensure we are under the default | ||
| * maximum packet size for NGINX web servers to avoid the request potentially timing out | ||
| * */ | ||
| export class DefaultBatchRequestBuilder implements RequestBuilderAbstratction { | ||
| buildRequest( | ||
| groups: GroupEntry[], | ||
| users: UserEntry[], | ||
| removeDisabled: boolean, | ||
| overwriteExisting: boolean, | ||
| ): OrganizationImportRequest[] { | ||
| const requests: OrganizationImportRequest[] = []; | ||
|
|
||
| if (users.length > 0) { | ||
| const usersRequest = users.map((u) => { | ||
| return { | ||
| email: u.email, | ||
| externalId: u.externalId, | ||
| deleted: u.deleted || (removeDisabled && u.disabled), | ||
| }; | ||
| }); | ||
|
|
||
| // Partition users | ||
| for (let i = 0; i < usersRequest.length; i += batchSize) { | ||
| const u = usersRequest.slice(i, i + batchSize); | ||
| const req = new OrganizationImportRequest({ | ||
| groups: [], | ||
| users: u, | ||
| largeImport: true, | ||
| overwriteExisting, | ||
| }); | ||
| requests.push(req); | ||
| } | ||
| } | ||
|
|
||
| if (groups.length > 0) { | ||
| const groupRequest = groups.map((g) => { | ||
| return { | ||
| name: g.name, | ||
| externalId: g.externalId, | ||
| memberExternalIds: Array.from(g.userMemberExternalIds), | ||
| }; | ||
| }); | ||
|
|
||
| // Partition groups | ||
| for (let i = 0; i < groupRequest.length; i += batchSize) { | ||
| const g = groupRequest.slice(i, i + batchSize); | ||
| const req = new OrganizationImportRequest({ | ||
| groups: g, | ||
| users: [], | ||
| largeImport: true, | ||
| overwriteExisting, | ||
| }); | ||
| requests.push(req); | ||
| } | ||
| } | ||
|
|
||
| return requests; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| import { GroupEntry } from "@/src/models/groupEntry"; | ||
| import { UserEntry } from "@/src/models/userEntry"; | ||
|
|
||
| import { DefaultBatchRequestBuilder } from "./default-batch-request-builder"; | ||
| import { DefaultSingleRequestBuilder } from "./default-single-request-builder"; | ||
|
|
||
| describe("BatchingService", () => { | ||
|
There was a problem hiding this comment.
|
||
| let batchRequestBuilder: DefaultBatchRequestBuilder; | ||
| let singleRequestBuilder: DefaultSingleRequestBuilder; | ||
|
|
||
| function userSimulator(userCount: number) { | ||
| return Array(userCount).fill(new UserEntry()); | ||
| } | ||
|
|
||
| function groupSimulator(groupCount: number) { | ||
| return Array(groupCount).fill(new GroupEntry()); | ||
| } | ||
|
|
||
| beforeEach(async () => { | ||
| batchRequestBuilder = new DefaultBatchRequestBuilder(); | ||
| singleRequestBuilder = new DefaultSingleRequestBuilder(); | ||
| }); | ||
|
|
||
| it("BatchRequestBuilder batches requests for > 2000 users", () => { | ||
| const mockGroups = groupSimulator(11000); | ||
| const mockUsers = userSimulator(11000); | ||
|
|
||
| const requests = batchRequestBuilder.buildRequest(mockGroups, mockUsers, true, true); | ||
|
|
||
| expect(requests.length).toEqual(12); | ||
| }); | ||
|
|
||
| it("SingleRequestBuilder returns single request for 200 users", () => { | ||
| const mockGroups = groupSimulator(200); | ||
| const mockUsers = userSimulator(200); | ||
|
|
||
| const requests = singleRequestBuilder.buildRequest(mockGroups, mockUsers, true, true); | ||
|
|
||
| expect(requests.length).toEqual(1); | ||
| }); | ||
|
|
||
| it("BatchRequestBuilder retuns an empty array when there are no users or groups", () => { | ||
| const requests = batchRequestBuilder.buildRequest([], [], true, true); | ||
|
|
||
| expect(requests).toEqual([]); | ||
| }); | ||
| }); | ||
|
There was a problem hiding this comment. This is not a big deal, but I could've been more explicit re: naming. If you had only 1 request builder class, you might do something like: DefaultRequestBuilder extends RequestBuilderThis is a most classes where you only have a single implementation. However, here we do have multiple implementations and we already have a way to distinguish them: SingleRequestBuilder extends RequestBuilder
BatchRequestBuilder extends RequestBuilder
// there is no "default" implementation, you have to choose one or the other |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| import { OrganizationImportRequest } from "@/jslib/common/src/models/request/organizationImportRequest"; | ||
|
|
||
| import { GroupEntry } from "@/src/models/groupEntry"; | ||
| import { UserEntry } from "@/src/models/userEntry"; | ||
|
|
||
| import { RequestBuilderAbstratction } from "../abstractions/request-builder.service"; | ||
|
|
||
| /** | ||
| * This class is responsible for building small (<2k users) syncs as a single | ||
| * request to the /import endpoint. This is done to be backwards compatible with | ||
| * existing functionality for sync requests that are sufficiently small enough to not | ||
| * exceed default maximum packet size limits on NGINX web servers. | ||
| * */ | ||
| export class DefaultSingleRequestBuilder implements RequestBuilderAbstratction { | ||
| buildRequest( | ||
| groups: GroupEntry[], | ||
| users: UserEntry[], | ||
| removeDisabled: boolean, | ||
| overwriteExisting: boolean, | ||
| ): OrganizationImportRequest[] { | ||
| return [ | ||
| new OrganizationImportRequest({ | ||
| groups: (groups ?? []).map((g) => { | ||
| return { | ||
| name: g.name, | ||
| externalId: g.externalId, | ||
| memberExternalIds: Array.from(g.userMemberExternalIds), | ||
| }; | ||
| }), | ||
| users: (users ?? []).map((u) => { | ||
| return { | ||
| email: u.email, | ||
| externalId: u.externalId, | ||
| deleted: u.deleted || (removeDisabled && u.disabled), | ||
| }; | ||
| }), | ||
| overwriteExisting: overwriteExisting, | ||
| largeImport: false, | ||
| }), | ||
| ]; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import { I18nService } from "@/jslib/common/src/abstractions/i18n.service"; | ||
| import { LogService } from "@/jslib/common/src/abstractions/log.service"; | ||
|
|
||
| import { DirectoryFactoryService } from "../abstractions/directory-factory.service"; | ||
| import { StateService } from "../abstractions/state.service"; | ||
| import { DirectoryType } from "../enums/directoryType"; | ||
|
|
||
| import { AzureDirectoryService } from "./azure-directory.service"; | ||
| import { GSuiteDirectoryService } from "./gsuite-directory.service"; | ||
| import { LdapDirectoryService } from "./ldap-directory.service"; | ||
| import { OktaDirectoryService } from "./okta-directory.service"; | ||
| import { OneLoginDirectoryService } from "./onelogin-directory.service"; | ||
|
|
||
| export class DefaultDirectoryFactoryService implements DirectoryFactoryService { | ||
| constructor( | ||
| private logService: LogService, | ||
| private i18nService: I18nService, | ||
| private stateService: StateService, | ||
| ) {} | ||
|
|
||
| createService(directoryType: DirectoryType) { | ||
| switch (directoryType) { | ||
| case DirectoryType.GSuite: | ||
| return new GSuiteDirectoryService(this.logService, this.i18nService, this.stateService); | ||
| case DirectoryType.AzureActiveDirectory: | ||
| return new AzureDirectoryService(this.logService, this.i18nService, this.stateService); | ||
| case DirectoryType.Ldap: | ||
| return new LdapDirectoryService(this.logService, this.i18nService, this.stateService); | ||
| case DirectoryType.Okta: | ||
| return new OktaDirectoryService(this.logService, this.i18nService, this.stateService); | ||
| case DirectoryType.OneLogin: | ||
| return new OneLoginDirectoryService(this.logService, this.i18nService, this.stateService); | ||
| default: | ||
| throw new Error("Invalid Directory Type"); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this naming... open to suggestions. Alternatively we could rename the existing state service since we have 2 (one in jslib and one in src)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The src StateService extends the jslib StateService, so you can just use the extended class everywhere and it'll satisfy the type constraints.
However the better answer is that you don't need to add to
jslib-services.module.ts. Like the rest of the jslib folder, it's essentially deprecated (but I acknowledge this isn't documented anywhere). Register your new dependencies insrc/app/services/services.module.ts. That file only references the src StateService.