fixes for https://github.com/DockYard/openid_connect/issues/24 #30
Conversation
lib/openid_connect.ex
Outdated
| {:ok, doc} = GenServer.call(name, {:discovery_document, provider}) | ||
| doc | ||
| end | ||
|
|
||
| defp jwk(provider, name) do | ||
| GenServer.call(name, {:jwk, provider}) | ||
| {:ok, jwk} = GenServer.call(name, {:jwk, provider}) | ||
| jwk |
There was a problem hiding this comment.
Doesn't this make this code more brittle? If the GenServer.call call returns an error, this code will now raise an error instead of return the {:error, reason} tuple as it did before.
Don't the callers of these functions need to be updated to handle possible errors instead?
There was a problem hiding this comment.
@kenny-evitt, you're right, at least partially. This particular part of the change can be done better. I will push an additional commit to make some improvements.
There was a problem hiding this comment.
@kenny-evitt perhaps I should have been clear that I had addressed this comment on my branch.
This is now 3 years old and I'd like to get it merged. It'll need a rebase. My apologies for letting it sit so long, but I think this is important to merge as this library is basically unusable in our environment without this error-handling fix. At the office, I've been running this branch in prod for 3 years now.
There was a problem hiding this comment.
@nbryant42 Apparently this PR fixes/'closes' the issue. That PR was just merged two weeks ago.
I haven't tested it and it doesn't seem like they published a new version of the Hex package either yet.
There was a problem hiding this comment.
@kenny-evitt their PR must fix the issue since it eliminates the Worker entirely. Recall the issue was that Worker failed to start, preventing app startup. Can't fail if it no longer exists!
I use the library pretty narrowly, with a single 3rdparty provider (small company you've never heard of) and we'll be regression testing the new master branch against that soon. If all goes well I'll just close out this PR.
Update to use {:error, name, reason} convention. Hoist
proper error checks into various calling functions.
Changes public API spec such that authorization_uri can now
return an error case, but this shouldn't be any worse than
the previous behavior of process EXITs resulting from failed
GenServer calls.
nbryant42
force-pushed
the
error-handling
branch
from
e58e0f8
to
a6fac89
Compare
|
Closing. Fixed on the master branch via another PR. Looks like the fix will be released in 1.0.0. |
No description provided.