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

Pass client_id, audience at Auth0::Api::V2::ClientGrants#client_grants #209

Merged
merged 3 commits into from
Apr 2, 2020

Conversation

hkdnet
Copy link
Contributor

@hkdnet hkdnet commented Mar 30, 2020

Changes

I'd like to fetch a client grant by its client id.
We can't call Auth0::Api::V2::ClientGrants#client_grants with client_id while GET /api/v2/client-grants accepts client_id.

References

Public doc for GET /api/v2/client-grants
https://auth0.com/docs/api/management/v2#!/client_grants/get_client_grants

Testing

Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

  • This change adds unit test coverage
  • This change adds integration test coverage
    • I'm very happy with adding integration tests for this but I can't run master's integration tests on my local.
  • This change has been tested on the latest version of Ruby

Checklist

@hkdnet hkdnet requested a review from a team March 30, 2020 11:59
@hkdnet hkdnet force-pushed the pass-client-id-get-client-grants branch from f363be3 to 007f2bf Compare March 30, 2020 12:28
@hkdnet
Copy link
Contributor Author

hkdnet commented Mar 30, 2020

I met two GitHub check failures.

snyk

GitHub check ci/circleci: snyk is marked as Required but I think this is not required for all pull requests.
CircleCI workflow snyk only runs with context snyk-env, which is not available for me.
So snyk workflow for my PR is skipped as UNAUTHORIZED.
I believe this is a configuration error. Can someone take a look?

codecov/changes

Checked the error and read https://docs.codecov.io/docs/unexpected-coverage-changes but I have no idea why this happens.
Let me investigate more 🙏

# @param page [int] Page number to get, 0-based.
# @param per_page [int] Results per page if also passing a page number.
# @return [json] Returns the client grants.
def client_grants (page: nil, per_page: nil)
def client_grants (client_id: nil, page: nil, per_page: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be less breaking if put at the end. WDYT @davidpatrick ?

Also, I think we should include audience as well. Also optional @hkdnet 👍

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 would be less breaking if put at the end.

An order of keyword arguments does not matter in Ruby.
I prefer putting before page and per_page because giving client_id changes responses more than other parameters. (If you prefer another style, I'm happy to follow it!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added audience as well. f7c3c9d

Copy link
Contributor

Choose a reason for hiding this comment

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

Those calling it with named arguments will not have a problem 👍 however those still using the standard way of passing arguments would run into a breaking change, as the argument they were sending is now pointing to a different variable.

Say someone today has this call:

# This works today
client_grants(1, 2) 

That would be the same as calling with named arguments:

# Without this PR
# def client_grants (page: nil, per_page: nil)
client_grants(page = 1, per_page = 2) 

Now say we put new parameters at the beginning:

# With this PR
# def client_grants (audience = nil, client_id = nil, page: nil, per_page: nil)
client_grants(audience = 'myaudience', client_id = 'yourclient', page = 1, per_page = 2)

That works, but as soon as we remove the name of the params, the user who had working code is now running into issues:

# With this PR
# def client_grants (audience = nil, client_id = nil, page: nil, per_page: nil)
client_grants(1, 2)

Behavior changes as 1 is taken as the audience, and 2 as the client id.

AFAIK Ruby works that way. I could be wrong, it's been years without coding ruby. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ruby has ordinary parameters, optional parameters, and keyword parameters.
Your concern is correct with optional parameters but this method is defined with keyword parameters.

Labels are required to pass values as keyword arguments. All users pass values with labels (or pass no values).

client.client_grants(1, 2) # this does not work. ArugmentError is raised
client.client_grants(page: 1, per_page: 2) # works
client.client_grants() # This also works because both page and per_page are optional.

In this case, adding keyword parameters before the existing keywords does not break anything. Here is a smaller example.

def f(a: nil)
  puts "a = #{a}"
end

f(a: 1) # a = 1

def f(b: nil, a: nil)
  puts "a = #{a}, b = #{b}"
end

f(a: 1, b: 2) # a = 1, b = 2
f(b: 2, a: 1) # a = 1, b = 2

# This still works well :)
f(a: 1) # a = 1, b = 

You can confirm users can use client_grants the same as before with these examples. These specs work well without changing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect. Then this is solved 👍 Thanks for your contribution

spec/lib/auth0/api/v2/client_grants_spec.rb Outdated Show resolved Hide resolved
@lbalmaceda
Copy link
Contributor

Thanks for pointing out the failing snyk check. I've updated that on this PR #210.

@lbalmaceda lbalmaceda added this to the v4.10.0 milestone Apr 1, 2020
@lbalmaceda lbalmaceda added tiny waiting for customer This issue is waiting for a response from the issue or PR author labels Apr 1, 2020
@hkdnet hkdnet force-pushed the pass-client-id-get-client-grants branch from 2375c74 to 8e1ed2e Compare April 2, 2020 11:27
@hkdnet hkdnet changed the title Pass client_id at Auth0::Api::V2::ClientGrants#client_grants Pass client_id, audience at Auth0::Api::V2::ClientGrants#client_grants Apr 2, 2020
@hkdnet hkdnet requested a review from lbalmaceda April 2, 2020 11:51
@lbalmaceda lbalmaceda removed the waiting for customer This issue is waiting for a response from the issue or PR author label Apr 2, 2020
@lbalmaceda lbalmaceda merged commit c0d00a3 into auth0:master Apr 2, 2020
@hkdnet hkdnet deleted the pass-client-id-get-client-grants branch April 3, 2020 01:48
@hkdnet
Copy link
Contributor Author

hkdnet commented Apr 3, 2020

Thank you @lbalmaceda !

@Widcket Widcket mentioned this pull request Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants