Skip to content
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

N21-2313 Improve schulconnex group provisioning runtime #5394

Merged
merged 7 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export interface SchulconnexClientConfig {
SCHULCONNEX_CLIENT__PERSON_INFO_TIMEOUT_IN_MS: number;
SCHULCONNEX_CLIENT__PERSONEN_INFO_TIMEOUT_IN_MS: number;
SCHULCONNEX_CLIENT__POLICIES_INFO_TIMEOUT_IN_MS: number;
SCHULCONNEX_CLIENT__API_URL?: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { SchulconnexRestClientOptions } from './schulconnex-rest-client-options'

@Module({})
export class SchulconnexClientModule {
static registerAsync(): DynamicModule {
public static registerAsync(): DynamicModule {
return {
imports: [HttpModule, LoggerModule],
module: SchulconnexClientModule,
Expand All @@ -27,6 +27,7 @@ export class SchulconnexClientModule {
tokenEndpoint: configService.get<string>('SCHULCONNEX_CLIENT__TOKEN_ENDPOINT'),
clientId: configService.get<string>('SCHULCONNEX_CLIENT__CLIENT_ID'),
clientSecret: configService.get<string>('SCHULCONNEX_CLIENT__CLIENT_SECRET'),
personInfoTimeoutInMs: configService.get<number>('SCHULCONNEX_CLIENT__PERSON_INFO_TIMEOUT_IN_MS'),
personenInfoTimeoutInMs: configService.get<number>('SCHULCONNEX_CLIENT__PERSONEN_INFO_TIMEOUT_IN_MS'),
policiesInfoTimeoutInMs: configService.get<number>('SCHULCONNEX_CLIENT__POLICIES_INFO_TIMEOUT_IN_MS'),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ export interface SchulconnexRestClientOptions {

clientSecret?: string;

personInfoTimeoutInMs?: number;

personenInfoTimeoutInMs?: number;

policiesInfoTimeoutInMs?: number;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ describe(SchulconnexRestClient.name, () => {
clientId: 'clientId',
clientSecret: 'clientSecret',
tokenEndpoint: 'https://schulconnex.url/token',
personenInfoTimeoutInMs: 30000,
policiesInfoTimeoutInMs: 30000,
personInfoTimeoutInMs: 30001,
personenInfoTimeoutInMs: 30002,
policiesInfoTimeoutInMs: 30003,
};

beforeAll(() => {
Expand Down Expand Up @@ -100,6 +101,7 @@ describe(SchulconnexRestClient.name, () => {
Authorization: `Bearer ${accessToken}`,
'Accept-Encoding': 'gzip',
},
timeout: options.personInfoTimeoutInMs,
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,14 @@ export class SchulconnexRestClient implements SchulconnexApiInterface {
this.SCHULCONNEX_API_BASE_URL = options.apiUrl || '';
}

public async getPersonInfo(accessToken: string, options?: { overrideUrl: string }): Promise<SchulconnexResponse> {
public getPersonInfo(accessToken: string, options?: { overrideUrl: string }): Promise<SchulconnexResponse> {
const url: URL = new URL(options?.overrideUrl ?? `${this.SCHULCONNEX_API_BASE_URL}/person-info`);

const response: Promise<SchulconnexResponse> = this.getRequest<SchulconnexResponse>(url, accessToken);
const response: Promise<SchulconnexResponse> = this.getRequest<SchulconnexResponse>(
url,
accessToken,
this.options.personInfoTimeoutInMs
);

return response;
}
Expand Down
13 changes: 8 additions & 5 deletions apps/server/src/modules/idp-console/idp-console.config.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { Configuration } from '@hpi-schul-cloud/commons';
import { ConsoleWriterConfig } from '@infra/console';
import { LoggerConfig } from '@src/core/logger';
import { RabbitMqConfig } from '@infra/rabbitmq';
import { SchulconnexClientConfig } from '@infra/schulconnex-client';
import { AccountConfig } from '@modules/account';
import { UserConfig } from '@modules/user';
import { SynchronizationConfig } from '@modules/synchronization';
import { SchulconnexClientConfig } from '@infra/schulconnex-client';
import { Configuration } from '@hpi-schul-cloud/commons';
import { UserConfig } from '@modules/user';
import { LanguageType } from '@shared/domain/interface';
import { RabbitMqConfig } from '@infra/rabbitmq';
import { LoggerConfig } from '@src/core/logger';

export interface IdpConsoleConfig
extends ConsoleWriterConfig,
Expand All @@ -33,6 +33,9 @@ const config: IdpConsoleConfig = {
TEACHER_VISIBILITY_FOR_EXTERNAL_TEAM_INVITATION: Configuration.get(
'TEACHER_VISIBILITY_FOR_EXTERNAL_TEAM_INVITATION'
) as string,
SCHULCONNEX_CLIENT__PERSON_INFO_TIMEOUT_IN_MS: Configuration.get(
'SCHULCONNEX_CLIENT__PERSON_INFO_TIMEOUT_IN_MS'
) as number,
SCHULCONNEX_CLIENT__PERSONEN_INFO_TIMEOUT_IN_MS: Configuration.get(
'SCHULCONNEX_CLIENT__PERSONEN_INFO_TIMEOUT_IN_MS'
) as number,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { externalGroupDtoFactory, externalGroupUserDtoFactory } from '../testing';
import { GroupProvisioningInfoLoggable } from './group-provisioning-info.loggable';

describe(GroupProvisioningInfoLoggable.name, () => {
describe('getLogMessage', () => {
const setup = () => {
const groupCount = 2;
const otherUserCount = 5;
const totalUserCount = groupCount * otherUserCount + groupCount;
const externalGroups = externalGroupDtoFactory.buildList(groupCount, {
otherUsers: externalGroupUserDtoFactory.buildList(otherUserCount),
});

const loggable = new GroupProvisioningInfoLoggable(externalGroups, 100);

return {
loggable,
totalUserCount,
groupCount,
};
};

it('should return a loggable message', () => {
const { loggable, totalUserCount, groupCount } = setup();

const message = loggable.getLogMessage();

expect(message).toEqual({
message: 'Group provisioning has finished.',
data: {
groupCount,
userCount: totalUserCount,
durationMs: 100,
},
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger';
import { ExternalGroupDto } from '../dto';

export class GroupProvisioningInfoLoggable implements Loggable {
constructor(private readonly groups: ExternalGroupDto[], private readonly durationMs: number) {}

public getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage {
const userCount = this.groups.reduce(
(count: number, group: ExternalGroupDto) => count + (group.otherUsers?.length ?? 0),
this.groups.length
);

return {
message: 'Group provisioning has finished.',
data: {
groupCount: this.groups.length,
userCount,
durationMs: this.durationMs,
},
};
}
}
1 change: 1 addition & 0 deletions apps/server/src/modules/provisioning/loggable/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ export { FetchingPoliciesInfoFailedLoggable } from './fetching-policies-info-fai
export { PoliciesInfoErrorResponseLoggable } from './policies-info-error-response-loggable';
export { UserRoleUnknownLoggableException } from './user-role-unknown.loggable-exception';
export { SchoolMissingLoggableException } from './school-missing.loggable-exception';
export { GroupProvisioningInfoLoggable } from './group-provisioning-info.loggable';
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ export interface ProvisioningConfig {
FEATURE_SCHULCONNEX_COURSE_SYNC_ENABLED: boolean;
FEATURE_SCHULCONNEX_MEDIA_LICENSE_ENABLED: boolean;
PROVISIONING_SCHULCONNEX_POLICIES_INFO_URL: string;
PROVISIONING_SCHULCONNEX_GROUP_USERS_LIMIT?: number;
FEATURE_SANIS_GROUP_PROVISIONING_ENABLED: boolean;
FEATURE_OTHER_GROUPUSERS_PROVISIONING_ENABLED: boolean;
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ export class SanisProvisioningStrategy extends SchulconnexProvisioningStrategy {
protected readonly schulconnexLicenseProvisioningService: SchulconnexLicenseProvisioningService,
protected readonly schulconnexToolProvisioningService: SchulconnexToolProvisioningService,
protected readonly configService: ConfigService<ProvisioningConfig, true>,
protected readonly logger: Logger,
private readonly responseMapper: SchulconnexResponseMapper,
private readonly schulconnexRestClient: SchulconnexRestClient,
private readonly logger: Logger
private readonly schulconnexRestClient: SchulconnexRestClient
) {
super(
schulconnexSchoolProvisioningService,
Expand All @@ -58,7 +58,8 @@ export class SanisProvisioningStrategy extends SchulconnexProvisioningStrategy {
schulconnexLicenseProvisioningService,
schulconnexToolProvisioningService,
groupService,
configService
configService,
logger
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ describe(SchulconnexResponseMapper.name, () => {
mapper = module.get(SchulconnexResponseMapper);
});

beforeEach(() => {
config.FEATURE_OTHER_GROUPUSERS_PROVISIONING_ENABLED = false;
config.PROVISIONING_SCHULCONNEX_GROUP_USERS_LIMIT = undefined;
});

describe('mapToExternalSchoolDto', () => {
describe('when a schulconnex response is provided', () => {
const setup = () => {
Expand Down Expand Up @@ -316,6 +321,8 @@ describe(SchulconnexResponseMapper.name, () => {

describe('when other participants have unknown roles', () => {
const setup = () => {
config.FEATURE_OTHER_GROUPUSERS_PROVISIONING_ENABLED = true;

const schulconnexResponse: SchulconnexResponse = schulconnexResponseFactory.build();
schulconnexResponse.personenkontexte[0].gruppen![0]!.sonstige_gruppenzugehoerige = [
{
Expand Down Expand Up @@ -514,6 +521,56 @@ describe(SchulconnexResponseMapper.name, () => {
);
});
});

describe('when there are too many users in groups', () => {
const setup = () => {
config.FEATURE_OTHER_GROUPUSERS_PROVISIONING_ENABLED = true;
config.PROVISIONING_SCHULCONNEX_GROUP_USERS_LIMIT = 1;

const schulconnexResponse: SchulconnexResponse = schulconnexResponseFactory.build();

return {
schulconnexResponse,
};
};

it('should not map other group users', () => {
const { schulconnexResponse } = setup();

const result: ExternalGroupDto[] | undefined = mapper.mapToExternalGroupDtos(schulconnexResponse);

expect(result).toEqual([
expect.objectContaining<Partial<ExternalGroupDto>>({
otherUsers: undefined,
}),
]);
});
});

describe('when there are not too many users in groups', () => {
const setup = () => {
config.FEATURE_OTHER_GROUPUSERS_PROVISIONING_ENABLED = true;
config.PROVISIONING_SCHULCONNEX_GROUP_USERS_LIMIT = 10;

const schulconnexResponse: SchulconnexResponse = schulconnexResponseFactory.build();

return {
schulconnexResponse,
};
};

it('should not map other group users', () => {
const { schulconnexResponse } = setup();

const result: ExternalGroupDto[] | undefined = mapper.mapToExternalGroupDtos(schulconnexResponse);

expect(result).not.toEqual([
expect.objectContaining({
otherUsers: undefined,
}),
]);
});
});
});

describe('mapLernperiode', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,25 @@ export class SchulconnexResponseMapper {
return undefined;
}

const usersInGroupsCount: number = groups.reduce(
(count: number, group: SchulconnexGruppenResponse) => count + (group.sonstige_gruppenzugehoerige?.length ?? 0),
groups.length
);
const limit: number | undefined = this.configService.get('PROVISIONING_SCHULCONNEX_GROUP_USERS_LIMIT');
const shouldProvisionOtherUsers: boolean = limit === undefined || usersInGroupsCount < limit;

const mapped: ExternalGroupDto[] = groups
.map((group) => this.mapExternalGroup(source, group))
.filter((group): group is ExternalGroupDto => group !== null);
.map((group: SchulconnexGruppenResponse) => this.mapExternalGroup(source, group, shouldProvisionOtherUsers))
.filter((group: ExternalGroupDto | null): group is ExternalGroupDto => group !== null);

return mapped;
}

private mapExternalGroup(source: SchulconnexResponse, group: SchulconnexGruppenResponse): ExternalGroupDto | null {
private mapExternalGroup(
source: SchulconnexResponse,
group: SchulconnexGruppenResponse,
shouldProvisionOtherUsers: boolean
): ExternalGroupDto | null {
const groupType: GroupTypes | undefined = GroupTypeMapping[group.gruppe.typ];

if (!groupType) {
Expand All @@ -144,7 +155,7 @@ export class SchulconnexResponseMapper {
}

let otherUsers: ExternalGroupUserDto[] | undefined;
if (this.configService.get('FEATURE_OTHER_GROUPUSERS_PROVISIONING_ENABLED')) {
if (this.configService.get('FEATURE_OTHER_GROUPUSERS_PROVISIONING_ENABLED') && shouldProvisionOtherUsers) {
otherUsers = group.sonstige_gruppenzugehoerige
? group.sonstige_gruppenzugehoerige
.map((relation): ExternalGroupUserDto | null => this.mapToExternalGroupUser(relation))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
legacySchoolDoFactory,
userDoFactory,
} from '@shared/testing';
import { Logger } from '@src/core/logger';
import {
ExternalGroupDto,
ExternalSchoolDto,
Expand Down Expand Up @@ -98,6 +99,10 @@ describe(SchulconnexProvisioningStrategy.name, () => {
get: jest.fn().mockImplementation((key: keyof ProvisioningConfig) => config[key]),
},
},
{
provide: Logger,
useValue: createMock<Logger>(),
},
],
}).compile();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ import { Group, GroupService } from '@modules/group';
import { Injectable } from '@nestjs/common';
import { ConfigService } from '@nestjs/config';
import { LegacySchoolDo, UserDO } from '@shared/domain/domainobject';
import { Logger } from '@src/core/logger';
import { ExternalGroupDto, OauthDataDto, ProvisioningDto } from '../../dto';
import { GroupProvisioningInfoLoggable } from '../../loggable';
import { ProvisioningConfig } from '../../provisioning.config';
import { ProvisioningStrategy } from '../base.strategy';
import {
Expand All @@ -24,7 +26,8 @@ export abstract class SchulconnexProvisioningStrategy extends ProvisioningStrate
protected readonly schulconnexLicenseProvisioningService: SchulconnexLicenseProvisioningService,
protected readonly schulconnexToolProvisioningService: SchulconnexToolProvisioningService,
protected readonly groupService: GroupService,
protected readonly configService: ConfigService<ProvisioningConfig, true>
protected readonly configService: ConfigService<ProvisioningConfig, true>,
protected readonly logger: Logger
) {
super();
}
Expand Down Expand Up @@ -61,6 +64,8 @@ export abstract class SchulconnexProvisioningStrategy extends ProvisioningStrate
}

private async provisionGroups(data: OauthDataDto, school?: LegacySchoolDo): Promise<void> {
const startTime = performance.now();

await this.removeUserFromGroups(data);

if (data.externalGroups) {
Expand Down Expand Up @@ -96,6 +101,9 @@ export abstract class SchulconnexProvisioningStrategy extends ProvisioningStrate

await Promise.all(groupProvisioningPromises);
}

const endTime = performance.now();
this.logger.warning(new GroupProvisioningInfoLoggable(data.externalGroups ?? [], endTime - startTime));
}

private async removeUserFromGroups(data: OauthDataDto): Promise<void> {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { UUID } from 'bson';
import { Factory } from 'fishery';
import { GroupTypes } from '../../group';
import { ExternalGroupDto } from '../dto';
import { externalGroupUserDtoFactory } from './external-group-user-dto.factory';

export const externalGroupDtoFactory = Factory.define<ExternalGroupDto>(
({ sequence }) =>
new ExternalGroupDto({
type: GroupTypes.CLASS,
name: `External Group ${sequence}`,
externalId: new UUID().toString(),
user: externalGroupUserDtoFactory.build(),
otherUsers: externalGroupUserDtoFactory.buildList(2),
from: new Date(),
until: new Date(),
})
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { RoleName } from '@shared/domain/interface';
import { UUID } from 'bson';
import { Factory } from 'fishery';
import { ExternalGroupUserDto } from '../dto';

export const externalGroupUserDtoFactory = Factory.define<ExternalGroupUserDto>(
() =>
new ExternalGroupUserDto({
externalUserId: new UUID().toString(),
roleName: RoleName.TEACHER,
})
);
Loading
Loading