Switching from Xml mappings to Annotations

[mherman22 (herman muhereza)]

Hey Folks, allow me bring back this discussion about jpa annotations. I have been working on switching role domain from using hibernate xml mappings to jpa and i am a bit stuck on what to do next. The role table has attributes role, uuid and description, however the Role class extends BaseChangeableOpenmrsMetadata which in turn extends BaseOpenmrsMetadata which means that the Role class inherits most (if not all) of the attributes from the BaseOpenmrsMetadata which was annotated with @MappedSuperClass.

However, the snag i have as shown below indicates that i can’t insert data into the role entity minus providing the date_created column and the same goes for creator, retired, retire_reason, name. Here’s the code changes that landed me here → Switching from hibernate mappings to annotations-Role by mherman22 · Pull Request #4293 · openmrs/openmrs-core · GitHub.

Caused by: org.h2.jdbc.JdbcSQLIntegrityConstraintViolationException: NULL not allowed for column "DATE_CREATED"; SQL statement:
insert into ROLE (ROLE, DESCRIPTION, UUID) values (?, ?, ?) [23502-200]

Options

• So what i had initially done in the pull request above is to manually change the datasets initialInMemoryTestDataset.xml and standardTestDataset.xml and added those columns in the role table hence having the OrderServiceTest tests passing. However, i realised i would have to change all the testdatasets depended on by most ServiceTests classes ie AdministrativeServiceTest, UserServiceTest, etc… Is this the way to go? (my guts tell me no).

• The other option that i tested out and it seemed to work for the role entity was going to the BaseOpenmrsMetadata class and removing nullable=false from name, date_created, creator, retired, retire_reason. Is this viable?

• Another option is the one i read from @ibacher’s suggestion at GSoC-2020: Switching from XML Mappings to Annotations on OpenMRS domain Objects - #9 by ibacher which i tried out but seems not to do the trick for me. It involves adding the @Transient annotation to the getter overriden from the super class.

Any advice for me? /cc: @ibacher @dkayiwa @burke @mseaton @mogoodrich @mksd @ruhanga @abertnamanya, @dev3, @dev4 , @dev5

[mherman22 (herman muhereza)]

Overriding the attributes that are not needed by adding this line @AttributeOverride(name = "dateCreated", column = @Column(name = "date_created")) and three others to cater for retire. retire_reason and name seems to work. However, the same doesn’t work for creator and i believe its because the creator field/column has been annotated with @ManyToOne(optional=false). The only work-around i have come across is to remove the optional=false and instead add nullable=false to the @JoinColumn.

[dkayiwa (Daniel Kayiwa)]

@mherman22

The only work-around i have come across is to remove the optional=false and instead add nullable=false to the @JoinColumn.

Would that make all tests pass?

[ibacher (Ian Bacher)]

@mherman22

minus providing the date_created column and the same goes for creator, retired, retire_reason, name

I don’t see why we wouldn’t just add a creator, date_created, retired, retired_reason columns for the role table. name is trickier given that role is basically the same property…

The other option is to remove the Role class from the OpenmrsMetadata hierarchy altogether… I don’t know if we’re relying on it extending OpenmrsMetadata anywhere, but the way it’s currently mapped is a mess (i.e., it has a ton of instance-level properties that aren’t actually persisted anywhere).

[dkayiwa (Daniel Kayiwa)]

@ibacher

The other option is to remove the Role class from the OpenmrsMetadata hierarchy altogether… I don’t know if we’re relying on it extending OpenmrsMetadata anywhere, but the way it’s currently mapped is a mess (i.e., it has a ton of instance-level properties that aren’t actually persisted anywhere).

This is exactly what i was about to suggest. I even tried it out locally and openmrs-core compiled successfully. But then realised that even this would need to be changed accordingly: https://github.com/openmrs/openmrs-module-webservices.rest/blob/2.39.0/omod-1.8/src/main/java/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/RoleResource1_8.java#L41

[mherman22 (herman muhereza)]

Thanks @dkayiwa, @ibacher for your suggestions. Removing Role class from the BaseOpenmrsMetadata hierarchy looks to be the chosen approach.

I am gonna create the ticket to fix the resource in webservices as suggested.

[mherman22 (herman muhereza)]

@dkayiwa

But then realised that even this would need to be changed accordingly: openmrs-module-webservices.rest/RoleResource1_8.java at 2.39.0 · openmrs/openmrs-module-webservices.rest · GitHub

https://issues.openmrs.org/browse/RESTWS-910

[mherman22 (herman muhereza)]

So in the attempt to prove whether the changes at Change RoleResource from extending the MetadataDelegatingCrudResource class by mherman22 · Pull Request #575 · openmrs/openmrs-module-webservices.rest · GitHub and Switching from hibernate mappings to annotations-Role by mherman22 · Pull Request #4293 · openmrs/openmrs-core · GitHub would run well, what attributes of the Role resource are we maintaining? we have role, name (which is transient)description, inheritedroles, childroles and privileges that we are maintaining in the role resource. My question is on the retired and retire_reason attributes, should they be included aswell?

FYI, i made a GET request using http://localhost:8080/openmrs/ws/rest/v1/role?v=full and i get a 400 bad request with the error message below.

Note: i built this distro using the changes on the two pull requests above

“message”: “[converting class org.openmrs.Role to org.openmrs.module.webservices.rest.web.representation.FullRepresentation@146cff29 => retired on class org.openmrs.Role => Unknown property ‘retired’ on class ‘class org.openmrs.Role’]”,

/cc: @dkayiwa @ibacher

[dkayiwa (Daniel Kayiwa)]

This is exactly why i wanted you to run your changes to check if all is well. As for properties, use only those which the role object has after your changes in openmrs core.

[mherman22 (herman muhereza)]

should the Role resource sort of implement retirable aswell?

This is more in relation to how we are gonna handle deleting of a role in this pull request → https://github.com/openmrs/openmrs-module-webservices.rest/pull/575. I am certain we would not want to make roles voidable.

FYI, The purge implementation here → openmrs-core/UserServiceImpl.java at 7e9a3b692d4b913e6c028791bf803b96af59651e · openmrs/openmrs-core · GitHub does not allow for directly deleting a role to happen if its a core role , has child roles hence the implementation below would not work.

	@Override
	protected void delete(Role delegate, String reason, RequestContext context) throws ResponseException {
		if (delegate == null) {
			// DELETE is idempotent, so we return success here
			return;
		}
		Context.getUserService().purgeRole(delegate);
	}

/cc: @dkayiwa @ibacher

[dkayiwa (Daniel Kayiwa)]

Do not introduce any new functionality in regards to Roles. If roles have not been voidable or retirable, do not try to implement that now. It would be out of scope for the current work.