Skip to content

fix: Update GET proof record endpoint to return 404 for non-existing records #1190

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

Merged
merged 1 commit into from
Apr 25, 2025

Conversation

amrrdev
Copy link
Contributor

@amrrdev amrrdev commented Apr 22, 2025

Issue Fixed:
I addressed the error handling in the proof record retrieval endpoint where it was returning a 500 status code for non-existing records. The issue was in the verification service's error handling logic.

Changes Made:

  1. Modified error handling in verification.service.ts for the getProofPresentationById method
  2. Updated the error detection to properly identify "record not found" cases
  3. Implemented proper 404 response using NestJS's NotFoundException
  4. Maintained the original error message for clarity

Before:

  • API returned 500 status code with a generic error
  • Error structure was inconsistent with API standards
  • Generic "Something went wrong!" message wasn't helpful

After:

  • API now returns 404 status code for non-existing records
  • Clear error message indicates the specific proofId that wasn't found
  • Consistent error structure throughout the API
  • Better error handling helps client applications handle missing records appropriately

This change improves the API's error handling consistency and provides better feedback for client applications trying to access non-existing proof records.

@GHkrishna
Copy link
Contributor

Thank you for the PR @amrrdev

I've left some comments and requested a review from @bhavanakarwade

Also, in the meantime can you please check DCO since it seems to be failing

@GHkrishna GHkrishna linked an issue Apr 23, 2025 that may be closed by this pull request
@amrrdev amrrdev force-pushed the main branch 2 times, most recently from 884ea85 to 22e6439 Compare April 24, 2025 06:11
Copy link
Contributor

@GHkrishna GHkrishna left a comment

Choose a reason for hiding this comment

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

Looks good to me just requesting some minor changes

Copy link
Contributor

@GHkrishna GHkrishna left a comment

Choose a reason for hiding this comment

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

Thank for the contribution and for your patience @amrrdev

@GHkrishna
Copy link
Contributor

Some of your commits don't seem to be verified, can you please check them once?

@amrrdev amrrdev force-pushed the main branch 5 times, most recently from 4382c9d to 2a5555b Compare April 25, 2025 06:39
…records

Signed-off-by: Amr Mubarak <amrrdev@gmail.com>
Copy link

@amrrdev
Copy link
Contributor Author

amrrdev commented Apr 25, 2025

@GHkrishna, I have verified the commit.

@GHkrishna GHkrishna merged commit 57f74e9 into credebl:main Apr 25, 2025
4 checks passed
@GHkrishna
Copy link
Contributor

That's great, thank you for your contribution!

@GHkrishna GHkrishna added the bug Something isn't working label Apr 25, 2025
@bhavanakarwade
Copy link
Contributor

@amrrdev, Thank you for your patience and contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: Error handling for missing proof record by proof Id
3 participants