Skip to content

Trunk 5866 : OrderSet Domain - Switching from Hibernate Mappings to Annotations #5472

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Trunk 5866 : OrderSet Domain - Switching from Hibernate Mappings to Annotations #5472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0cc41b4
TRUNK-5866: OrderSet Domain - Switching from Hibernate Mappings to An…
sudhanshu-raj Nov 10, 2025
565e9fe
TRUNK-5866: Removed unused import that I had used earlier for testing
sudhanshu-raj Nov 10, 2025
1b8e0a6
Overriding attributes field column name in OrderSet
sudhanshu-raj Nov 10, 2025
ffc3856
TRUNK-5866:Replaced Generationtype of SEQUENCE with IDENTITY
sudhanshu-raj Nov 12, 2025
206cad7
TRUNK-5866:Removed unused imports for previous of my chnage
sudhanshu-raj Nov 13, 2025
4f7b185
Merge branch 'master' into TRUNK-5866
dkayiwa Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions api/src/main/java/org/openmrs/OrderSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,23 @@
import java.util.ArrayList;
import java.util.List;

import jakarta.persistence.Entity;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.Id;
import jakarta.persistence.Table;
import jakarta.persistence.Column;
import jakarta.persistence.GenerationType;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.OneToMany;
import jakarta.persistence.OrderColumn;
import jakarta.persistence.Enumerated;
import jakarta.persistence.EnumType;
import jakarta.persistence.CascadeType;
import jakarta.persistence.AssociationOverride;
import org.hibernate.annotations.JdbcTypeCode;
import org.hibernate.envers.Audited;
import org.hibernate.type.SqlTypes;
import org.openmrs.api.APIException;

