chore: Refactor NewCachedDescriptorProvider

[ruanxin]

remove DescriptorCache as input paremeter

[lindnerby]

Can you explain what is the motivation to reduce the coverage, remove encapsulation and promote modification of the descriptor cache directly?
I don't see a reason for it. You could introduce a specific constructor function instead, so we have one with auto generated cache and one for cache injection for the tests.

[ruanxin]

Can you explain what is the motivation to reduce the coverage, remove encapsulation and promote modification of the descriptor cache directly? I don't see a reason for it. You could introduce a specific constructor function instead, so we have one with auto generated cache and one for cache injection for the tests.

the reason is by previous coverage, it fails the test coverage check (from 68 to 66.7) of provider_test.go, https://github.com/kyma-project/lifecycle-manager/actions/runs/9980148681/job/27580851622#step:6:278, and by this refactoring, it's not changing(adding or remove) any existing behaviour of this CachedDescriptorProvider, also spend time to add more test for this to get a higher coverage also not in this scope of this refactoring task.

With the existing test cases and anonymous function constructor, IMO, it should be configured with current percentage: 66.

I don't get the reason why needs to inject a cache to this CachedDescriptorProvider, you already tested the GetDescriptor as well as Add, what's the purpose of injecting a cache? If you need to verify the content inside the cache, we can use provider.DescriptorCache directly.

[lindnerby]

The problem is that devs will start to use and modify DescriptorCache directly in the production code, what we don't want, because it is an internal dependency of the DescriptorProvider. We want the cache encapsulated and not accessible on the Provider level. Encapsulation is a well known principle in software engineering. If we keep violating it, we will increase the risk of bugs being introduced in our system by uncontrolled access and increase the tight coupling of dependencies. We want to start introducing clear interfaces to indicate usage and care about internal data integrity.

If we don't need to test the state of the cache in tests, then we don't need a separate constructor function for tests where you can inject the cache reference and later do assertions on it, true.
But if we want to do assertions on an internal dependency like we do in this test, then we can do that by keeping a reference to it in the test.

I don't get the reason why needs to inject a cache to this CachedDescriptorProvider, you already tested the GetDescriptor as well as Add, what's the purpose of injecting a cache? If you need to verify the content inside the cache, we can use provider.DescriptorCache directly.

That's the point. DescriptorProvider adds another layer of logic on top the cache when adding and getting. We want to be able to verify each function separately. We don't want to test the cache, we want to test Provider, so the implementation of cache should be exchangeable.

If we keep this pattern of pragmatic opening up of all internal dependencies of our components, then soon run into more cases of the Train Wreck Anti-pattern, or just Train Wreck.
This is a situation in programming where a chain of get calls using the dot notation is used to navigate through multiple objects in order to get or set a value. This results in code that is hard to read, difficult to debug, and tightly coupled.
This will make the code prone to breaking because more objects in the chain can cause a failure.
An extreme pseudo-code example is this:
person.getAddress().getCity().getZipCode().getArea().getNeighborhood().getName();

The principle of Least Knowledge also known as the Law of Demeter is basically the guideline for my argumentation here. Feel free to have a look!

[ruanxin]

I think we are mixing several concepts here. The initial reason for this refactoring is just trying to get rid of initializing this provider with the nil parameter NewCachedDescriptorProvider(nil), currently spreading in the production code, which is meaningless.

Regarding the answer you provided that you want to test Provider with a different cache, I don't see this in your original code tbh (that's why I don't get the reason in the beginning). If you want to achieve this goal, why give *cache.DescriptorCache as a member, it's a struct, the only thing you can provide is a single implementation which is here, it's not an interface.

type CachedDescriptorProvider struct {
	DescriptorCache *cache.DescriptorCache
}

Then I would design like this below:

type DescriptorCache interface {
	Get(key DescriptorKey) *v1beta2.Descriptor
	Set(key DescriptorKey, value *v1beta2.Descriptor)
}

type CachedDescriptorProvider struct {
	descriptorCache DescriptorCache
}

I would also design a dedicated constructor for injecting different DescriptorCache implementations, since currently on the production code, there is no other implementation, if really in the production code, we rely on different cache implementations, then it makes sense to get rid of the one without parameter.

// this is default which used in production
func NewCachedDescriptorProvider() *CachedDescriptorProvider
// this is for providing different implementations, probably useful for testing purpose
func NewCachedDescriptorProviderForTest(descriptorCache DescriptorCache) *CachedDescriptorProvider

So in short summary, in the original implmentation, you already tightly coupled this provider with cache.DescriptorCache, it's against what you want to achieve.

Secondly, regarding address encapsulation of the descriptorCache and considering this as an internal member of this Provider, which I agree with also, then I would refactor this Provider to offer an additional function GetDescriptorByCacheKey() to make this function work https://github.com/kyma-project/lifecycle-manager/pull/1695/files#diff-36cc16a1916ddd61fd387154219e8a413c14e5cff7cb6c0839016cff7140ef62R227, but the reason why I think it's ok to public this member is I don't think we as developer is stupid enough to directly assign a different DescriptorCache to this provider after initialized. And regarding the chain of get calls, it's also no harm here, the chain call for this provider is basically provider.DescriptorCache.Get(key), provider.DescriptorCache.Set(), I see certain valid usage of them, you can of course argue that we design different method to encapsulate them.

last but not least, I think we should apply the design principle which aligned with the current state of the code, also consider the trade-off, take your Law of Demeter as an example, it also brings disadvantage if we take it strictly. So I think we should not use some extreme examples to influence the current state.

Also, I like this discussion and open for further improvement, and I appreciate the thoughts you bring for keeping our code quality and aligned with best practices, please don't feel demotivated.