Skip to content

Conversation

@pidsonn
Copy link
Contributor

@pidsonn pidsonn commented Jun 27, 2025

…a and its mapping tests

Description of what I changed

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-5861

Checklist: I completed these to help reviewers :)

  • [x ] My IDE is configured to follow the code style of this project.

    No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

  • [ x] I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)

    No? -> write tests and add them to this commit git add . && git commit --amend

  • [ x] I ran mvn clean package right before creating this pull request and
    added all formatting changes to my commit.

    No? -> execute above command

  • [ x] All new and existing tests passed.

    No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

  • [ x] My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

@pidsonn
Copy link
Contributor Author

pidsonn commented Aug 29, 2025

Hi @ibacher, could you please help approve the CI checks for this PR (#5095: TRUNK-5861 - Add DrugIngredientId and tests, update DrugIngredient.java)? Thank you!

@pidsonn
Copy link
Contributor Author

pidsonn commented Sep 2, 2025

Hi @ibacher,
I’ve just updated this branch with the master. The workflows are now awaiting approval — could you please approve them so CI can run?
Thanks!

@pidsonn
Copy link
Contributor Author

pidsonn commented Sep 3, 2025

Hi @ibacher,
I’ve fixed the malformed pom.xml and removed the duplicate dependency. Could you please approve the CI checks so that they can run?
Thanks!

@pidsonn
Copy link
Contributor Author

pidsonn commented Sep 29, 2025

Hi @ibacher,
Could you please approve the CI checks (TRUNK-5861: #5095 )
Thanks!

@pidsonn pidsonn reopened this Nov 10, 2025
@pidsonn
Copy link
Contributor Author

pidsonn commented Nov 10, 2025

Hi@dkayiwa,
TRUNK-5861: Switch DrugIngredient domain from Hibernate mapping to an… needs review and approval of workflows. Please could you help?
Thank you.

@Id
@MapsId("drugId")
@JoinColumn(name = "drug_id", nullable = false)
private Drug drug;
Copy link
Member

Choose a reason for hiding this comment

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

How are these changes related to the ticket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks — great question!
The change is part of the migration from Hibernate XML mapping to JPA annotations and doesn’t alter the model itself.

Explanation:

. drug_id and ingredient_id are a composite primary key (both were primaryKey="true" in the Liquibase snapshot).

. JPA doesn’t support having two separate @id annotations directly on @manytoone relations in a clean, maintainable way.
To represent the same composite key structure using annotations, we:

  1. Introduced DrugIngredientId (@embeddable) to hold drugId and ingredientId.

  2. Annotated the entity with @EmbeddedId private DrugIngredientId id.

  3. Used @mapsid("drugId") and @mapsid("ingredientId") on the @manytoone relations so they map into the embedded ID fields — this mirrors the composite-id behavior from the old .hbm.xml.

In short:
Both drug_id and ingredient_id form a composite primary key, so we switched to using @EmbeddedId + @mapsid, which is the standard JPA approach for this case.
If you’d prefer the older style mapping, I can adjust accordingly.

Also, if you think it would help, I can add a one-line comment near the @mapsid annotations to make this intent clearer in the code.

@dkayiwa
Copy link
Member

dkayiwa commented Nov 10, 2025

Can you include a link to the ticket as advised at? https://openmrs.atlassian.net/wiki/display/docs/Pull+Request+Tips

@pidsonn pidsonn changed the title TRUNK-5861: Add DrugIngredientId and tests, update DrugIngredient.jav… TRUNK-5861: Switch DrugIngredient domain from Hibernate mapping to annotations Nov 11, 2025
@pidsonn
Copy link
Contributor Author

pidsonn commented Nov 11, 2025

Can you include a link to the ticket as advised at? https://openmrs.atlassian.net/wiki/display/docs/Pull+Request+Tips

@dkayiwa, Thank you for the reminder. I’ve already included the link to the JIRA ticket in the PR description under the “Issue I worked on” section.

@dkayiwa
Copy link
Member

dkayiwa commented Nov 11, 2025

Isn't this already done here? https://openmrs.atlassian.net/browse/TRUNK-6238

@pidsonn
Copy link
Contributor Author

pidsonn commented Nov 11, 2025

Isn't this already done here? https://openmrs.atlassian.net/browse/TRUNK-6238

@dkayiwa , Yes i have checked and it's true it is already done but it remained open on jira board where i picked it from.

Thank you for that clarification and reference!

@dkayiwa
Copy link
Member

dkayiwa commented Nov 11, 2025

Sorry it looked like someone created a duplicate ticket. Do you mind closing it and the pull request?

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.

2 participants