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

Cannot POST to /petclinic/api/pets #139

Open
breedx-splk opened this issue Aug 12, 2024 · 5 comments
Open

Cannot POST to /petclinic/api/pets #139

breedx-splk opened this issue Aug 12, 2024 · 5 comments
Labels

Comments

@breedx-splk
Copy link
Contributor

It used to be that POSTing to /petclinic/api/pets was a reasonable and functioning way to create a pet. This is simply no longer the case. It doesn't work.

The Pet model object has an owner field of type Owner but the DTOs don't seem to wire this up correctly from the input JSON, and when we reach this.clinicService.savePet(pet); in the PetController, the call fails because the db requires a non-null foreign key for owner.id. This change has broken existing API clients, such as my k6 test harness that I use to drive traffic.

The example(s) in the swagger UI also don't work and result in a 400.

The current workaround is to POST to /petclinic/api/owners/{id}/pets instead...which is fine, but there shouldn't be two apis that apparently do the same thing (create a pet associated with an owner).

I don't know enough about this mapstruct magic to propose a solution that fixes the current dto shortcoming, but an alternative approach would be to just remove POST from /petclinic/api/pets from the public API surface.

@arey arey added the bug label Aug 15, 2024
@arey
Copy link
Member

arey commented Aug 15, 2024

Having 2 endpoints to add a pet to an existing owner may be discussed. I'm not sure there's any point to it.

Technically, the Pet schema has a readonly ownerId property. We could remove the readonly flag and lookup the owner. We need to check that there is no side effect. I will create an issue to have a non-regression test harness

Maybe we have to remove the POST to the /petclinic/api/pets ressource. @alexandre-touret what do you think?

@alexandre-touret
Copy link
Contributor

Hi @arey
I'm currently off.
I'll get back to you next week

@alexandre-touret
Copy link
Contributor

Hi,
I would say, it depends of the functional requirements.

If in a functional point of view, creating Pet without any ownerId doesn't mean a thing, I think the solution proposed by @breedx-splk would be OK.

In my view, regarding the constraints of the schema, having two ways to create a Pet is weird.

@arey
Copy link
Member

arey commented Aug 24, 2024

Thanks Alexandre for your feedback. Let's simplify the API by removing the POST to the /petclinic/api/pets ressource.
Do you want to take it in charge @breedx-splk ?

@breedx-splk
Copy link
Contributor Author

@arey Sorry I don't have time to pick this up in the near future. Thanks for the consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants