-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PM-14360] Import Batching #703
base: main
Are you sure you want to change the base?
Changes from all commits
1249419
80b863d
c25d4be
e08ef3d
c7cde29
d0f260d
3b0d3c1
c26431b
79cf720
dfb54b7
6c0506e
87f4cb2
47f564b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
} |
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[]; | ||
} |
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; | ||
} | ||
} |
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 RequestBuilder This 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 |
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, | ||
}), | ||
]; | ||
} | ||
} |
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"); | ||
} | ||
} | ||
} |
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.