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

Fix issue with duplicated IDs inside the Knowledge Manager #2262

Closed
wants to merge 2 commits into from

Conversation

vitorpamplona
Copy link
Collaborator

Search by ID in the KnowledgeManager was used to return only the first reference. If the ID was duplicated in the DB, it would not filter by type and return the wrong one. This fixes it while also enforcing the ID to be a complete word in the URL

Type
Bug fix

Screenshots (if applicable)

Checklist

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • 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 demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

…plicated, it would not filter by type and return the wrong one. This fixes it while also enforcing the id to be a complete word in the URL
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Oct 13, 2023
  - With unmerged PR #11
  - Wup google#1669
  - Wup #9
  - Wup google#2178
  - Wup #10
  - Wup google#2230
  - Wup google#2262
@omarismail94 omarismail94 enabled auto-merge (squash) October 13, 2023 10:53
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.

  1. please add tests.
  2. is this definitely the right fix? trying to match part of the url is still weird to me. if there is an id that is a substring of another id, the result isn't guaranteed... i have been a little worried about this actually regarding id and name... seems really confusing in our code.

@jingtang10
Copy link
Collaborator

let's not merge this as is - i think we still have issues to address here.

@jingtang10
Copy link
Collaborator

closing in favour of the new pr. thx vitor!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

3 participants