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

Observation.getEffectiveDateTimeType throwing NullPointerException when EffectiveDateTime is null [v 3.0.0, 3.2.0] #846

Closed
XcrigX opened this Issue Feb 6, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@XcrigX

XcrigX commented Feb 6, 2018

Calling Observation.getEffectiveDateTimeType on an Observation with a null effectiveDateTime value results in a NullPointerException. The exception is thrown as the effective value is de-referenced to get the classname for the FHIRException message:
this.effective.getClass().getName()

I would likely expect a null return for a null value in this case. Or at the least a FHIRException thrown rather than NPE.
Tested this against 3.0.0 and 3.2.0.

Code in question is at line Observation.java:2125:

public DateTimeType getEffectiveDateTimeType() throws FHIRException { if (!(this.effective instanceof DateTimeType)) throw new FHIRException("Type mismatch: the type DateTimeType was expected, but "+this.effective.getClass().getName()+" was encountered"); return (DateTimeType) this.effective; }

@jamesagnew

This comment has been minimized.

Owner

jamesagnew commented Feb 7, 2018

Ah yeah, this definitlely needs fixing.

Would you expect an NPE? I think I'd lean towards just returning null in this case, although I don't feel strongly about it.

@XcrigX

This comment has been minimized.

XcrigX commented Feb 7, 2018

I would lean towards a null return as well, with no exception thrown. I haven't looked at how the other resources handle polymorphic types like this however, so not sure what would be consistent.

@XcrigX

This comment has been minimized.

XcrigX commented Feb 7, 2018

Looking closer into Observation.java, it looks like many of the methods follow the same pattern and will have the same issue when dealing with null values.
For example, all the getValueXXX methods do this and will throw a NPE on a null value.

I checked Patient as well and Patient.getDeceasedBooleanType and Patient.getDeceasedDateTimeType are the same.
I'm assuming this is generated code and all the polymorphically typed elements will do this.

So for all these, users would need to call hasXXX() prior to getXXX() to avoid the NPE. Again, I'd rather receive a null for these cases, or 2nd choice would be the FHIRException since it's a checked exception and has to be dealt with anyway. Many users might not think to check for null or a NPE in these cases, especially since they already have to try-catch a FHIRException to use these methods.

If my conclusions are correct, I'm kind of surprised that nobody else has run into this though. Which gives me a little pause that I'm not overlooking something..

@jamesagnew

This comment has been minimized.

Owner

jamesagnew commented May 13, 2018

The suggested fixes have been implemented and synced into our codebase.

@jamesagnew jamesagnew closed this May 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment