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

Make Web API response headers accessible #378

Open
jmanian opened this issue Jun 16, 2021 · 9 comments · May be fixed by #382
Open

Make Web API response headers accessible #378

jmanian opened this issue Jun 16, 2021 · 9 comments · May be fixed by #382

Comments

@jmanian
Copy link
Collaborator

jmanian commented Jun 16, 2021

The response headers in Slack's Web API contain information that is sometimes useful. For example x-oauth-scopes is a list of scopes that the token has. This is the only way (that I know of) to check the scopes on an existing token through the API. My use case is I'd like to be able to use auth.test to check the scopes on a token via x-oauth-scopes.

The current implementation doesn't give a way to access the response headers on a successful request, since it returns only the body:

The headers are accessible on the error object raised on failed requests since the entire response object is on the error:

slack_error.response.headers

I'm trying to think how we could make the headers accessible on successful responses. Some bad ideas:

  1. Shove them into the body object under the key response_headers. This object is then wrapped in Slack::Messages::Message and returned. I don't like this because it pollutes the body.
  2. An option that can be passed to any request (possibly also set on a client) that causes it to return something different:
    1. The entire raw response object, giving access to the body as well as the headers (and much else).
    2. [body, response_headers]
    3. etc.
@jmanian
Copy link
Collaborator Author

jmanian commented Jun 16, 2021

@dblock any ideas?

@dblock
Copy link
Collaborator

dblock commented Jun 17, 2021

  1. Return response in , which is private, and change all methods above to do, for example, request(:get, path, options).body. Make request public and invoke it to get headers.
  2. Return response in , and change all template code to call .body, so def get(path, options = {}) now returns a response object and not a .body. It's an API change, but YOLO.
  3. Extract invoke from request, so the latter becomes invoke(method, path, options).body. Make both public. Maybe we should then rename request to body, as well, and alias it to request with deprecation.

@jmanian
Copy link
Collaborator Author

jmanian commented Jun 17, 2021

I see what you mean on #379 about option (2)(i) being bad from an API standpoint. Options 3-5 are less convenient, but since this is likely a rare use case that's OK with me. In my case I need to do this in only 1 spot at the moment.

I guess my leaning is option 5.

@dblock
Copy link
Collaborator

dblock commented Jun 18, 2021

Want to try to implement 5?

@dblock
Copy link
Collaborator

dblock commented Jun 18, 2021

Thinking about this more, maybe we should (also) add client.oauth_scopes that invokes the auth test api and returns the result from the header. Another alternative would be to supply an optional response middleware that always inspects the response for oauth_scopes and stores/updates that.

@jmanian
Copy link
Collaborator Author

jmanian commented Jun 18, 2021

The middleware is an interesting idea. Are you thinking it would store them on the instance of the client?

@dblock
Copy link
Collaborator

dblock commented Jun 21, 2021

The middleware is an interesting idea. Are you thinking it would store them on the instance of the client?

Yep.

@jmanian
Copy link
Collaborator Author

jmanian commented Jul 2, 2021

I'm taking a crack at the middleware approach now. Do you think it should be optional via a new web config setting/attribute?

@jmanian jmanian linked a pull request Jul 2, 2021 that will close this issue
@dblock
Copy link
Collaborator

dblock commented Jul 4, 2021

I'm taking a crack at the middleware approach now. Do you think it should be optional via a new web config setting/attribute?

Only if it creates significant overhead. Without looking at the code, I would try and enable that by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants