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

fix(j-s): String Length Validation #16924

Merged
merged 2 commits into from
Nov 18, 2024
Merged

fix(j-s): String Length Validation #16924

merged 2 commits into from
Nov 18, 2024

Conversation

gudjong
Copy link
Member

@gudjong gudjong commented Nov 18, 2024

String Length Validation

Setja limit á lengd texta í "efni kröfu" þegar verið er að búa til nýtt mál

What

  • Validates varchar 255 input on the backend.
  • Limits investigation case description to 255 characters in the client.

Why

  • To prevent unexpected excheptions when writing to the database.

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Formatting passes locally with my changes
  • I have rebased against main before asking for a review

Summary by CodeRabbit

  • New Features

    • Enhanced input validation for various fields across multiple forms, enforcing a maximum length of 255 characters for string inputs.
    • Updated logic for creating and managing defendants within the working case, ensuring correct context and state management.
  • Bug Fixes

    • Improved navigation logic to check for the existence of the working case before proceeding.
  • Documentation

    • Updated validation rules to reflect the new maximum length constraints.

@gudjong gudjong requested a review from a team as a code owner November 18, 2024 15:05
Copy link
Contributor

coderabbitai bot commented Nov 18, 2024

Walkthrough

The pull request introduces validation enhancements across multiple Data Transfer Objects (DTOs) in the judicial system application. Specifically, the @MaxLength(255) decorator has been added to various string properties in several DTO classes, ensuring that input values do not exceed 255 characters. This change applies to fields in CreateCaseDto, UpdateCaseDto, CreateDefendantDto, UpdateDefendantDto, CreateFileDto, UpdateFileDto, and others. Additionally, some properties have been updated to include minimum size requirements for arrays, enhancing overall data validation.

Changes

File Path Change Summary
apps/judicial-system/backend/src/app/modules/case/dto/createCase.dto.ts Added @MaxLength(255) to properties: description, policeCaseNumbers, defenderName, defenderNationalId, defenderEmail, defenderPhoneNumber, leadInvestigator.
apps/judicial-system/backend/src/app/modules/case/dto/updateCase.dto.ts Added @MaxLength(255) to properties: location, description, policeCaseNumbers, defenderName, defenderNationalId, defenderEmail, defenderPhoneNumber, leadInvestigator, translator, courtCaseNumber, courtLocation, appealCaseNumber. Added @ArrayMinSize(1) to policeCaseNumbers.
apps/judicial-system/backend/src/app/modules/defendant/dto/createDefendant.dto.ts Added @MaxLength(255) to properties: nationalId, name, address, citizenship, defenderName, defenderNationalId, defenderEmail, defenderPhoneNumber.
apps/judicial-system/backend/src/app/modules/defendant/dto/updateCivilClaimant.dto.ts Added @MaxLength(255) to properties: name, nationalId, spokespersonNationalId, spokespersonName, spokespersonEmail, spokespersonPhoneNumber.
apps/judicial-system/backend/src/app/modules/defendant/dto/updateDefendant.dto.ts Added @MaxLength(255) to properties: nationalId, name, address, citizenship, defenderName, defenderNationalId, defenderEmail, defenderPhoneNumber, requestedDefenderNationalId, requestedDefenderName.
apps/judicial-system/backend/src/app/modules/file/dto/createFile.dto.ts Added @MaxLength(255) to properties: type, key, policeCaseNumber, policeFileId, userGeneratedFilename.
apps/judicial-system/backend/src/app/modules/file/dto/createPresignedPost.dto.ts Added @MaxLength(255) to properties: fileName, type.
apps/judicial-system/backend/src/app/modules/file/dto/updateFile.dto.ts Added @MaxLength(255) to property: userGeneratedFilename.
apps/judicial-system/backend/src/app/modules/indictment-count/dto/updateIndictmentCount.dto.ts Added @MaxLength(255) to properties: policeCaseNumber, vehicleRegistrationNumber.
apps/judicial-system/backend/src/app/modules/subpoena/dto/updateSubpoena.dto.ts Added @MaxLength(255) to properties: servedBy, defenderNationalId, defenderName, defenderEmail, defenderPhoneNumber, requestedDefenderNationalId, requestedDefenderName.
apps/judicial-system/backend/src/app/modules/user/dto/createUser.dto.ts Added @MaxLength(255) to properties: nationalId, name, title, mobileNumber, email.
apps/judicial-system/backend/src/app/modules/user/dto/updateUser.dto.ts Added @MaxLength(255) to properties: name, title, mobileNumber, email.
apps/judicial-system/web/src/routes/Prosecutor/InvestigationCase/Defendant/Defendant.tsx Added maxLength property to the Input component for the description field and updated logic for creating and navigating defendants based on the existence of workingCase.id.

Possibly related PRs

Suggested labels

automerge

Suggested reviewers

  • unakb

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (19)
apps/judicial-system/backend/src/app/modules/file/dto/createPresignedPost.dto.ts (1)

Line range hint 1-18: Consider centralizing validation constraints.

Since these length validations are being added across multiple DTOs, consider creating a shared constant or configuration for these magic numbers to ensure consistency and easier maintenance.

Example implementation:

// shared/constants/validation.ts
export const DATABASE_VARCHAR_LENGTH = 255;

// Then in DTOs:
@MaxLength(DATABASE_VARCHAR_LENGTH)
apps/judicial-system/backend/src/app/modules/user/dto/createUser.dto.ts (3)

16-16: Consider field-specific length constraints

While adding @MaxLength(255) aligns with the database varchar(255) limitation, consider these field-specific recommendations:

  • nationalId: Should typically have a fixed length based on the Icelandic national ID format
  • email: RFC 5321 limits email addresses to 254 characters
  • mobileNumber: Consider a shorter, reasonable maximum length for phone numbers

Example implementation for nationalId:

  @IsNotEmpty()
- @MaxLength(255)
+ @MaxLength(10)
  @IsString()

For email:

  @IsNotEmpty()
- @MaxLength(255)
+ @MaxLength(254)
  @IsString()

For mobileNumber:

  @IsNotEmpty()
- @MaxLength(255)
+ @MaxLength(20)
  @IsString()

Also applies to: 22-22, 28-28, 34-34, 40-40


Line range hint 16-44: Add format validation for specific fields

Consider adding format validation for specialized fields:

  • nationalId: Add pattern validation for Icelandic national ID format
  • email: Add email format validation
  • mobileNumber: Add phone number format validation

Add these decorators:

  // For nationalId
+ @Matches(/^\d{6}-?\d{4}$/)

  // For email
+ @IsEmail()

  // For mobileNumber
+ @Matches(/^\+?[1-9]\d{1,14}$/)

Line range hint 1-65: Enhance TypeScript type safety

