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: issue130 Return a 404 error while adding a pet to an unexisting owner #138

Merged
merged 3 commits into from
Aug 17, 2024

Conversation

earlspilner
Copy link
Contributor

@earlspilner earlspilner commented Aug 10, 2024

Improved handling of 404 errors for non-existing owners and pets in the POST /owners/{ownerId}/pets endpoint. Also switched to using records for error information to simplify and modernize the code.

This PR addresses #130 and enhances both the error handling and the structure of the error responses.

Changes:

  1. Fixed the endpoint to return a 404 Not Found status for non-existing owners or pets instead of a 400 Bad Request.
  2. Refactored error handling by replacing the previous class-based error representation with Java record, which stands for a more concise and immutable way to handle error details.
  3. Few test changes, because the old version was wrong

@arey, please review and merge this update.

@earlspilner
Copy link
Contributor Author

Okay, finally this build was successful :)
I've made some changes in tests, because from my point there're mistakes

@earlspilner earlspilner changed the title Fix/issue130 fix: issue130 Aug 11, 2024
@arey arey added the bug label Aug 15, 2024
@arey arey self-assigned this Aug 15, 2024
@arey
Copy link
Member

arey commented Aug 15, 2024

Hi @earlspilner
Thank your for your contribution
In the openapi.yaml file, mayve change the Pet not found. by Owner not found?

        404:
          description: Pet not found.
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/RestError'

@arey arey changed the title fix: issue130 fix: issue130 Return a 404 error while adding a pet to an unexisting owner Aug 15, 2024
@earlspilner
Copy link
Contributor Author

In the openapi.yaml file, mayve change the Pet not found. by Owner not found?

I hear you, but here’s the thing — sometimes the issue might not just be with the owner. It could be that neither the ownerId nor the typeId is found in the request. So if we just say "Owner not found," it might not cover all the bases. I’d suggest we go with something like "Pet or Owner not found" to make sure we’re addressing any possible issue that could come up.

@arey
Copy link
Member

arey commented Aug 15, 2024

"Pet or Owner not found"
I'm agree. I didn't think to the pet's type. Good catch.

@earlspilner
Copy link
Contributor Author

I'm agree. I didn't think to the pet's type. Good catch.

I’ve made the commit and everything’s ready for the merge.

@arey
Copy link
Member

arey commented Aug 17, 2024

All it's ok. Thanks a lot. Feel free to contribute to any other petclinic project / issue.

@arey arey merged commit 311c3ce into spring-petclinic:master Aug 17, 2024
1 check passed
@earlspilner earlspilner deleted the fix/issue130 branch August 17, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants