Added findByPetTypeByName and findSpecialtiesByName, caching responses #123
Conversation
Adding these methods both at repository, service and controller levels, caching responses as they perform a database call in order to obtain the PetType and Specialty objects. Invalidating cache on updates/deletes.
| @@ -140,6 +140,8 @@ public ResponseEntity<PetDto> addPetToOwner(Integer ownerId, PetFieldsDto petFie | |||
| Owner owner = new Owner(); | |||
| owner.setId(ownerId); | |||
| pet.setOwner(owner); | |||
| PetType petType = this.clinicService.findPetTypeByName(pet.getType().getName()); | |||
There was a problem hiding this comment.
For the past few years, I've almost not followed changes in this repo, but it seems like you trying to solve the existing OpenAPI issue by changing the implementation. Maybe it's the right way for "api first" approach, but we added OpenAPI support layer over the existing code. so I'm not sure.
The provided changes look strange, but I guess @arey can decide if they solve the problem.
There was a problem hiding this comment.
Hey Vitaliy, thanks for the review.
You're correct, I'm trying to solve the issue in an "api first" approach.
I understand that you can't decide, if the changes solve the issue or not, but I'd be more than happy to address any code smells you've found.
|
Hi @dnokov Searching by Like @stevenvanophem said in the issue #103, Waiting this big change we could merge your pull request. Maybe split it in both MR (cache and temporary fix) @alexandre-touret @vfedoriv @stevenvanophem what is your opinion? |
|
Hi BTW, I think you can merge it. |
|
@dnokov could you please open a new PR for the caching part? |
Purpose
These changes aim to fix issues #122 #103 while maintaining API first approach.
What and Why
Key changes
I've taken an oppinioned approach, I'm requesting specialties/pettypes by name in order to retrieve their IDs and thus ignoring the "id" in PetTypeDTO and SpecialtyDTO, however I've not made any changes to the DTO as to not break clients.
PetTypes and Specialties seem like good data to cache as frequent changes to the specialties/pettypes are not expected, so I've also added caching logic in order to reduce load on the database, as this solution naturally puts more load onto it.
Validation
Already existing integration tests and created additional ones for finding pettype and specialties
I apologise for the large commit, will split if needed/fix any issues with code quality