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

Enable the 'no_body' option when authorizing against 3scale backend #483

Merged
merged 4 commits into from
Nov 15, 2017

Conversation

davidor
Copy link
Contributor

@davidor davidor commented Nov 15, 2017

When no_body is enabled via the 3scale-options-header, backend does not return a response body. The body normally has lots of detail: metrics, limits, plans, etc. In the normal auth flow, all this information is not required. We only need to check the status code of the response. By enabling the no_body option, we will save some work to the 3scale backend and also reduce network traffic.

no_body cannot be enabled in the oauth flow, as there we need to parse the response body.

@davidor davidor changed the title No body option backend Enable the 'no_body' option when authorizing against 3scale backend Nov 15, 2017
@davidor davidor requested a review from mikz November 15, 2017 15:19
-- body has many information like metrics, limits, etc. This information is
-- parsed only when using oauth. By enabling this option will save some work
-- to the 3scale backend and reduce network traffic.
local function authorize_options(using_oauth)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't be nicer to pass self instead of using_oauth ?

Because if the condition changes we would have to change only one place - this method.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that it would also require changing the authrep_path method to expect self instead of just boolean.

Tough choice. Not so sure now. I guess it is fine as it is then!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this option because it makes explicit that the only thing the method needs to know about is if we are running the oauth flow or not. Although as you said, we might need to change the params this method receives and the callers in the future.

Copy link
Contributor

@mikz mikz left a comment

Choose a reason for hiding this comment

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

Execellent 👍 🔥 🔥 🔥

When no_body is enabled via the 3scale-options-header, backend does not
return a response body. The body normally has lots of detail: metrics,
limits, plans, etc. In the normal auth flow, all this information is not
required. We only need to check the status code of the response. By
enabling the no_body option, we will save some work to the 3scale
backend and also reduce network traffic.

no_body cannot be enabled in the oauth flow, as in that case, we need to
parse the response body.
@davidor
Copy link
Contributor Author

davidor commented Nov 15, 2017

Added a changelog entry and rebased on top of master so it doesn't give conflicts.

Copy link
Contributor

@mikz mikz left a comment

Choose a reason for hiding this comment

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

Great 👍 🥇 👍

@davidor davidor merged commit a6226ec into master Nov 15, 2017
@davidor davidor deleted the no_body_option_backend branch November 15, 2017 16:06
local headers = { ['3scale-options'] = 'rejection_reason_header=1' }

if not using_oauth then
headers['3scale-options'] = headers['3scale-options'] .. '&no_body=1'
Copy link
Contributor

Choose a reason for hiding this comment

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

@davidor why don't you first construct the string and then create the headers table... just look at the amount of accesses you are performing.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is all going to be JITed ! :trollface:

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.

3 participants