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

Complete rewrite of the library #6

Merged
merged 6 commits into from
Apr 3, 2023
Merged

Complete rewrite of the library #6

merged 6 commits into from
Apr 3, 2023

Conversation

AndrewDryga
Copy link

@AndrewDryga AndrewDryga commented Jan 11, 2023

The library was storing a state in a GenServer which is causing various issues:

  1. When the configuration is changed we needed to restart the process having a small OIDC downtime during the process;

  2. There was no way to run async tests because GenServer was querying discovery document URL's in the background, which means we weren't able to use Bypass in integration tests

  3. Storing a lot of configuration on the library side would cause a lot of issues once we have multi-tenancy and a ton of different configurations, the GenServer will be a bottleneck.

  4. HTTPPoison is not maintained any more (and archived) so we should not rely on it. (edit: HTTPotion is the one not maintained, while HTTPPoison is still supported.) Moreover, even though the library accepted :http_client opt it did not really allow to override it because it matched on the HTTPoison.Response struct everywhere.

  5. The library itself was tested poorly mocking pretty much everything including its own GenServer implementation, so this did not feel like a reliable piece to be at the core of the product.

The library was storing a state in a GenServer which is causing various issues:
1. When configuration is changed we needed to restart the process having a small OIDC downtime during the process;
2. There were no way to run async tests because GenServer was querying discovery document URL's on the background, which means we wasn't able to use Bypass in integration tests
3. Storing a lot of configuration on the library side would cause a lot of issues once we have multi-tenancy and a ton of different configuration, the GenServer will be a bottleneck.
@coveralls
Copy link

coveralls commented Jan 11, 2023

Pull Request Test Coverage Report for Build 12ebf94f530c25a06636d590c11cac25eb520b6a-PR-6

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 129 of 129 (100.0%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+20.3%) to 99.306%

Files with Coverage Reduction New Missed Lines %
lib/openid_connect.ex 1 98.08%
Totals Coverage Status
Change from base Build c1eb8856e59ff49081e60fefce2dbb33a41383bf: 20.3%
Covered Lines: 143
Relevant Lines: 144

💛 - Coveralls

Copy link
Member

@jamilbk jamilbk left a comment

Choose a reason for hiding this comment

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

This looks great! Sorry for the delayed review. One minor comment about versioning, and a question:

This library seems to still use a GenServer for the documents cache -- apologies for my confusion, but was that one of the things we were trying to steer clear of? I guess we can't avoid having some kind of state management.

@@ -1,13 +1,13 @@
defmodule OpenIDConnect.Mixfile do
use Mix.Project

@version "0.2.2"
@version "1.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Nice ;-). Hm, thoughts on maybe reserving the 1.0 for when we pass OpenID's Relying Party conformance tests?

Don't feel too strongly about, just a thought.

Copy link
Author

Choose a reason for hiding this comment

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

We can bump a version to 2.0 later if tests will uncover something to introduce breaking changes, otherwise, it could remain 1.0 or 1.1. The main reason why I changed major version is because API is completely changed and somebody is going to use our fork then we don't want them to assume it's the same.

@AndrewDryga
Copy link
Author

@jamilbk the GenServer is just a maintainable and simple to comprehend way for now to maintain HTTP cache, later we can do something more complex if it's proven to be a bottleneck (eg. by using ParitionedSupervisor or ETS).

@AndrewDryga AndrewDryga merged commit c285419 into master Apr 3, 2023
@AndrewDryga AndrewDryga deleted the andrew/rewrite branch April 3, 2023 18:54
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.

None yet

3 participants