Fi 2311 store session ids #427

rpassas · 2024-01-05T15:28:56Z

Summary

Adding validator sessions to a separate table in the db (requires running migrate). At start up, the invoke validator session job will populate the table.

Comments re: adding the new table for validator sessions and session id retrieval are appreciated.

codecov · 2024-01-05T15:30:54Z

Codecov Report

Attention: Patch coverage is 89.79592% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 77.79%. Comparing base (123ab58) to head (416d0e2).

Files	Patch %	Lines
lib/inferno/dsl/fhir_resource_validation.rb	83.33%	2 Missing ⚠️
lib/inferno/jobs/invoke_validator_session.rb	0.00%	2 Missing ⚠️
lib/inferno/entities/validator_session.rb	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #427      +/-   ##
==========================================
+ Coverage   77.68%   77.79%   +0.10%     
==========================================
  Files         230      233       +3     
  Lines       11510    11583      +73     
  Branches     1083     1099      +16     
==========================================
+ Hits         8942     9011      +69     
- Misses       1936     1940       +4     
  Partials      632      632

Flag	Coverage Δ
backend	`93.90% <89.79%> (+0.01%)`	⬆️
frontend	`70.92% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Jammjammjamm

This needs unit tests.

It doesn't look like this handles multiple validators with the same name but different suite option requirements.

Jammjammjamm · 2024-01-24T13:59:21Z

lib/inferno/jobs/invoke_validator_session.rb

+            session_repo.create(test_suite_id: suite_id, validator_session_id: session_id, \
+                                validator_name:)
+          rescue Sequel::ValidationFailed => e
+            puts e.message


Need to use the logger. This error can just be ignored?

Thanks for pointing this out. I think we should not catch and log this exception because it is not actually ignore-able. If we cannot save a validated session, it defeats the purpose.

Jammjammjamm · 2024-01-24T14:01:17Z

lib/inferno/repositories/validator_sessions.rb

+        )
+      end
+
+      def validator_session(test_suite_id, validator_name)


This method needs a more descriptive name.

Jammjammjamm · 2024-01-24T14:02:21Z

lib/inferno/repositories/validator_sessions.rb

+
+      def validator_session(test_suite_id, validator_name)
+        session = self.class::Model
+          .find(test_suite_id:, validator_name:)


The table needs an index on test_suite_id and validator_name.

Jammjammjamm · 2024-01-24T14:03:25Z

lib/inferno/entities/validator_session.rb

+        super(params, ATTRIBUTES)
+      end
+
+      def test_suite


Is this actually used anywhere?

It is not - removing

Jammjammjamm · 2024-01-24T14:06:28Z

lib/inferno/dsl/fhir_resource_validation.rb

@@ -231,6 +233,9 @@ def issue_message(issue, resource)

        # @private
        def wrap_resource_for_hl7_wrapper(resource, profile_url)
+          validator_session_id = \


You shouldn't need a backslash here.

Jammjammjamm · 2024-01-24T14:08:14Z

lib/inferno/dsl/fhir_resource_validation.rb

@@ -231,6 +233,9 @@ def issue_message(issue, resource)

        # @private
        def wrap_resource_for_hl7_wrapper(resource, profile_url)
+          validator_session_id = \
+            Inferno::Repositories::ValidatorSessions.new.validator_session(test_suite_id,


What is the circumstance under which this would return nil and rely on the instance variable? If it doesn't return nil, wouldn't the instance variable need to be updated?

I could imagine a situation where the startup invoke-session process fails for some reason but we still want to continue and try validating again just in case. But if that happens, this call would return nil but the instance variable would also be nil. I don't think it's possible for the instance variable to be set but not get a result from the DB.

Also, good point about updating the instance variable. We may not keep all sessions forever, so if a session expires, the next validation call will return with a new session id. If the new session id != the old one then it should be persisted.

I figured this was in case the DB fails temporarily for some reason. Updating the instance variable makes sense here. It also gets set in operation_outcome_from_hl7_wrapped_response(), but that is after the response is received. In theory these would be sync'd up.

Jammjammjamm · 2024-01-24T14:13:13Z

lib/inferno/repositories/validator_sessions.rb

+      def validator_session(test_suite_id, validator_name)
+        session = self.class::Model
+          .find(test_suite_id:, validator_name:)
+        if session.nil?


I think return nil if session.nil? would be a lot clearer.

dehall · 2024-01-26T20:14:08Z

lib/inferno/repositories.rb

@@ -3,6 +3,7 @@
 require_relative 'repositories/test_groups'
 require_relative 'repositories/test_suites'
 require_relative 'repositories/tests'
+require_relative 'repositories/validator_sessions'


I'm not 100% sure which group this belongs with, but I don't think this needs to be require'd twice.

Jammjammjamm

What happens if two users are running a suite, a session expires, and one of them tries to create a new session while the other one is also trying to create a new session?

Jammjammjamm · 2024-02-13T15:32:52Z

lib/inferno/db/migrations/010_add_validator_sessions.rb

+      column :validator_name, String, null: false, size: 255
+      column :suite_options, String, text: true, size: 255
+      column :last_accessed, DateTime, null: false
+      index [:validator_name, :test_suite_id, :suite_options]


I believe the columns in the index have to be in the same order as they are in the query in order for the index to work.

Jammjammjamm · 2024-02-13T15:34:40Z

lib/inferno/db/migrations/010_add_validator_sessions.rb

+      column :validator_session_id,  String, null: false, size: 255, unique: true
+      column :test_suite_id, String, null: false, size: 255
+      column :validator_name, String, null: false, size: 255
+      column :suite_options, String, text: true, size: 255


Do you think we are likely to encounter issues with the length of this field since it's serialized JSON?

Valid concern given the length of options in g10. I will remove the size constraint.

dehall · 2024-02-13T16:23:41Z

What happens if two users are running a suite, a session expires, and one of them tries to create a new session while the other one is also trying to create a new session?

The hl7 validator wrapper itself doesn't have any special logic to "deconflict" that so there would be two new sessions created on that side. Depending on the sequence the two users do things in, I could believe either the cached session ID value flip-flops back and forth as each user makes subsequent validation calls, or the second one just overwrites the first and both users wind up using that. I don't think anything will break in either case, but we can think about the right places to check/save the session ID

dehall · 2024-02-13T16:36:39Z

I can't add a comment to the specific line since it's not modified, but I think one thing still missing is saving the session ID that comes back from the validator service to the DB if it's different from the one that was sent over (ie, if the first session expired and a new one was spun up). Currently the check that caches the session ID just as an instance variable is in lib/inferno/dsl/fhir_resource_validation.rb : operation_outcome_from_hl7_wrapped_response (line 279 or so)

rpassas · 2024-02-16T19:53:45Z

What happens if two users are running a suite, a session expires, and one of them tries to create a new session while the other one is also trying to create a new session?

The hl7 validator wrapper itself doesn't have any special logic to "deconflict" that so there would be two new sessions created on that side. Depending on the sequence the two users do things in, I could believe either the cached session ID value flip-flops back and forth as each user makes subsequent validation calls, or the second one just overwrites the first and both users wind up using that. I don't think anything will break in either case, but we can think about the right places to check/save the session ID

I think if both users tried at the same time, calling validateSource with the expired ID passed in, initializeValidator would generate a new session and engine twice. Subsequently, they would just start using the same session after checking the DB for an existing session. I agree with Dylan that I don't think it would cause any problems, but it may allow for the possibility that an extra session is created and ignored until it expires.

Do we think this merits testing?

lib/inferno/dsl/fhir_resource_validation.rb

Jammjammjamm · 2024-02-20T17:40:58Z

lib/inferno/db/schema.rb

+
+      primary_key [:id]
+
+      index [:validator_session_id], :name=>:sqlite_autoindex_validator_sessions_2, :unique=>true


Having this index name isn't great. If you replace 'unique: truewithindex [:validator_session_id], unique: true` does that get rid of it?

