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

Added findByPetTypeByName and findSpecialtiesByName, caching responses #123

Conversation

dnokov
Copy link
Contributor

@dnokov dnokov commented Nov 13, 2023

Purpose


These changes aim to fix issues #122 #103 while maintaining API first approach.

What and Why


Key changes
  • Added findByPetTypeByName
  • Added findSpecialtiesByName
  • Added caching logic

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

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());
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@arey
Copy link
Member

arey commented Nov 21, 2023

Hi @dnokov

Searching by PetType and Speciality names looks like a hack to fix the both issues without changing the OpenAPI interface contract.

Like @stevenvanophem said in the issue #103, Vet, Specialty (and maybe PetType) are different aggregates and should probably be linked by their ids for writing operations. Thus the best solution should be to change the OpenAPI interface (and the Angular client). We could image that for reading operation, in addition to their id, the name property of aggregates is returned to consumers.

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?

@alexandre-touret
Copy link
Contributor

Hi
IMO I'm agree w/ you about updating the OpenAPI specification and splitting this PR into 2 (cache & fix)

BTW, I think you can merge it.

@arey
Copy link
Member

arey commented Nov 26, 2023

@dnokov could you please open a new PR for the caching part?

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.

4 participants