Skip to content

fix: fix error handling#19

Merged
tarek-gritli merged 1 commit intomainfrom
exceptions-filter
Jun 7, 2025
Merged

fix: fix error handling#19
tarek-gritli merged 1 commit intomainfrom
exceptions-filter

Conversation

@tarek-gritli
Copy link
Collaborator

@tarek-gritli tarek-gritli commented Jun 7, 2025

PR Type

Bug fix, Enhancement


Description

  • Refined error handling in AuthService and FarmService

    • In AuthService, narrowed exception re-throwing to only BadRequestException
    • In FarmService, removed redundant try-catch blocks for cleaner code
    • Now only domain-specific exceptions are thrown, reducing unnecessary internal errors
  • Improved code clarity and maintainability by simplifying control flow


Changes walkthrough 📝

Relevant files
Bug fix
auth.service.ts
Refined error handling for email verification                       

src/auth/auth.service.ts

  • Updated error handling in verification email logic
  • Now only re-throws BadRequestException instead of both
    NotFoundException and BadRequestException
  • Reduces unnecessary internal server errors for not found cases
  • +1/-5     
    Enhancement
    farm.service.ts
    Simplified and improved error handling logic                         

    src/farm/farm.service.ts

  • Removed redundant try-catch blocks from all service methods
  • Now throws only domain-specific exceptions (ForbiddenException,
    NotFoundException)
  • Eliminated unnecessary InternalServerErrorException wrapping
  • Simplified and clarified method control flow
  • +33/-57 

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-code-review
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The change now only re-throws BadRequestException but not NotFoundException. Verify this is intentional and won't cause issues with error reporting for not found cases.

    if (error instanceof BadRequestException) {
      throw error;
    }
    Error Propagation

    The removal of try-catch blocks means database errors will now propagate directly to callers. Ensure this is the intended behavior and that callers can handle these errors appropriately.

    async createFarm(createFarmDto: CreateFarmDto, userId: number) {
      const existingFarm = await this.prisma.farm.findUnique({
        where: { user_id: userId },
      });
    
      if (existingFarm) {
        throw new ForbiddenException('User already has a farm');
      }
      return await this.prisma.farm.create({
        data: {
          name: createFarmDto.name,
          location: createFarmDto.location,
          area_unit: createFarmDto.areaUnit,
          total_area: createFarmDto.totalArea,
          user: { connect: { id: userId } },
        },
      });
    }

    @github-actions
    Copy link

    github-actions bot commented Jun 7, 2025

    Failed to generate code suggestions for PR

    @qodo-code-review
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Handle NotFoundException properly

    The error handling now ignores NotFoundException but doesn't handle it properly.
    Since the intent is to hide user existence, you should still catch
    NotFoundException but return the same message as a successful operation rather
    than converting it to an internal server error.

    src/auth/auth.service.ts [196-200]

     if (error instanceof BadRequestException) {
       throw error;
    +}
    +if (error instanceof NotFoundException) {
    +  return {
    +    message: 'If this email is registered and not verified, a verification email has been sent. Please check your inbox or spam folder.',
    +  };
     }
     throw new InternalServerErrorException(
       error,
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: This addresses a valid security concern by preventing information disclosure about user existence. The suggestion correctly identifies that NotFoundException should return the same success message rather than becoming an internal server error, maintaining the security pattern of not revealing whether an email exists.

    Medium
    • More

    @tarek-gritli tarek-gritli merged commit c02fd92 into main Jun 7, 2025
    1 check passed
    @tarek-gritli tarek-gritli deleted the exceptions-filter branch June 7, 2025 14:38
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    1 participant