fix: add test to cover behavior for invalid manifests

cmd/dbc/driver_list_test.go

@@ -23,11 +23,11 @@ func TestUnmarshalDriverList(t *testing.T) {
 		expected []dbc.PkgInfo
 		err      error
 	}{
-		{"basic", "[drivers]\nflightsql = {version = '1.7.0'}", []dbc.PkgInfo{
-			{Driver: dbc.Driver{Path: "flightsql"}, Version: semver.MustParse("1.7.0")},
+		{"basic", "[drivers]\nflightsql = {version = '1.8.0'}", []dbc.PkgInfo{
+			{Driver: dbc.Driver{Path: "flightsql"}, Version: semver.MustParse("1.8.0")},
 		}, nil},
-		{"less", "[drivers]\nflightsql = {version = '<=1.7.0'}", []dbc.PkgInfo{
-			{Driver: dbc.Driver{Path: "flightsql"}, Version: semver.MustParse("1.7.0")},
+		{"less", "[drivers]\nflightsql = {version = '<=1.8.0'}", []dbc.PkgInfo{
+			{Driver: dbc.Driver{Path: "flightsql"}, Version: semver.MustParse("1.8.0")},
 		}, nil},
 		{"greater", "[drivers]\nflightsql = {version = '>=1.7.0'}", []dbc.PkgInfo{
 			{Driver: dbc.Driver{Path: "flightsql"}, Version: semver.MustParse("1.8.0")},

cmd/dbc/search_test.go

@@ -15,7 +15,8 @@ func (suite *SubcommandTestSuite) TestSearchCmd() {
 	suite.validateOutput("• test-driver-1 - This is a test driver\r\n"+
 		"• test-driver-2 - This is another test driver\r\n"+
 		"• test-driver-manifest-only - This is manifest-only driver\r\n"+
-		"• test-driver-no-sig - Driver manifest missing Files.signature entry\r\n\r ", "", suite.runCmd(m))
+		"• test-driver-no-sig - Driver manifest missing Files.signature entry\r\n"+
+		"• test-driver-invalid-manifest - This is test driver with an invalid manfiest. See https://github.com/columnar-tech/dbc/issues/37.\r\n\r ", "", suite.runCmd(m))
 }
 
 func (suite *SubcommandTestSuite) TestSearchCmdWithInstalled() {
@@ -28,7 +29,8 @@ func (suite *SubcommandTestSuite) TestSearchCmdWithInstalled() {
 			downloadPkg: downloadTestPkg})
 	suite.validateOutput("• test-driver-1 - This is a test driver [installed: env=>1.1.0]\r\n"+
 		"• test-driver-2 - This is another test driver\r\n• test-driver-manifest-only - This is manifest-only driver\r\n"+
-		"• test-driver-no-sig - Driver manifest missing Files.signature entry\r\n\r ", "", suite.runCmd(m))
+		"• test-driver-no-sig - Driver manifest missing Files.signature entry\r\n"+
+		"• test-driver-invalid-manifest - This is test driver with an invalid manfiest. See https://github.com/columnar-tech/dbc/issues/37.\r\n\r ", "", suite.runCmd(m))
 }
 
 func (suite *SubcommandTestSuite) TestSearchCmdVerbose() {
@@ -46,6 +48,9 @@ func (suite *SubcommandTestSuite) TestSearchCmdVerbose() {
 		"Available Versions:\r\n    ╰── 1.0.0\r\n"+
 		"• test-driver-no-sig\r\n   Title: Test Driver No Signature\r\n   "+
 		"Description: Driver manifest missing Files.signature entry\r\n   License: Apache-2.0\r\n   "+
+		"Available Versions:\r\n    ╰── 1.0.0\r\n"+
+		"• test-driver-invalid-manifest\r\n   Title: Test Driver Invalid Manifest\r\n   "+
+		"Description: This is test driver with an invalid manfiest. See https://github.com/columnar-tech/dbc/issues/37.\r\n   License: Apache-2.0\r\n   "+
 		"Available Versions:\r\n    ╰── 1.0.0\r\n\r ", "", suite.runCmd(m))
 }
 
@@ -75,5 +80,11 @@ func (suite *SubcommandTestSuite) TestSearchCmdVerboseWithInstalled() {
 		"   Description: Driver manifest missing Files.signature entry\r\n"+
 		"   License: Apache-2.0\r\n"+
 		"   Available Versions:\r\n"+
+		"    ╰── 1.0.0\r\n"+
+		"• test-driver-invalid-manifest\r\n"+
+		"   Title: Test Driver Invalid Manifest\r\n"+
+		"   Description: This is test driver with an invalid manfiest. See https://github.com/columnar-tech/dbc/issues/37.\r\n"+
+		"   License: Apache-2.0\r\n"+
+		"   Available Versions:\r\n"+
 		"    ╰── 1.0.0\r\n\r ", "", suite.runCmd(m))
 }

cmd/dbc/sync_test.go

@@ -45,6 +45,8 @@ func downloadTestPkg(pkg dbc.PkgInfo) (*os.File, error) {
 		return os.Open(filepath.Join("testdata", "test-driver-manifest-only.tar.gz"))
 	case "test-driver-no-sig":
 		return os.Open(filepath.Join("testdata", "test-driver-no-sig.tar.gz"))
+	case "test-driver-invalid-manifest":
+		return os.Open(filepath.Join("testdata", "test-driver-invalid-manifest.tar.gz"))
 	default:
 		return nil, fmt.Errorf("unknown driver: %s", pkg.Driver.Path)
 	}

cmd/dbc/testdata/test_index.yaml

@@ -68,6 +68,21 @@ drivers:
       - version: v1.0.0
         packages:
           - platform: linux_amd64
+          - platform: linux_arm64
           - platform: macos_amd64
           - platform: macos_arm64
-          - platform: windows_amd64
+          - platform: windows_amd64
+          - platform: windows_arm64
+  - name: Test Driver Invalid Manifest
+    description: This is test driver with an invalid manfiest. See https://github.com/columnar-tech/dbc/issues/37.
+    license: Apache-2.0
+    path: test-driver-invalid-manifest
+    pkginfo:
+      - version: v1.0.0
+        packages:
+          - platform: linux_amd64
+          - platform: linux_arm64
+          - platform: macos_amd64
+          - platform: macos_arm64
+          - platform: windows_amd64
+          - platform: windows_arm64

cmd/dbc/uninstall_test.go

@@ -10,6 +10,7 @@ import (
 	"runtime"
 
 	"github.com/columnar-tech/dbc/config"
+	"github.com/pelletier/go-toml/v2"
 )
 
 func (suite *SubcommandTestSuite) TestUninstallNotFound() {
@@ -153,3 +154,50 @@ func (suite *SubcommandTestSuite) TestUninstallManifestOnlyDriver() {
 	}
 	suite.NoDirExists(filepath.Join(suite.tempdir, new_sidecar_path))
 }
+
+// See https://github.com/columnar-tech/dbc/issues/37
+func (suite *SubcommandTestSuite) TestUninstallInvalidManifest() {
+	if runtime.GOOS == "windows" {
+		suite.T().Skip()
+	}
+	m := InstallCmd{Driver: "test-driver-invalid-manifest", Level: config.ConfigEnv}.
+		GetModelCustom(baseModel{getDriverList: getTestDriverList, downloadPkg: downloadTestPkg})
+	suite.runCmd(m)
+	suite.FileExists(filepath.Join(suite.tempdir, "test-driver-invalid-manifest.toml"))
+
+	// The installed manifest should have a Driver.shared set to a folder, not the .so
+	// We only need a partial struct definition to read in the Driver.shared table
+	type partialManifest struct {
+		Driver struct {
+			Shared map[string]string `toml:"shared"`
+		}
+	}
+	var invalidManifest partialManifest
+	f, err := os.Open(filepath.Join(suite.tempdir, "test-driver-invalid-manifest.toml"))
+	if err != nil {
+		suite.Error(err)
+	}
+	err = toml.NewDecoder(f).Decode(&invalidManifest)
+	if err != nil {
+		suite.Error(err)
+	}
+	value := invalidManifest.Driver.Shared[config.PlatformTuple()]
+	// Assert that it's a folder
+	suite.DirExists(value)
+	// and continue
+
+	m = UninstallCmd{Driver: "test-driver-invalid-manifest", Level: config.ConfigEnv}.GetModel()
+	output := suite.runCmd(m)
+
+	// TODO: Currently failing with:
+	// Error: failed to uninstall driver: error removing driver test-driver-invalid-manifest: RemoveAll .: invalid argument
+	suite.validateOutput("Driver `test-driver-invalid-manifest` uninstalled successfully!\r\n\r\n\r ", "", output)
+
+	// Ensure we don't nuke the tempdir which is the original (major) issue
+	suite.DirExists(suite.tempdir)
+
+	// Ensure we removed the files from the package though
+	suite.NoFileExists(filepath.Join(suite.tempdir, "test-driver-invalid-manifest.toml"))
+	suite.NoFileExists(filepath.Join(suite.tempdir, "test-driver-invalid-manifest", "test-driver-invalid-manifest.so"))
+	suite.NoDirExists(filepath.Join(suite.tempdir, "test-driver-invalid-manifest"))
+}

config/config.go

@@ -331,7 +331,36 @@ func UninstallDriverShared(cfg Config, info DriverInfo) error {
 		// dbc installs drivers in a folder, other tools may not so we handle each
 		// differently.
 		if info.Source == "dbc" {
-			if err := os.RemoveAll(filepath.Dir(sharedPath)); err != nil {
+			shared_dir := filepath.Dir(sharedPath)
+
+			// Handle an edge case first. We've already verified that Driver.shared is
+			// an absolute path inside cfg.Location but we also expect the shared
+			// library to be in a subfolder, e.g.,
+			//
+			// ${cfg.Location}
+			// └── libadbc_driver_linux_amd64_v1.0.0
+			//     └── libadbc_driver_linux_amd64_v1.0.0.so
+			//
+			// If the MANIFEST we're installed has Driver.shared already set, this is
+			// invalid becuase it should have Files set instead. When this happens,
+			// the Driver.shared value in the installed manifest ends up as a absolute
+			// path to a folder, not a shared library.
+			shared_path_is_subdir, err := IsImmediateSubDir(cfg.Location, sharedPath)
+			if err != nil {
+				return fmt.Errorf("error removing driver %s: %w", info.ID, err)
+			}
+			if shared_path_is_subdir {
+				if err := os.RemoveAll(sharedPath); err != nil {
+					if errors.Is(err, fs.ErrNotExist) {
+						continue
+					}
+					return fmt.Errorf("error removing driver %s: %w", info.ID, err)
+				}
+				continue
+			}
+
+			// The rest of this is "normal" operation...
+			if err := os.RemoveAll(shared_dir); err != nil {
 				// Ignore only when not found. This supports manifest-only drivers.
 				// TODO: Come up with a better mechanism to handle manifest-only drivers
 				// and remove this continue when we do
@@ -371,3 +400,34 @@ func UninstallDriverShared(cfg Config, info DriverInfo) error {
 
 	return nil
 }
+
+// Determine whether target is an immediate subdirectory of base.
+func IsImmediateSubDir(base string, target string) (bool, error) {
+	base_abs, err := filepath.Abs(base)
+	if err != nil {
+		return false, err
+	}
+	target_abs, err := filepath.Abs(target)
+	if err != nil {
+		return false, err
+	}
+
+	result, err := filepath.Rel(base_abs, target_abs)
+	if err != nil {
+		return false, err
+	}
+
+	parts := strings.Split(result, string(filepath.Separator))
+
+	// return false if we're not a direct child
+	if len(parts) != 1 {
+		return false, nil
+	}
+
+	// return false if we're above
+	if strings.HasPrefix(result, ".") {
+		return false, nil
+	}
+
+	return true, nil
+}

config/config_test.go

@@ -3,6 +3,7 @@
 package config
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
@@ -45,3 +46,114 @@ func TestConfigEnvVarHierarchy(t *testing.T) {
 	cfg = loadConfig(ConfigEnv)
 	assert.Equal(t, "", cfg.Location)
 }
+
+func TestIsImmediateSubDir(t *testing.T) {
+	tests := []struct {
+		parent    string
+		child     string
+		expected  bool
+		expectErr bool
+	}{
+		{
+			parent:   "/foo",
+			child:    "/foo/bar",
+			expected: true,
+		},
+		{
+			parent:   "/foo/bar",
+			child:    "/foo/bar",
+			expected: false,
+		},
+		{
+			parent:   "/foo",
+			child:    "/foo/bar/baz",
+			expected: false,
+		},
+		{
+			parent:   "/foo/bar",
+			child:    "/foo/baz",
+			expected: false,
+		},
+		{
+			parent:   "/foo",
+			child:    "/bar",
+			expected: false,
+		},
+		{
+			parent:   "/foo/bar",
+			child:    "/foo/bar/../baz",
+			expected: false,
+		},
+		{
+			parent:   "/foo",
+			child:    "/foo/bar/",
+			expected: true,
+		},
+		{
+			parent:   "/foo/",
+			child:    "/foo/bar",
+			expected: true,
+		},
+		{
+			parent:   "/foo/",
+			child:    "/foo/bar/",
+			expected: true,
+		},
+		{
+			parent:   "/",
+			child:    "/foo",
+			expected: true,
+		},
+		{
+			parent:   "",
+			child:    "/foo",
+			expected: false,
+		},
+		{
+			parent:   "/foo",
+			child:    "",
+			expected: false,
+		},
+		{
+			parent:   "",
+			child:    "",
+			expected: false,
+		},
+		{
+			parent:   "foo",
+			child:    "foo/bar",
+			expected: true,
+		},
+		{
+			parent:   "/foo",
+			child:    "/foo/./bar",
+			expected: true,
+		},
+		{
+			parent:   "/foo",
+			child:    "/foo/bar/../baz",
+			expected: true,
+		},
+		{
+			parent:   "/foo",
+			child:    "/foo/bar/some_file.txt",
+			expected: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(fmt.Sprintf("%s vs %s", tt.parent, tt.child), func(t *testing.T) {
+			result, err := IsImmediateSubDir(tt.parent, tt.child)
+
+			if tt.expectErr {
+				assert.Error(t, err)
+				return
+			}
+
+			assert.NoError(t, err)
+			assert.Equal(t, tt.expected, result,
+				"IsImmediateSubDir(%q, %q) = %v, expected %v",
+				tt.parent, tt.child, result, tt.expected)
+		})
+	}
+}