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 3 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,41 @@
import { externalGroupDtoFactory, externalGroupUserDtoFactory, externalUserDtoFactory } 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 externalUser = externalUserDtoFactory.build();
const externalGroups = externalGroupDtoFactory.buildList(groupCount, {
otherUsers: externalGroupUserDtoFactory.buildList(otherUserCount),
});

const loggable = new GroupProvisioningInfoLoggable(externalUser, externalGroups, 100);

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

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

const message = loggable.getLogMessage();

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

export class GroupProvisioningInfoLoggable implements Loggable {
constructor(
private readonly groupUser: ExternalUserDto,
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: {
externalUserId: this.groupUser.externalId,
MarvinOehlerkingCap marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -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,11 @@ export abstract class SchulconnexProvisioningStrategy extends ProvisioningStrate

await Promise.all(groupProvisioningPromises);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment we only print log messages at the log level error on production.
So put it's to the log level error on the moment that we cann see it at production

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That logging go from error down to warning on production is not posible by data protection.
One example for to manny informations are logged on warning is https://github.com/hpi-schul-cloud/schulcloud-server/blob/main/apps/server/src/modules/group/uc/group.uc.ts#L48

}

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,
})
);
2 changes: 2 additions & 0 deletions apps/server/src/modules/provisioning/testing/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
export { externalUserDtoFactory } from './external-user-dto.factory';
export { externalGroupDtoFactory } from './external-group-dto.factory';
export { externalGroupUserDtoFactory } from './external-group-user-dto.factory';
Loading
Loading