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 #1961 - Optionally reject references without resource type #2230

Merged
merged 4 commits into from
Apr 14, 2021

Conversation

tbieste
Copy link
Contributor

@tbieste tbieste commented Apr 9, 2021

Issue #1961 - Optionally reject references without resource type

Signed-off-by: Troy Biesterfeld tbieste@us.ibm.com

@tbieste tbieste marked this pull request as ready for review April 9, 2021 18:16
@tbieste tbieste self-assigned this Apr 9, 2021
@tbieste tbieste marked this pull request as draft April 9, 2021 20:25
@tbieste tbieste force-pushed the tbieste-issue-1961 branch 3 times, most recently from 08a778d to 9bd9b81 Compare April 9, 2021 22:01
@tbieste
Copy link
Contributor Author

tbieste commented Apr 9, 2021

Need to regenerate fhir-examples, since many of them have malformed references.

@tbieste tbieste force-pushed the tbieste-issue-1961 branch 4 times, most recently from 1096019 to 0c288a7 Compare April 13, 2021 02:10
@tbieste
Copy link
Contributor Author

tbieste commented Apr 13, 2021

Summary of changes:

  1. Updated fhirServer/core/checkReferenceTypes config (default: true) to also require resource type in refererences.
  2. Added/updated testcases.
  3. Updated AuditEventMapper to put the component id in reference.display instead of reference.reference.
  4. Regenerated complete-mock examples (hence the large number of files), so reference ids are valid syntax.
  5. Updated a few spec examples, since a few included invalid resource types in references that did not match the spec.
  6. Opened $apply operation error when specifying practitioner #2240, which I found while running fhir-server-test testcases.

@tbieste tbieste requested review from lmsurpre and prb112 April 13, 2021 13:44
@tbieste tbieste marked this pull request as ready for review April 13, 2021 13:46
Copy link
Contributor

@prb112 prb112 left a comment

Choose a reason for hiding this comment

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

left some comments

@lmsurpre
Copy link
Member

lmsurpre commented Apr 13, 2021

Updated a few spec examples, since a few included invalid resource types in references that did not match the spec.

We usually try to keep some kind of change log when we edit spec artifacts.
The other option is to just mark them as invalid in our index...then they actually just become negative test cases.

@tbieste
Copy link
Contributor Author

tbieste commented Apr 13, 2021

Updated a few spec examples, since a few included invalid resource types in references that did not match the spec.

We usually try to keep some kind of change log when we edit spec artifacts.
The other option is to just mark them as invalid in our index...then they actually just become negative test cases.

I think all the spec example references that didn't include valid resource types started with "http:" or ":https", so I can revert those spec example changes if I skip validation of those as you suggested in another comment.

Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
Copy link
Member

@lmsurpre lmsurpre left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
@tbieste
Copy link
Contributor Author

tbieste commented Apr 14, 2021

Comments addressed. Ready to merge.

@tbieste tbieste merged commit da73eab into main Apr 14, 2021
@tbieste tbieste deleted the tbieste-issue-1961 branch April 14, 2021 13:45
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.

3 participants