OpenMRS Trunk

partial solution to prevent ClassCastException

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?

Does this throw an error on a zero element list?

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.

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.

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?

Fixed in 1.5.x in rev:9780.
Fixed in trunk in rev:9781.