Per the coding guidelines for apps/** paths, consider enhancing type safety:

  1. Add strict validation for enum values:
+ import { UserRole, isUserRole } from '@island.is/judicial-system/types'

  @IsNotEmpty()
  @IsEnum(UserRole)
+ @ValidateIf((o) => isUserRole(o.role))
  @ApiProperty({ enum: UserRole })
  readonly role!: UserRole
  1. Consider using a custom validation pipe to ensure type safety during runtime:
@Injectable()
export class UserValidationPipe implements PipeTransform {
  transform(value: CreateUserDto) {
    // Add runtime type checking
    return value;
  }
}
apps/judicial-system/backend/src/app/modules/indictment-count/dto/updateIndictmentCount.dto.ts (2)

23-26: Consider a tighter length constraint for vehicle registration

While the @MaxLength(255) prevents database issues, vehicle registration numbers are typically much shorter. Consider using a more specific length constraint that matches your business requirements for vehicle registration numbers.

Example:

-  @MaxLength(255)
+  @MaxLength(20) // Adjust based on your specific requirements

Line range hint 45-52: Add length validation to remaining string fields

The incidentDescription and legalArguments fields are missing length validation. To maintain consistency and prevent potential database issues, consider adding @MaxLength(255) to these fields as well.

Apply this diff:

  @IsOptional()
  @IsString()
+ @MaxLength(255)
  @ApiPropertyOptional({ type: String })
  readonly incidentDescription?: string

  @IsOptional()
  @IsString()
+ @MaxLength(255)
  @ApiPropertyOptional({ type: String })
  readonly legalArguments?: string
apps/judicial-system/backend/src/app/modules/file/dto/createFile.dto.ts (1)

68-71: Consider additional filename validation

While the length validation is correct, consider adding a pattern validation to ensure proper filename format and prevent directory traversal.

 @IsOptional()
 @MaxLength(255)
 @IsString()
+@Matches(/^[^/\\:*?"<>|]+$/, { message: 'Invalid filename format' })
 @ApiPropertyOptional({ type: String })
 readonly userGeneratedFilename?: string
apps/judicial-system/backend/src/app/modules/defendant/dto/updateCivilClaimant.dto.ts (2)

1-1: Consider centralizing validation constants.

To improve maintainability and consistency across DTOs, consider extracting the magic number 255 into a shared constant.

Example implementation:

// shared/constants/validation.ts
export const DATABASE_VARCHAR_LENGTH = 255;

// Then in this file:
import { DATABASE_VARCHAR_LENGTH } from '@shared/constants/validation';
@MaxLength(DATABASE_VARCHAR_LENGTH)

String fields across DTOs lack consistent length validation

The search results reveal that string fields in related DTOs lack consistent @MaxLength(255) validation. Several DTOs in the defendant module and other modules have @IsString() decorators without corresponding length constraints:

  • createDefendant.dto.ts: Missing length validation on fields like name, nationalId, address
  • updateDefendant.dto.ts: Missing length validation on fields like name, nationalId, defenderEmail
  • internalUpdateDefendant.dto.ts: Missing length validation on fields like defenderName, defenderEmail
🔗 Analysis chain

Line range hint 1-64: Verify consistent validation across related DTOs.

The AI summary mentions similar changes in other DTOs. Let's verify that the @MaxLength(255) decorator has been consistently applied across all related DTOs.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent @MaxLength(255) validation across related DTOs
# Expected: All string fields in related DTOs should have @MaxLength(255)

# Search for DTO files without @MaxLength(255) on string fields
rg -l '@IsString\(\)(?![\s\S]*@MaxLength\(255\))' apps/judicial-system/backend/src/app/modules/**/dto/*.ts

# List all DTOs for manual verification
echo "List of all DTOs for cross-reference:"
fd --type f --extension ts dto/ apps/judicial-system/backend

Length of output: 2896


Script:

#!/bin/bash
# Let's try a different approach to check string validations
# Search for string fields in DTOs and their validation decorators

# Check validation patterns in DTOs
rg -A 2 '@IsString\(\)' --type ts apps/judicial-system/backend/src/app/modules/**/dto/*.ts

# Also check specifically the related DTOs in the defendant module
echo "=== Defendant Module DTOs ==="
rg -A 2 '@IsString\(\)' apps/judicial-system/backend/src/app/modules/defendant/dto/*.ts

Length of output: 51407

apps/judicial-system/backend/src/app/modules/defendant/dto/createDefendant.dto.ts (2)

61-64: Consider adding email format validation

While the length validation is good, consider adding the @IsEmail() decorator for stronger email format validation.

  @IsOptional()
  @MaxLength(255)
  @IsString()
+ @IsEmail()
  @ApiPropertyOptional({ type: String })
  readonly defenderEmail?: string

67-70: Consider adding format validation for specific fields

For fields like phone numbers and national IDs, consider adding pattern validation using @Matches() decorator to ensure proper formatting:

  // Phone number
  @IsOptional()
  @MaxLength(255)
  @IsString()
+ @Matches(/^[0-9\-\+\s]+$/, { message: 'Invalid phone number format' })
  @ApiPropertyOptional({ type: String })
  readonly defenderPhoneNumber?: string

  // National IDs (both defendant and defender)
  @IsOptional()
  @MaxLength(255)
  @IsString()
+ @Matches(/^[0-9]{6}-?[0-9]{4}$/, { message: 'Invalid national ID format' })
  @ApiPropertyOptional({ type: String })
  readonly nationalId?: string

Also applies to: 55-58, 20-23

apps/judicial-system/backend/src/app/modules/subpoena/dto/updateSubpoena.dto.ts (3)

35-56: Consider adding field-specific validation rules.

While the MaxLength validation is correctly applied, these fields could benefit from additional validation:

  • defenderEmail: Add @isemail()
  • defenderPhoneNumber: Add a pattern validator for Icelandic phone numbers
  • defenderNationalId: Add a pattern validator for Icelandic national ID format

Example implementation:

@IsOptional()
@MaxLength(255)
@IsEmail()
@ApiPropertyOptional({ type: String })
readonly defenderEmail?: string

@IsOptional()
@MaxLength(255)
@Matches(/^(\+354 )?\d{3}[ -]?\d{4}$/)
@ApiPropertyOptional({ type: String })
readonly defenderPhoneNumber?: string

@IsOptional()
@MaxLength(255)
@Matches(/^\d{6}-?\d{4}$/)
@ApiPropertyOptional({ type: String })
readonly defenderNationalId?: string

64-73: Maintain consistency with primary defender field validations.

If you implement the suggested additional validations for the primary defender fields, please ensure the same validation rules are applied to these requested defender fields for consistency.


Case description field lacks length validation

The case description field ("efni kröfu") in both CreateCaseDto and UpdateCaseDto only has @IsString() validation but lacks @MaxLength() validation, unlike other string fields in the codebase. This should be addressed to prevent potential issues with overly long descriptions.

  • apps/judicial-system/backend/src/app/modules/case/dto/createCase.dto.ts: Add @MaxLength(255) to description field
  • apps/judicial-system/backend/src/app/modules/case/dto/updateCase.dto.ts: Add @MaxLength(255) to description field
🔗 Analysis chain

Line range hint 1-73: Verify case description validation implementation.

While this file correctly implements string length validation for subpoena-related fields, the PR objectives mention validating "efni kröfu" (case description). This validation isn't present in this file.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for case description validation in related DTOs
echo "Searching for case description validation..."
rg -l "efni kröfu|case description" apps/judicial-system/backend/src/app/modules/**/dto/

Length of output: 667


Script:

#!/bin/bash
# Let's try a broader search for validation patterns in DTOs and related files
# First, let's check for any files that might contain case description validation
rg -l "caseDescription|description|efni" apps/judicial-system/backend/src/app/modules/**/dto/

# Also search for class-validator decorators in DTO files
rg "@IsString|@MaxLength" apps/judicial-system/backend/src/app/modules/**/dto/

# Let's also check the types definition since we see imports from there
fd -e ts -e tsx . apps/judicial-system/types/ -x rg -l "caseDescription|description|efni" {}

Length of output: 19090


Script:

#!/bin/bash
# Let's check the case-related DTOs more specifically since we see them in the output
echo "Checking case DTOs content..."
rg -A 5 "class (Create|Update)Case" apps/judicial-system/backend/src/app/modules/case/dto/

# Also check for any description-related fields in case DTOs
echo "Checking for description fields in case DTOs..."
rg "description|efni|caseDescription" apps/judicial-system/backend/src/app/modules/case/dto/

Length of output: 1700


Script:

#!/bin/bash
# Let's check the validation decorators for the description field in both DTOs
echo "Checking validation for description field..."
rg -B 2 -A 1 "readonly description\?" apps/judicial-system/backend/src/app/modules/case/dto/updateCase.dto.ts apps/judicial-system/backend/src/app/modules/case/dto/createCase.dto.ts

Length of output: 1037

apps/judicial-system/backend/src/app/modules/case/dto/createCase.dto.ts (1)

43-45: Consider removing redundant validation

The field has both @IsNotEmpty and @ArrayMinSize(1) decorators, which are redundant as they serve the same purpose for arrays.

Consider keeping only @ArrayMinSize(1):

-  @IsNotEmpty()
   @IsArray()
   @ArrayMinSize(1)
   @MaxLength(255)
   @IsString({ each: true })
apps/judicial-system/backend/src/app/modules/defendant/dto/updateDefendant.dto.ts (2)

28-37: Consider additional validation rules for personal information

While the MaxLength validation is good, consider enhancing the validation rules:

  • For nationalId: Add a pattern validation to match the expected format
  • For name: Consider a reasonable minimum length validation

Example enhancement:

 @IsOptional()
 @MaxLength(255)
 @IsString()
+@Matches(/^[0-9]{6}-?[0-9]{4}$/, { message: 'Invalid national ID format' })
 @ApiPropertyOptional({ type: String })
 readonly nationalId?: string

 @IsOptional()
 @MaxLength(255)
+@MinLength(2)
 @IsString()
 @ApiPropertyOptional({ type: String })
 readonly name?: string

Line range hint 1-137: Document the rationale for character limits

