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.

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

RESTWS-909: Condition resource fails when using a custom representati… #573

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

mseaton
Copy link
Member

@mseaton mseaton commented Mar 30, 2023

…on of codedOrNonCoded

Issue I worked on

see https://issues.openmrs.org/browse/RESTWS-909

@mseaton mseaton requested review from dkayiwa and ibacher March 30, 2023 00:13
@coveralls
Copy link

Coverage Status

Coverage: 46.982% (+0.04%) from 46.94% when pulling 9304366 on RESTWS-909 into 938b7d7 on master.

}

throw new ConversionException("Don't know how to get " + getClass().getSimpleName() + "(" + delegate.getClass()
+ ") as " + representation.getRepresentation(), null);
}

/**
Copy link
Member Author

Choose a reason for hiding this comment

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

This is moved to a public static method in ConversionUtil without any changes

@@ -504,5 +505,70 @@ public static SimpleObject getAuditInfo(Object delegate) {

return ret;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This is moved here to a public static method in ConversionUtil without any changes, previously was a private instance method on BaseDelegatingResource

if (repDescription != null) {
return convertDelegateToRepresentation(delegate, repDescription);
}
repDescription = ConversionUtil.getCustomRepresentationDescription((CustomRepresentation) 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.

My IDE was telling me to change this because repDescription will never be null

request.addParameter("patientUuid", "da7f524f-27ce-4bb2-86d6-6d1d05312bd5");
request.addParameter("includeInactive", "true");
request.addParameter("v", "custom:(uuid,display,clinicalStatus,onsetDate,endDate,additionalDetail,condition:(coded:(id,uuid)))");
SimpleObject result = deserialize(handle(request));
Copy link
Member Author

Choose a reason for hiding this comment

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

Without the fix, this test fails here. It throws an error trying to process this request.


if (instance.getNonCoded() != null) {
codedOfFreeText.add("nonCoded", instance.getNonCoded());
else {
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 don't know why this is showing such an extensive change. All of the pre-existing code was simply moved unchanged into this else block. So the only thing I changed was to say - if the Representation is a CustomRepresentation, then process it differently. If it not a custom representation, no behavior has changed here.

.getResourceBySupportedClass(Concept.class);
codedOfFreeText.add("coded", conceptResource.asRepresentation(instance.getCoded(), rep));
SimpleObject codedOrFreeText = new SimpleObject();
if (rep instanceof CustomRepresentation) {
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 don't know why the framework doesn't automatically handle this, but it doesn't seem to. So here, I'm explicitly parsing the custom representation and using that to determine which properties of the CodedOrNonCoded to convert, and how.

for (String propertyName : drd.getProperties().keySet()) {
DelegatingResourceDescription.Property property = drd.getProperties().get(propertyName);
Object o = ConversionUtil.getPropertyWithRepresentation(instance, propertyName, property.getRep());
if (o != null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this is correct - should I include this even if it is null, since it is specified in the custom rep?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, I would guess skipping in the correct behavior. At first instinct it might made sense to do something like ' uuid:"" ' but I doubt we'd want to included a nested set of empty properties, so I think just skipping makes sense, unless someone else as an opinion otherwise.

@mseaton mseaton requested a review from mogoodrich March 30, 2023 00:25
@dkayiwa dkayiwa merged commit 8931523 into master Mar 31, 2023
Copy link
Member

@mogoodrich mogoodrich left a comment

Choose a reason for hiding this comment

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

LGTM, and it appears this answers why I had never seen any problems like this with custom reps: this only applies to the CodedOrFreeText type, which is only currently used for Diagnoses and Conditions, which I've never tried to fetch via REST.

for (String propertyName : drd.getProperties().keySet()) {
DelegatingResourceDescription.Property property = drd.getProperties().get(propertyName);
Object o = ConversionUtil.getPropertyWithRepresentation(instance, propertyName, property.getRep());
if (o != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure, I would guess skipping in the correct behavior. At first instinct it might made sense to do something like ' uuid:"" ' but I doubt we'd want to included a nested set of empty properties, so I think just skipping makes sense, unless someone else as an opinion otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants