Introduce container ordinal column in the search parameter index tables and populate it #5832

codeforgreen · 2024-04-08T21:40:57Z

No description provided.

* failing test * fix * changelog * spotless * fix tests

…irHibernateJpaDialect (#5791) * prefixed the log message with "Unit test: " * resolved review comments

* CVE-2024-1597 * CVE-2024-22243

…ing (#5803) * wip * cleaning up test * adding changelog and passing spotless. --------- Co-authored-by: peartree <etienne.poirier@smilecdr.com>

* Reducer LOB usage in Batch2 and Search (#5748) * Reducer LOB usage in Batch2 and Search * Add changelog * Rework a number of LOB columns * Test fix * Test fix * Column fixes * Test fix * Formatting * Fixes * patching bad oracle test * Apply spotless --------- Co-authored-by: Tadgh <garygrantgraham@gmail.com> * Clean up * Revert change * One more revert * Resolve compile issue --------- Co-authored-by: Tadgh <garygrantgraham@gmail.com>

* Add support for latest version of CR and new $questionnaire operation * Fix formatting in javadoc causing warning * Update to 3.0.0 CR release * Add support for expected parameter names from updated IG's * Update Clinical Reasoning documentation * Update pom.xml * Create 5750-update-cr-operations.yaml * fix doc * Update to latest CR version * Update pom.xml * In version * Update changelog * Remove commented code

* failing test * fix * add @nullable

* Limit bulk export chunk size * Cleanup * Work on tests * Working * Add some docs * Address revuew comments * Spotless * Test fix

* Add _language param to providers and tests * add pr number to docs * fix test * remove unnecessary code --------- Co-authored-by: Lila Mikalson <lila.mikalson@smilecdr.com>

* update model * convert results * migration * extend test coverage in cli * fix broken test * change log * code review feedback * spotless

Stop writing to hfj_forced_id Stop reading ForcedId Cleanup client-id assignment Fix query counts in tests Drop the unused indexes and fk

…ify the capability statement (#5814) * added new customizer filter factory. have not yet deleted old code it replaces. * added new customizer filter factory. have not yet deleted old code it replaces. * replaced websocket filter. works. still some cleanup to do * replaced websocket filter. works. still some cleanup to do * cosmetic change * add coverage and fix bugs it found * spotless * move capability statement classes * add changelog and rename new classes * review feedback

* step one of unifying the bulk exports * adding changelog * review points * spotless --------- Co-authored-by: leif stawnyczy <leifstawnyczy@leifs-mbp.home>

* refactoring * test refactor only * spotless * bumping version --------- Co-authored-by: leif stawnyczy <leifstawnyczy@leifs-mbp.home>

* licenses * remove mdm placeholder interceptor * remove mdm placeholder interceptor * fix regression * Remove RTE Placeholder Interceptor. Add tests to assert proper boot sequence. * consolidate LogbackCaptureTestExtension (removed the cdr class) * consolidate LogbackCaptureTestExtension (removed the cdr class)

* adding a catch for failing reduction step * spotless * added changelog --------- Co-authored-by: leif stawnyczy <leifstawnyczy@leifs-mbp.home>

…late the value when the indexes are built. Refactor to decouple entities from settings and from hashing logic.

github-actions · 2024-04-08T21:42:11Z

Formatting check succeeded!

…itionSubscriptionEnabled if it's true. Make that setting is true by default (#5810) * First commit with TODOs and logging. * Try to add cross partition config at startup to subscription module. * Barely working solution with JpaStorageSettings injected into the Subscription module with the correct config for cross partition enabled. * Implement agreed upon solution where StorageSettings used in the subscription module uses the JpaStorageSettings cross partition enabled setting. Fix all compile errors. TODOs for tests to add and known test failures. * Fix test errors caused by bad log code. Ensure all modules use StorageSettings for canonicalizer. * Cleanup. * Reintroduce old SubscriptionCanonicalizer constructor, but add a StorageSettings and deprecate it. Cleanup logs and TODOs. * Deprecate FHIR_PATCH. More cleanup. * Deprecate FHIR_PATCH correctly. * Small fix. * Set myCrossPartitionSubscriptionEnabled to true by default. * Fix test failures. * Fix another test. * Code review feedback. * Resolve static analysis warnings.

…ce-container-ord-seach-parameter-and-populate

codecov · 2024-04-10T03:27:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (mm-20240318-support-for-contained-true-search-parameter@57fdc2d). Click here to learn what that means.

Additional details and impacted files

@@                                    Coverage Diff                                     @@
##             mm-20240318-support-for-contained-true-search-parameter    #5832   +/-   ##
==========================================================================================
  Coverage                                                           ?   83.44%           
  Complexity                                                         ?    26920           
==========================================================================================
  Files                                                              ?     1688           
  Lines                                                              ?   104025           
  Branches                                                           ?    13195           
==========================================================================================
  Hits                                                               ?    86805           
  Misses                                                             ?    11578           
  Partials                                                           ?     5642

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

michaelabuckley · 2024-04-09T16:56:16Z

...aserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/BaseResourceIndexedSearchParam.java

-	private transient StorageSettings myStorageSettings;
+	@GenericField
+	@Column(name = "CONTAINED_ORD", nullable = true)
+	private Integer myContainedOrd;


minor: should this be Short instead? The migration used tinyint which is just 2 bytes.

Oh, I had updated this to Short in one of the sub-sequent commits, you're probably seeing an older version: https://github.com/hapifhir/hapi-fhir/pull/5832/files#diff-78a0ab6d4a89738d68dc1c231adcd7a75c64a81ee918cdcde761dde8b4f48637R69

michaelabuckley · 2024-04-12T14:55:00Z

hapi-fhir-base/src/main/java/ca/uhn/fhir/util/FhirTerser.java

-		public void assignIdsToContainedResources() {
-
-			if (!getContainedResources().isEmpty()) {
+	public static class ContainedResources extends EmbeddedResources {


question: Do we need a new EmbeddedResources intermediate class? Is there a reason we can't apply ordinals to the other cases?
problem: I can't find a test for the ordinal assignment.

Embedded resources can also mean resource entries in a bundle. Basically any resource that's embedded into another resource. I guess there may even be other cases. I kept is separate.
I added this test here for now: https://github.com/hapifhir/hapi-fhir/pull/5832/files#diff-d9ea0db9e7f4eb2987dbfb2fa7376d67cac04733f6fca71caeaa363bf207b12aR119. I plan to add some more after I implement the query/search part of the functionality.

michaelabuckley · 2024-04-12T14:56:20Z

...li/hapi-fhir-cli-api/src/test/java/ca/uhn/fhir/cli/HapiFlywayMigrateDatabaseCommandTest.java

@@ -121,7 +121,7 @@ public void testMigrateFrom340() throws IOException, SQLException {
 			assertEquals("identifier", values.get(0).get("SP_NAME"));
 			assertEquals("12345678", values.get(0).get("SP_VALUE"));
 			assertTrue(values.get(0).keySet().contains("HASH_IDENTITY"));
-			assertEquals(7001889285610424179L, values.get(0).get("HASH_IDENTITY"));
+			assertEquals(792023395537367244L, values.get(0).get("HASH_IDENTITY"));


serious problem: we must never change the hashes of existing data. Why did this change?

I added the bit in the hash (boolean) to distinguish contained indexes. I guess I can always leave as is if the property to index them is false. I did think about it, the impact would be less it just requires more branching in the code.

michaelabuckley · 2024-04-12T14:57:22Z

...hir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirMigrationTasksV7.java

+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+public class HapiFhirMigrationTasksV7 {


minor: Hapi style is to keep adding them to he main class

Alright, I guess it's ok if the class becomes gigantic then. I will move it there.

michaelabuckley · 2024-04-12T14:58:52Z

...r-model/src/test/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamStringTest.java


 		// Make sure our hashing function gives consistent results
-		assertEquals(6598082761639188617L, token.getHashNormalizedPrefix().longValue());
-		assertEquals(-1970227166134682431L, token.getHashExact().longValue());
+		assertEquals(-3910281295602822115L, token.getHashNormalizedPrefix().longValue());


catastrophic: these hashes are wrong. They must never change.

michaelabuckley · 2024-04-12T15:35:31Z

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

@@ -2827,7 +2827,7 @@ public void testTransaction_MultipleConditionalUpdates() {
 		myCaptureQueriesListener.logSelectQueriesForCurrentThread();
 		assertEquals(2, myCaptureQueriesListener.countSelectQueriesForCurrentThread());
 		assertThat(myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false), containsString("rispt1_0.PARTITION_ID in ('1')"));
-		assertThat(myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false), containsString("rispt1_0.HASH_SYS_AND_VALUE in ('7432183691485874662','-3772330830566471409','-4132452001562191669')"));


problem: hash

michaelabuckley · 2024-04-12T15:36:17Z

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

@@ -69,7 +69,7 @@ public void testRawSql_Search() {
 			String query = list.get(0).getSql(true, false);
 			ourLog.info("Query: {}", query);

-			assertThat(query, containsString("HASH_SYS_AND_VALUE = '3788488238034018567'"));


problem: hash

michaelabuckley · 2024-04-12T15:38:52Z

...server-test-utilities/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/CalculateHashesTest.java

-		task.addCalculator("HASH_SYS", t -> ResourceIndexedSearchParamToken.calculateHashSystem(new PartitionSettings(), RequestPartitionId.defaultPartition(), t.getResourceType(), t.getParamName(), t.getString("SP_SYSTEM")));
-		task.addCalculator("HASH_SYS_AND_VALUE", t -> ResourceIndexedSearchParamToken.calculateHashSystemAndValue(new PartitionSettings(), RequestPartitionId.defaultPartition(), t.getResourceType(), t.getParamName(), t.getString("SP_SYSTEM"), t.getString("SP_VALUE")));
-		task.addCalculator("HASH_VALUE", t -> ResourceIndexedSearchParamToken.calculateHashValue(new PartitionSettings(), RequestPartitionId.defaultPartition(), t.getResourceType(), t.getParamName(), t.getString("SP_VALUE")));
+		task.addCalculator("HASH_IDENTITY", t -> hasher.hash(RequestPartitionId.defaultPartition(), t.getResourceType(), t.getString("SP_NAME")));


suggestion: I much prefer the explict names like calculateHashSystemAndValue than an anonymous list of strings whose order is important, but unspecified.

michaelabuckley · 2024-04-12T15:39:07Z

...server-test-utilities/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/CalculateHashesTest.java

-			assertEquals(2686400398917843456L, map.get("HASH_SYS"));
-			assertEquals(-3943098850992523411L, map.get("HASH_SYS_AND_VALUE"));
-			assertEquals(845040519142030272L, map.get("HASH_VALUE"));
+			assertEquals(792023395537367244L, map.get("HASH_IDENTITY"));


problem: change hash

michaelabuckley · 2024-04-12T15:39:22Z

.../src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderDialectPostgresTest.java

@@ -50,7 +50,7 @@ public void testOrdinalSearchesUseIntegerParameters() {

 		assertEquals(4, StringUtils.countMatches(sql, "?"));
 		assertEquals(4, generatedSql.getBindVariables().size());
-		assertEquals(123682819940570799L, generatedSql.getBindVariables().get(0));
+		assertEquals(45648902074593574L, generatedSql.getBindVariables().get(0));


problem: change hash

michaelabuckley · 2024-04-16T18:05:27Z

...hir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirMigrationTasksV7.java

+
+		AtomicInteger i = new AtomicInteger();
+		indexTableList.forEach(idxTable -> {
+			Builder.BuilderWithTableName tableBuilder = myVersion.onTable(idxTable);


minor: this is a bit fancy for migrations. One of the challenges of migrations is ensuring that old migrations don't change as we write new code. We should think of old migrations as a historical document, not living code. But fancy code like this tempts refactoring. We prefer simple and explicit here. Even more so given the production problems caused by bad migrations.

nathandoef and others added 19 commits March 20, 2024 16:08

Retain X-Request-ID header key casing in responses (#5787)

7742aa7

* failing test * fix * changelog * spotless * fix tests

Resolve 5790 Prefix a unit test log for hibernate exception in HapiFh…

4a726dd

…irHibernateJpaDialect (#5791) * prefixed the log message with "Unit test: " * resolved review comments

CVE 2024 03 26 (#5804)

4d4d8b2

* CVE-2024-1597 * CVE-2024-22243

MDM expansion doesn't work when using the :mdm qualifier in query str…

2f9693a

…ing (#5803) * wip * cleaning up test * adding changelog and passing spotless. --------- Co-authored-by: peartree <etienne.poirier@smilecdr.com>

Fix BaseOutcomeReturningMethodBinding NullPointer (#5808)

9e5db19

* failing test * fix * add @nullable

Enforce maximum chunk size in bulk export (#5800)

6ce3b17

* Limit bulk export chunk size * Cleanup * Work on tests * Working * Add some docs * Address revuew comments * Spotless * Test fix

fixed resource fullUrl in _history bundles (#5815)

70843cd

Add _language param to providers (#5801)

e39ee7f

* Add _language param to providers and tests * add pr number to docs * fix test * remove unnecessary code --------- Co-authored-by: Lila Mikalson <lila.mikalson@smilecdr.com>

Jr 20240325 concept map unmatched codes (#5809)

1736dad

* update model * convert results * migration * extend test coverage in cli * fix broken test * change log * code review feedback * spotless

Stop writing to hfj_forced_id (#5817)

fcf0a85

Stop writing to hfj_forced_id Stop reading ForcedId Cleanup client-id assignment Fix query counts in tests Drop the unused indexes and fk

Merge history table prefetch with resource table. (#5825)

bdea4b6

unify bulk export patient type and bulk export patient instance (#5822)

620b46d

* step one of unifying the bulk exports * adding changelog * review points * spotless --------- Co-authored-by: leif stawnyczy <leifstawnyczy@leifs-mbp.home>

Batch2 test refactor only (#5823)

5d55594

* refactoring * test refactor only * spotless * bumping version --------- Co-authored-by: leif stawnyczy <leifstawnyczy@leifs-mbp.home>

reduction step failing fails jobs (#5831)

107de2c

* adding a catch for failing reduction step * spotless * added changelog --------- Co-authored-by: leif stawnyczy <leifstawnyczy@leifs-mbp.home>

Introduce container ordinal in the search paramter index tables. Popu…

65a4fcc

…late the value when the indexes are built. Refactor to decouple entities from settings and from hashing logic.

codeforgreen requested a review from a team as a code owner April 8, 2024 21:40

lukedegruchy and others added 4 commits April 9, 2024 14:27

Fix build errors. Update containerOrd field to short instead of integer.

8b0441a

Merge remote-tracking branch 'origin/master' into mm-20240408-introdu…

f7b5871

…ce-container-ord-seach-parameter-and-populate

Resolve conflicts after merge.

71361a5

codeforgreen requested review from fil512, tadgh and jamesagnew as code owners April 9, 2024 19:20

codeforgreen added 2 commits April 9, 2024 14:34

Small changes to fix remaining failing tests

079f497

Fix static code analysis problem with class name

88289dc

codeforgreen added 2 commits April 9, 2024 15:12

Fix static code analysis warning

5eb3b9b

Fix a new more hash values in tests

238a4b3

codeforgreen requested a review from nathandoef as a code owner April 9, 2024 22:47

Include new bean in subscription test configs.

04e3305

Fix static code analysis warning

05f4870

michaelabuckley requested changes Apr 12, 2024

View reviewed changes

michaelabuckley reviewed Apr 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce container ordinal column in the search parameter index tables and populate it #5832

Introduce container ordinal column in the search parameter index tables and populate it #5832

codeforgreen commented Apr 8, 2024

github-actions bot commented Apr 8, 2024 •

edited

codecov bot commented Apr 10, 2024 •

edited

michaelabuckley Apr 9, 2024

codeforgreen Apr 12, 2024 •

edited

michaelabuckley Apr 12, 2024

codeforgreen Apr 12, 2024

michaelabuckley Apr 12, 2024

codeforgreen Apr 12, 2024 •

edited

michaelabuckley Apr 12, 2024

codeforgreen Apr 12, 2024

michaelabuckley Apr 12, 2024

michaelabuckley Apr 12, 2024

michaelabuckley Apr 12, 2024

michaelabuckley Apr 12, 2024

michaelabuckley Apr 12, 2024

michaelabuckley Apr 12, 2024

michaelabuckley Apr 16, 2024

Introduce container ordinal column in the search parameter index tables and populate it #5832

Are you sure you want to change the base?

Introduce container ordinal column in the search parameter index tables and populate it #5832

Conversation

codeforgreen commented Apr 8, 2024

github-actions bot commented Apr 8, 2024 • edited

codecov bot commented Apr 10, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

codeforgreen Apr 12, 2024 • edited

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

codeforgreen Apr 12, 2024 • edited

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

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

github-actions bot commented Apr 8, 2024 •

edited

codecov bot commented Apr 10, 2024 •

edited

codeforgreen Apr 12, 2024 •

edited

codeforgreen Apr 12, 2024 •

edited