Simplify proofing components by zachmargolis · Pull Request #5737 · 18F/identity-idp

zachmargolis · 2021-12-20T20:34:46Z

So as part of #5721, I looked at some of our proofing component code, and I think we can simplify it and use ActiveRecord built-in methods for some similar validation (such as checking if columns exist or not), and some slight query optimizations

This will conflict with #5721, so let's not merge this until that is wrapped up, but I did want to start a convo about it

zachmargolis · 2021-12-20T20:35:59Z

app/services/idv/steps/doc_auth_base_step.rb

        )
-        Db::ProofingComponent::Add.call(user_id, :document_check, session_doc_auth_vendor)
-        Db::ProofingComponent::Add.call(user_id, :document_type, 'state_id')
-        return unless liveness_checking_enabled?
-        Db::ProofingComponent::Add.call(user_id, :liveness_check, session_doc_auth_vendor)
+
+        proofing_component = current_user.proofing_component || current_user.build_proofing_component
+        component_attributes = {
+          document_check: session_doc_auth_vendor,
+          document_type: 'state_id',
+        }
+        if liveness_checking_enabled?
+          component_attributes[:liveness_check] = session_doc_auth_vendor
+        end
+        proofing_component.update(component_attributes)
      end


in the old way, each of these 3 separate attributes resulted in a find-or-create + save, so that's like 2-3 queries x 3 attributes, and this is now 1 find + 1 save

zachmargolis · 2021-12-20T20:37:31Z

app/services/db/proofing_component/add.rb

-        return unless user_id
-        return unless TOKEN_ALLOWLIST.index(token.to_sym)
-
-        proofing_cost = ::ProofingComponent.create_or_find_by(user_id: user_id)


So arguably changing this to the user.proofing_component || user.build_proofing_component is more sensitive to race conditions than this existing code, I was not sure how to leverage the associations to do this same behavior

Another option: user.proofing_component || user.create_proofing_component which shortens the time between queries

I'm not entirely sure I follow what practical impact we might expect from building vs. creating, but to your alternative, maybe we don't bother update-ing at all if the entry doesn't exist?

if current_user.proofing_component current_user.proofing_component.update(component_attributes) else current_user.create_proofing_component(component_attributes) end

Or assign with an upsert ?

current_user.proofing_component = ProofingComponent.upsert( user: current_user, **component_attributes, )

The previous implementation was resilient to a race-condition where two conflicting records were created at the same time, and I think would be fine in the update case since we're only typically updating a column or two at a time. The current implementation I don't think accounts for either case.

Upsert would make the most sense imo both for performance and race-condition reasons.

I like that idea!

A few small snags:

from the upsert docs, it skips ActiveRecord callbacks, which means no updated_at and no created_at

