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

issue #2207 - add type-checking for lists in fhir-model constructors #2209

Merged
merged 2 commits into from
Apr 7, 2021

Conversation

lmsurpre
Copy link
Member

@lmsurpre lmsurpre commented Apr 7, 2021

if a clever user adds objects of the wrong type to the builder (via
reflection), we were never catching this and so the data could become
corrupted

Signed-off-by: Lee Surprenant lmsurpre@us.ibm.com

if a clever user adds objects of the wrong type to the builder (via
reflection), we were never catching this and so the data could become
corrupted

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@JohnTimm
Copy link
Collaborator

JohnTimm commented Apr 7, 2021

Per our conversation, I think that:

if (Objects.isNull(element)) {
    throw new IllegalStateException(String.format("Repeating element: '%s' does not permit null elements", elementName));
}
Class<?> elementType = element.getClass();
if (!type.isAssignableFrom(elementType)) {
    throw new IllegalStateException(String.format("Invalid type: %s for repeating element: '%s' must be: %s", elementType.getSimpleName(), elementName, type.getSimpleName()));
}

or

if (Objects.isNull(element)) {
    throw new IllegalStateException(String.format("Repeating element: '%s' does not permit null elements", elementName));
}
if (!type.isInstance(element)) {
    throw new IllegalStateException(String.format("Invalid type: %s for repeating element: '%s' must be: %s", element.getClass().getSimpleName(), elementName, type.getSimpleName()));
}

are both easier to read / understand than the current structure because it is painfully obvious that we are performing a null check followed by a type check on each repeating element. Combining things into an outer if and inner if-then-else structure seems less clear to me. Other than that, LGTM.

NOTE: I also tweaked the error messages (above) to be more consistent with the rest of what's in the ValidationSupport class.

Copy link
Contributor

@michaelwschroeder michaelwschroeder left a comment

Choose a reason for hiding this comment

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

LGTM - one minor comment

for john

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
Copy link
Collaborator

@JohnTimm JohnTimm left a comment

Choose a reason for hiding this comment

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

LGTM

@lmsurpre lmsurpre merged commit 0e97e01 into main Apr 7, 2021
@lmsurpre lmsurpre deleted the issue-2207 branch April 7, 2021 14:31
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.

None yet

3 participants