Skip to content

LG-7814: Cache USPS auth token as class member #7147

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.

Sign up for GitHub

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

Merged
sheldon-b merged 9 commits into from
Oct 18, 2022
Merged

LG-7814: Cache USPS auth token as class member #7147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
55ed59e
Use class members instead of instance members
Oct 14, 2022
248de40
Refresh token if it has less than 1 second left
Oct 14, 2022
633e743
Initialize class member variables
Oct 14, 2022
d1eb342
Revert "Refresh token if it has less than 1 second left"
Oct 14, 2022
26b6e9e
changelog: Upcoming Features, In-person proofing, Cache usps api toke…
Oct 18, 2022
c7f3a06
Use mattr_reader so members are readable in spec
Oct 18, 2022
0800018
Update app/services/usps_in_person_proofing/proofer.rb
Oct 18, 2022
c8043f5
Update app/services/usps_in_person_proofing/proofer.rb
Oct 18, 2022
7612e69
Update app/services/usps_in_person_proofing/proofer.rb
Oct 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 6 additions & 10 deletions app/services/usps_in_person_proofing/proofer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
module UspsInPersonProofing
class Proofer
mattr_reader :token, :token_expires_at
Copy link
Contributor

@zachmargolis zachmargolis Oct 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be safe to use Rails.cache for this to share across instances?

Copy link
Contributor Author

@sheldon-b sheldon-b Oct 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so but is there a benefit to that over a class member?

Copy link
Contributor

@zachmargolis zachmargolis Oct 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class members can be hard to test/reason about sometimes mattr_reader vs cattr_reader and then have to be reset explicitly in tests, but Rails.cache has easy methods for clearing it.

Also with like 10+ boxes, with probably 4+ processes each, each one will have to cache separately, so having a shared cache (we use Redis to back our Rails.cache) might save extra requests to the API overall

Copy link
Contributor Author

@sheldon-b sheldon-b Oct 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh cool it's shared across processes and instances? Yeah I'll go ahead and switch it to Rails.cache

Copy link
Contributor

@zachmargolis zachmargolis Oct 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in prod, yes!, locally it might just be in-memory

Copy link
Contributor

@allthesignals allthesignals Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, seeing this conversation after PRing an identical approach. @sheldon-b will this be updated in a future PR? maybe we can pair on this and this one ...

Copy link
Contributor Author

@sheldon-b sheldon-b Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the original ticket is already marked Done and shipped to prod. I've created LG-7918 to move the caching to the rails cache

Happy to pair! I'll bring LG-7918 to refinement today

attr_reader :token, :token_expires_at

# Makes HTTP request to get nearby in-person proofing facilities
Expand All @@ -18,13 +19,8 @@ def request_facilities(location)
zipCode: location.zip_code,
}.to_json

headers = request_headers.merge(
'Authorization' => @token,
'RequestID' => request_id,
)

parse_facilities(
faraday.post(url, body, headers) do |req|
faraday.post(url, body, dynamic_headers) do |req|
req.options.context = { service_name: 'usps_facilities' }
end.body,
)
Expand Down Expand Up @@ -112,12 +108,12 @@ def request_enrollment_code(unique_id)
# @return [String] the token
def retrieve_token!
body = request_token
@token_expires_at = Time.zone.now + body['expires_in']
@token = "#{body['token_type']} #{body['access_token']}"
@@token_expires_at = Time.zone.now + body['expires_in']
@@token = "#{body['token_type']} #{body['access_token']}"
end

def token_valid?
@token.present? && @token_expires_at.present? && @token_expires_at.future?
token.present? && token_expires_at.present? && token_expires_at.future?
end

private
Expand Down Expand Up @@ -152,7 +148,7 @@ def dynamic_headers
retrieve_token! unless token_valid?

{
'Authorization' => @token,
'Authorization' => token,
'RequestID' => request_id,
}
end
Expand Down
4 changes: 2 additions & 2 deletions spec/services/usps_in_person_proofing/proofer_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
stub_request_token
subject.retrieve_token!

expect(subject.token).to be_present
expect(subject.token_expires_at).to be_present
expect(subject.class.token).to be_present
expect(subject.class.token_expires_at).to be_present
end

it 'calls the endpoint with the expected params' do
Expand Down