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

Option to store scopes from response header on client #382

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jmanian
Copy link
Collaborator

@jmanian jmanian commented Jul 2, 2021

Resolves #378

This adds an optional response middleware to the Web client. When used the middleware pulls the OAuth scopes from the response header x-oauth-scopes and puts them on the instance of the client at client.oauth_scopes.

I wasn't sure the best way to gain access to the client itself from the middleware, so I followed the pattern used by the Mashify middleware to pass in an option. Maybe there's a better way?

I'm also not sure about the naming of the middleware class (StoreScopes) or the config option (store_scopes).

No tests or documentation yet.

@jmanian
Copy link
Collaborator Author

jmanian commented Jul 2, 2021

I intentionally did not add the option as a request-level parameter, because it would only work on the first request made by each client instance, given that connection is reused after the first request.

@dangerpr-bot
Copy link

1 Warning
⚠️ There're library changes, but not tests. That's OK as long as you're refactoring existing code.

Generated by 🚫 Danger

@jmanian jmanian marked this pull request as ready for review July 2, 2021 18:02
@jmanian jmanian requested a review from dblock July 2, 2021 18:02
@@ -6,7 +6,7 @@ class Client
include Faraday::Request
include Api::Endpoints

attr_accessor(*Config::ATTRIBUTES)
attr_accessor(*Config::ATTRIBUTES, :oauth_scopes)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about making this attr_reader :oauth_scopes so that it wouldn't appear that you can set the scopes here. But then it would need to use instance_variable_set from the middleware to set them. Not sure which is better.

@@ -26,9 +26,9 @@ Metrics/AbcSize:
# Offense count: 2
# Configuration parameters: IgnoredMethods.
Metrics/CyclomaticComplexity:
Max: 9
Max: 10
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This rule and PerceivedComplexity were failing on connection with the newly added middleware.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally I just run rubocop -a ; rubocop --auto-gen-config.

@dblock
Copy link
Collaborator

dblock commented Jul 4, 2021

This is good, but I think we can do better. What do you think of being able to write this:

config.connection do |connection, options|
   connection.use ::Slack::Web::Faraday::Response::StoreScopes, options
end

This way clients can install any middleware.

Another option could be adding .middleware:

config.middleware << ::Slack::Web::Faraday::Response::StoreScopes

Not sure which one I prefer, would take either.

@jmanian
Copy link
Collaborator Author

jmanian commented Jul 5, 2021

@dblock How would your first option work exactly? Would the block be stored in the config as a proc, and then called with the connection instance during the ::Faraday::Connection.new(endpoint, options) do |connection| block? Or is there a way to use middleware before the connection is instantiated?

In both cases how would it handle the fact that I'm passing the instance of the client to use? If I understand how the first option works, then the user would need to know about this and account for it in the config block. In the latter option how would the code know what options to use when it calls use for each config.middleware?

Also with these options would the user be able to set this at the client instance level, or only at the config level? In my case, since I only need to use this in a specific place with auth_test, my plan was to set it on the instance, e.g.:

Slack::Web::Client.new(token: token, store_scopes: true).auth_test

@dblock
Copy link
Collaborator

dblock commented Jul 5, 2021

@dblock How would your first option work exactly? Would the block be stored in the config as a proc, and then called with the connection instance during the ::Faraday::Connection.new(endpoint, options) do |connection| block? Or is there a way to use middleware before the connection is instantiated?

Right, you would yield at the right time the stored proc.

In both cases how would it handle the fact that I'm passing the instance of the client to use? If I understand how the first option works, then the user would need to know about this and account for it in the config block. In the latter option how would the code know what options to use when it calls use for each config.middleware?

I think you could hide this away. I didn't write the code but I would want the supporting config code to always pass in client: self when the proc is evaluated.

Also with these options would the user be able to set this at the client instance level, or only at the config level? In my case, since I only need to use this in a specific place with auth_test, my plan was to set it on the instance, e.g.:

Slack::Web::Client.new(token: token, store_scopes: true).auth_test

It should work in both.

If you are having trouble implementing this LMK and I could give it a shot. I don't know whether it's possible - I am only thinking about what the user would want to write, and leaving the implementation to the professionals ;)

@dblock dblock force-pushed the master branch 4 times, most recently from 1963b52 to 097b3ca Compare April 30, 2023 03:14
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.

Make Web API response headers accessible
3 participants