Skip to content
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

Jump to bottom

RESTWS-904 Support custom representation for Map objects #602

Merged
merged 1 commit into from
Jul 29, 2024
Merged

RESTWS-904 Support custom representation for Map objects #602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 17 additions & 0 deletions ...bservices/rest/web/v1_0/controller/openmrs1_8/PatientIdentifierTypeController1_8Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,4 +168,21 @@ public void shouldReturnTheAuditInfoForTheFullRepresentation() throws Exception

assertNotNull(PropertyUtils.getProperty(result, "auditInfo"));
}

@Test
public void shouldReturnCustomRepresentationForAuditInfo() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be in ConversionUtilTest?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Are you directly calling this private method from the test?

Copy link
Member Author

@gayanW gayanW Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

And the private method is fully covered by this test.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@gayanW did you see the above comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a new unit test to ConversionUtilTest.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dkayiwa Can you check :)

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

assertNotNull(PropertyUtils.getProperty(result, "auditInfo"));
assertNotNull(PropertyUtils.getProperty(result, "auditInfo.creator"));
assertNotNull(PropertyUtils.getProperty(result, "auditInfo.creator.uuid"));
assertNotNull(PropertyUtils.getProperty(result, "auditInfo.dateCreated"));

assertNull(PropertyUtils.getProperty(result, "name"));
assertNull(PropertyUtils.getProperty(result, "links"));
assertNull(PropertyUtils.getProperty(result, "auditInfo.creator.display"));
assertNull(PropertyUtils.getProperty(result, "auditInfo.creator.links"));
}
}
21 changes: 21 additions & 0 deletions omod-common/src/main/java/org/openmrs/module/webservices/rest/web/ConversionUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,9 @@ public static <S> Object convertToRepresentation(S o, Representation rep, Conver
}
return ret;
} else if (o instanceof Map) {
if (rep instanceof CustomRepresentation) {
return convertToCustomRepresentation(o, (CustomRepresentation) rep);
}
SimpleObject ret = new SimpleObject();
for (Map.Entry<?, ?> entry : ((Map<?, ?>) o).entrySet()) {
ret.put(entry.getKey().toString(),
Expand All @@ -414,6 +417,24 @@ public static <S> Object convertToRepresentation(S o, Representation rep, Conver
}
}

/**
* Converts an object to its custom representation
* 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 convertToCustomRepresentation(Object o, CustomRepresentation rep) {
DelegatingResourceDescription drd = ConversionUtil.getCustomRepresentationDescription(rep);

SimpleObject result = new SimpleObject();
for (String propertyName : drd.getProperties().keySet()) {
DelegatingResourceDescription.Property property = drd.getProperties().get(propertyName);
Object propertyValue = ConversionUtil.getPropertyWithRepresentation(o, propertyName, property.getRep());
result.add(propertyName, propertyValue);
}

return result;
}

/**
* Gets the type for the specified generic type variable.
*
Expand Down
25 changes: 24 additions & 1 deletion omod-common/src/test/java/org/openmrs/module/webservices/rest/web/ConversionUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
package org.openmrs.module.webservices.rest.web;

import static org.hamcrest.core.Is.is;
import org.junit.Assert;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;

import java.lang.reflect.Method;
Expand All @@ -25,8 +25,11 @@
import java.util.TimeZone;

import org.apache.commons.beanutils.PropertyUtils;
import org.junit.Assert;
import org.junit.Test;
import org.openmrs.api.ConceptNameType;
import org.openmrs.module.webservices.rest.SimpleObject;
import org.openmrs.module.webservices.rest.web.representation.CustomRepresentation;
import org.openmrs.module.webservices.rest.web.representation.Representation;
import org.openmrs.web.test.BaseModuleWebContextSensitiveTest;

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

@Test
public void convert_shouldConvertSimpleObjectToCustomRepresentation() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dkayiwa Can we merge this fix?


SimpleObject child = new SimpleObject();
child.put("child_key_1", "child_val_1");
child.put("child_key_2", "child_val_2");
SimpleObject parent = new SimpleObject();
parent.put("parent_key_1", child);
parent.put("parent_key_2", "parent_val_2");

Object o = ConversionUtil.convertToRepresentation(parent, new CustomRepresentation("parent_key_1:(child_key_1)"));

SimpleObject expectedChild = new SimpleObject();
expectedChild.put("child_key_1", "child_val_1");
SimpleObject expectedParent = new SimpleObject();
expectedParent.put("parent_key_1", expectedChild);

assertEquals(expectedParent, o);
}

public void convert_shouldConvertIntToDouble() throws Exception {
assertThat((Double) ConversionUtil.convert(5, Double.class), is(5d));
}
Expand Down
Loading