fix: file exporter path handling (#2498)

* fix: file exporter path handling * Change permission to the same level as the flags * Adding test when we have no output dir * back to 755 --------- Co-authored-by: Thomas Poignant <[email protected]>
thomaspoignant · Oct 12, 2024 · c1bab4f · c1bab4f
1 parent 2613710
commit c1bab4f
Show file tree

Hide file tree

Showing 4 changed files with 243 additions and 134 deletions.
diff --git a/exporter/fileexporter/exporter.go b/exporter/fileexporter/exporter.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"path/filepath"
 	"runtime"
 	"strings"
 	"sync"
@@ -23,7 +24,6 @@ type Exporter struct {
 	Format string
 
 	// OutputDir is the location of the directory where to store the exported files
-	// It should finish with a /
 	// Default: the current directory
 	OutputDir string
 
@@ -72,7 +72,19 @@ func (f *Exporter) Export(_ context.Context, _ *fflog.FFLogger, featureEvents []
 		return err
 	}
 
-	filePath := f.OutputDir + "/" + filename
+	// Handle empty OutputDir and remove trailing slash
+	outputDir := strings.TrimRight(f.OutputDir, "/")
+
+	var filePath string
+	if outputDir == "" {
+		filePath = filename
+	} else {
+		// Ensure OutputDir exists or create it
+		if err := os.MkdirAll(outputDir, 0755); err != nil {
+			return fmt.Errorf("failed to create output directory: %v", err)
+		}
+		filePath = filepath.Join(outputDir, filename)
+	}
 
 	if f.Format == "parquet" {
 		return f.writeParquet(filePath, featureEvents)

diff --git a/exporter/fileexporter/exporter_test.go b/exporter/fileexporter/exporter_test.go
@@ -3,10 +3,13 @@ package fileexporter_test
 import (
 	"context"
 	"os"
+	"path/filepath"
 	"runtime"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"github.com/thomaspoignant/go-feature-flag/exporter"
 	"github.com/thomaspoignant/go-feature-flag/exporter/fileexporter"
 	"github.com/thomaspoignant/go-feature-flag/utils/fflog"
@@ -16,6 +19,13 @@ import (
 )
 
 func TestFile_Export(t *testing.T) {
+	// Create a temporary directory for test file operations
+	tempDir, err := os.MkdirTemp("", "fileexporter-test")
+	if err != nil {
+		t.Fatalf("Failed to create temp dir: %v", err)
+	}
+	defer os.RemoveAll(tempDir) // Clean up after tests
+
 	hostname, _ := os.Hostname()
 	type fields struct {
 		Format                  string
@@ -39,6 +49,8 @@ func TestFile_Export(t *testing.T) {
 		args     args
 		wantErr  bool
 		expected expected
+		setup    func(t *testing.T, dir string)
+		teardown func(t *testing.T, dir string)
 	}{
 		{
 			name:    "all default json",
@@ -233,10 +245,10 @@ func TestFile_Export(t *testing.T) {
 			},
 		},
 		{
-			name:    "invalid outputdir",
-			wantErr: true,
+			name:    "non-existent outputdir",
+			wantErr: false,
 			fields: fields{
-				OutputDir: "/tmp/foo/bar/",
+				OutputDir: filepath.Join(tempDir, "non-existent-dir"),
 			},
 			args: args{
 				featureEvents: []exporter.FeatureEvent{
@@ -246,10 +258,14 @@ func TestFile_Export(t *testing.T) {
 					},
 					{
 						Kind: "feature", ContextKind: "anonymousUser", UserKey: "EFGH", CreationDate: 1617970701, Key: "random-key",
-						Variation: "Default", Value: "YO2", Default: false, Source: "SERVER",
+						Variation: "Default", Value: "YO2", Default: false, Version: "127", Source: "SERVER",
 					},
 				},
 			},
+			expected: expected{
+				fileNameRegex: "^flag-variation-" + hostname + "-[0-9]*\\.json$",
+				content:       "./testdata/all_default.json",
+			},
 		},
 		{
 			name:    "invalid filename template",
@@ -291,13 +307,54 @@ func TestFile_Export(t *testing.T) {
 			},
 		},
 		{
-			name:    "invalid parquet outputdir",
+			name:    "outputdir with invalid permissions",
 			wantErr: true,
 			fields: fields{
 				Format:    "parquet",
-				OutputDir: "/tmp/foo/bar/",
+				OutputDir: filepath.Join(tempDir, "invalid-permissions-dir"),
+			},
+			args: args{
+				featureEvents: []exporter.FeatureEvent{
+					{
+						Kind: "feature", ContextKind: "anonymousUser", UserKey: "ABCD", CreationDate: 1617970547, Key: "random-key",
+						Variation: "Default", Value: "YO", Default: false, Source: "SERVER",
+					},
+				},
+			},
+			setup: func(t *testing.T, dir string) {
+				err := os.MkdirAll(dir, 0755)
+				assert.NoError(t, err)
+				err = os.Chmod(dir, 0000) // Remove all permissions
+				assert.NoError(t, err)
+			},
+			teardown: func(t *testing.T, dir string) {
+				err := os.Chmod(dir, 0755) // Restore permissions for cleanup
+				assert.NoError(t, err)
+			},
+		},
+		{
+			name:    "outputdir with trailing slash",
+			wantErr: false,
+			fields: fields{
+				Format:    "json",
+				OutputDir: filepath.Join(tempDir, "dir-with-trailing-slash") + "/",
+			},
+			args: args{
+				featureEvents: []exporter.FeatureEvent{
+					{
+						Kind: "feature", ContextKind: "anonymousUser", UserKey: "ABCD", CreationDate: 1617970547, Key: "random-key",
+						Variation: "Default", Value: "YO", Default: false, Source: "SERVER",
+					},
+					{
+						Kind: "feature", ContextKind: "anonymousUser", UserKey: "EFGH", CreationDate: 1617970701, Key: "random-key",
+						Variation: "Default", Value: "YO2", Default: false, Version: "127", Source: "SERVER",
+					},
+				},
+			},
+			expected: expected{
+				fileNameRegex: "^flag-variation-" + hostname + "-[0-9]*\\.json$",
+				content:       "./testdata/all_default.json",
 			},
-			args: args{},
 		},
 	}
 	for _, tt := range tests {
@@ -308,6 +365,14 @@ func TestFile_Export(t *testing.T) {
 				defer os.Remove(outputDir)
 			}
 
+			if tt.setup != nil {
+				tt.setup(t, outputDir)
+			}
+
+			if tt.teardown != nil {
+				defer tt.teardown(t, outputDir)
+			}
+
 			f := &fileexporter.Exporter{
 				Format:                  tt.fields.Format,
 				OutputDir:               outputDir,
@@ -321,6 +386,12 @@ func TestFile_Export(t *testing.T) {
 				return
 			}
 
+			assert.NoError(t, err)
+
+			// Check if the directory was created
+			_, err = os.Stat(outputDir)
+			assert.NoError(t, err, "Output directory should exist")
+
 			files, _ := os.ReadDir(outputDir)
 			assert.Equal(t, 1, len(files), "Directory %s should have only one file", outputDir)
 			assert.Regexp(t, tt.expected.fileNameRegex, files[0].Name(), "Invalid file name")
@@ -350,3 +421,32 @@ func TestFile_IsBulk(t *testing.T) {
 	exporter := fileexporter.Exporter{}
 	assert.True(t, exporter.IsBulk(), "DeprecatedExporter is a bulk exporter")
 }
+
+func TestExportWithoutOutputDir(t *testing.T) {
+	featureEvents := []exporter.FeatureEvent{{
+		Kind: "feature", ContextKind: "anonymousUser", UserKey: "ABCD", CreationDate: 1617970547, Key: "random-key",
+		Variation: "Default", Value: "YO", Default: false, Source: "SERVER",
+	}}
+
+	filePrefix := "test-flag-variation-EXAMPLE-"
+	e := fileexporter.Exporter{
+		Format:   "json",
+		Filename: filePrefix + "{{ .Timestamp}}.{{ .Format}}",
+	}
+	err := e.Export(context.Background(), nil, featureEvents)
+	require.NoError(t, err)
+
+	// check that a file exist
+	files, err := os.ReadDir("./")
+	require.NoError(t, err)
+
+	countFileWithPrefix := 0
+	for _, file := range files {
+		if strings.HasPrefix(file.Name(), filePrefix) {
+			countFileWithPrefix++
+			err := os.Remove(file.Name())
+			require.NoError(t, err)
+		}
+	}
+	assert.True(t, countFileWithPrefix > 0, "At least one file should have been created")
+}
diff --git a/website/docs/go_module/data_collection/file.md b/website/docs/go_module/data_collection/file.md
@@ -29,7 +29,7 @@ ffclient.Config{
 
 | Field  | Description  |
 |---|---|
-|`OutputDir`   | OutputDir is the location of the directory to store the exported files.<br/>It should finish with a `/`.  |
+|`OutputDir`   | OutputDir is the location of the directory to store the exported files.  |
 |`Format`   |   _(Optional)_ Format is the output format you want in your exported file.<br/>Available format: **`JSON`**, **`CSV`**, **`Parquet`**.<br/>**Default: `JSON`** |
 |`Filename`   | _(Optional)_ Filename is the name of your output file.<br/>You can use a templated config to define the name of your exported files.<br/>Available replacements are `{{ .Hostname}}`, `{{ .Timestamp}}` and `{{ .Format}}`<br/>**Default: `flag-variation-{{ .Hostname}}-{{ .Timestamp}}.{{ .Format}}`**|
 |`CsvTemplate`   | _(Optional)_ CsvTemplate is used if your output format is CSV.<br/>This field will be ignored if you are using format other than CSV.<br/>You can decide which fields you want in your CSV line with a go-template syntax, please check [internal/exporter/feature_event.go](https://github.com/thomaspoignant/go-feature-flag/blob/main/internal/exporter/feature_event.go) to see the available fields.<br/>**Default:** `{{ .Kind}};{{ .ContextKind}};{{ .UserKey}};{{ .CreationDate}};{{ .Key}};{{ .Variation}};{{ .Value}};{{ .Default}}\n` |