Jammjammjamm · 2024-02-20T18:04:08Z

lib/inferno/config/boot/validator.rb

        validators.each_with_index do |validator, index|
          if validator.is_a? Inferno::DSL::FHIRResourceValidation::Validator
-            Inferno::Jobs.perform(Inferno::Jobs::InvokeValidatorSession, suite.id, name.to_s, index)
+            Inferno::Jobs.perform(Inferno::Jobs::InvokeValidatorSession, suite.id, name.to_s, index,
+                                  required_suite_options)


It doesn't seem like there is currently any code in core that exercises creating validator sessions with options. Have you tried this with Dylan's g10 PR? It doesn't work when I try it with this change.

13:01:46 worker.1 | E, [2024-02-20T13:01:46.587085 #51795] ERROR -- : SQLite3::SQLException: no such column: us_core_version: SELECT * FROM `validator_sessions` WHERE ((`test_suite_id` = 'g10_certification') AND (`validator_name` = 'default') AND (`suite_options` = (`us_core_version` = 'us_core_4'))) LIMIT 1 13:01:46 worker.1 | 2024-02-20T18:01:46.594Z pid=51795 tid=9gb class=Inferno::Jobs::InvokeValidatorSession jid=55fc3b4eebfae2b253df5e6d elapsed=0.009 INFO: fail 13:01:46 worker.1 | 2024-02-20T18:01:46.594Z pid=51795 tid=9gb WARN: {"context":"Job raised exception","job":{"retry":true,"queue":"default","args":["g10_certification","default",1,null],"class":"Inferno::Jobs::InvokeValidatorSession","jid":"55fc3b4eebfae2b253df5e6d","created_at":1708451870.651393,"enqueued_at":1708452106.580876,"error_message":"SQLite3::SQLException: no such column: us_core_version","error_class":"Sequel::DatabaseError","failed_at":1708451870.684702,"retry_count":3,"retried_at":1708451988.8564491}} 13:01:46 worker.1 | 2024-02-20T18:01:46.594Z pid=51795 tid=9gb WARN: Sequel::DatabaseError: SQLite3::SQLException: no such column: us_core_version

Based on the query it looks like it's converting the dict to an invalid query structure.

SELECT * FROM `validator_sessions` WHERE ((`test_suite_id` = 'g10_certification') AND (`validator_name` = 'default') AND (`suite_options` = (`us_core_version` = 'us_core_4')))

Specifically the (`suite_options` = (`us_core_version` = 'us_core_4')) part.

We may have to turn the options dict into a string before sending it to the database

Yes, it's also important to verify that it is being properly serialized/deserialized when being passed to the job.

Co-authored-by: Dylan Hall <[email protected]>

dehall · 2024-02-23T19:38:26Z

It doesn't look like this handles multiple validators with the same name but different suite option requirements.

@Jammjammjamm this was an old comment but I think I just now grasped it in a working session with @rpassas . Is it possible for two validators to have the same name but different requirements within a single test suite? Since requirements is a field on the Validator instance, it seems to me like the minimum "unique key" for a validator is just [test suite id, validator name].

Jammjammjamm · 2024-02-23T19:52:50Z

@Jammjammjamm this was an old comment but I think I just now grasped it in a working session with @rpassas . Is it possible for two validators to have the same name but different requirements within a single test suite? Since requirements is a field on the Validator instance, it seems to me like the minimum "unique key" for a validator is just [test suite id, validator name].

Yes, we rely on this currently in g10.

https://github.com/onc-healthit/onc-certification-g10-test-kit/blob/main/lib/onc_certification_g10_test_kit.rb#L81

dehall · 2024-02-26T13:41:24Z

Of course I forgot that [test suite id, validator name] points to an array of validators, not a single validator. That's why InvokeValidatorSession job takes the index of that array as a parameter. But then to do lookups later each Validator instance doesn't know its own index in that array, but within a given test suite the combination [name, requirements] will be unique.

Rob's latest changes look like they should handle that properly now (though I haven't run the code to test yet)

