Downloads Documentation Community Contribute Demo






Show Sidebar
Login | Register

Ticket #1613 (closed defect: fixed)

Opened 9 months ago

Last modified 7 months ago

RequiredDataHandler throws ClassCastException while saving ReportSchema

Reported by: jmiranda Assigned to: jmiranda
Priority: critical Milestone: OpenMRS 1.5
Component: OpenMRS Code Base Keywords:
Cc: mseaton, djazayeri, bwolfe Introductory Ticket: 0
Code Review Status: Approved

Description

RequiredDataHandler throws ClassCastException while checking for Collection of OpenmrsObjects during save operation on ReportSchema.

Attachments

stacktrace-ClassCastException-on-saving-report-form.txt (10.3 kB) - added by jmiranda on 07/03/09 14:15:44.
1613.patch (1.8 kB) - added by jmiranda on 07/03/09 14:19:46.
partial solution to prevent ClassCastException
requiredDataAdvice.patch (2.6 kB) - added by mseaton on 08/13/09 03:22:17.

Change History

07/03/09 14:15:44 changed by jmiranda

  • attachment stacktrace-ClassCastException-on-saving-report-form.txt added.

07/03/09 14:19:46 changed by jmiranda

  • attachment 1613.patch added.

partial solution to prevent ClassCastException

07/03/09 14:38:17 changed by jmiranda

  • cc set to mseaton, djazayeri, bwolfe.

The attached patch removes the ClassCastException but I'm not sure if it's

The exception occurred on ReportSchema.getDataSetDefinitions(). This method returns Mapped<? extends DataSetDefinition>. DataSetDefinition is of type OpenmrsMetadata, so the RequiredDataHandler should return false on the isOpenmrsObjectCollection() call.

However, what should be the case if we had something like NewPatientListModel.getPatients() which returns a Collection of SomeParameterizedType<? extends Patient>. I assume we would want to handle that, right? Right now, the first check ("Is the object a collection?") would pass, but then the second check ("Are the objects in the collection of type OpenmrsObject?") would fail.

For this we probably need to do something like:

if (type instanceof Class) { 
   // Same as current code
}
else if (type instanceof ParameterizedType) { 
   // TODO Need to inspect to see if <? extends DataSetDefinition> is an OpenmrsObject
}
else { 
   throw new APIException("Encountered unknown type " + type.getClass().getName());
}

How do we inspect the Type <? extends DataSetDefinition> to figure out if DataSetDefinition is an OpenmrsObject?

07/06/09 14:03:17 changed by bwolfe

Does this throw an error on a zero element list?

08/13/09 03:21:00 changed by mseaton

  • review_status set to Needs Review.
  • milestone set to OpenMRS 1.5.

Hi guys,

Looking into this, the actual issue is that the method will fail whenever we have nested ParameterizedTypes. For example,

<pre> List<List<String>> thisPropertyWillFail. </pre>

This is due to the fact that we try to cast the first parameterized type to a Class (in the above case, this is the inner List<String> on line 333 of RequiredDataAdvice. Since List<String> is another ParameterizedType and not directly a class, this throws an Exception.

This has reared it's head in the Reporting module as Justin indicates because we have properties like this on our objects (i.e. List<Mapped<CohortDefinition>>)

Given the @should annotations on the method, I think we basically just need to account for such errors, and return false if they occur (a List<List<String>> is not a Collection of OpenmrsObjects after all.

See my attached patch as a proposed solution.

This should be put in 1.5 if at all possible, since it is a bug fix. I have no problem with RequiredDataAdvice not handling this type of property, but it should at least do those that it can without throwing an error so that we can create additional RequiredDataAdvice handlers in our module to account for the unhandled cases.

08/13/09 03:22:17 changed by mseaton

  • attachment requiredDataAdvice.patch added.

08/13/09 03:36:57 changed by djazayeri

  • review_status changed from Needs Review to Reviewed.

Looks good to me, and more minimal than what I was going to do.

I think this is fair to commit to 1.5 before release. Ben, please comment on that.

08/13/09 18:46:32 changed by bwolfe

  • review_status changed from Reviewed to Approved.

Looks cool. Gold star for you for also making the unit test. :-) I would prefer it be in its own test though. I did a poor job of naming that test you adding your assert to. Just make a new @should and unit test for the assert you did there.

I agree that this should be a 1.5 fix, can you commit it to trunk and 1.5?

08/13/09 20:06:06 changed by mseaton

  • status changed from new to closed.
  • resolution set to fixed.

Fixed in 1.5.x in [9780]. Fixed in trunk in [9781].