Skip to content
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.

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

feat: Cache configuration for a given ClientSession #959

Merged
merged 38 commits into from
Aug 8, 2024

Conversation

NQuinn27
Copy link
Contributor

@NQuinn27 NQuinn27 commented Aug 1, 2024

Description

ACC-3672
DPS-587

This PR introduces a Configuration Cache which will avoid hitting /configuration when the Config for a clientToken has been cached.

Manual Testing

Manual Testing can be done on another branch, to come. (Hybrid Checkout)

Contributor Checklist

  • All status checks have passed prior to code review
  • I have added unit tests to a reasonable level of coverage where suitable
  • I have added UI tests to new user flows, if applicable
  • I have manually tested newly added UX
  • I have open a documentation PR, if applicable

Reviewer Checklist

  • I have verified that a suitable set of automated tests has been added
  • I have verified that the title prefix aligns to the code changes + whether a release is expected after merging the PR
  • I have verified the documentation PR aligns with this PR, if applicable

Before Merging

  • If introducing a breaking change, I have communicated it internally
  • Any related documentation PRs are ready to merge

Other Stuff

  • You can find out more about our automation checks here
  • Find out more about conventional commits here

@NQuinn27 NQuinn27 requested review from a team as code owners August 1, 2024 14:44
Copy link
Contributor

github-actions bot commented Aug 1, 2024

Warnings
⚠️ > Pull Request size seems relatively large. If this Pull Request contains multiple changes, please split each into separate PR will helps faster, easier review.
⚠️ Please assign someone aside from CODEOWNERS (@checkout-pci-reviewers) to review this PR.

Generated by 🚫 Danger Swift against 9663e6d

Copy link
Contributor

@semirp semirp left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

github-actions bot commented Aug 2, 2024

@@ -714,6 +729,8 @@ struct SDKProperties: Codable {
self.sdkSettings = try container.decodeIfPresent([String: AnyCodable].self, forKey: .sdkSettings)
self.sdkType = try container.decodeIfPresent(String.self, forKey: .sdkType)
self.sdkVersion = try container.decodeIfPresent(String.self, forKey: .sdkVersion)
self.context = try container.decodeIfPresent([String: AnyCodable].self, forKey: .context)
Copy link
Contributor

@jnewc jnewc Aug 5, 2024

Choose a reason for hiding this comment

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

nit: We have a (spurious ..) extension to decode [String: Any] - could use that to keep this symmetric.

}

func setData(_ data: ConfigurationCachedData, forKey key: String) {
// Cache includes at most one cached configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a potential case for having a SingleItemCache wrapper around NSCache instead? Then the key could be passed in the init and removed from all the upstream methods here

My concern would be someone seeing this and thinking that it supports multiple configurations because of the provision of a key

}

promise.ensure {
PrimerAPIConfigurationModule.queue.async {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect there is a small chance here that you could end up adding a callback here that is never returned because it was added after ensure was called but prior to the queued item completing.

I don't think this will play out in practice but worth thinking of whether there's a way to further streamline the promise-nesting to avoid this in the future. (Which is admittedly easier said than done!)

NQuinn27 and others added 7 commits August 7, 2024 12:55
…t flag is true (#957)

* Validate PENDING in resume if flag is true

* Remove unused code

* Added unit tests for CreateResumeService

* Make new flag a bool

* Added strings for fintechture

* Remove redundant enum

* Split out createresume into seperate protocol
[create-pull-request] automated change

Co-authored-by: NQuinn27 <3179752+NQuinn27@users.noreply.github.com>
@NQuinn27
Copy link
Contributor Author

NQuinn27 commented Aug 7, 2024

@jnewc - recent changes are to align with this ADR

Namely:

  • Adding clientSessionCachingEnabled param in PrimerSettings

Copy link

sonarqubecloud bot commented Aug 7, 2024

@NQuinn27 NQuinn27 merged commit ee4b223 into master Aug 8, 2024
13 checks passed
@NQuinn27 NQuinn27 deleted the nq/cache_configuration branch August 8, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants