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

Add bidirectional association for patients in person mapping #1083

Merged
merged 16 commits into from
Jun 25, 2024

Conversation

rahul6603
Copy link
Contributor

@rahul6603 rahul6603 commented Jun 10, 2024

Issue - #1049

Summary

  • We will add <set> element to Person mapping to establish bidirectional association with Patient.
  • This change is needed to prevent the Hibernate Search exception caused when using @IndexedEmbedded with a unidirectional association. We will be using @IndexedEmbedded to embed the fields of Person object in the main Patient object.
  • Reference documentation

Copy link
Contributor

@CalebSLane CalebSLane left a comment

Choose a reason for hiding this comment

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

A few questions for @rahul6603:

How does this work with the Provider class? I see that the Person class does include a Set for holding Patients (which should be Set<Patient> instead of just Set as it is now), but the Person object is also used by the Provider class. Does this class need to get a mapping like this for Provider as will, and does adding the mapping cause either class to be unhappy?

Does saving logic need to be reviewed and changed? Will saving a Person with an empty Set<Patient>, then adding the Person to the Patient, and saving the Patient (without adding the Set<Patient> to the Person) allow entities to be saved properly? Does the bi-directionality demand that the Patient be added into the Person object's Set<Patient> as well?

Are there integration or unit tests to test these cases? There are a few classes with methods that call the insert, save, and/or update on these objects that I'm unsure of how they'll handle the change ex persistPatientData in PatientManagementUpdate.java

@rahul6603
Copy link
Contributor Author

rahul6603 commented Jun 13, 2024

Hi @CalebSLane, thanks for the review!

  1. We need bidirectional association between Patient and Person entity for @IndexedEmbedded to work (reference), you can see the use of @IndexedEmbedded in this draft PR - Add mapping for indexing Patient and Person entities #1095. This should not affect other unidirectional associations such as the one between Provider and Person.
    PS - Thanks for mentioning Set should be Set<Patient>, I have made that change in the mentioned PR.

  2. Yes, bidirectional associations require updates on both sides of the association when we persist an entity. But in our case, there is no usage of person.getPatients() in the codebase, so I don't think change in the persistence logic is needed. (reference)

  3. I don't think the unit tests and integration tests exist for these cases. I will add them as part of my GSoC project in the upcoming weeks.

Please let me know if you can think of any side effects this change might cause.
Thank you.

@rahul6603
Copy link
Contributor Author

Hi @mozzy11 and @CalebSLane, could you please review this PR?
Thank you.

@github-actions github-actions bot added the merge conflict Merge Conflicts label Jun 17, 2024
Copy link

👋 Hi, @rahul6603,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@github-actions github-actions bot removed the merge conflict Merge Conflicts label Jun 18, 2024
@mozzy11
Copy link
Collaborator

mozzy11 commented Jun 19, 2024

Thanks @rahul6603 .
The change here calls for modifying the patients field in the person class from Set patients to Set<Patient> patients .
Please do all that change in this same PR and add some integration tests on the person.getPatients() method to ensure no Logic breaks.
We already have Person integration tests set up . so its a matter of adding a few tests to ensure we are safe

@mozzy11
Copy link
Collaborator

mozzy11 commented Jun 19, 2024

@CalebSLane , In theory , no change is needed in the saving Logic and neither for the Provider Class.
The maapings are generally used internally by hibernate to map associated parent and child objects . in otherwords instead of writing manual queries to get associated objects in case no hibernamte maaping exists , one can just do person.getPatients() , and hibernate does the querying internally

Copy link
Collaborator

@mozzy11 mozzy11 left a comment

Choose a reason for hiding this comment

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

see above comment

@rahul6603 rahul6603 marked this pull request as draft June 24, 2024 06:13
@rahul6603 rahul6603 marked this pull request as ready for review June 24, 2024 14:30
@mozzy11 mozzy11 merged commit 2c5d874 into I-TECH-UW:develop Jun 25, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants