Add patch resource server #157
Conversation
Merge from upstream
|
Thanks @philomory! I'll review this ASAP. |
lib/auth0/api/v2/resource_servers.rb
Outdated
| # @param id [string] The id or audience of the resource server to update. | ||
| # @param options [hash] The Hash options used to define the resource servers's properties. | ||
| def patch_resource_server(id, options) | ||
| raise Auth0::InvalidParameter, 'Must specify a resource server id' if id.to_s.empty? |
There was a problem hiding this comment.
Let's say:
Must specify a resource server ID or audience
... since it can accept both.
There was a problem hiding this comment.
Made the requested change.
That said, on this topic, should we URI-escape this parameter, or except the caller to do it? Obviously irrelevant if the caller is passing an id, but if passing an audience, and the audience is of the common form https://something.com/ then that needs to be URI-escaped before being added to the PATH string.
In my PR I left it up the to caller to URI escape audiences, since the delete_resource_server method (the only existing method that puts an id-or-audience in the URI's path segment) doesn't handle URI escaping automatically, but an argument could be made to add it. Do you want me to go ahead and add that here, and to the delete method as well? Or, since that's theoretically a backwards-incompatible change (anyone previously using the delete method with an audience would be escaping before passing it in, and if we start escaping as well then it ends up double-escaped), should it wait for a separate PR that could be part of 5.0.0?
There was a problem hiding this comment.
That's a great question.
I would prefer it be done in the SDK but none of the other methods I looked at for any of the endpoints do so I think we should be consistent here. We're putting together a roadmap for a major release on this library and I'll bring that up for discussion.
There was a problem hiding this comment.
It's worth noting that most endpoints don't accept anything in the the path segment of the URL that could ever possibly need URI escaping; the resource servers endpoint is the only one I can think of off the top of my head.
Just to play devils advocate, you could argue that adding this functionality isn't a compatibility-breaking change because the library did not previously document any support for accepting an audience rather than an id in these methods, so anyone who was passing one in was relying on undocumented behavior (of the library; the actual REST API does document this of course). That said, I'm not sure I buy that argument myself and am content to wait for 5.0.0 to introduce the automatic escaping.
There was a problem hiding this comment.
It's definitely the minority of cases, true. But also a consideration for URL parameters.
Every time I talk myself into changing something that shouldn't be a breaking change but is, there is inevitably someone affected! I think it makes more sense to address it gem-wide in a major. I appreciate the opportunity to talk it through, though.
| expect(@instance).to receive(:patch).with('/api/v2/resource-servers/1', fields: 'fields') | ||
| expect { @instance.patch_resource_server('1', fields: 'fields') }.not_to raise_error | ||
| end | ||
| it { expect { @instance.patch_resource_server('', nil) }.to raise_error 'Must specify a resource server id' } |
There was a problem hiding this comment.
Tests are failing on this:
https://travis-ci.org/auth0/ruby-auth0/jobs/488774537#L565
I would just check for Auth0::InvalidParameter like the integration tests.
There was a problem hiding this comment.
Made the requested change.
This actually reminds me of something I've been wondering about: why are Auth0::MissingClientId and Auth0::MissingUserId the only dedicated exceptions for a specific property? There's no Auth0::MissingResourceServerId or Auth0::MissingConnectionId, all other resources just use Auth0::InvalidParameter for a missing id, same as they do for other missing parameters or for present-but-invalid parameters.
Changes
Adds a
patch_resource_servermethod that takes a resource server id and a hash of options. Includes spec tests that pass and tentative integration tests awaiting VCR cassettes.References
Discussion in #156.
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.
Checklist