feat: Set connector refs for models

[k-anshul]

closes https://linear.app/rilldata/issue/PLAT-30/updating-connector-yaml-files-or-variables-should-restart-the

Checklist:

• Covered by tests
• Ran it and it works as intended
• Reviewed the diff before requesting a review
• Checked for unhandled edge cases
• Linked the issues it closes
• Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
• Intend to cherry-pick into the release branch
• I'm proud of this work!

[begelundmuller]

I tried to review this PR, but unfortunately I don't quite follow it, so I have some questions:

• Can you explain what the purpose of resourcesForInferredRef is? I’m trying to wrap my head around it, but not quite getting it (like, why can’t it just loop over Refs to check?)
• Did you consider manipulating rawRefs instead of Refs? Or maybe only allowing a hook to rewrite raw refs and nothing else (to make it safe to run repeatedly).
• About the issue you raised here. Could it be fixed by integrating with inferUnspecifiedRefs? Since I believe it already has logic for propagating when inferred refs change.

[k-anshul]

Can you explain what the purpose of resourcesForInferredRef is? I’m trying to wrap my head around it, but not quite getting it (like, why can’t it just loop over Refs to check?)

This is just to make sure that hooks are processed fast. Since all the hooks for all nodes are processed after any resource is updated I felt it necessary to create a lookup. A simple loop can also be added if you feel otherwise.

Did you consider manipulating rawRefs instead of Refs? Or maybe only allowing a hook to rewrite raw refs and nothing else (to make it safe to run repeatedly).

Not sure I fully understand this. The rawRefs are processed in insertResource but the hooks run afterwards. If the hook modified the raw refs then how will it get added to refs ?

About the issue you raised #8591 (comment). Could it be fixed by integrating with inferUnspecifiedRefs? Since I believe it already has logic for propagating when inferred refs change.

Yeah sorry it was more about should we do this i.e. if a connector is removed should the model continue to work fine. Should this be extended to all connectors ?

[begelundmuller]

The rawRefs are processed in insertResource but the hooks run afterwards.

Actually, it gets processed into Refs in inferUnspecifiedRefs, which runs after all resources have been parsed.

So inferUnspecifiedRefs is kind of already like a hard-coded post-parse hook. It is also designed to be re-run for all resources that may be impacted by a re-parse. (It basically re-converts rawRefs into Refs each time a related resource is changed – thus addressing problems like the one you mentioned #8591 (comment).)

So what I was wondering was if inferUnspecifiedRefs could be made generic to run any ref-rewriting logic. So it would work for connectors without adding much new logic, and using its existing logic to address #8591 (comment).

This is just to make sure that hooks are processed fast.

Okay. This is also something inferUnspecifiedRefs already accounts for – it only runs inference for resources impacted by the current change during a re-parse. That logic is here:

rill/runtime/parser/parser.go

Lines 501 to 534 in 21a4347

	// Infer or re-infer refs for...
	inferRefsSeen := make(map[ResourceName]bool)
	// ... all inserted resources
	for _, r := range p.insertedResources {
	inferRefsSeen[r.Name.Normalized()] = true
	p.inferUnspecifiedRefs(r)
	}
	// ... all updated resources
	for _, r := range p.updatedResources {
	inferRefsSeen[r.Name.Normalized()] = true
	p.inferUnspecifiedRefs(r)
	}
	// ... any unchanged resource that may have an unspecified ref to a deleted resource
	for _, r1 := range p.deletedResources {
	for _, r2 := range p.resourcesForUnspecifiedRef[strings.ToLower(r1.Name.Name)] {
	n := r2.Name.Normalized()
	if !inferRefsSeen[n] {
	inferRefsSeen[n] = true
	p.inferUnspecifiedRefs(r2)
	p.updatedResources = append(p.updatedResources, r2) // inferRefsSeen ensures it's not already in insertedResources or updatedResources
	}
	}
	}
	// ... any unchanged resource that might have an unspecified ref (previously unmatched) that now matches a newly inserted resource
	for _, r1 := range p.insertedResources {
	for _, r2 := range p.resourcesForUnspecifiedRef[strings.ToLower(r1.Name.Name)] {
	n := r2.Name.Normalized()
	if !inferRefsSeen[n] {
	inferRefsSeen[n] = true
	p.inferUnspecifiedRefs(r2)
	p.updatedResources = append(p.updatedResources, r2) // inferRefsSeen ensures it's not already in insertedResources or updatedResources
	}
	}
	}

[begelundmuller]

Why does this existing case not already handle this? It's generic for all resources, so would expect it to also work for connector refs.

// ... any unchanged resource that might have an unspecified ref (previously unmatched) that now matches a newly inserted resource

[k-anshul]

It only iterates over the resources that has refs to ResourceKindUnspecified:

rill/runtime/parser/parser.go

Lines 524 to 527 in 49df2c2

	// ... any unchanged resource that might have an unspecified ref (previously unmatched) that now matches a newly inserted resource
	for _, r1 := range p.insertedResources {
	for _, r2 := range p.resourcesForUnspecifiedRef[strings.ToLower(r1.Name.Name)] {
	n := r2.Name.Normalized()

I modified it slightly to handle both connector and ResourceKindUnspecified.

[begelundmuller]

1. It looks like the logic in here can be simplified quite a bit by removing the ResourceKindSource logic (since that resource type is never emitted anymore, it's dead code anyway)
2. "Unspecified" is not really the right word anymore, since e.g. connectors are explicitly specified, they just may not necessarily exist. Maybe rename the function to inferAmbiguousRefs?

[k-anshul]

2. Done. Also modified the index a bit to handle the above comment.

[begelundmuller]

LGTM