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

Refactoring aggregates #125

Open
arey opened this issue Nov 26, 2023 · 2 comments
Open

Refactoring aggregates #125

arey opened this issue Nov 26, 2023 · 2 comments

Comments

@arey
Copy link
Member

arey commented Nov 26, 2023

This issue is related to the conversation from the pull request #123 and the 2 issues #122 and #103

To fix the issues without changing the API specification, we temporary had the operations findSpecialtiesByName and findPetTypeByName.

Vet, Specialty (and maybe PetType) are different aggregates and should probably be linked by their ids for writing operations. A better solution should be to change the OpenAPI interface (and the Angular client). We could imagine that for reading operation, in addition to their id, the name property of aggregates is returned to consumers.

@dnokov
Copy link
Contributor

dnokov commented Dec 17, 2023

Issue with DTO Usage in Different Contexts


I think at it's core the problem is caused, because the same DTOs are being used in different contexts and both contexts have conflicting requirements.What I've written is regaring PetType, but the same applies to Specialty

PS: Writing this as I don't want to jump straight to a solution, maybe someone will have a better idea.

PetType Creation Context:

  • In the context of creating a PetType, the id field is marked as readOnly: true in the OpenAPI specification. This is appropriate since the id is not needed in the request body during creation.
  • However, this leads to a conflict in another context.

Creating a Pet/Modifying a PetType Context:

  • When creating or modifying a Pet, the PetType's id is required for mapping purposes. The current workaround involves using the name, which is not ideal.
  • The readOnly: true attribute in this context does not align with the actual requirements, creating inconsistency.
    angularClientRequestFormat

Specification Definition of readOnly: true:

spec definition

Relevant only for Schema "properties" definitions. Declares the property as "read only". This means that it MAY be sent as part of a response but SHOULD NOT be sent as part of the request. If the property is marked as readOnly being true and is in the required list, the required will take effect on the response only. A property MUST NOT be marked as both readOnly and writeOnly being true. Default value is false.

- Notably, a setter is still generated even if a field is declared as `readOnly: true`, which may not be intuitive.
Details on what purpose "readOnly:true" servers in this use case

`readOnly:True` for `id` allows the DTO to be used for creation of a `petType` as the id is no longer required in the request body, but the same will be in effect when having a `PetType` within a `Pet`. So I'm guessing it has been added as a workaround to the validation of the `id` field when creating a pet type.

image

(Taken from PetTypeDTO generated class, with id readOnly: true and not required)

Possible approach:

I see a few, but this one makes the most sense as it will have a consistent API spec and will not require any hacky workarounds.

  • Creating a DTO that requires only a name when creating a petType, and removing the readOnly from PetType.id
    PetType:
     ...
          properties:
              ...
              readOnly: true <- remove this
          required:
            - id

and create PetTypeCreationDTO / or reuse petFieldsDTO and use it for creating the object, that way the specification will be completely adhered to

arey added a commit to arey/spring-petclinic-rest that referenced this issue Dec 31, 2023
@arey
Copy link
Member Author

arey commented Dec 31, 2023

Hi @dnokov
Thank you very much for your analysis and your suggestion.
I agree with your approach.
I prefer to reuse the PetFieldsDTO spec.
I've done the exercice on the PR #125 for the PetType

To add a pet to an owner, we could now use the pet id.
But we still have to post a pet name even if we don't use this value.

POST /petclinic/api/owners/5/pets
{
  "name": "Leo",
  "birthDate": "2023-12-31",
  "type": {
    "name": "cat",
    "id": 6
  }
}

arey added a commit that referenced this issue Jul 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants