-
Notifications
You must be signed in to change notification settings - Fork 0
Server/feat/academic year context #30
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
Changes from 2 commits
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 |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import { Injectable, NotFoundException } from '@nestjs/common'; | ||
| import { Prisma } from '../../../prisma/generated/prisma/client.js'; | ||
| import { AcademicYearContextService } from '../../common/academic-year-context/academic-year-context.service.js'; | ||
| import { PaginatedResponseDto } from '../../common/dto/paginated-response.dto.js'; | ||
| import { PrismaService } from '../../database/prisma/prisma.service.js'; | ||
| import { AcademicYearResponseDto } from './dto/academic-year-response.dto.js'; | ||
|
|
@@ -9,7 +10,10 @@ import { UpdateAcademicYearDto } from './dto/update-academic-year.dto.js'; | |
|
|
||
| @Injectable() | ||
| export class AcademicYearsService { | ||
| constructor(private readonly prisma: PrismaService) {} | ||
| constructor( | ||
| private readonly prisma: PrismaService, | ||
| private readonly academicYearContext: AcademicYearContextService, | ||
| ) {} | ||
|
|
||
| async create( | ||
| createAcademicYearDto: CreateAcademicYearDto, | ||
|
|
@@ -112,13 +116,13 @@ export class AcademicYearsService { | |
| return await this.prisma.academicYear.update({ | ||
| where: { id }, | ||
| data: { | ||
| name: updateAcademicYearDto.name, | ||
| startDate: updateAcademicYearDto.startDate | ||
| ? new Date(updateAcademicYearDto.startDate) | ||
| : undefined, | ||
| endDate: updateAcademicYearDto.endDate | ||
| ? new Date(updateAcademicYearDto.endDate) | ||
| : undefined, | ||
| isCurrent: updateAcademicYearDto.isCurrent, | ||
| }, | ||
| }); | ||
|
Comment on lines
116
to
127
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. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'server/src/modules/academic-years/*' 'server/src/modules/*academic*year*/*' | sed 's#^`#-` #'
printf '\n== Service outline ==\n'
ast-grep outline server/src/modules/academic-years/academic-years.service.ts --view expanded || true
printf '\n== DTO candidates ==\n'
fd -a '.*dto.*' server/src/modules/academic-years server/src/modules | sed 's#^`#-` #'
printf '\n== Search for academic year DTOs and decorators ==\n'
rg -n "class .*AcademicYear|PartialType|IsNotEmpty|IsOptional|trim\\(|new Date\\(" server/src/modules/academic-years server/src/modules -g '*.ts' -A 3 -B 3 || trueRepository: thureinhtet99/student-os Length of output: 50381 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n== update() method ==\n'
sed -n '105,130p' server/src/modules/academic-years/academic-years.service.ts
printf '\n== academic-years service tests around update ==\n'
rg -n "update\\(|trim|name|startDate|endDate" server/src/modules/academic-years/academic-years.service.spec.ts -A 4 -B 4 || trueRepository: thureinhtet99/student-os Length of output: 1020 Trim 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,24 @@ | ||
| import { PartialType } from '@nestjs/mapped-types'; | ||
| import { OmitType } from '@nestjs/swagger'; | ||
| import { ApiProperty, OmitType } from '@nestjs/swagger'; | ||
| import { IsDateString, IsNotEmpty, IsString, MaxLength } from 'class-validator'; | ||
| import { CreateAcademicYearDto } from './create-academic-year.dto.js'; | ||
|
|
||
| export class UpdateAcademicYearDto extends PartialType( | ||
| OmitType(CreateAcademicYearDto, ['name'] as const), | ||
| ) {} | ||
| OmitType(CreateAcademicYearDto, ['isCurrent'] as const), | ||
| ) { | ||
| @ApiProperty({ type: String, example: '2025-2026' }) | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| @MaxLength(100) | ||
| name!: string; | ||
|
|
||
| @ApiProperty({ type: String, example: '2025-06-01T00:00:00.000Z' }) | ||
| @IsDateString() | ||
| @IsNotEmpty() | ||
| startDate!: string; | ||
|
|
||
| @ApiProperty({ type: String, example: '2026-05-31T23:59:59.000Z' }) | ||
| @IsDateString() | ||
| @IsNotEmpty() | ||
| endDate!: string; | ||
| } |
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.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: thureinhtet99/student-os
Length of output: 162
🏁 Script executed:
Repository: thureinhtet99/student-os
Length of output: 12270
🌐 Web query:
NestJS request-scoped provider injected into singleton service makes consumer request-scoped💡 Result:
Yes, in NestJS, injecting a request-scoped provider into a singleton service causes the singleton service to become request-scoped as well [1][2][3]. This behavior is known as scope bubbling [1][2][4]. When a provider is marked as request-scoped (using Scope.REQUEST), NestJS must create a new instance of that provider for every incoming request [1][2][3]. Because a singleton service is expected to be shared across the entire application lifetime, NestJS cannot safely inject a volatile, request-specific instance into it [2][3]. To maintain the integrity of the dependency injection container, NestJS automatically promotes the singleton service to request scope [1][2]. This promotion behavior has several implications: 1. Scope Bubbling: The promotion propagates up the dependency graph [1][2][3]. Any controller or service that depends on that promoted singleton will also be forced into request scope [1][2]. 2. Performance Impact: Because the promoted services are now instantiated per request rather than being cached as singletons, this can increase the overhead of handling requests, potentially slowing down application performance [1][2][4]. 3. Background Contexts: If a service that has been promoted to request scope is later injected into a context where no request exists (such as a background worker, cron job, or application bootstrap), the application may fail to resolve the dependency because there is no request-specific DI container available [3]. It is generally recommended to use the default singleton scope whenever possible and only use request scope when absolutely necessary [1][4]. For cases involving request-specific metadata (like user context), developers often use AsyncLocalStorage or pass request metadata explicitly through method arguments to avoid forcing services into request scope [3].
Citations:
Remove the unused
AcademicYearContextServiceinjection, or use it in a request-aware method.It only promotes
AcademicYearsServiceto request scope, adding per-request DI overhead with no benefit. If it stays,server/src/modules/academic-years/academic-years.service.spec.tsalso needs a mock provider forAcademicYearContextService.🤖 Prompt for AI Agents