Skip to content

Avoid extraneous read for SpMetadataInSession after saving#7644

Merged
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/save-a-redis-get-on-sp-session
Jan 17, 2023
Merged

Avoid extraneous read for SpMetadataInSession after saving#7644
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/save-a-redis-get-on-sp-session

Conversation

@mitchellhenke
Copy link
Contributor

@mitchellhenke mitchellhenke commented Jan 17, 2023

🛠 Summary of changes

StoreSpMetadataInSession#call reads the value written to Redis in ServiceProviderRequestProxy.create!(attributes) to put the information in the session.

I couldn't find anything that suggested this pattern was necessary since the value returned is the value that was written. To avoid the extra read, this change adds an option to pass the value directly rather than read it from Redis.

changelog: Internal, Performance, Avoid extraneous read for SpMetadataInSession after saving
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/save-a-redis-get-on-sp-session branch from 0876865 to e5d1e4c Compare January 17, 2023 16:19
@mitchellhenke mitchellhenke marked this pull request as ready for review January 17, 2023 17:52
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

ServiceProviderRequestProxy.create!(attributes)

StoreSpMetadataInSession.new(session: session, request_id: request_id).call
service_provider_request = ServiceProviderRequestProxy.create!(attributes)
Copy link
Contributor

Choose a reason for hiding this comment

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

separate topic, but while we're here: I think that it's probably time to combine ServiceProviderRequest and ServiceProviderRequestProxy. The proxy is leftover from a migration from an AR model to a redis model, and now that we only have the redis one, we could probably simplify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, we can probably pull the classes together since ServiceProviderRequest is basically a struct at this point.

@mitchellhenke mitchellhenke merged commit 55b1656 into main Jan 17, 2023
@mitchellhenke mitchellhenke deleted the mitchellhenke/save-a-redis-get-on-sp-session branch January 17, 2023 22:28
@mdiarra3 mdiarra3 mentioned this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants