fix: import package with same prefix name as the root package

[leovct]

Description

An attempt to fix the issue when importing a package with the same prefix name as the root package (e.g. importing kurtosis-test-xyz inside kurtosis-test)

I added two new test cases to show it's not working properly:

func TestGetAbsoluteLocator_RepositoriesWithSamePrefixNameShouldNotBeBlocked(t *testing.T) {
	provider := NewGitPackageContentProvider("", "", NewGitHubPackageAuthProvider(""), nil)

	packageId := "github.com/main-package"
	locatorOfModuleInWhichThisBuiltInIsBeingCalled := "github.com/main-package-xyz/main.star" // same package prefix
	maybeRelativeLocator := "github.com/main-package/file.star"

	_, err := provider.GetAbsoluteLocator(packageId, locatorOfModuleInWhichThisBuiltInIsBeingCalled, maybeRelativeLocator, noPackageReplaceOptions)
	require.Nil(t, err)
}

func Test_isNotSamePackageLocalAbsoluteLocator_TestRepositoriesWithSamePrefixNames(t *testing.T) {
	result := shouldBlockAbsoluteLocatorBecauseIsInTheSameSourceModuleLocatorPackage("github.com/author/package2/main.star", "github.com/author/package/main.star", "github.com/author/package")
	require.False(t, result)
}

--- FAIL: TestGetAbsoluteLocator_RepositoriesWithSamePrefixNameShouldNotBeBlocked (0.00s)
    git_package_content_provider_test.go:611: 
                Error Trace:    /home/circleci/project/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/git_package_content_provider_test.go:611
                Error:          Expected nil, but got: &startosis_errors.InterpretationError{msg:"Locator 'github.com/main-package/file.star' is referencing a file within the same package using absolute import syntax, but only relative import syntax (path starting with '/' or '.') is allowed for within-package imports", cause:error(nil), stacktrace:[]startosis_errors.CallFrame(nil)}
                Test:           TestGetAbsoluteLocator_RepositoriesWithSamePrefixNameShouldNotBeBlocked
FAIL
FAIL    github.com/kurtosis-tech/kurtosis/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider     5.120s

--- FAIL: Test_isNotSamePackageLocalAbsoluteLocator_TestRepositoriesWithSamePrefixNames (0.00s)
    git_package_content_provider_test.go:626: 
                Error Trace:    /home/circleci/project/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/git_package_content_provider_test.go:626
                Error:          Should be false
                Test:           Test_isNotSamePackageLocalAbsoluteLocator_TestRepositoriesWithSamePrefixNames

The problem comes from the fact that the function only checks if the locator contains the package id (prefix check) instead of carrying out a more rigorous check:

func shouldBlockAbsoluteLocatorBecauseIsInTheSameSourceModuleLocatorPackage(relativeOrAbsoluteLocator string, sourceModuleLocator string, rootPackageId string) bool {
	isSourceModuleInRootPackage := strings.HasPrefix(sourceModuleLocator, rootPackageId)
	isAbsoluteLocatorInRootPackage := strings.HasPrefix(relativeOrAbsoluteLocator, rootPackageId)
	return isSourceModuleInRootPackage && isAbsoluteLocatorInRootPackage
}

To fix this I made sure the rootPackageId ends with a trailing slash. This way it should not complain when checking absolute locators with the same prefix.

func shouldBlockAbsoluteLocatorBecauseIsInTheSameSourceModuleLocatorPackage(relativeOrAbsoluteLocator string, sourceModuleLocator string, rootPackageId string) bool {
	// Make sure the root package id ends with a trailing slash.
	rootPackageId = strings.TrimSuffix(rootPackageId, "/") + "/"
	isSourceModuleInRootPackage := strings.HasPrefix(sourceModuleLocator, rootPackageId)
	isAbsoluteLocatorInRootPackage := strings.HasPrefix(relativeOrAbsoluteLocator, rootPackageId)
	return isSourceModuleInRootPackage && isAbsoluteLocatorInRootPackage
}

REMINDER: Tag Reviewers, so they get notified to review

Is this change user facing?

YES

References (if applicable)

[tedim52]

Thanks @leovct !

[tedim52]

What do you think about this test case I added that fails? I think the fix won't work if relativeOrAbsoluteLocator is in a package that is nested in the root package.

func Test_isNotSamePackageLocalAbsoluteLocator_TestImportSubPackageWithSamePrefixNameAsRootPackages(t *testing.T) {
	// Without root package id trailing slash.
	result := shouldBlockAbsoluteLocatorBecauseIsInTheSameSourceModuleLocatorPackage("github.com/author/package/package2/main.star", "github.com/author/package/main.star", "github.com/author/package")
	require.False(t, result)

	// With root package id trailing slash.
	result = shouldBlockAbsoluteLocatorBecauseIsInTheSameSourceModuleLocatorPackage("github.com/author/package/package2/main.star", "github.com/author/package/main.star", "github.com/author/package/")
	require.False(t, result)
}

Prefix isn't sufficient bc you can't distinguish between any prefix being a package or just another nested directory in this part of the code. Ideally , we'd just compare directly the package id of sourceModuleLocator and package id of relativeOrAbsoluteLocator but there's no way to extract package id of relativeOrAbsoluteLocator here.

I don't mind merging this now since it at addresses the non nested package case and doesn't break anything. I can think on the nested package case or maybe you have an idea.

[tedim52]

approved but commented above