[3] pry(main)> ProofingComponent.upsert(user_id: 1000) D, [2021-12-21T09:25:36.713454 #44575] DEBUG -- : ProofingComponent Upsert (16.5ms) INSERT INTO "proofing_components" ("user_id") VALUES (1000) ON CONFLICT ("id") DO UPDATE SET updated_at=(CASE WHEN ("proofing_components"."user_id" IS NOT DISTINCT FROM excluded."user_id") THEN "proofing_components".updated_at ELSE CURRENT_TIMESTAMP END),"user_id"=excluded."user_id" RETURNING "id" ActiveRecord::NotNullViolation: PG::NotNullViolation: ERROR: null value in column "created_at" of relation "proofing_components" violates not-null constraint DETAIL: Failing row contains (6, 1000, null, null, null, null, null, null, null, null, null).

passing created_at to upsert would override an existing created_at which we don't want...

it doesn't return an AR model, so the current_user.proofing_component = part wouldn't work but we don't really need that

I'm going to keep noodling on this and see if I can find a way to do what we want

updated in b238e99

I had a few ideas:

switch the assocation to has_many :proofing_components (even though there's only one) because then we can do user.proofing_components.first_or_create

unfortunately that is not the same as create_or_find_by which still require the user to be passed in, so I just went with that

…ofing-components

aduth

Nice simplification 👍

aduth · 2021-12-21T13:06:41Z

app/services/idv/steps/doc_auth_base_step.rb

      end

      def save_proofing_components
+        return unless current_user


Do we know when this would be the case? I was wondering about hybrid flow, where in other places we use the EffectiveUser mixin to manage the user associated with the anonymous session. But are we ever actually calling save_proofing_components in that hybrid session (i.e. do we need to bother with that mixin?)?

I am hoping it was just an artifact of how sloppily the individual feature tests for steps were done, it doesn't make sense why we need this check

Curious if it's still needed with the recent round of revisions. Thinking the ProofingComponent.create_or_find_by would be more tolerant to the absence of current_user.

There's a null constraint on the column so this blows up:

[1] pry(main)> ProofingComponent.create_or_find_by(user: nil) TRANSACTION (0.1ms) BEGIN ProofingComponent Create (2.4ms) INSERT INTO "proofing_components" ("created_at", "updated_at") VALUES ($1, $2) RETURNING "id" [["created_at", "2021-12-21 22:05:23.908635"], ["updated_at", "2021-12-21 22:05:23.908635"]] TRANSACTION (0.2ms) ROLLBACK ActiveRecord::NotNullViolation: PG::NotNullViolation: ERROR: null value in column "user_id" of relation "proofing_components" violates not-null constraint DETAIL: Failing row contains (9, null, null, null, null, null, null, null, 2021-12-21 22:05:23.908635, 2021-12-21 22:05:23.908635, null).

aduth · 2021-12-21T13:12:47Z

app/services/db/proofing_component/add.rb

-        return unless user_id
-        return unless TOKEN_ALLOWLIST.index(token.to_sym)
-
-        proofing_cost = ::ProofingComponent.create_or_find_by(user_id: user_id)


I'm not entirely sure I follow what practical impact we might expect from building vs. creating, but to your alternative, maybe we don't bother update-ing at all if the entry doesn't exist?

if current_user.proofing_component current_user.proofing_component.update(component_attributes) else current_user.create_proofing_component(component_attributes) end

Or assign with an upsert ?

current_user.proofing_component = ProofingComponent.upsert( user: current_user, **component_attributes, )

zachmargolis · 2021-12-21T19:14:27Z

@mitchellhenke @aduth updated from comments! PTAL when you can

aduth

Looks great 👍

aduth · 2021-12-21T21:44:37Z

app/services/idv/steps/doc_auth_base_step.rb

      end

      def save_proofing_components
+        return unless current_user


Curious if it's still needed with the recent round of revisions. Thinking the ProofingComponent.create_or_find_by would be more tolerant to the absence of current_user.

* Inline Db::ProofingComponent::DeleteAll, use ActiveRecord methods * Inline Db::ProofingComponent::Add, use AR methods

zachmargolis added 2 commits December 20, 2021 12:22

Inline Db::ProofingComponent::DeleteAll, use ActiveRecord methods

01f5fa7

Inline Db::ProofingComponent::Add, use AR methods

8333e0b

zachmargolis added the status - do not merge label Dec 20, 2021

zachmargolis requested review from aduth, mitchellhenke and stevegsa December 20, 2021 20:34

zachmargolis commented Dec 20, 2021

View reviewed changes

zachmargolis mentioned this pull request Dec 20, 2021

Record IAL2 proofing components (LG-5324) #5721

Merged

fix reference to current user

1e69ec2

zachmargolis removed the status - do not merge label Dec 20, 2021

zachmargolis added 3 commits December 20, 2021 13:28

Merge remote-tracking branch 'origin/main' into margolis-simplify-pro…

71542eb

…ofing-components

lints

828947a

sometimes current_user is nil :[ :[

04f1e6b

aduth approved these changes Dec 21, 2021

View reviewed changes

Bring back create_or_find_by for better race condition checking

b238e99

zachmargolis requested a review from aduth December 21, 2021 18:28

zachmargolis added 2 commits December 21, 2021 10:35

chain .update, we don't need the variable

422efeb

fewer lines where we can

1c73498

aduth approved these changes Dec 21, 2021

View reviewed changes

zachmargolis merged commit 1e6a899 into main Dec 21, 2021

zachmargolis deleted the margolis-simplify-proofing-components branch December 21, 2021 22:06

jmhooper pushed a commit that referenced this pull request Dec 28, 2021

Simplify proofing components (#5737)

6b97243

* Inline Db::ProofingComponent::DeleteAll, use ActiveRecord methods * Inline Db::ProofingComponent::Add, use AR methods

Conversation

zachmargolis commented Dec 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchellhenke Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis commented Dec 21, 2021

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aduth Dec 21, 2021 •

edited

Loading

mitchellhenke Dec 21, 2021 •

edited

Loading

aduth Dec 21, 2021 •

edited

Loading