fixing fulltext search wit h_lastUpdated filter #6411

TipzCM · 2024-10-28T15:46:35Z

closes #6404

* Contained bug * more tests * changelog, tests, implementation * code review * backwards logic

github-actions · 2024-10-28T15:47:42Z

Formatting check succeeded!

...rc/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6404-fulltext-search-with-lastupdate.yaml

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/upgrade.md

...ir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchSearchBuilder.java

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSearchDaoR4Test.java

jamesagnew · 2024-10-30T17:32:41Z

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/upgrade.md

+# Fulltext Search with _lastUpdated Filter
+
+Fulltext searches have been updated to support `_lastUpdated` search parameter. A reindexing of Search Parameters
+is required to migrate old data to support the `_lastUpdated` search parameter.


This should be expanded to clarify under what exact conditions it would be necessary to do this, since requiring a reindex of all of your data is a huge ask.. Presumably this only matters if XXX is set to YYY and you are intending to perform a search containing ZZZ

It's kinda required if you're using lucene/fulltext search at all.

Currently, any searches with _lastIndex will not use lucene (if that's the only parameter).

If _lastIndex is provided with any other parameter, it will not work.

This includes in our $mdm-clear job (which was the original bug reported)

Doesn't this require explicitly configuring AdvancedHSearchIndexing (which is disabled by default) in order for it to try using lucene to satisfy that query? If it's going to lucene even with that setting disabled, we should fix that bug too.

Yes, this is only required if they are already using lucene. Which is why i included the migration note.

If they aren't using lucene/fulltext search, then obviously they won't have to migrate data to fill lucene data in. It doesn't exist.

No but does this happen to anyone who has lucene enabled, or do they specifically need to have the AdvancedHSearchIndexing feature enabled (again, a feature which is disabled by default)? Please test this if you aren't sure. That flag is the thing that determines whether we use lucene for parameters other than _text and _content. Most people will have it disabled, and if we're using lucene to handle _lastUpdated even with that flag disabled, then that in itself is probably a bug.

Please remember, you are asking people with potentially a lot of data to perform what would be an expensive and lengthy reindex. You need to be very specific about what circumstances would lead to someone needing to do this.

jamesagnew · 2024-10-30T17:36:50Z

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java

-				if (myFulltextSearchSvc != null && !myFulltextSearchSvc.isDisabled()) {
+				if (myFulltextSearchSvc != null && !myFulltextSearchSvc.isDisabled() && changed.isChanged()) {
+					// set the lastUpdated dates so we can use them to search in lucene
+					theResource.getMeta().setLastUpdated(theTransactionDetails.getTransactionDate());


This is a super weird thing to be doing here. Why is it necessary to modify the resource passed in? We index the entity, not the FHIR resource model class.

The fields to index are determined by what's in the resource (not the entity) since the resource contains the changed fields.

ExtendedHSearchIndexExtractor is where we create the ExtendedHSearchIndexData and SearchParamTextPropertyBinder is what consumes it to wrie the indexes

…or $validate-code operation. (#6438) * Move the validation providers to the test utilities package such that they can be reused. * Update IValidationSupport.CodeValidationIssue structure such that it meets the FHIR specification. Update RemoteTerminologyServiceValidationSupport and VersionSpecificWorkerContextWrapper translation for issues. * New tests for issue translation. Move test class to a different package so that we can add another test class. * Some simplification in the issue API to simplify building of issues. * Fix compilation errors. * Update providers to allow multiple responses and add support for the fetchCodeSystem call through method find. * Setup the first test for resource validation with remote terminology providers. * Fix NullPointerException * avoid calling validateCode for CodeSystem where system is null * Keep old public API methods (and class name) in IValidationSupport and mark them as deprecated to avoid breaking dependencies. * Revert local change to debug for duplicate errors. * Add more test cases for resource validation. Throw exception to signal missing test setup to make it obvious. * Simplify test setup. * Add some more javadoc * Add javadoc for the new test class * Add more tests * Address code review comments in IValidationSupport. * Add changelog

Change Map to Multimap to support multiple and clauses.

…RAGE_PRECHECK_FOR_CACHED_SEARCH (#6436) * fix interceptor hooks from requestDetails not getting called for STORAGE_PRECHECK_FOR_CACHED_SEARCH * added unit tests and updated changelog * added one more test case

Lightweight implementation of the adapter pattern.

jamesagnew · 2024-11-07T14:37:58Z

hapi-fhir-jpaserver-elastic-test-utilities/src/test/resources/logback-test.xml

-<!--	<logger name="tracer" level="TRACE" additivity="false">-->
-<!--		<appender-ref ref="STDOUT" />-->
-<!--	</logger>-->
+	<logger name="org.hibernate.search.elasticsearch.request" additivity="false" level="trace">


This should be switched back off before merging.

jamesagnew · 2024-11-07T14:38:19Z

hapi-fhir-jpaserver-test-r4/src/test/resources/logback-test.xml

@@ -31,15 +31,15 @@
 	<logger name="ca.uhn.fhir.jpa.bulk" level="info"/>

 	<!-- more debugging -->
-	<!--
+<!--	&lt;!&ndash;-->


This should be switched back off before merging

codecov · 2024-11-07T16:35:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (rel_7_6@7faf0f7). Learn more about missing BASE report.

Additional details and impacted files

@@            Coverage Diff             @@
##             rel_7_6    #6411   +/-   ##
==========================================
  Coverage           ?   83.54%           
  Complexity         ?    27781           
==========================================
  Files              ?     1741           
  Lines              ?   107563           
  Branches           ?    13507           
==========================================
  Hits               ?    89859           
  Misses             ?    11917           
  Partials           ?     5787

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tadgh and others added 2 commits October 26, 2024 01:09

Contained bug (#6402)

e78a0b8

* Contained bug * more tests * changelog, tests, implementation * code review * backwards logic

fixing fulltext search wit h_lastUpdated filter

9d3b8c9

barhodes and others added 2 commits October 28, 2024 11:38

Fix Questionnaire doc (#6400)

5fdce4e

cleanup

c85ce2f

tadgh requested changes Oct 29, 2024

View reviewed changes

leif stawnyczy and others added 5 commits October 29, 2024 14:23

review fixes

e0bb4fb

spotless

d91316b

Failing test

5dc278f

review fixes round 2

7dfe040

spotless

5722b75

tadgh approved these changes Oct 29, 2024

View reviewed changes

leif stawnyczy added 2 commits October 30, 2024 10:01

fixing tests

54ae463

Merge branch 'rel_7_6' into 6404-lucene-search-with-last-updated

50e7ab0

TipzCM enabled auto-merge (squash) October 30, 2024 17:12

jamesagnew reviewed Oct 30, 2024

View reviewed changes

mrdnctrk and others added 9 commits October 31, 2024 12:03

fix reindex optimizeStorage=ALL_VERSIONS (#6421)

59b9750

fixing broken rename of last step of reindex (#6429)

34359b2

Change Repository search interface from Map to Multimap (#6445)

d0eb855

Change Map to Multimap to support multiple and clauses.

licenses

57f4076

fix interceptor hooks from requestDetails not getting called for STO…

6c4c6bd

…RAGE_PRECHECK_FOR_CACHED_SEARCH (#6436) * fix interceptor hooks from requestDetails not getting called for STORAGE_PRECHECK_FOR_CACHED_SEARCH * added unit tests and updated changelog * added one more test case

elastisearch tester data

808bb0a

Add Adapter api (#6450)

7faf0f7

Lightweight implementation of the adapter pattern.

Merge branch 'rel_7_6' into 6404-lucene-search-with-last-updated

358f05c

jamesagnew reviewed Nov 7, 2024

View reviewed changes

updating tests

ff28eae

leif stawnyczy added 2 commits November 7, 2024 17:09

spotless

68da721

logbackchanges

fd56f14

TipzCM changed the base branch from rel_7_6 to master November 8, 2024 15:50

TipzCM requested review from michaelabuckley, fil512 and AD1306 as code owners November 8, 2024 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixing fulltext search wit h_lastUpdated filter #6411

fixing fulltext search wit h_lastUpdated filter #6411

TipzCM commented Oct 28, 2024

github-actions bot commented Oct 28, 2024 •

edited

Loading

jamesagnew Oct 30, 2024

TipzCM Nov 1, 2024

jamesagnew Nov 7, 2024

TipzCM Nov 7, 2024

jamesagnew Nov 8, 2024 •

edited

Loading

jamesagnew Oct 30, 2024

TipzCM Nov 1, 2024

jamesagnew Nov 7, 2024

jamesagnew Nov 7, 2024

codecov bot commented Nov 7, 2024

fixing fulltext search wit h_lastUpdated filter #6411

Are you sure you want to change the base?

fixing fulltext search wit h_lastUpdated filter #6411

Conversation

TipzCM commented Oct 28, 2024

github-actions bot commented Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesagnew Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 7, 2024

Codecov Report

github-actions bot commented Oct 28, 2024 •

edited

Loading

jamesagnew Nov 8, 2024 •

edited

Loading