The implementation consistently applies the 255-character limit across all string fields, which aligns well with the PR objectives. Consider adding a class-level JSDoc comment to document why this specific limit was chosen (e.g., database column constraints).

Example enhancement:

+/**
+ * DTO for updating defendant information.
+ * All string fields are limited to 255 characters to match the database varchar(255) constraints
+ * and prevent unexpected exceptions when writing to the database.
+ */
 export class UpdateDefendantDto {
apps/judicial-system/web/src/routes/Prosecutor/InvestigationCase/Defendant/Defendant.tsx (2)

Line range hint 332-341: Consider enhancing form validation feedback.

While the maxLength constraint is implemented, consider adding visual feedback to show the remaining character count, especially since this is a critical field with a strict limit.

 <Input
   data-testid="description"
   name="description"
   label={formatMessage(
     m.sections.investigationType.description.label,
   )}
   placeholder={formatMessage(
     m.sections.investigationType.description.placeholder,
   )}
   value={workingCase.description || ''}
   autoComplete="off"
+  hasError={workingCase.description?.length === 255}
+  errorMessage={workingCase.description?.length === 255 ? 
+    formatMessage(m.sections.investigationType.description.maxLengthError) : undefined}
+  helperText={`${workingCase.description?.length || 0}/255`}
   onChange={(evt) => {
     setWorkingCase((prevWorkingCase) => ({
       ...prevWorkingCase,
       description: evt.target.value,
     }))
   }}
   onBlur={(evt) =>
     setAndSendCaseToServer(
       [
         {
           description: evt.target.value.trim(),
           force: true,
         },
       ],
       workingCase,
       setWorkingCase,
     )
   }
   maxLength={255}
 />

Line range hint 332-341: Consider implementing form state management library.

The component handles complex form state with multiple fields and validation rules. Consider using a form management library like react-hook-form or formik to:

  • Reduce boilerplate code
  • Centralize validation logic
  • Improve performance by reducing rerenders
  • Handle form submission more efficiently
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 760b5fe and 84d5826.

📒 Files selected for processing (13)
  • apps/judicial-system/backend/src/app/modules/case/dto/createCase.dto.ts (3 hunks)
  • apps/judicial-system/backend/src/app/modules/case/dto/updateCase.dto.ts (8 hunks)
  • apps/judicial-system/backend/src/app/modules/defendant/dto/createDefendant.dto.ts (3 hunks)
  • apps/judicial-system/backend/src/app/modules/defendant/dto/updateCivilClaimant.dto.ts (3 hunks)
  • apps/judicial-system/backend/src/app/modules/defendant/dto/updateDefendant.dto.ts (4 hunks)
  • apps/judicial-system/backend/src/app/modules/file/dto/createFile.dto.ts (5 hunks)
  • apps/judicial-system/backend/src/app/modules/file/dto/createPresignedPost.dto.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/file/dto/updateFile.dto.ts (2 hunks)
  • apps/judicial-system/backend/src/app/modules/indictment-count/dto/updateIndictmentCount.dto.ts (2 hunks)
  • apps/judicial-system/backend/src/app/modules/subpoena/dto/updateSubpoena.dto.ts (4 hunks)
  • apps/judicial-system/backend/src/app/modules/user/dto/createUser.dto.ts (2 hunks)
  • apps/judicial-system/backend/src/app/modules/user/dto/updateUser.dto.ts (2 hunks)
  • apps/judicial-system/web/src/routes/Prosecutor/InvestigationCase/Defendant/Defendant.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
apps/judicial-system/backend/src/app/modules/case/dto/createCase.dto.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/case/dto/updateCase.dto.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/defendant/dto/createDefendant.dto.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/defendant/dto/updateCivilClaimant.dto.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/defendant/dto/updateDefendant.dto.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/dto/createFile.dto.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/dto/createPresignedPost.dto.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/dto/updateFile.dto.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/indictment-count/dto/updateIndictmentCount.dto.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/subpoena/dto/updateSubpoena.dto.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/user/dto/createUser.dto.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/user/dto/updateUser.dto.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/routes/Prosecutor/InvestigationCase/Defendant/Defendant.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (29)
apps/judicial-system/backend/src/app/modules/file/dto/createPresignedPost.dto.ts (3)

7-10: LGTM! Validation constraint added for fileName property.

The @maxlength(255) decorator properly validates the fileName length to prevent database exceptions.


13-16: LGTM! Validation constraint added for type property.

The @maxlength(255) decorator properly validates the type length to prevent database exceptions.


Line range hint 1-18: Verify MIME type length compatibility.

Since the type property likely stores MIME types, let's verify that 255 characters is sufficient for all possible MIME types.

apps/judicial-system/backend/src/app/modules/user/dto/updateUser.dto.ts (2)

7-7: LGTM! Import statement correctly added

The MaxLength validator import is properly added to the existing class-validator imports.


16-37: Verify consistent validation patterns across DTOs

Let's ensure these validation patterns are consistent with other DTOs in the codebase.

apps/judicial-system/backend/src/app/modules/user/dto/createUser.dto.ts (1)

7-7: LGTM: Clean import addition

The MaxLength validator import is correctly added to the existing class-validator imports.

apps/judicial-system/backend/src/app/modules/file/dto/updateFile.dto.ts (2)

10-10: LGTM: Import changes are well-organized

The addition of MaxLength to the validator imports is properly grouped with related validators.


Line range hint 1-58: LGTM: Well-structured DTOs with proper TypeScript usage

The DTOs demonstrate excellent TypeScript practices:

  • Proper use of readonly properties
  • Comprehensive validation decorators
  • Clear API documentation with Swagger
  • Proper nested validation for arrays
apps/judicial-system/backend/src/app/modules/indictment-count/dto/updateIndictmentCount.dto.ts (2)

7-7: LGTM: Import addition is correctly placed

The MaxLength decorator import is properly grouped with other validation imports from class-validator.


17-20: Verify consistent validation across related DTOs

The @MaxLength(255) constraint is correctly added to prevent database exceptions. However, let's ensure this validation is consistent with related DTOs.

apps/judicial-system/backend/src/app/modules/file/dto/createFile.dto.ts (5)

9-9: LGTM: Import statement is properly organized

The MaxLength import is correctly grouped with other validation decorators from class-validator.


40-43: LGTM: Consistent with other DTOs

The @MaxLength(255) decorator is consistently applied across DTOs for the policeCaseNumber field.


62-65: Verify the police file ID format

While the validation is correctly implemented, please verify that 255 characters aligns with the police system's file ID format.

✅ Verification successful

Police file ID validation is appropriate

The codebase analysis shows that policeFileId is consistently used as a UUID format across the application:

  • Test files use uuid() to generate police file IDs
  • The field is used to match files from the police system API
  • The database model shows it's stored as a string column without additional constraints

The 255-character limit is more than sufficient for UUID values (which are typically 36 characters) and provides future flexibility while maintaining reasonable bounds.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check police file ID format in the codebase
# Expected: Consistent format and length across usage

# Search for police file ID patterns
rg -t typescript "policeFileId.*=.*['\"]" -A 1 -B 1

Length of output: 90


Script:

#!/bin/bash
# Search for police file ID patterns and usage across the codebase
rg "policeFileId" -A 2 -B 2

# Also search for any test files that might contain sample police file IDs
rg "policeFileId.*['\"].*['\"]" -g "*.test.*" -g "*.spec.*" -A 1 -B 1

# Look for database schema or migration files that might define the field
fd -e sql | xargs cat

Length of output: 15111


18-21: LGTM: Type field validation is complete

The @MaxLength(255) decorator is appropriately added to prevent database exceptions.


29-32: Verify if 255 characters is sufficient for file keys

The validation is correctly implemented, but please verify that 255 characters is sufficient for file keys/paths in your storage system.

✅ Verification successful

Based on the search results, I can verify that 255 characters is sufficient for file keys. Here's why:

255 characters is sufficient for file keys

The codebase shows that file keys are primarily used for:

  1. S3 storage operations where 255 characters is well within S3's key length limits
  2. Database storage where 255 is a common and safe length for string fields
  3. File uploads where keys are typically UUIDs or short paths combined with filenames

No evidence was found of file keys approaching or exceeding this limit in the existing implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check existing file key lengths in the codebase
# Expected: No file keys exceeding or close to 255 characters

# Search for file key assignments or usage
rg -t typescript "key.*=.*'" -A 1 -B 1

Length of output: 78


Script:

#!/bin/bash
# Search for file key assignments or usage with ripgrep
rg "key.*=.*['\"]" --type ts -A 1 -B 1

# Also search for file-related operations that might involve keys
rg "createFile|uploadFile|fileKey" --type ts -A 2 -B 2

# Look for any existing file storage configurations
fd -e ts -e js -e json -X rg "storage|bucket|filepath"

Length of output: 267434

apps/judicial-system/backend/src/app/modules/defendant/dto/updateCivilClaimant.dto.ts (2)

12-14: LGTM: Personal information field validations are properly implemented.

The @MaxLength(255) decorator has been correctly added to the name and nationalId fields, preventing potential database exceptions.

Also applies to: 18-20


34-36: LGTM: Spokesperson information field validations are properly implemented.

The @MaxLength(255) decorator has been correctly added to all spokesperson-related string fields:

  • spokespersonNationalId
  • spokespersonName
  • spokespersonEmail
  • spokespersonPhoneNumber

Also applies to: 40-42, 46-48, 52-54

apps/judicial-system/backend/src/app/modules/defendant/dto/createDefendant.dto.ts (2)

1-7: LGTM: Import statements are correctly updated

The MaxLength decorator is properly imported from class-validator along with other necessary decorators.


Line range hint 20-70: LGTM: Consistent length validation implementation

The addition of @MaxLength(255) decorators to all string fields is a good approach to prevent database exceptions. The implementation is consistent across all fields and aligns well with the PR objectives.

Let's verify the database column constraints match these validations:

apps/judicial-system/backend/src/app/modules/subpoena/dto/updateSubpoena.dto.ts (1)

1-1: LGTM! Validation decorator properly imported and applied.

The MaxLength decorator is correctly imported and appropriately applied to the servedBy field, aligning with typical varchar(255) database constraints.

Also applies to: 14-17

apps/judicial-system/backend/src/app/modules/case/dto/createCase.dto.ts (3)

9-9: LGTM: Import statement is correctly placed

The MaxLength decorator import is properly added alongside other validation decorators.


78-81: LGTM: Consistent validation implementation

The validation for the lead investigator field follows the same pattern as other optional string fields.


35-38: Verify database column constraint

The validation rule aligns with the PR objectives for case description. The decorator order follows best practices.

Let's verify the database column constraint matches this validation:

apps/judicial-system/backend/src/app/modules/defendant/dto/updateDefendant.dto.ts (1)

8-8: LGTM: Import statement is correctly placed

The MaxLength decorator import is properly grouped with other validation decorators from class-validator.

apps/judicial-system/backend/src/app/modules/case/dto/updateCase.dto.ts (3)

12-12: LGTM: Import addition is correct

The MaxLength decorator import is properly added alongside other validation decorators.


47-50: LGTM: Proper validation for location field

The MaxLength validation is correctly implemented with proper decorator ordering (optional -> validation -> type).


Line range hint 65-422: LGTM: Consistent string length validation

The MaxLength(255) validation has been consistently applied across all string fields, maintaining proper decorator ordering and optional status.

apps/judicial-system/web/src/routes/Prosecutor/InvestigationCase/Defendant/Defendant.tsx (2)

339-339: LGTM: String length validation implemented correctly.

The maxLength={255} property on the Input component aligns with the backend validation requirements for the case description field.


Line range hint 332-341: Verify error handling for server-side validation failures.

The current implementation handles client-side validation, but we should ensure proper error handling for cases where server-side validation fails.

✅ Verification successful

Let me analyze the error handling in the context of the review comment. I'll search for error handling patterns around the specific file and component.


Based on the analysis of the code, I can now provide a final response regarding the server-side validation concern.

Server-side validation is properly handled through the application's error handling system

The codebase shows comprehensive error handling through:

  1. GraphQL error policies with errorPolicy: 'all' configuration
  2. Toast notifications for server errors using toast.error()
  3. Client-side validation with removeTabsValidateAndSet() utility that:
    • Handles input validation
    • Sets error messages in the UI
    • Updates the working case state

The component in question is part of a form system that properly propagates and displays validation errors from both client and server sides through the established error handling patterns.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for error handling patterns in API calls and form submissions
rg -A 5 "catch|error|fail" apps/judicial-system/web/src/

# Look for toast or error message implementations
ast-grep --pattern 'toast.error($$$)'

Length of output: 246469


Script:

#!/bin/bash
# Search for error handling in the Defendant component and related files
rg -A 5 "catch|error|fail" apps/judicial-system/web/src/routes/Prosecutor/InvestigationCase/Defendant/

# Look for validation and error handling utilities used
rg -A 5 "validate|setError|errorMessage" apps/judicial-system/web/src/routes/Prosecutor/InvestigationCase/Defendant/

# Search for form submission and API call patterns
ast-grep --pattern 'removeTabsValidateAndSet($$$)'

Length of output: 44770

@gudjong gudjong added the automerge Merge this PR as soon as all checks pass label Nov 18, 2024
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.46%. Comparing base (d33043f) to head (1562e0c).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #16924   +/-   ##
=======================================
  Coverage   36.46%   36.46%           
=======================================
  Files        6903     6903           
  Lines      144580   144580           
  Branches    41284    41284           
=======================================
  Hits        52718    52718           
  Misses      91862    91862           
Flag Coverage Δ
air-discount-scheme-web 0.00% <ø> (ø)
api 3.34% <ø> (ø)
application-api-files 62.35% <ø> (ø)
application-core 71.06% <ø> (+0.07%) ⬆️
application-system-api 41.02% <ø> (ø)
application-template-api-modules 27.72% <ø> (ø)
application-templates-accident-notification 28.98% <ø> (ø)
application-templates-car-recycling 3.12% <ø> (ø)
application-templates-criminal-record 25.87% <ø> (ø)
application-templates-driving-license 18.14% <ø> (ø)
application-templates-estate 12.14% <ø> (ø)
application-templates-example-payment 24.80% <ø> (ø)
application-templates-financial-aid 15.48% <ø> (ø)
application-templates-general-petition 23.07% <ø> (ø)
application-templates-inheritance-report 6.52% <ø> (ø)
application-templates-marriage-conditions 15.04% <ø> (ø)
application-templates-mortgage-certificate 43.36% <ø> (ø)
application-templates-parental-leave 29.74% <ø> (-0.12%) ⬇️
application-types 6.60% <ø> (ø)
application-ui-components 1.27% <ø> (ø)
application-ui-shell 20.83% <ø> (ø)
clients-charge-fjs-v2 24.11% <ø> (ø)
judicial-system-backend 55.53% <100.00%> (ø)
judicial-system-web 27.16% <ø> (ø)
web 1.77% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...backend/src/app/modules/case/dto/createCase.dto.ts 100.00% <ø> (ø)
...backend/src/app/modules/case/dto/updateCase.dto.ts 86.72% <ø> (ø)
...c/app/modules/defendant/dto/createDefendant.dto.ts 100.00% <100.00%> (ø)
...p/modules/defendant/dto/updateCivilClaimant.dto.ts 100.00% <100.00%> (ø)
...c/app/modules/defendant/dto/updateDefendant.dto.ts 92.85% <ø> (ø)
...backend/src/app/modules/file/dto/createFile.dto.ts 93.75% <ø> (ø)
...rc/app/modules/file/dto/createPresignedPost.dto.ts 100.00% <100.00%> (ø)
...backend/src/app/modules/file/dto/updateFile.dto.ts 73.33% <ø> (ø)
.../indictment-count/dto/updateIndictmentCount.dto.ts 100.00% <ø> (ø)
...src/app/modules/subpoena/dto/updateSubpoena.dto.ts 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d33043f...1562e0c. Read the comment docs.

---- 🚨 Try these New Features:

Copy link
Member

@unakb unakb left a comment

Choose a reason for hiding this comment

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

lgtm !

@datadog-island-is
Copy link

Datadog Report

All test runs 72dae4c 🔗

2 Total Test Services: 0 Failed, 2 Passed
➡️ Test Sessions change in coverage: 1 no change

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
judicial-system-backend 0 0 0 19347 0 0s N/A Link
judicial-system-web 0 0 0 333 0 56.18s 1 no change Link

@kodiakhq kodiakhq bot merged commit a8e03f5 into main Nov 18, 2024
91 checks passed
@kodiakhq kodiakhq bot deleted the j-s/max-length-validation branch November 18, 2024 16:21
@oddsson oddsson mentioned this pull request Nov 18, 2024
6 tasks
jonnigs pushed a commit that referenced this pull request Nov 26, 2024
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this PR as soon as all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants