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.

Sign up for GitHub

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

Jump to bottom

Support Contains for string search #426

Merged
merged 14 commits into from
May 14, 2021
Merged

Support Contains for string search #426

merged 14 commits into from
May 14, 2021

Conversation

epicadk
Copy link
Contributor

@epicadk epicadk commented Apr 5, 2021 •
edited by jingtang10
Loading

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #376
Description

Support contains for string search.

Alternative(s) considered

Explained in the comment on linked issue. I've removed the ParamPrefixEnum as a parameter of string filter because those params are only meant for Number Quantity and date parameters
refer https://www.hl7.org/fhir/search.html#prefix or the ParamPrefixEnum Javadoc

I also cannot find a StringParamModifier Enum like the TokenParamModifier and UriParamQualifierEnum Enums that's why I used the string param class. I think we could possibly create our own implementation of the StringParamModifier class.

The tests I have added to the FHIREngineImpl are from https://www.hl7.org/fhir/search.html#string

Type
Choose one: Feature

Screenshots (if applicable)

Checklist

  • I have read and acknowledged the Code of conduct
  • I have read How to Contribute
  • I have read the Developer's guide
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally
  • I have built and run the reference app(s) to verify my change fixes the issue and/or does not break the reference app(s)

jingtang10
jingtang10 requested changes May 12, 2021
View reviewed changes
Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

Thanks @epicadk for this change! Just a comment on the syntax for the search API.

engine/src/main/java/com/google/android/fhir/search/SearchDsl.kt Outdated Show resolved Hide resolved
@epicadk epicadk mentioned this pull request May 13, 2021
Closed
@jingtang10
Copy link
Collaborator

Thanks @epicadk - I would really love to focus on the contains issue and handle the change of indexing in a separate PR. Is it ok?

@epicadk
Copy link
Contributor Author

epicadk commented May 14, 2021

Thanks @epicadk - I would really love to focus on the contains issue and handle the change of indexing in a separate PR. Is it ok?

Yep sorry. Reverting the change.

@epicadk epicadk requested a review from jingtang10 May 14, 2021 13:24
@epicadk
Copy link
Contributor Author

epicadk commented May 14, 2021

@jingtang10 done.

jingtang10
jingtang10 requested changes May 14, 2021
View reviewed changes
Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

thanks!

engine/src/main/java/com/google/android/fhir/search/params/StringSearchModifier.kt Outdated Show resolved Hide resolved
engine/src/main/java/com/google/android/fhir/search/params/StringSearchModifier.kt Outdated Show resolved Hide resolved
engine/src/main/java/com/google/android/fhir/search/params/StringSearchModifier.kt Outdated Show resolved Hide resolved
engine/src/test/java/com/google/android/fhir/search/SearchTest.kt Outdated Show resolved Hide resolved
engine/src/test/java/com/google/android/fhir/search/SearchTest.kt Outdated Show resolved Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
@epicadk
Refractor: StringSearchModifier.kt
1f8c390 
Improve tests
Improve Formatting
@epicadk epicadk requested a review from jingtang10 May 14, 2021 16:28
@epicadk
Copy link
Contributor Author

epicadk commented May 14, 2021

@jingtang10 done

jingtang10
jingtang10 approved these changes May 14, 2021
View reviewed changes
Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

Great! Thanks so much for this. Please feel free to merge after addressing the final few (nit) comments.

engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Show resolved Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated
}
assertThat(res).hasSize(1)
assertThat(res[0].id).isEqualTo("Patient/${patient1.id}")
assertThat(res[0].nameFirstRep.given.any { it.toString() == "Eve" }).isTrue()
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think just checking the patient id is right is enough?

engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
@epicadk
fix: assertions
e0b2705 
chore: fix formatting
@epicadk epicadk enabled auto-merge May 14, 2021 19:22
@epicadk epicadk merged commit 4a2c5d5 into google:master May 14, 2021
@jingtang10
Copy link
Collaborator

Thanks again @epicadk for this PR and merging!

@epicadk epicadk deleted the string_search branch May 17, 2021 20:09
@jingtang10 jingtang10 added this to In progress in FHIR engine library via automation May 19, 2021
@jingtang10 jingtang10 linked an issue May 19, 2021 that may be closed by this pull request
Make the string search match the beginning of the string by default #401
Closed
@fredhersch fredhersch mentioned this pull request May 28, 2021
Closed
8 tasks
@Tarun-Bhardwaj Tarun-Bhardwaj added this to the Q2 June 2021 milestone Jun 4, 2021
@f-odhiambo f-odhiambo mentioned this pull request Aug 2, 2021
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jingtang10 jingtang10 jingtang10 approved these changes

@deepankarb deepankarb Awaiting requested review from deepankarb

@kevinmost kevinmost Awaiting requested review from kevinmost

@pankajkatoch pankajkatoch Awaiting requested review from pankajkatoch

@yigit yigit Awaiting requested review from yigit

Assignees

@epicadk epicadk

Labels
None yet
Projects
Android FHIR SDK
Archived in project
FHIR engine library
  
In progress
Milestone
Q2 June 2021
Development

Successfully merging this pull request may close these issues.

Make the string search match the beginning of the string by default Support contains for string search
3 participants
@epicadk @jingtang10 @Tarun-Bhardwaj