Jammjammjamm

When I try this fresh with Dylan's g10 PR I get:

11:11:09 worker.1  | 2024-02-26T16:11:09.037Z pid=55352 tid=1j0 class=Inferno::Jobs::InvokeValidatorSession jid=717fe026905ef24320b53083 INFO: start
11:11:09 worker.1  | 2024-02-26T16:11:09.039Z pid=55352 tid=1ko class=Inferno::Jobs::InvokeValidatorSession jid=8bc5f2b5aaddb01b1f30c6f9 INFO: start
11:11:09 worker.1  | 2024-02-26T16:11:09.037Z pid=55352 tid=1hw class=Inferno::Jobs::InvokeValidatorSession jid=a4d185bd97e9de4115bfbda8 INFO: start
11:11:09 worker.1  | 2024-02-26T16:11:09.037Z pid=55352 tid=1i8 class=Inferno::Jobs::InvokeValidatorSession jid=3bc5dd915949cb64ed1d885d INFO: start
11:11:23 worker.1  | E, [2024-02-26T11:11:23.328527 #55352] ERROR -- : InvokeValidatorSession - error from validator:
11:11:23 worker.1  | 2024-02-26T16:11:23.328Z pid=55352 tid=1ko class=Inferno::Jobs::InvokeValidatorSession jid=8bc5f2b5aaddb01b1f30c6f9 elapsed=14.289 INFO: done
11:11:25 worker.1  | E, [2024-02-26T11:11:25.742478 #55352] ERROR -- : InvokeValidatorSession - error from validator:
11:11:25 worker.1  | 2024-02-26T16:11:25.742Z pid=55352 tid=1i8 class=Inferno::Jobs::InvokeValidatorSession jid=3bc5dd915949cb64ed1d885d elapsed=16.705 INFO: done
11:11:26 worker.1  | E, [2024-02-26T11:11:26.083959 #55352] ERROR -- : InvokeValidatorSession - error from validator:
11:11:26 worker.1  | 2024-02-26T16:11:26.084Z pid=55352 tid=1hw class=Inferno::Jobs::InvokeValidatorSession jid=a4d185bd97e9de4115bfbda8 elapsed=17.046 INFO: done

After stopping and restarting inferno, but leaving the services running, I get:

11:12:52 worker.1  | 2024-02-26T16:12:52.882Z pid=55833 tid=13x class=Inferno::Jobs::InvokeValidatorSession jid=8f02737ccc4c6b655706c0df INFO: start
11:12:52 worker.1  | 2024-02-26T16:12:52.884Z pid=55833 tid=135 class=Inferno::Jobs::InvokeValidatorSession jid=ad7e3874268a09ab365468f9 INFO: start
11:12:52 worker.1  | 2024-02-26T16:12:52.884Z pid=55833 tid=14l class=Inferno::Jobs::InvokeValidatorSession jid=15ec110ad367c8662411234a INFO: start
11:12:52 worker.1  | 2024-02-26T16:12:52.884Z pid=55833 tid=15l class=Inferno::Jobs::InvokeValidatorSession jid=107302688ee036e4270f0220 INFO: start
11:12:53 worker.1  | E, [2024-02-26T11:12:53.355174 #55833] ERROR -- : InvokeValidatorSession - error from validator: "/home/ktor/.fhir/packages/1d3a3a7e-aea5-4340-8ab9-b13f93966d1f"
11:12:53 worker.1  | 2024-02-26T16:12:53.355Z pid=55833 tid=135 class=Inferno::Jobs::InvokeValidatorSession jid=ad7e3874268a09ab365468f9 elapsed=0.471 INFO: done
11:12:53 worker.1  | E, [2024-02-26T11:12:53.449162 #55833] ERROR -- : InvokeValidatorSession - error from validator: "/home/ktor/.fhir/packages/1d3a3a7e-aea5-4340-8ab9-b13f93966d1f"
11:12:53 worker.1  | 2024-02-26T16:12:53.449Z pid=55833 tid=15l class=Inferno::Jobs::InvokeValidatorSession jid=107302688ee036e4270f0220 elapsed=0.565 INFO: done
11:12:53 worker.1  | E, [2024-02-26T11:12:53.465213 #55833] ERROR -- : InvokeValidatorSession - error from validator: "/home/ktor/.fhir/packages/1d3a3a7e-aea5-4340-8ab9-b13f93966d1f"
11:12:53 worker.1  | 2024-02-26T16:12:53.465Z pid=55833 tid=14l class=Inferno::Jobs::InvokeValidatorSession jid=15ec110ad367c8662411234a elapsed=0.581 INFO: done
11:12:53 worker.1  | E, [2024-02-26T11:12:53.667094 #55833] ERROR -- : InvokeValidatorSession - error from validator: "/home/ktor/.fhir/packages/1d3a3a7e-aea5-4340-8ab9-b13f93966d1f"
11:12:53 worker.1  | 2024-02-26T16:12:53.667Z pid=55833 tid=13x class=Inferno::Jobs::InvokeValidatorSession jid=8f02737ccc4c6b655706c0df elapsed=0.785 INFO: done

Jammjammjamm · 2024-02-26T16:13:45Z

lib/inferno/db/migrations/010_add_validator_sessions.rb

+  change do
+    create_table :validator_sessions do
+      column :id, String, primary_key: true, null: false, size: 36
+      column :validator_session_id,  String, null: false, size: 255 # , unique: true


Don't leave in commented out code.

dehall · 2024-02-26T16:27:10Z

Hmm I have seen that error of just a filename once or twice but never multiple times in a row. I'll look into that.

The "blank" error is tough to debug (hence FI-2476 for better error handling) but in my experience that happens when an Error is thrown in the core validator, which can either be something really broken like OutOfMemory or more likely the profile that is trying to be validated can't be found. But that shouldn't happen with our setup.

8fb8107

Jammjammjamm · 2024-02-26T18:53:06Z

spec/inferno/repositories/validator_sessions_spec.rb

+      repo.save(session1_params)
+    end
+
+    it 'single record' do


The descriptions on these it calls don't make sense. They should read like the ones above. I think this is good to go otherwise.

rpassas force-pushed the fi-2311-store-session-ids branch 2 times, most recently from 3387adb to 56b63d3 Compare January 9, 2024 17:51

rpassas requested review from dehall and Jammjammjamm and removed request for dehall January 19, 2024 20:31

rpassas force-pushed the fi-2311-store-session-ids branch from e76b3dd to b9da3e2 Compare January 19, 2024 20:32

rpassas marked this pull request as ready for review January 19, 2024 20:32

Jammjammjamm reviewed Jan 24, 2024

View reviewed changes

dehall reviewed Jan 26, 2024

View reviewed changes

rpassas force-pushed the fi-2311-store-session-ids branch from 3ba8195 to 173d259 Compare February 8, 2024 14:47

rpassas requested review from dehall and Jammjammjamm February 13, 2024 02:01

rpassas force-pushed the fi-2311-store-session-ids branch from 15fb46b to 802c366 Compare February 13, 2024 02:03

Jammjammjamm reviewed Feb 13, 2024

View reviewed changes

rpassas force-pushed the fi-2311-store-session-ids branch from 27e3a20 to b8feee3 Compare February 16, 2024 20:07

id saved from response + rename spec file

80e8717

rpassas force-pushed the fi-2311-store-session-ids branch from b8feee3 to 80e8717 Compare February 16, 2024 20:09

Merge branch 'main' into fi-2311-store-session-ids

3c69867

rpassas requested a review from Jammjammjamm February 16, 2024 20:10

rpassas and others added 3 commits February 20, 2024 10:18

Merge branch 'main' into fi-2311-store-session-ids

c7e3977

spec test fix

cc59043

linting

e46a467

rpassas force-pushed the fi-2311-store-session-ids branch from b6bba58 to e46a467 Compare February 20, 2024 17:22

dehall reviewed Feb 20, 2024

View reviewed changes

lib/inferno/dsl/fhir_resource_validation.rb Outdated Show resolved Hide resolved

Jammjammjamm reviewed Feb 20, 2024

View reviewed changes

Update lib/inferno/dsl/fhir_resource_validation.rb

4eaf38d

Co-authored-by: Dylan Hall <[email protected]>

rpassas and others added 2 commits February 25, 2024 23:57

Merge branch 'main' into fi-2311-store-session-ids

c192e6c

db + repo insert conflict, sorted options, idx fix

4572ad0

rpassas force-pushed the fi-2311-store-session-ids branch from 1204d22 to 8fb8107 Compare February 26, 2024 15:03

rpassas requested a review from Jammjammjamm February 26, 2024 15:13

Jammjammjamm reviewed Feb 26, 2024

View reviewed changes

validator job fix + more tests

3840893

8fb8107

rpassas force-pushed the fi-2311-store-session-ids branch from a573226 to 3840893 Compare February 26, 2024 17:42

Jammjammjamm reviewed Feb 26, 2024

View reviewed changes

rpassas requested a review from Jammjammjamm February 27, 2024 16:18

rpassas force-pushed the fi-2311-store-session-ids branch 2 times, most recently from ccc049a to 89333d5 Compare February 27, 2024 17:08

unit test clean up

da80a3d

rpassas force-pushed the fi-2311-store-session-ids branch from 89333d5 to da80a3d Compare February 27, 2024 17:08

Merge branch 'main' into fi-2311-store-session-ids

416d0e2

Jammjammjamm approved these changes Feb 27, 2024

View reviewed changes

rpassas merged commit cfabde4 into main Feb 27, 2024
10 checks passed

rpassas deleted the fi-2311-store-session-ids branch February 27, 2024 17:21

rpassas mentioned this pull request Mar 7, 2024

Inferno Core 0.4.30 release #463

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fi 2311 store session ids #427

Fi 2311 store session ids #427

rpassas commented Jan 5, 2024 •

edited

Loading

codecov bot commented Jan 5, 2024 •

edited

Loading

Jammjammjamm left a comment

Jammjammjamm Jan 24, 2024

rpassas Feb 7, 2024

Jammjammjamm Jan 24, 2024

Jammjammjamm Jan 24, 2024

Jammjammjamm Jan 24, 2024

rpassas Feb 7, 2024

Jammjammjamm Jan 24, 2024

Jammjammjamm Jan 24, 2024

dehall Jan 26, 2024

rpassas Feb 7, 2024

Jammjammjamm Jan 24, 2024

dehall Jan 26, 2024

Jammjammjamm left a comment

Jammjammjamm Feb 13, 2024

Jammjammjamm Feb 13, 2024

rpassas Feb 15, 2024

dehall commented Feb 13, 2024

dehall commented Feb 13, 2024

rpassas commented Feb 16, 2024 •

edited

Loading

Jammjammjamm Feb 20, 2024

Jammjammjamm Feb 20, 2024

dehall Feb 20, 2024

Jammjammjamm Feb 20, 2024

dehall commented Feb 23, 2024

Jammjammjamm commented Feb 23, 2024

dehall commented Feb 26, 2024

Jammjammjamm left a comment

Jammjammjamm Feb 26, 2024

dehall commented Feb 26, 2024

Jammjammjamm Feb 26, 2024

rpassas Feb 27, 2024


		primary_key [:id]

		index [:validator_session_id], :name=>:sqlite_autoindex_validator_sessions_2, :unique=>true

Fi 2311 store session ids #427

Fi 2311 store session ids #427

Conversation

rpassas commented Jan 5, 2024 • edited Loading

Summary

codecov bot commented Jan 5, 2024 • edited Loading

Codecov Report

Jammjammjamm left a 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

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

Jammjammjamm left a 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

dehall commented Feb 13, 2024

dehall commented Feb 13, 2024

rpassas commented Feb 16, 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

dehall commented Feb 23, 2024

Jammjammjamm commented Feb 23, 2024

dehall commented Feb 26, 2024

Jammjammjamm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dehall commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpassas commented Jan 5, 2024 •

edited

Loading

codecov bot commented Jan 5, 2024 •

edited

Loading

rpassas commented Feb 16, 2024 •

edited

Loading