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

Fixes #705 - Client correctly validates the server's conformance statement FHIR version #706

Merged
merged 2 commits into from Aug 13, 2017

Conversation

Projects
None yet
2 participants
@InfiniteLoop90
Contributor

InfiniteLoop90 commented Aug 6, 2017

Fixed #705 by changing the FHIR version string comparisons that the client side code does when validating the server's conformance statement.

Fixed #705 by making RestfulClientFactory check for the correct FHIR …
…version strings when the client code tries to validate the server's conformance statement. Also fixed the corresponding unit tests.
@InfiniteLoop90

This comment has been minimized.

Show comment
Hide comment
@InfiniteLoop90

InfiniteLoop90 Aug 6, 2017

Owner

I changed all of these for DSTU1, DSTU2, and DSTU2(HL7.org) to be consistent with what was already done for DSTU2.1 and DSTU3 conformance/compatibility statement providers. I'm assuming/hoping there wasn't a reason why 0.0.82 previously wasn't the only "correct" way to represent the DSTU1 FHIR version, as per https://www.hl7.org/fhir/history.html

I changed all of these for DSTU1, DSTU2, and DSTU2(HL7.org) to be consistent with what was already done for DSTU2.1 and DSTU3 conformance/compatibility statement providers. I'm assuming/hoping there wasn't a reason why 0.0.82 previously wasn't the only "correct" way to represent the DSTU1 FHIR version, as per https://www.hl7.org/fhir/history.html

@InfiniteLoop90 InfiniteLoop90 changed the title from Fixed #705 to #705 Client validation of the server's conformance statement FHIR doesn't work as expected Aug 6, 2017

@InfiniteLoop90 InfiniteLoop90 changed the title from #705 Client validation of the server's conformance statement FHIR doesn't work as expected to #705 Client correctly validates the server's conformance statement FHIR version Aug 6, 2017

@InfiniteLoop90 InfiniteLoop90 changed the title from #705 Client correctly validates the server's conformance statement FHIR version to Fixes #705 - Client correctly validates the server's conformance statement FHIR version Aug 6, 2017

@InfiniteLoop90

This comment has been minimized.

Show comment
Hide comment
@InfiniteLoop90

InfiniteLoop90 Aug 6, 2017

Owner

Since STU3 is finalized, I'm assuming that looking up the FHIR version for STU3 via Constants#VERSION is no longer required, hence the deletion of the Dstu3Version private class.

Since STU3 is finalized, I'm assuming that looking up the FHIR version for STU3 via Constants#VERSION is no longer required, hence the deletion of the Dstu3Version private class.

@InfiniteLoop90

This comment has been minimized.

Show comment
Hide comment
@InfiniteLoop90

InfiniteLoop90 Aug 7, 2017

Owner

@jamesagnew Should we explicitly account for FhirVersionEnum#DSTU2_1 as well? I'm guessing that since DSTU2.1 was never released, it probably isn't widely used and may not be a worth explicitly accounting for here. On the other hand... it would take no effort to account for it here. Let me know your thoughts. Thanks!

@jamesagnew Should we explicitly account for FhirVersionEnum#DSTU2_1 as well? I'm guessing that since DSTU2.1 was never released, it probably isn't widely used and may not be a worth explicitly accounting for here. On the other hand... it would take no effort to account for it here. Let me know your thoughts. Thanks!

This comment has been minimized.

Show comment
Hide comment
@InfiniteLoop90

InfiniteLoop90 Aug 7, 2017

Owner

I went ahead and added a check for DSTU2.1 in commit 9a32202

Owner

InfiniteLoop90 replied Aug 7, 2017

I went ahead and added a check for DSTU2.1 in commit 9a32202

Also related to issue #705, added an explicit check for DSTU2.1 as we…
…ll when the client checks the server's conformance statement FHIR version
@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Aug 13, 2017

Owner

This looks great, thanks for the PR!

I'm going to merge, although I'm reverting one small bit: the removal of Dstu3Version.java. The reason we use this instead of hardcoding the string is that when we update the model classes (which may happen again if there are any further patch releases to FHIR DSTU3 itself) it's good to pull the version from the structures themselves to ensure we're outputting the correct one.

Owner

jamesagnew commented Aug 13, 2017

This looks great, thanks for the PR!

I'm going to merge, although I'm reverting one small bit: the removal of Dstu3Version.java. The reason we use this instead of hardcoding the string is that when we update the model classes (which may happen again if there are any further patch releases to FHIR DSTU3 itself) it's good to pull the version from the structures themselves to ensure we're outputting the correct one.

jamesagnew added a commit that referenced this pull request Aug 13, 2017

@jamesagnew jamesagnew merged commit 72be7ab into jamesagnew:master Aug 13, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

jamesagnew added a commit that referenced this pull request Aug 13, 2017

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