check path equivalency (#4635)

<!-- PR description-->

This change is to address the path related issue that comes up while `corso repo connect filesystem`.
Below are the steps to reciprocate:
1. Initialize repo with filesystem provider (Notice that the path has a trailing slash): 
command: `./corso repo init filesystem --path ~/Documents/Backups/`
output:
Logging to file: ~/.cache/corso/logs/2023-11-09T05-21-57Z.log
Connecting to M365:         done 
Initializing repository   
Initialized a repository at path ~/Documents/Backups/

2. Connect repo with filesystem provider:
command: `./corso repo connect filesystem --path ~/Documents/Backups/`
output:
Logging to file: ~/.cache/corso/logs/2023-11-09T05-23-30Z.log
Error: retrieving storage provider details: verifying storage configs in corso config file: verifying configs in corso config file: value of path (~/Documents/Backups) does not match corso configuration value (~/Documents/Backups/)

#### Does this PR need a docs update or release note?

- [ ]  Yes, it's included
- [ ] 🕐 Yes, but in a later PR
- [x]  No

#### Type of change

<!--- Please check the type of change your PR introduces: --->
- [ ] 🌻 Feature
- [x] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [ ] 🧹 Tech Debt/Cleanup

#### Test Plan

<!-- How will this be tested prior to merging.-->
- [x] 💪 Manual
- [x]  Unit test
- [x] 💚 E2E
This commit is contained in:
Hitesh Pattanayak 2023-11-11 01:29:12 +05:30 committed by GitHub
parent 5362137116
commit 52c788eac5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 260 additions and 8 deletions

View File

@ -30,6 +30,9 @@ func m365Overrides(in map[string]string) map[string]string {
} }
} }
// add m365 config key names that require path related validations
var m365PathKeys = []string{}
// configureAccount builds a complete account configuration from a mix of // configureAccount builds a complete account configuration from a mix of
// viper properties and manual overrides. // viper properties and manual overrides.
func configureAccount( func configureAccount(
@ -57,7 +60,7 @@ func configureAccount(
return acct, clues.New("unsupported account provider: [" + providerType + "]") return acct, clues.New("unsupported account provider: [" + providerType + "]")
} }
if err := mustMatchConfig(vpr, m365Overrides(overrides)); err != nil { if err := mustMatchConfig(vpr, m365Overrides(overrides), m365PathKeys); err != nil {
return acct, clues.Wrap(err, "verifying m365 configs in corso config file") return acct, clues.Wrap(err, "verifying m365 configs in corso config file")
} }
} }

View File

@ -4,6 +4,7 @@ import (
"context" "context"
"os" "os"
"path/filepath" "path/filepath"
"slices"
"strings" "strings"
"github.com/alcionai/clues" "github.com/alcionai/clues"
@ -16,6 +17,7 @@ import (
"github.com/alcionai/corso/src/pkg/account" "github.com/alcionai/corso/src/pkg/account"
"github.com/alcionai/corso/src/pkg/control/repository" "github.com/alcionai/corso/src/pkg/control/repository"
"github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/logger"
"github.com/alcionai/corso/src/pkg/path"
"github.com/alcionai/corso/src/pkg/storage" "github.com/alcionai/corso/src/pkg/storage"
) )
@ -333,7 +335,7 @@ var constToTomlKeyMap = map[string]string{
// If any value differs from the viper value, an error is returned. // If any value differs from the viper value, an error is returned.
// values in m that aren't stored in the config are ignored. // values in m that aren't stored in the config are ignored.
// TODO(pandeyabs): This code is currently duplicated in 2 places. // TODO(pandeyabs): This code is currently duplicated in 2 places.
func mustMatchConfig(vpr *viper.Viper, m map[string]string) error { func mustMatchConfig(vpr *viper.Viper, m map[string]string, pathKeys []string) error {
for k, v := range m { for k, v := range m {
if len(v) == 0 { if len(v) == 0 {
continue // empty variables will get caught by configuration validators, if necessary continue // empty variables will get caught by configuration validators, if necessary
@ -345,7 +347,16 @@ func mustMatchConfig(vpr *viper.Viper, m map[string]string) error {
} }
vv := vpr.GetString(tomlK) vv := vpr.GetString(tomlK)
if v != vv { areEqual := false
// some of the values maybe paths, hence they require more than just string equality
if len(pathKeys) > 0 && slices.Contains(pathKeys, k) {
areEqual = path.ArePathsEquivalent(v, vv)
} else {
areEqual = v == vv
}
if !areEqual {
return clues.New("value of " + k + " (" + v + ") does not match corso configuration value (" + vv + ")") return clues.New("value of " + k + " (" + v + ") does not match corso configuration value (" + vv + ")")
} }
} }

View File

@ -216,6 +216,8 @@ func (suite *ConfigSuite) TestMustMatchConfig() {
s3Cfg := &storage.S3Config{Bucket: bkt} s3Cfg := &storage.S3Config{Bucket: bkt}
m365 := account.M365Config{AzureTenantID: tid} m365 := account.M365Config{AzureTenantID: tid}
m365PathKeys := []string{}
err = writeRepoConfigWithViper(vpr, s3Cfg, m365, repository.Options{}, "repoid") err = writeRepoConfigWithViper(vpr, s3Cfg, m365, repository.Options{}, "repoid")
require.NoError(t, err, "writing repo config", clues.ToCore(err)) require.NoError(t, err, "writing repo config", clues.ToCore(err))
@ -272,7 +274,7 @@ func (suite *ConfigSuite) TestMustMatchConfig() {
} }
for _, test := range table { for _, test := range table {
suite.Run(test.name, func() { suite.Run(test.name, func() {
test.errCheck(suite.T(), mustMatchConfig(vpr, test.input), clues.ToCore(err)) test.errCheck(suite.T(), mustMatchConfig(vpr, test.input, m365PathKeys), clues.ToCore(err))
}) })
} }
} }

View File

@ -52,6 +52,7 @@ package path
import ( import (
"fmt" "fmt"
"path/filepath"
"strings" "strings"
"github.com/alcionai/clues" "github.com/alcionai/clues"
@ -317,6 +318,16 @@ func Split(segment string) []string {
return res return res
} }
func ArePathsEquivalent(path1, path2 string) bool {
normalizedPath1 := strings.TrimSpace(filepath.Clean(path1))
normalizedPath2 := strings.TrimSpace(filepath.Clean(path2))
normalizedPath1 = strings.TrimSuffix(normalizedPath1, string(filepath.Separator))
normalizedPath2 = strings.TrimSuffix(normalizedPath2, string(filepath.Separator))
return normalizedPath1 == normalizedPath2
}
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
// Unexported Helpers // Unexported Helpers
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------

View File

@ -574,3 +574,74 @@ func (suite *PathUnitSuite) TestBuildRestorePaths() {
}) })
} }
} }
func (suite *PathUnitSuite) TestArePathsEquivalent() {
tests := []struct {
name string
path1 string
path2 string
equivalent bool
}{
// Test cases with equivalent paths
{
name: "same linux normal path",
path1: "/path/to/dir",
path2: "/path/to/dir",
equivalent: true,
},
{
name: "same windows normal path",
path1: "C:\\Users\\User\\Documents",
path2: "C:\\Users\\User\\Documents",
equivalent: true,
},
{
name: "same linux extra trailing slash path",
path1: "/path/with/trailing/slash/",
path2: "/path/with/trailing/slash",
equivalent: true,
},
{
name: "same linux relative path",
path1: "relative/path",
path2: "./relative/path",
equivalent: true,
},
// Test cases with non-equivalent paths
{
name: "different linux normal path",
path1: "/path/to/dir",
path2: "/path/to/different/dir",
equivalent: false,
},
{
name: "different windows normal path",
path1: "C:\\Users\\User\\Documents",
path2: "D:\\Users\\User\\Documents",
equivalent: false,
},
{
name: "different linux extra trailing slash path",
path1: "/path/with/trailing/slash/",
path2: "/different/path/with/trailing/slash",
equivalent: false,
},
{
name: "different linux relative path",
path1: "relative/path",
path2: "../relative/path",
equivalent: false,
},
}
for _, test := range tests {
t := suite.T()
suite.Run(test.name, func() {
result := ArePathsEquivalent(test.path1, test.path2)
if result != test.equivalent {
t.Errorf("Paths %s and %s are not equivalent as expected.", test.path1, test.path2)
}
})
}
}

View File

@ -1,10 +1,13 @@
package storage package storage
import ( import (
"strings"
"github.com/alcionai/clues" "github.com/alcionai/clues"
"github.com/spf13/cast" "github.com/spf13/cast"
"github.com/alcionai/corso/src/internal/common/str" "github.com/alcionai/corso/src/internal/common/str"
"github.com/alcionai/corso/src/pkg/path"
) )
const ( const (
@ -16,6 +19,9 @@ var fsConstToTomlKeyMap = map[string]string{
FilesystemPath: FilesystemPath, FilesystemPath: FilesystemPath,
} }
// add filesystem config key names that require path related validations
var fsPathKeys = []string{FilesystemPath}
type FilesystemConfig struct { type FilesystemConfig struct {
Path string Path string
} }
@ -77,13 +83,17 @@ func (c *FilesystemConfig) ApplyConfigOverrides(
} }
// This is matching override values from config file. // This is matching override values from config file.
if err := mustMatchConfig(g, fsConstToTomlKeyMap, fsOverrides(overrides)); err != nil { if err := mustMatchConfig(g, fsConstToTomlKeyMap, fsOverrides(overrides), fsPathKeys); err != nil {
return clues.Wrap(err, "verifying storage configs in corso config file") return clues.Wrap(err, "verifying storage configs in corso config file")
} }
} }
} }
c.Path = str.First(overrides[FilesystemPath], c.Path) sanitizePath := func(p string) string {
return path.TrimTrailingSlash(strings.TrimSpace(p))
}
c.Path = str.First(sanitizePath(overrides[FilesystemPath]), sanitizePath(c.Path))
return c.validate() return c.validate()
} }

View File

@ -62,6 +62,9 @@ var s3constToTomlKeyMap = map[string]string{
StorageProviderTypeKey: StorageProviderTypeKey, StorageProviderTypeKey: StorageProviderTypeKey,
} }
// add s3 config key names that require path related validations
var s3PathKeys = []string{}
func (s Storage) ToS3Config() (*S3Config, error) { func (s Storage) ToS3Config() (*S3Config, error) {
return buildS3ConfigFromMap(s.Config) return buildS3ConfigFromMap(s.Config)
} }
@ -176,7 +179,7 @@ func (c *S3Config) ApplyConfigOverrides(
return clues.New("unsupported storage provider: [" + providerType + "]") return clues.New("unsupported storage provider: [" + providerType + "]")
} }
if err := mustMatchConfig(kvg, s3constToTomlKeyMap, s3Overrides(overrides)); err != nil { if err := mustMatchConfig(kvg, s3constToTomlKeyMap, s3Overrides(overrides), s3PathKeys); err != nil {
return clues.Stack(err) return clues.Stack(err)
} }
} }

View File

@ -2,11 +2,13 @@ package storage
import ( import (
"fmt" "fmt"
"slices"
"github.com/alcionai/clues" "github.com/alcionai/clues"
"github.com/spf13/cast" "github.com/spf13/cast"
"github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/internal/common"
"github.com/alcionai/corso/src/pkg/path"
) )
var ErrVerifyingConfigStorage = clues.New("verifying configs in corso config file") var ErrVerifyingConfigStorage = clues.New("verifying configs in corso config file")
@ -156,6 +158,7 @@ func mustMatchConfig(
g Getter, g Getter,
tomlMap map[string]string, tomlMap map[string]string,
m map[string]string, m map[string]string,
pathKeys []string,
) error { ) error {
for k, v := range m { for k, v := range m {
if len(v) == 0 { if len(v) == 0 {
@ -168,7 +171,17 @@ func mustMatchConfig(
} }
vv := cast.ToString(g.Get(tomlK)) vv := cast.ToString(g.Get(tomlK))
if v != vv {
areEqual := false
// some of the values maybe paths, hence they require more than just string equality check
if len(pathKeys) > 0 && slices.Contains(pathKeys, k) {
areEqual = path.ArePathsEquivalent(v, vv)
} else {
areEqual = v == vv
}
if !areEqual {
err := clues.New("value of " + k + " (" + v + ") does not match corso configuration value (" + vv + ")") err := clues.New("value of " + k + " (" + v + ") does not match corso configuration value (" + vv + ")")
return clues.Stack(ErrVerifyingConfigStorage, err) return clues.Stack(ErrVerifyingConfigStorage, err)
} }

View File

@ -62,3 +62,131 @@ func (suite *StorageUnitSuite) TestNewStorage() {
}) })
} }
} }
type testGetter struct {
storeMap map[string]string
}
func (tg testGetter) Get(key string) any {
val, ok := tg.storeMap[key]
if ok {
return val
}
return ""
}
func (suite *StorageUnitSuite) TestMustMatchConfig() {
t := suite.T()
table := []struct {
name string
tomlMap map[string]string
overrideMap map[string]string
getterMap map[string]string
pathKeys []string
errorCheck assert.ErrorAssertionFunc
}{
{
name: "s3 config match",
tomlMap: s3constToTomlKeyMap,
overrideMap: map[string]string{
Bucket: "test-bucket",
Endpoint: "https://aws.s3",
Prefix: "test-prefix",
"additional-key": "additional-value",
},
getterMap: map[string]string{
"bucket": "test-bucket",
"endpoint": "https://aws.s3",
"prefix": "test-prefix",
},
errorCheck: assert.NoError,
},
{
name: "s3 config match - bucket mismatch",
tomlMap: s3constToTomlKeyMap,
overrideMap: map[string]string{
Bucket: "test-bucket",
Endpoint: "https://aws.s3",
Prefix: "test-prefix",
"additional-key": "additional-value",
},
getterMap: map[string]string{
"bucket": "test-bucket-new",
"endpoint": "https://aws.s3",
"prefix": "test-prefix",
},
errorCheck: assert.Error,
},
{
name: "s3 config match - endpoint mismatch",
tomlMap: s3constToTomlKeyMap,
overrideMap: map[string]string{
Bucket: "test-bucket",
Endpoint: "https://aws.s3",
Prefix: "test-prefix",
"additional-key": "additional-value",
},
getterMap: map[string]string{
"bucket": "test-bucket",
"endpoint": "https://aws.s3/new",
"prefix": "test-prefix",
},
errorCheck: assert.Error,
},
{
name: "s3 config match - prefix mismatch",
tomlMap: s3constToTomlKeyMap,
overrideMap: map[string]string{
Bucket: "test-bucket",
Endpoint: "https://aws.s3",
Prefix: "test-prefix",
"additional-key": "additional-value",
},
getterMap: map[string]string{
"bucket": "test-bucket",
"endpoint": "https://aws.s3",
"prefix": "test-prefix-new",
},
errorCheck: assert.Error,
},
{
name: "filesystem config match - success case",
tomlMap: fsConstToTomlKeyMap,
overrideMap: map[string]string{
StorageProviderTypeKey: "filesystem",
FilesystemPath: "/path/to/dir",
"additional-key": "additional-value",
},
getterMap: map[string]string{
"provider": "filesystem",
"path": "/path/to/dir",
},
pathKeys: []string{FilesystemPath},
errorCheck: assert.NoError,
},
{
name: "filesystem config match - path mismatch",
tomlMap: fsConstToTomlKeyMap,
overrideMap: map[string]string{
StorageProviderTypeKey: "filesystem",
FilesystemPath: "/path/to/dir",
"additional-key": "additional-value",
},
getterMap: map[string]string{
"provider": "filesystem",
"path": "/path/to/dir/new",
},
pathKeys: []string{FilesystemPath},
errorCheck: assert.Error,
},
}
for _, test := range table {
suite.Run(test.name, func() {
tg := testGetter{test.getterMap}
err := mustMatchConfig(tg, test.tomlMap, test.overrideMap, test.pathKeys)
test.errorCheck(t, err)
})
}
}