Skip to content

Skip redundant delete and save of service provider authorization requests#7910

Merged
mitchellhenke merged 2 commits intomainfrom
mitchellhenke/less-writes-redis
Mar 1, 2023
Merged

Skip redundant delete and save of service provider authorization requests#7910
mitchellhenke merged 2 commits intomainfrom
mitchellhenke/less-writes-redis

Conversation

@mitchellhenke
Copy link
Contributor

@mitchellhenke mitchellhenke commented Mar 1, 2023

🛠 Summary of changes

Currently we read, delete, and re-write Service Provider requests, but in my testing so far it seems redundant. The values are based on parsing the request URL, so we should be able to skip the delete and re-write if they are the same since we'd be deleting and re-writing with the same data.

There is more invasive work that we probably could do around being more precise around when we need to save to Redis and using the session as the primary store for SP requests.

…ests

changelog: Internal, Service Provider Authorization Requests, Skip redundant delete and save of authorization requests
end

def call
return if sp_url_stored_in_session == url
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has the same vibes as the change here that was fixed here. This should still allow for the same service provider with differing nonces (or other attributes).

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

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@mitchellhenke mitchellhenke merged commit 4607236 into main Mar 1, 2023
@mitchellhenke mitchellhenke deleted the mitchellhenke/less-writes-redis branch March 1, 2023 18:38
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