-
Notifications
You must be signed in to change notification settings - Fork 0
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
BC-8092 - implement config validation #21
base: main
Are you sure you want to change the base?
Conversation
@@ -1,9 +1,11 @@ | |||
import { Module } from '@nestjs/common'; | |||
import { PassportModule } from '@nestjs/passport'; | |||
import { ConfigurationModule } from '../configuration/configuration.module.js'; |
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.
configuration und config sind sich zu ähnlich um ausdrücken zu können was für was ist.
export class XApiKeyConfig { | ||
@Transform(({ value }) => value.split(',').map((part: string) => (part.split(':').pop() ?? '').trim())) | ||
@IsArray() | ||
public ADMIN_API__ALLOWED_API_KEYS = ['randomString']; |
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.
Sollte der default Wert wirklich bei der Anforderung festgelegt werden? Ich finde das der bessere Ort die Validierung, bzw. das lesen/zuweisen ist.
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.
isDefined() ?
@@ -1,5 +1,4 @@ | |||
import { Injectable, UnauthorizedException } from '@nestjs/common'; | |||
import { ConfigService } from '@nestjs/config'; | |||
import { PassportStrategy } from '@nestjs/passport'; | |||
import { Strategy } from 'passport-headerapikey/lib/Strategy.js'; | |||
import { XApiKeyConfig } from '../config/index.js'; |
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.
index ist hier unnötig.
|
||
export class AuthorizationConfig { | ||
@IsUrl({ require_tld: false }) | ||
public API_HOST = 'http://localhost:3030'; |
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.
Die verschiedenen Werte für prod, test, local sollten aus einer anderen Quelle kommen. Den default als local zu nutzen finde ich nicht gut. Man könnte sicher argumentieren das test eh im test file definiert werden muss. Nur gibt es einzelne envs die immer wieder gesetzt werden müssten und einmalig definiert einfach praktischer sind.
src/infra/redis/redis.config.ts
Outdated
public REDIS!: string; | ||
|
||
@IsString() | ||
@IsOptional() |
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.
Ist das nicht ein Wiederspruch? Es muss string sein, es ist optional, aber ! sagt das es nicht undefined, oder null ist am Ende.
config.REDIS_SENTINEL_SERVICE_NAME = sentinelServiceName; | ||
config.REDIS_PREFIX = redisPrefix; | ||
config.REDIS_SENTINEL_NAME = sentinelName; | ||
config.REDIS_SENTINEL_PASSWORD = sentinelPassword; |
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.
Wäre ein überschreiben zu einem späteren Zeitpunkt an anderen Stellen auch möglich?
import { App } from 'uws'; | ||
import { createMock } from '@golevelup/ts-jest'; | ||
import { RedisService } from '../../../../infra/redis/redis.service.js'; |
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.
Der Pfad ist unglücklich. In dem Repo auch mit @infra zu arbeiten ist denke ich sinnvoller.
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.
Ja es soll auch so sein, leider wegen ESM Module ist das nicht so einfach wie im server. bzw noch nicht nachgeforscht
} | ||
|
||
private validate<T extends object>(config: Record<string, unknown>, Constructor: new () => T): T { | ||
const validatedConfig = plainToInstance(Constructor, config); |
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.
vs doku
const validatedConfig = plainToInstance(
EnvironmentVariables,
config,
{ enableImplicitConversion: true },
);
const errors = validateSync(validatedConfig, { skipMissingProperties: false });
public getAllValidConfigsByType<T extends object>(Constructor: new () => T): T { | ||
const config = new Constructor(); | ||
const configKeys = Object.keys(config); | ||
|
||
const configValues = configKeys.reduce((acc: Record<string, unknown>, key) => { | ||
const value = this.configService.get(key); | ||
|
||
if (value) { | ||
acc[key] = value; | ||
} | ||
|
||
return acc; | ||
}, {}); | ||
|
||
const validatedConfig = this.validate(configValues, Constructor); | ||
|
||
return validatedConfig; | ||
} |
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.
Kompliziert, brauchen wir das wirklich.
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.
Wie gesagtes ist nur so möglich auf eingelesene Werte vom ConfigService zuzugreifen und nur die keys zu holen die wir brauchen für dieses Config
Quality Gate passedIssues Measures |
Description
Links to Tickets or other pull requests
Changes
Datasecurity
Deployment
New Repos, NPM pakages or vendor scripts
Screenshots of UI changes
Approval for review