/**
Expand All @@ -21,7 +37,13 @@
*
* @since 1.12
*/
@Entity
@Table(name = "order_set")
@Audited
@AssociationOverride(
name = "attributes",
Copy link
Member

@dkayiwa dkayiwa Nov 12, 2025

Choose a reason for hiding this comment

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

What is this override for?

Copy link
Contributor Author

@sudhanshu-raj sudhanshu-raj Nov 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This overrides the join column name for attributes field ,actually in the OrderSet's parent class which is BaseCustomizableMetadata it has join column name location_id but in orderset.hbm.xml it has order_set_id join column name so it's overriding that join column name from it's parent.

Copy link
Member

@dkayiwa dkayiwa Nov 12, 2025

Choose a reason for hiding this comment

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

Did you evaluate using AttributeOverride?

Copy link
Contributor Author

@sudhanshu-raj sudhanshu-raj Nov 12, 2025

Choose a reason for hiding this comment

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

AttributeOverride works well for normal fields not for relationship attributes so here AssociationOverride will work for relationships columns like OneToMany

Copy link
Member

@dkayiwa dkayiwa Nov 12, 2025

Choose a reason for hiding this comment

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

Even when you are not modifying a relationship but just renaming a filed like here?

Copy link
Contributor Author

@sudhanshu-raj sudhanshu-raj Nov 12, 2025

Choose a reason for hiding this comment

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

yes , this just renaming the column name , it's just can change how it should be mapped to databased like joincolumn name or jointable name , but can't actually modify the relationship like onetomany to manytoone , fetch type etc

Copy link
Member

@dkayiwa dkayiwa Nov 12, 2025

Choose a reason for hiding this comment

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

Doesn't that then mean AttributeOverride is enough for this use case?

Copy link
Contributor Author

@sudhanshu-raj sudhanshu-raj Nov 12, 2025

Choose a reason for hiding this comment

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

I don’t think @AttributeOverride works here. It’s used for simple attributes for example, renaming a column on a basic field like @Column(name = "idt") private String identifier; or for fields inside an @Embeddable. It only changes how a single column is named or mapped.

But for relationships like @OneToMany or @ManyToOne, we use @AssociationOverride. That’s what lets us change the name of the join column or join table for an existing relation.
This is what I understand, please let me know if I’m missing something.

Copy link
Member

@dkayiwa dkayiwa Nov 12, 2025

Choose a reason for hiding this comment

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

What is the difference between this and https://github.com/openmrs/openmrs-core/blob/2.8.1/api/src/main/java/org/openmrs/Location.java#L52 ?

Copy link
Contributor Author

@sudhanshu-raj sudhanshu-raj Nov 13, 2025

Choose a reason for hiding this comment

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

The @AttributeOverride(name = "attributes", column = @column(name = "location_id")) annotation on the Location entity is not doing anything meaningful and can be safely removed. Why ? because :@AttributeOverride only works for simple fields (String, Integer, Date, etc.), not relationships like @onetomany

In BaseCustomizableMetadata, the attributes field is a relationship (@onetomany), not a basic column:

@OneToMany(cascade = CascadeType.ALL, orphanRemoval = true, fetch = FetchType.LAZY)
@JoinColumn(name = "location_id")
private Set<A> attributes = new LinkedHashSet<>();

Since @AttributeOverride cannot override relationships, it gets silently ignored during entity mapping. The application runs fine without it which i also verified .

Ref : https://www.baeldung.com/jpa-attributeoverride
Recommendation: Remove the @AttributeOverride annotation from Location as it serves no purpose.

joinColumns = @JoinColumn(name = "order_set_id")
)
public class OrderSet extends BaseCustomizableMetadata<OrderSetAttribute> {

public static final long serialVersionUID = 72232L;
Expand All @@ -36,12 +58,23 @@ public enum Operator {
ALL, ONE, ANY
}

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
@Column(name ="order_set_id" )
private Integer orderSetId;

@Enumerated(EnumType.STRING)
@JdbcTypeCode(SqlTypes.VARCHAR)
@Column(name = "operator", nullable = false)
private Operator operator;


@OneToMany(cascade = CascadeType.ALL, orphanRemoval = true)
Copy link
Member

@dkayiwa dkayiwa Nov 11, 2025

Choose a reason for hiding this comment

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

How did you deal with the lazy attribute?

Copy link
Contributor Author

@sudhanshu-raj sudhanshu-raj Nov 12, 2025

Choose a reason for hiding this comment

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

I think OneToMany has default fetch type lazy , ref : https://stackoverflow.com/questions/26601032/default-fetch-type-for-one-to-one-many-to-one-and-one-to-many-in-hibernate

Copy link
Member

@dkayiwa dkayiwa Nov 14, 2025

Choose a reason for hiding this comment

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

Do you think you can come up with a test in https://github.com/openmrs/openmrs-core/blob/master/api/src/test/java/org/openmrs/api/OrderSetServiceTest.java to catch this problem?

As in a test which would fail when using AttributeOverride but passes with AssociationOverride

@JoinColumn(name = "order_set_id", nullable = false)
@OrderColumn(name = "sequence_number")
private List<OrderSetMember> orderSetMembers;


@ManyToOne
@JoinColumn(name = "category")
private Concept category;

/**
Expand Down
1 change: 0 additions & 1 deletion api/src/main/resources/hibernate.cfg.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
<mapping resource="org/openmrs/api/db/hibernate/Order.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/OrderAttribute.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/OrderAttributeType.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/OrderSet.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/OrderSetMember.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/OrderSetAttributeType.hbm.xml" />
<mapping resource="org/openmrs/api/db/hibernate/OrderSetAttribute.hbm.xml" />
Expand Down
79 changes: 0 additions & 79 deletions api/src/main/resources/org/openmrs/api/db/hibernate/OrderSet.hbm.xml
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions api/src/test/java/org/openmrs/api/OrderServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2936,6 +2936,7 @@ public void saveOrder_shouldFailIfTheJavaTypeOfThePreviousOrderDoesNotMatch() th
.addAnnotatedClass(Privilege.class)
.addAnnotatedClass(LoginCredential.class)
.addAnnotatedClass(ConceptDatatype.class)
.addAnnotatedClass(OrderSet.class)
.getMetadataBuilder().build();


Expand Down