Improve comments around the global and local provider schema cache. (#958)

Signed-off-by: Jakub Martin <kubam@spacelift.io>

internal/plugin/grpc_provider.go

@@ -79,8 +79,12 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp
 	p.mu.Lock()
 	defer p.mu.Unlock()
 
-	// check the global cache
+	// First, we check the global cache.
+	// The cache could contain this schema if an instance of this provider has previously been started.
 	if !p.Addr.IsZero() {
+		// Even if the schema is cached, GetProviderSchemaOptional could be false. This would indicate that once instantiated,
+		// this provider requires the get schema call to be made at least once, as it handles part of the provider's setup.
+		// At this point, we don't know if this is the first call to a provider instance or not, so we don't use the result in that case.
 		if schemaCached, ok := providers.SchemaCache.Get(p.Addr); ok && schemaCached.ServerCapabilities.GetProviderSchemaOptional {
 			logger.Trace("GRPCProvider: GetProviderSchema: serving from global schema cache", "address", p.Addr)
 			return schemaCached
@@ -88,7 +92,8 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp
 	}
 
 	// If the local cache is non-zero, we know this instance has called
-	// GetProviderSchema at least once and we can return early.
+	// GetProviderSchema at least once, so has satisfied the possible requirement of `GetProviderSchemaOptional=false`.
+	// This means that we can return early now using the locally cached schema, without making this call again.
 	if p.schema.Provider.Block != nil {
 		return p.schema
 	}
@@ -142,13 +147,21 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp
 		resp.ServerCapabilities.GetProviderSchemaOptional = protoResp.ServerCapabilities.GetProviderSchemaOptional
 	}
 
-	// set the global cache if we can
+	// Set the global provider cache so that future calls to this provider can use the cached value.
+	// Crucially, this doesn't look at GetProviderSchemaOptional, because the layers above could use this cache
+	// *without* creating an instance of this provider. And if there is no instance,
+	// then we don't need to set up anything (cause there is nothing to set up), so we need no call
+	// to the providers GetSchema rpc.
 	if !p.Addr.IsZero() {
 		providers.SchemaCache.Set(p.Addr, resp)
 	}
 
-	// always store this here in the client for providers that are not able to
-	// use GetProviderSchemaOptional
+	// Always store this here in the client for providers that are not able to use GetProviderSchemaOptional.
+	// Crucially, this indicates that we've made at least one call to GetProviderSchema to this instance of the provider,
+	// which means in the future we'll be able to return using this cache
+	// (because the possible setup contained in the GetProviderSchema call has happened).
+	// If GetProviderSchemaOptional is true then this cache won't actually ever be used, because the calls to this method
+	// will be satisfied by the global provider cache.
 	p.schema = resp
 
 	return resp

internal/plugin6/grpc_provider.go

@@ -79,8 +79,12 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp
 	p.mu.Lock()
 	defer p.mu.Unlock()
 
-	// check the global cache
+	// First, we check the global cache.
+	// The cache could contain this schema if an instance of this provider has previously been started.
 	if !p.Addr.IsZero() {
+		// Even if the schema is cached, GetProviderSchemaOptional could be false. This would indicate that once instantiated,
+		// this provider requires the get schema call to be made at least once, as it handles part of the provider's setup.
+		// At this point, we don't know if this is the first call to a provider instance or not, so we don't use the result in that case.
 		if schemaCached, ok := providers.SchemaCache.Get(p.Addr); ok && schemaCached.ServerCapabilities.GetProviderSchemaOptional {
 			logger.Trace("GRPCProvider: GetProviderSchema: serving from global schema cache", "address", p.Addr)
 			return schemaCached
@@ -88,7 +92,8 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp
 	}
 
 	// If the local cache is non-zero, we know this instance has called
-	// GetProviderSchema at least once and we can return early.
+	// GetProviderSchema at least once, so has satisfied the possible requirement of `GetProviderSchemaOptional=false`.
+	// This means that we can return early now using the locally cached schema, without making this call again.
 	if p.schema.Provider.Block != nil {
 		return p.schema
 	}
@@ -142,13 +147,21 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp
 		resp.ServerCapabilities.GetProviderSchemaOptional = protoResp.ServerCapabilities.GetProviderSchemaOptional
 	}
 
-	// set the global cache if we can
+	// Set the global provider cache so that future calls to this provider can use the cached value.
+	// Crucially, this doesn't look at GetProviderSchemaOptional, because the layers above could use this cache
+	// *without* creating an instance of this provider. And if there is no instance,
+	// then we don't need to set up anything (cause there is nothing to set up), so we need no call
+	// to the providers GetSchema rpc.
 	if !p.Addr.IsZero() {
 		providers.SchemaCache.Set(p.Addr, resp)
 	}
 
-	// always store this here in the client for providers that are not able to
-	// use GetProviderSchemaOptional
+	// Always store this here in the client for providers that are not able to use GetProviderSchemaOptional.
+	// Crucially, this indicates that we've made at least one call to GetProviderSchema to this instance of the provider,
+	// which means in the future we'll be able to return using this cache
+	// (because the possible setup contained in the GetProviderSchema call has happened).
+	// If GetProviderSchemaOptional is true then this cache won't actually ever be used, because the calls to this method
+	// will be satisfied by the global provider cache.
 	p.schema = resp
 
 	return resp

internal/tofu/context_plugins.go

@@ -70,6 +70,11 @@ func (cp *contextPlugins) ProviderSchema(addr addrs.Provider) (providers.Provide
 	// This cache is only written by the provider client, and transparently
 	// used by GetProviderSchema, but we check it here because at this point we
 	// may be able to avoid spinning up the provider instance at all.
+	//
+	// It's worth noting that ServerCapabilities.GetProviderSchemaOptional is ignored here.
+	// That is because we're checking *prior* to the provider's instantiation.
+	// GetProviderSchemaOptional only says that *if we instantiate a provider*,
+	// then we need to run the get schema call at least once.
 	schemas, ok := providers.SchemaCache.Get(addr)
 	if ok {
 		log.Printf("[TRACE] tofu.contextPlugins: Serving provider %q schema from global schema cache", addr)