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

FHIR Repository API (in-memory) #312

Merged
merged 5 commits into from
Jun 23, 2023
Merged

Conversation

c-schuler
Copy link
Contributor

Added an in-memory repository with search functionality and tests.

Question for reviewer:
Is the search functionality needed for the Bundle repository? (It is an easy add if it is needed)

@c-schuler
Copy link
Contributor Author

@barhodes I appreciate the contributions to this PR. However, I would like to know why these changes are necessary. It would have been nice to have you review the code and request changes that I could apply. One change I added that you rolled back was ignoring the provided resource ID for the create operation (as the spec outlines). What is the reasoning to not ignore the ID? How does create differ from the update operation?

@barhodes
Copy link
Contributor

@barhodes I appreciate the contributions to this PR. However, I would like to know why these changes are necessary. It would have been nice to have you review the code and request changes that I could apply. One change I added that you rolled back was ignoring the provided resource ID for the create operation (as the spec outlines). What is the reasoning to not ignore the ID? How does create differ from the update operation?

I ran into issues using the IdDt type when matching that were resolved by using the Ids helper like we did in the TestRepository. Looks like I got a little overzealous in merging those on the create and missed the random id getting created. I'll correct that. The rest of the changes were all done to ensure compatibility with hapi-fhir so the search parameters work correctly when using the Clinical Reasoning module against Smile CDR.

@c-schuler
Copy link
Contributor Author

Thanks for the info! So this PR is kind of in a weird state as you are both a contributor and a reviewer. Should I review your code or will someone else be providing that review?

@barhodes
Copy link
Contributor

Yeah, I've asked JP to review. You are more than welcome to review as well.

@JPercival JPercival merged commit 40c8e08 into master Jun 23, 2023
2 checks passed
@JPercival JPercival deleted the repository-api-param-matching branch June 23, 2023 18:42
import ca.uhn.fhir.context.FhirContext;

/**
* This class has been deprecated. Use InMemoryFhirRepository instead.
Copy link

Choose a reason for hiding this comment

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

@barhodes could there be a reason why you did not use the deprecated annotation for this class?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only my ignorance of that annotation. I will get it added in. Thanks!

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

4 participants