Add FnCache.GetIfExists method

[smallinsky]

What

Add GetIfExists method that allows to "Peek" for the cache key without triggering the Cache key creation function.

What is the difference between and FnCacheGet vs GetIfExists

The FnCacheGet always triggers keys creation if key don't exist in cache where FnCacheGetIfExists is designed to not change the state cache and just return existing state.

Why this function is needed. Do we have any buisness case for this peeking cache funtionality ?

Yes, There are severals business cases where we want to check if key exist without creating a value in cache like logic where produce and consumer are independed, like checking rate limits for IP key without applying rate limit to IP key.

See #61788 for the Usage

Why not a global function like: utils.FnCacheGetIfExists[K comparable, T any](cache *FnCache, key K) (T, bool) { ?

The global funtion usage is every obscure because go compiler can't deduce the generic type during the call so the call will look like FnCacheGetIfExists[string, string](cache, key) vs cache.GetIfExists(key)

Where currently cache have methods like cache.Set cache.Remove so the cache.Get... method is missing.

[rosstimothy]

Can we make this a free function similar to FnCacheGet instead of a receiver so that we can leverage generics and eliminate the need to cast from an any by the caller?

Suggested change

	func (c *FnCache) GetIfExists(key any) (any, bool) {
	func FnCacheGetIfExists[K comparable, T any](cache *FnCache, key K) (T, bool) {

[smallinsky]

See the PR desc: #61776 (comment)

Why not a global function like: utils.FnCacheGetIfExists[K comparable, T any](cache *FnCache, key K) (T, bool) { ?

The main reason is cumbersome usageutils.FnCacheGetIfExists[string, string](cache, key) where you need to define return types because compiler can't deduce this types (I have prototyped this and after seeing the function call signature I decided to revert back to cache.GetIfExists that is much readable in case of readability for me vs FnCacheGetIfExists[string, string](cache, key) callback )

Also cache already have Public functions like cache.Set cache.Remove so the cache.Get... method seems to be missing.

Where even if we have FnCacheGet(cache) function you can still call cache.Set() or cache.Remove and the free function doesn't restrict the cache.Public Function usage.

[rosstimothy]

Hrmm, that doesn't appear to be the case for existing calls to FnCacheGet - why is FnCacheGetIfExists different?

[smallinsky]

Please see the PR description where this is explained:

Why not a global function like: utils.FnCacheGetIfExists[K comparable, T any](cache *FnCache, key K) (T, bool) { ?

... go compiler can't deduce the generic type during the call so the call will look like FnCacheGetIfExists[string, string](cache, key)

So having raw call withut explict types nCacheGetIfExistsstring, string like:

 FnCacheGetIfExists(cache, "test-key")

gives compiler error

in call to FnCacheGetIfExists, cannot infer T (declared at ./fncache.go:185:39)

You can find many articles how GO generic types are deduced. In this case it is not possible.

So you need to call FnCacheGetIfExists[string, string](cache, "test-key") with explicit types provided [string, string] that is very ugly and obscure for the usage and is quite easy to misused the types

[rosstimothy]

Why is this not the case for existing calls to FnCacheGet?

https://github.com/search?q=repo%3Agravitational%2Fteleport%20utils.FnCacheGet&type=code

[smallinsky]

Please see https://go.googlesource.com/proposal/+/refs/heads/master/design/43651-type-parameters.md#function-argument-type-inference

Go complier can't infer function type arguments from function like func FnCacheGetIfExists[K comparable, T any](cache *FnCache, key K) (T, bool) {

where FnCacheGetIfExists and FnCacheGet have totally diffrent signature.

One more time please see PR desc:

Why not a global function like: utils.FnCacheGetIfExists[K comparable, T any](cache *FnCache, key K) (T, bool) { ?

... go compiler can't deduce the generic type during the call ...

Where I would like avoid spending time discussing go compiler limitation

So as I said in #61776 (comment) to make it work you need to specify the [string, string] type explicitly: FnCacheGetIfExists[string, string](cache, "test-key")

[r0mant]

I think the issue here is the following. The existing FnCacheGet method has the following signature:

func FnCacheGet[K comparable, T any](ctx context.Context, cache *FnCache, key K, loadfn func(ctx context.Context) (T, error)) (T, error)

Here, the T type is present as a part of the loadfn callback function among the method's input parameters, so the Go compiler can infer its type from there.

The new one being added has the following signature:

func FnCacheGetIfExists[K comparable, T any](cache *FnCache, key K) (T, bool)

The difference is that here, T is not among input arguments so Go compiler can't infer its type and hence it has to be specified explicitly at the call site.

[rosstimothy]

I understand the motivation for the change, I was seeking an answer as to what was different between FnCacheGet and FnCacheGetIfExists that allows the former to work but not the latter. The answer is that callers define T explicitly in the loadFn provided to FnCacheGet. I think that information would've been helpful to include in the PR description.

In regards to the generic types being specified at the call site explicitly, is that truly worse than the alternative. Consider the following two contrived examples.

valueAny, ok := cache.GetIfExists("foo")
if ok {
	if foo, ok := valueAny.(string); ok {
		// use foo here
	}
}


if foo, ok := FnCacheGetIfExists[string, string](cache, "foo"); ok {
}

[smallinsky]

So you need to call FnCacheGetIfExistsstring, string with explicit types provided [string, string] that is very ugly and obscure for the usage and is quite easy to misused the types

I prefer:

	bucketSet, ok := l.rateLimits.GetIfExists(token)
	if !ok {
		return false
	}
	bucket, ok := bucketSet.(*ratelimit.TokenBucketSet)
	if !ok {
		return false
	}
 ... 

vs:

 bucketSet, ok := FnCacheGetIfExists[string, *ratelimit.TokenBucketSet)](cache, "foo")
if  !ok {
 return false
}

because FnCacheGetIfExists[string, *ratelimit.TokenBucketSet)]( l.rateLimits., "foo") is very confusing for me.

[r0mant]

I've no strong preference towards one way or another being discussed in the thread here but @smallinsky I'm wondering if you considered a potential alternative - can we just use existing FnCacheGet method and provide it a no-op load function in cases where we don't want to load the object on miss? Would it work for your use-case?

[r0mant]

I think the issue here is the following. The existing FnCacheGet method has the following signature:

func FnCacheGet[K comparable, T any](ctx context.Context, cache *FnCache, key K, loadfn func(ctx context.Context) (T, error)) (T, error)

Here, the T type is present as a part of the loadfn callback function among the method's input parameters, so the Go compiler can infer its type from there.

The new one being added has the following signature:

func FnCacheGetIfExists[K comparable, T any](cache *FnCache, key K) (T, bool)

The difference is that here, T is not among input arguments so Go compiler can't infer its type and hence it has to be specified explicitly at the call site.

[smallinsky]

@r0mant

Can we just use existing FnCacheGet method and provide it a no-op load function in cases where we don't want to load the object on miss? Would it work for your use-case?

I think that having explicit behavior via named function is more straightforward that injecting the empty callback to the FnCacheGet and mixing the logic in one flow.

But I don't have strong preference

[r0mant]

I think that having explicit behavior via named function is more straightforward that injecting the empty callback to the FnCacheGet and mixing the logic in one flow.
But I don't have strong preference

lol yeah, me neither. I think in the case where neither side has a strong preference, going with the author's preference is reasonable.