RESTWS-904 Support custom representation for Map objects #602

gayanW · 2024-03-28T15:11:40Z

Description of what I changed

This issue is that, ConversionUtil#convertToRepresentation(object, representation, converter), does not use the representation param, when the object param is of type Map. So it'll always return an object with all the elements in the map, even if the caller wants to convert it to a custom representation.

openmrs-module-webservices.rest/omod-common/src/main/java/org/openmrs/module/webservices/rest/web/ConversionUtil.java

Lines 379 to 397 in 6d01f5f

    
           public static <S> Object convertToRepresentation(S o, Representation rep, Converter specificConverter) 
        
                   throws ConversionException { 
        
           	if (o == null) 
        
           		return null; 
        
           	o = new HibernateLazyLoader().load(o); 
        
           	if (o instanceof Collection) { 
        
           		List ret = new ArrayList(); 
        
           		for (Object item : ((Collection) o)) { 
        
           			ret.add(convertToRepresentation(item, rep, specificConverter)); 
        
           		} 
        
           		return ret; 
        
           	} else if (o instanceof Map) { 
        
           		SimpleObject ret = new SimpleObject(); 
        
           		for (Map.Entry<?, ?> entry : ((Map<?, ?>) o).entrySet()) { 
        
           			ret.put(entry.getKey().toString(), 
        
           			    convertToRepresentation(entry.getValue(), Representation.REF, (Converter) null)); 
        
           		} 
        
           		return ret;

As shown below every value in the map is converted to it's REF representation.

openmrs-module-webservices.rest/omod-common/src/main/java/org/openmrs/module/webservices/rest/web/ConversionUtil.java

Lines 393 to 395 in 6d01f5f

    
           for (Map.Entry<?, ?> entry : ((Map<?, ?>) o).entrySet()) { 
        
           	ret.put(entry.getKey().toString(), 
        
           	    convertToRepresentation(entry.getValue(), Representation.REF, (Converter) null));

auditInfo objects are of type Map, hence facing the same issue as reported in RESTWS-904

This PR refactors ConversionUtil, so objects of type Map can be converted to custom representations.

Issue I worked on

see https://openmrs.atlassian.net/browse/RESTWS-904

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

gayanW · 2024-04-02T11:53:59Z

@dkayiwa
It's ready to be reviewed :)

dkayiwa · 2024-04-02T13:52:04Z

omod-common/src/main/java/org/openmrs/module/webservices/rest/web/ConversionUtil.java

+	 * This could be used to convert any domain objects that does not have any specific converter associated with them
+	 * such as SimpleObject's, Map's, etc
+	 */
+	private static SimpleObject convertToRepresentation(Object o, CustomRepresentation rep) {


Is there any reason why the method name is generic, yet from the description, you are dealing with only custom representations?

I did think about that a bit. I thought of overloading the method, to make it consistent with the rest of the methods.

If the concern is that, calls to convertToRepresentation resolving to a wrong method, we could refactor the method name. Since this method is private, there's no such thing from happening, though.

Would you like make it more specific like convertToCustomRepresentation?

I don't have a preference on which one to use :)

If it cannot deal with any representation but custom, then i would have the name explicitly state that.

I refactored the method name.

dkayiwa · 2024-04-10T13:44:17Z

.../webservices/rest/web/v1_0/controller/openmrs1_8/PatientIdentifierTypeController1_8Test.java

@@ -168,4 +168,21 @@ public void shouldReturnTheAuditInfoForTheFullRepresentation() throws Exception

 		assertNotNull(PropertyUtils.getProperty(result, "auditInfo"));
 	}
+
+	@Test
+	public void shouldReturnCustomRepresentationForAuditInfo() throws Exception {


Shouldn't this be in ConversionUtilTest?

PatientIdentifierTypeController1_8Test also include a test named shouldReturnTheAuditInfoForTheFullRepresentation. So it's nice to have a matching E2E test for converting to a custom representation.

The new method that we introuduced in ConversionUtil is private. So I think it makes sense to have it here instead.

Are you directly calling this private method from the test?

No, the private method is not called directly in the test. The test makes a GET call to the resource with custom representation as the v param.

openmrs-module-webservices.rest/omod-1.8/src/test/java/org/openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_8/PatientIdentifierTypeController1_8Test.java

Lines 174 to 176 in 9eb0e4d

SimpleObject result = deserialize(

handle(newGetRequest(getURI() + "/" + getUuid(),

new Parameter("v", "custom:(auditInfo:(creator:(uuid),dateCreated))"))));

Assertions in this test should be enough to prevent someone from breaking things again :)

And the private method is fully covered by this test.

If the test is for a change in ConversionUtil, is there a reason why you do not put the test in ConversionUtilTest?

@gayanW did you see the above comment?

Sorry for the late reply. I've been busy with work.

I don't think it would be good to put Mock Http test in ConversionUtil.

Additionally I could write a new unit test for ConversionUtil#convertToRepresentation in ConversionUtilTest. Let me check that on the weekend.

I added a new unit test to ConversionUtilTest.

@dkayiwa Can you check :)

dkayiwa · 2024-05-02T11:10:44Z

omod-common/src/test/java/org/openmrs/module/webservices/rest/web/ConversionUtilTest.java

@@ -145,6 +148,26 @@ public void convert_shouldConvertToAClass() throws Exception {
 		Assert.assertTrue(converted.isAssignableFrom(String.class));
 	}

+	@Test
+	public void convert_shouldConvertSimpleObjectToCustomRepresentation() throws Exception {


The method name does not seem to reflect the work being done by the ticket?

The ticket is about not being able to get custom representations with audit info. auditInfo is a simple object that implements map interface.

So bassically it's enough to assert that it can properly convert simple objects to a given custom representation.

@dkayiwa Can we merge this fix?

dkayiwa reviewed Apr 2, 2024

View reviewed changes

gayanW force-pushed the RESTWS-904 branch from e9e2852 to 9eb0e4d Compare April 5, 2024 04:18

dkayiwa reviewed Apr 10, 2024

View reviewed changes

RESTWS-904 Support custom representation for Map objects

bc69bc9

gayanW force-pushed the RESTWS-904 branch from 9eb0e4d to bc69bc9 Compare April 20, 2024 14:27

dkayiwa reviewed May 2, 2024

View reviewed changes

dkayiwa merged commit 70882a4 into openmrs:master Jul 29, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RESTWS-904 Support custom representation for Map objects #602

RESTWS-904 Support custom representation for Map objects #602

gayanW commented Mar 28, 2024 •

edited by jira bot

Loading

gayanW commented Apr 2, 2024

dkayiwa Apr 2, 2024 •

edited

Loading

gayanW Apr 3, 2024 •

edited

Loading

dkayiwa Apr 4, 2024 •

edited

Loading

gayanW Apr 5, 2024

dkayiwa Apr 10, 2024

gayanW Apr 11, 2024

dkayiwa Apr 11, 2024

gayanW Apr 12, 2024 •

edited

Loading

gayanW Apr 12, 2024

dkayiwa Apr 14, 2024

dkayiwa Apr 17, 2024

gayanW Apr 18, 2024

gayanW Apr 20, 2024

gayanW May 1, 2024

dkayiwa May 2, 2024

gayanW May 4, 2024

gayanW Jul 29, 2024

	public static <S> Object convertToRepresentation(S o, Representation rep, Converter specificConverter)
	throws ConversionException {
	if (o == null)
	return null;
	o = new HibernateLazyLoader().load(o);

	if (o instanceof Collection) {
	List ret = new ArrayList();
	for (Object item : ((Collection) o)) {
	ret.add(convertToRepresentation(item, rep, specificConverter));
	}
	return ret;
	} else if (o instanceof Map) {
	SimpleObject ret = new SimpleObject();
	for (Map.Entry<?, ?> entry : ((Map<?, ?>) o).entrySet()) {
	ret.put(entry.getKey().toString(),
	convertToRepresentation(entry.getValue(), Representation.REF, (Converter) null));
	}
	return ret;

	SimpleObject result = deserialize(
	handle(newGetRequest(getURI() + "/" + getUuid(),
	new Parameter("v", "custom:(auditInfo:(creator:(uuid),dateCreated))"))));

RESTWS-904 Support custom representation for Map objects #602

RESTWS-904 Support custom representation for Map objects #602

Conversation

gayanW commented Mar 28, 2024 • edited by jira bot Loading

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

gayanW commented Apr 2, 2024

dkayiwa Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

gayanW Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

dkayiwa Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gayanW Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gayanW commented Mar 28, 2024 •

edited by jira bot

Loading

dkayiwa Apr 2, 2024 •

edited

Loading

gayanW Apr 3, 2024 •

edited

Loading

dkayiwa Apr 4, 2024 •

edited

Loading

gayanW Apr 12, 2024 •

edited

Loading