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

chore(deps): bump github.com/dgraph-io/ristretto from v0.1.1 to v1.0.0 #830

Closed
wants to merge 1 commit into from

Conversation

lithammer
Copy link

@lithammer lithammer commented Oct 16, 2024

https://github.com/dgraph-io/ristretto/releases/tag/v1.0.0

The background is mostly that we depend on this library, but want to use Ristretto v1.0.0 for its new generics API. But those two are in conflict right now.

Related Issue or Design Document

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security vulnerability,
    I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added the necessary documentation within the code base (if appropriate).

Further comments

@CLAassistant
Copy link

CLAassistant commented Oct 16, 2024

CLA assistant check
All committers have signed the CLA.

@@ -71,7 +71,7 @@ func JKWKSFetcherWithDefaultTTL(ttl time.Duration) func(*DefaultJWKSFetcherStrat
}

// JWKSFetcherWithCache sets the cache to use.
func JWKSFetcherWithCache(cache *ristretto.Cache) func(*DefaultJWKSFetcherStrategy) {
func JWKSFetcherWithCache(cache *ristretto.Cache[string, *jose.JSONWebKeySet]) func(*DefaultJWKSFetcherStrategy) {
Copy link
Author

@lithammer lithammer Oct 17, 2024

Choose a reason for hiding this comment

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

I guess this is technically a breaking change since it will require changes on the user side of things. This is evident by the failing oidc-conformity test. Although that's more related to updating ristretto than the API of this function (I can provide a PR for that as well if necessary).

/go/pkg/mod/github.com/ory/[email protected]/configx/schema_cache.go:16:26: cannot use generic type ristretto.Config[K z.Key, V any] without instantiation
/go/pkg/mod/github.com/ory/[email protected]/configx/schema_path_cache.go:17:30: cannot use generic type ristretto.Config[K z.Key, V any] without instantiation

We could get around that by introducing an interface that implements the necessary methods (Get(), SetWithTTL() and Wait()). A downside is that now we cement what the cache can do without making a breaking change (e.g. introducing something like Del() would then be breaking). Something like:

type Cacher interface {
	Get(key string) (any, bool)
	SetWithTTL(key string, value any, cost int64, ttl time.Duration) bool
	Wait()
}

So I guess it's tradeoff between making a breaking change now, or potentially making a breaking change in the future.

Copy link
Author

@lithammer lithammer Oct 17, 2024

Choose a reason for hiding this comment

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

On the other hand, the usage of this function seems extremely limited. Not a single use in any public repository on GitHub. At least not with the search query I used:

https://github.com/search?q=JWKSFetcherWithCache+NOT+is%3Afork&type=code

Copy link
Member

Choose a reason for hiding this comment

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

I agree that the "breakingness" of this is limited, fine by me.

@lithammer lithammer changed the title chore(deps): bumpe github.com/dgraph-io/ristretto from v0.1.1 to v1.0.0 chore(deps): bump github.com/dgraph-io/ristretto from v0.1.1 to v1.0.0 Oct 17, 2024
@lithammer lithammer marked this pull request as ready for review October 17, 2024 06:36
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

I think you will need to wait for ory/x#817 to be merged so you can create a PR in Hydra and get this one to pass.

As a workaround, you can probably bump ristretto in your own project by using this replace hack in your go.mod:

replace github.com/dgraph-io/ristretto1 => github.com/dgraph-io/ristretto v1.0.0

require github.com/dgraph-io/ristretto v0.1.1

and then use it in your own codebase with the v1 alias.

@@ -71,7 +71,7 @@ func JKWKSFetcherWithDefaultTTL(ttl time.Duration) func(*DefaultJWKSFetcherStrat
}

// JWKSFetcherWithCache sets the cache to use.
func JWKSFetcherWithCache(cache *ristretto.Cache) func(*DefaultJWKSFetcherStrategy) {
func JWKSFetcherWithCache(cache *ristretto.Cache[string, *jose.JSONWebKeySet]) func(*DefaultJWKSFetcherStrategy) {
Copy link
Member

Choose a reason for hiding this comment

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

I agree that the "breakingness" of this is limited, fine by me.

@lithammer
Copy link
Author

lithammer commented Oct 21, 2024

I think you will need to wait for ory/x#817 to be merged [...]

Merged!

[...] so you can create a PR in Hydra and get this one to pass.

Hmm you mean to update ory/x in Hydra to v0.0.663 which includes ory/x#817? Isn't a new version of fosite needed first that's compatible with [email protected]? Seems like a chick and egg problem or am I missing something?

@lithammer
Copy link
Author

Seems like v1.0.0 was retracted pending v2. Since they they adding an entirely new module, I guess both can live in harmony.

@aeneasr aeneasr closed this Nov 1, 2024
@lithammer lithammer mentioned this pull request Nov 1, 2024
6 tasks
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.

4 participants