TRUNK-6276: Fix ValidateHibernateMappingsDatabaseIT test errors when running with MySQL #4806

wikumChamith · 2024-10-27T15:29:15Z

Description of what I changed

Issue I worked on

see https://openmrs.atlassian.net/browse/TRUNK-6276

Checklist: I completed these to help reviewers :)

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
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
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
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.
My pull request is based on the latest changes of the master branch.

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

dkayiwa · 2024-10-29T09:46:13Z

api/src/main/java/org/openmrs/BaseOpenmrsMetadata.java

@@ -38,7 +39,8 @@ public abstract class BaseOpenmrsMetadata extends BaseOpenmrsObject implements O
 	@Field
 	private String name;

-	@Column(name = "description", length = 255)
+	@Column(name = "description", columnDefinition = "text")
+	@Lob


I do not think a description of length 255 is a Lob.

Some tables use text for the description field, while others use varchar. Defining description as text in the superclass and overriding it as varchar in the child classes when necessary helps mitigate compatibility issues between H2 and MySQL during tests

Because the majority of the descriptions are small, it does not make sense to me making it a LOB in the base class.

If we don't include lob here we'll get an error when running ValidHibernateMappings with H2.
Schema-validation: wrong column type encountered in column [description] in table [order_type]; found [clob (Types#CLOB)], but expecting [text (Types#VARCHAR)]

if we set this to "varchar" and try to override to "text" when necessary we still get the error as we can't add @Lob for overrides.

Thinking about this again, which object would need text that is too big to require a LOB, for just a mere description?

Tables like global_property, order_type, and form use text for description.
Should we update the tables that use text for description to something like varchar(1024)?

I think we should do so. I cannot think of a reason why any of those would require a LOB. Let us change them.

dkayiwa · 2024-10-29T12:27:31Z

api/src/main/java/org/openmrs/BaseOpenmrsMetadata.java

@@ -38,7 +39,7 @@ public abstract class BaseOpenmrsMetadata extends BaseOpenmrsObject implements O
 	@Field
 	private String name;

-	@Column(name = "description", length = 255)
+	@Column(name = "description", length = 1024)


Why don't we leave the default as 255?

I updated the PR.

dkayiwa · 2024-10-29T13:07:08Z

api/src/main/java/org/openmrs/BaseOpenmrsMetadata.java

@@ -38,7 +39,7 @@ public abstract class BaseOpenmrsMetadata extends BaseOpenmrsObject implements O
 	@Field
 	private String name;

-	@Column(name = "description", length = 255)


Shouldn't the default length be 255?

The default length of varchar is set to 255 by JPA, so there's no need to explicitly define it

dkayiwa · 2024-10-29T13:08:56Z

api/src/main/java/org/openmrs/PatientIdentifierType.java

@@ -32,7 +33,8 @@
 @Table(name = "patient_identifier_type")
 @Audited
 @AttributeOverrides({
-	@AttributeOverride(name = "name", column = @Column(name = "name", nullable = false, length = 50))
+	@AttributeOverride(name = "name", column = @Column(name = "name", nullable = false, length = 50)),
+	@AttributeOverride(name = "description", column = @Column(name = "description", columnDefinition = "text"))


Why should a patient identifier type description have a column definition of text?

dkayiwa · 2024-10-29T13:09:32Z

api/src/main/java/org/openmrs/PersonAttributeType.java

@@ -31,6 +33,9 @@
 @Entity
 @Table(name = "person_attribute_type")
 @Audited
+@AttributeOverrides({
+	@AttributeOverride(name = "description", column = @Column(name = "description", columnDefinition = "text"))


Same question as above

wikumChamith · 2024-10-29T13:13:05Z

@dkayiwa I updated the PR

dkayiwa · 2024-10-29T13:21:24Z

api/src/main/java/org/openmrs/Location.java

-@AttributeOverride(name = "attributes", column = @Column(name = "location_id"))
+@AttributeOverrides(value = {
+	@AttributeOverride(name = "attributes", column = @Column(name = "location_id")),
+	@AttributeOverride(name = "description", column = @Column(name = "description", columnDefinition = "varchar(255)", length = 255))


do we need to duplicate the above length?

dkayiwa · 2024-10-29T13:32:54Z

api/src/main/java/org/openmrs/PatientIdentifierType.java

@@ -32,7 +32,7 @@
 @Table(name = "patient_identifier_type")
 @Audited
 @AttributeOverrides({
-	@AttributeOverride(name = "name", column = @Column(name = "name", nullable = false, length = 50))
+	@AttributeOverride(name = "name", column = @Column(name = "name", nullable = false, length = 50)),


Is this extra comma needed?

dkayiwa · 2024-10-29T13:34:09Z

api/src/main/java/org/openmrs/api/db/SerializedObject.java

@@ -28,6 +31,9 @@
 @Entity
 @Table(name = "serialized_object")
 @Audited
+@AttributeOverrides(value = {
+	@AttributeOverride(name = "description", column = @Column(name = "description", length = 5000))


Shouldn't this also be text?

Oh wait, why should a description be that large?

create table serialized_object ( serialized_object_id int auto_increment primary key, name varchar(255) not null, description varchar(5000) null, type varchar(255) not null, subtype varchar(255) not null, serialization_class varchar(255) not null, serialized_data mediumtext not null, date_created datetime not null, creator int not null, date_changed datetime null, changed_by int null, retired tinyint(1) default 0 not null, date_retired datetime null, retired_by int null, retire_reason varchar(1000) null, uuid char(38) not null, constraint uuid unique (uuid), constraint serialized_object_changed_by foreign key (changed_by) references users (user_id), constraint serialized_object_creator foreign key (creator) references users (user_id), constraint serialized_object_retired_by foreign key (retired_by) references users (user_id) );

DDL for this table looks like this. What do you suggest?

Looks like that was a typo 😊

should it be text or varchar ?

…running with MySQL

dkayiwa · 2024-10-29T13:41:52Z

api/src/main/resources/org/openmrs/api/db/hibernate/DiagnosisAttribute.hbm.xml

@@ -46,7 +46,9 @@

 		<property name="voidReason" type="java.lang.String" column="void_reason" length="255" />

-		<property name="uuid" type="java.lang.String" column="uuid" length="38" unique="true" />
+		<property name="uuid" type="java.lang.String" unique="true">


For the changes on uuids, were they failing on any of the databases?

wikumChamith force-pushed the TRUNK-6276 branch 2 times, most recently from 5542fbf to f0493e6 Compare October 29, 2024 06:27