From 0c1ef1978d99be68a5d2e1313c9223deb62db36e Mon Sep 17 00:00:00 2001 From: Charlie Egan Date: Tue, 10 Sep 2024 14:31:48 +0100 Subject: [PATCH] lsp: Rename on template (#1085) * lsp: Rename on template Fixes https://github.com/StyraInc/regal/issues/1061 Signed-off-by: Charlie Egan * lsp: template edits for rename in one operation Signed-off-by: Charlie Egan * lsp: Include deleting dirs as part of wksp changes Signed-off-by: Charlie Egan * lsp: templating add docs Signed-off-by: Charlie Egan --------- Signed-off-by: Charlie Egan --- .../idiomatic/directory-package-mismatch.md | 12 +- internal/lsp/server.go | 105 ++++-- internal/lsp/server_template_test.go | 322 ++++++++++++++++-- internal/lsp/types/types.go | 11 + internal/util/util.go | 86 +++++ internal/util/util_test.go | 101 +++++- pkg/fixer/fixes/directorypackagemismatch.go | 4 + 7 files changed, 594 insertions(+), 47 deletions(-) diff --git a/docs/rules/idiomatic/directory-package-mismatch.md b/docs/rules/idiomatic/directory-package-mismatch.md index 4c17d81a..46c3b0a9 100644 --- a/docs/rules/idiomatic/directory-package-mismatch.md +++ b/docs/rules/idiomatic/directory-package-mismatch.md @@ -91,11 +91,17 @@ Editors integrating Regal's [language server](https://docs.styra.com/regal/langu suggestions for idiomatic package paths based on the directory structure in which a policy resides. The image below demonstrates a new policy being created inside an `authorization/rbac/roles` directory, and the editor ([via Regal](https://docs.styra.com/regal/language-server#code-completions)) suggesting the package path - `authorization.rbac.roles`. +`authorization.rbac.roles`. Package path auto-completion in VS Code +src={require('../../assets/rules/pkg_name_completion.png').default} +alt="Package path auto-completion in VS Code"/> + +In addition, empty files will be be 'formatted' to have the correct package +based on the directory structure. Newly created Rego files are treated in much +the same way. When a new file is created, the server will send a series of edits +back to set the content. If `exclude-test-suffix` is set to `false`, the file +will also be moved if required to the `_test` directory for that package. ## Configuration Options diff --git a/internal/lsp/server.go b/internal/lsp/server.go index 858be886..febb77dd 100644 --- a/internal/lsp/server.go +++ b/internal/lsp/server.go @@ -478,10 +478,7 @@ func (l *LanguageServer) StartCommandWorker(ctx context.Context) { // nolint:mai break } - // handle this ourselves as it's a rename and not a content edit - fixed = false - - err = l.conn.Call(ctx, methodWorkspaceApplyEdit, renameParams, nil) + err := l.conn.Call(ctx, methodWorkspaceApplyEdit, renameParams, nil) if err != nil { l.logError(fmt.Errorf("failed %s notify: %v", methodWorkspaceApplyEdit, err.Error())) } @@ -496,6 +493,8 @@ func (l *LanguageServer) StartCommandWorker(ctx context.Context) { // nolint:mai } } + // handle this ourselves as it's a rename and not a content edit + fixed = false case "regal.eval": if len(params.Arguments) != 3 { l.logError(fmt.Errorf("expected three arguments, got %d", len(params.Arguments))) @@ -729,6 +728,7 @@ func (l *LanguageServer) StartTemplateWorker(ctx context.Context) { case <-ctx.Done(): return case evt := <-l.templateFile: + // determine the new contents for the file, if permitted newContents, err := l.templateContentsForFile(evt.URI) if err != nil { l.logError(fmt.Errorf("failed to template new file: %w", err)) @@ -736,29 +736,85 @@ func (l *LanguageServer) StartTemplateWorker(ctx context.Context) { continue } - // generate the edit params for the templating operation - templateParams := &types.ApplyWorkspaceEditParams{ - Label: "Template new Rego file", - Edit: types.WorkspaceEdit{ - DocumentChanges: []types.TextDocumentEdit{ - { - TextDocument: types.OptionalVersionedTextDocumentIdentifier{URI: evt.URI}, - Edits: ComputeEdits("", newContents), - }, + // set the contents of the new file in the cache immediately as + // these must be update to date in order for fixRenameParams + // to work + l.cache.SetFileContents(evt.URI, newContents) + + var edits []any + + edits = append(edits, types.TextDocumentEdit{ + TextDocument: types.OptionalVersionedTextDocumentIdentifier{URI: evt.URI}, + Edits: ComputeEdits("", newContents), + }) + + // determine if a rename is needed based on the new file contents + renameParams, err := l.fixRenameParams( + "Rename file to match package path", + &fixes.DirectoryPackageMismatch{}, + evt.URI, + ) + if err != nil { + l.logError(fmt.Errorf("failed to fix directory package mismatch: %w", err)) + + continue + } + + // move the file and clean up any empty directories ifd required + fileURI := evt.URI + + if len(renameParams.Edit.DocumentChanges) > 0 { + edits = append(edits, renameParams.Edit.DocumentChanges[0]) + } + + // check if there are any dirs to clean + if len(renameParams.Edit.DocumentChanges) > 0 { + dirs, err := util.DirCleanUpPaths( + uri.ToPath(l.clientIdentifier, renameParams.Edit.DocumentChanges[0].OldURI), + []string{ + // stop at the root + l.workspacePath(), + // also preserve any dirs needed for the new file + uri.ToPath(l.clientIdentifier, renameParams.Edit.DocumentChanges[0].NewURI), }, - }, + ) + if err != nil { + l.logError(fmt.Errorf("failed to delete empty directories: %w", err)) + + continue + } + + for _, dir := range dirs { + edits = append( + edits, + types.DeleteFile{ + Kind: "delete", + URI: uri.FromPath(l.clientIdentifier, dir), + Options: &types.DeleteFileOptions{ + Recursive: true, + IgnoreIfNotExists: true, + }, + }, + ) + } + + l.cache.Delete(renameParams.Edit.DocumentChanges[0].OldURI) } - err = l.conn.Call(ctx, methodWorkspaceApplyEdit, templateParams, nil) + err = l.conn.Call(ctx, methodWorkspaceApplyEdit, map[string]any{ + "label": "Template new Rego file", + "edit": map[string]any{ + "documentChanges": edits, + }, + }, nil) if err != nil { l.logError(fmt.Errorf("failed %s notify: %v", methodWorkspaceApplyEdit, err.Error())) } - // finally, update the cache contents and run diagnostics to clear - // empty module warning. + // finally, trigger a diagnostics run for the new file updateEvent := fileUpdateEvent{ Reason: "internal/templateNewFile", - URI: evt.URI, + URI: fileURI, Content: newContents, } @@ -835,6 +891,10 @@ func (l *LanguageServer) templateContentsForFile(fileURI string) (string, error) pkg = "main" } + if strings.HasSuffix(fileURI, "_test.rego") { + pkg += "_test" + } + return fmt.Sprintf("package %s\n\nimport rego.v1\n", pkg), nil } @@ -898,7 +958,7 @@ func (l *LanguageServer) fixRenameParams( ) (types.ApplyWorkspaceRenameEditParams, error) { contents, ok := l.cache.GetFileContents(fileURL) if !ok { - return types.ApplyWorkspaceRenameEditParams{}, fmt.Errorf("failed to get module for file %q", fileURL) + return types.ApplyWorkspaceRenameEditParams{}, fmt.Errorf("failed to file contents %q", fileURL) } roots, err := config.GetPotentialRoots(l.workspacePath()) @@ -923,6 +983,13 @@ func (l *LanguageServer) fixRenameParams( return types.ApplyWorkspaceRenameEditParams{}, fmt.Errorf("failed attempted fix: %w", err) } + if len(results) == 0 { + return types.ApplyWorkspaceRenameEditParams{ + Label: label, + Edit: types.WorkspaceRenameEdit{}, + }, nil + } + result := results[0] renameEdit := types.WorkspaceRenameEdit{ diff --git a/internal/lsp/server_template_test.go b/internal/lsp/server_template_test.go index 8e60c294..b8921c41 100644 --- a/internal/lsp/server_template_test.go +++ b/internal/lsp/server_template_test.go @@ -1,58 +1,332 @@ package lsp import ( + "context" + "encoding/json" + "fmt" "os" "path/filepath" + "strings" "testing" + "time" + + "github.com/sourcegraph/jsonrpc2" "github.com/styrainc/regal/internal/lsp/clients" + "github.com/styrainc/regal/internal/lsp/types" "github.com/styrainc/regal/internal/lsp/uri" ) -func TestServerTemplateContentsForFile(t *testing.T) { +func TestTemplateContentsForFile(t *testing.T) { t.Parallel() - s := NewLanguageServer( - &LanguageServerOptions{ - ErrorLog: os.Stderr, + testCases := map[string]struct { + FileKey string + CacheFileContents string + DiskContents map[string]string + RequireConfig bool + ExpectedContents string + ExpectedError string + }{ + "existing contents in file": { + FileKey: "foo/bar.rego", + CacheFileContents: "package foo", + ExpectedError: "file already has contents", + }, + "existing contents on disk": { + FileKey: "foo/bar.rego", + CacheFileContents: "", + DiskContents: map[string]string{ + "foo/bar.rego": "package foo", + }, + ExpectedError: "file on disk already has contents", + }, + "empty file is templated as main when no root": { + FileKey: "foo/bar.rego", + CacheFileContents: "", + DiskContents: map[string]string{ + "foo/bar.rego": "", + }, + ExpectedContents: "package main\n\nimport rego.v1\n", + }, + "empty file is templated based on root": { + FileKey: "foo/bar.rego", + CacheFileContents: "", + DiskContents: map[string]string{ + "foo/bar.rego": "", + ".regal/config.yaml": "", + }, + ExpectedContents: "package foo\n\nimport rego.v1\n", }, - ) + "empty test file is templated based on root": { + FileKey: "foo/bar_test.rego", + CacheFileContents: "", + DiskContents: map[string]string{ + "foo/bar_test.rego": "", + ".regal/config.yaml": "", + }, + RequireConfig: true, + ExpectedContents: "package foo_test\n\nimport rego.v1\n", + }, + "empty deeply nested file is templated based on root": { + FileKey: "foo/bar/baz/bax.rego", + CacheFileContents: "", + DiskContents: map[string]string{ + "foo/bar/baz/bax.rego": "", + ".regal/config.yaml": "", + }, + ExpectedContents: "package foo.bar.baz\n\nimport rego.v1\n", + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + td := t.TempDir() + + // init the state on disk + for f, c := range tc.DiskContents { + dir := filepath.Dir(f) + + err := os.MkdirAll(filepath.Join(td, dir), 0o755) + if err != nil { + t.Fatalf("failed to create directory %s: %s", dir, err) + } + + err = os.WriteFile(filepath.Join(td, f), []byte(c), 0o600) + if err != nil { + t.Fatalf("failed to write file %s: %s", f, err) + } + } + + // create a new language server + s := NewLanguageServer(&LanguageServerOptions{ErrorLog: newTestLogger(t)}) + s.workspaceRootURI = uri.FromPath(clients.IdentifierGeneric, td) + + fileURI := uri.FromPath(clients.IdentifierGeneric, filepath.Join(td, tc.FileKey)) + + s.cache.SetFileContents(fileURI, tc.CacheFileContents) + + newContents, err := s.templateContentsForFile(fileURI) + if tc.ExpectedError != "" { + if !strings.Contains(err.Error(), tc.ExpectedError) { + t.Fatalf("expected error to contain %q, got %q", tc.ExpectedError, err) + } + } else if err != nil { + t.Fatalf("unexpected error: %s", err) + } - td := t.TempDir() + if newContents != tc.ExpectedContents { + t.Fatalf("expected contents to be\n%s\ngot\n%s", tc.ExpectedContents, newContents) + } + }) + } +} + +func TestNewFileTemplating(t *testing.T) { + t.Parallel() - filePath := filepath.Join(td, "foo/bar/baz.rego") - regalPath := filepath.Join(td, ".regal/config.yaml") + var err error - initialState := map[string]string{ - filePath: "", - regalPath: "", + // set up the workspace content with some example rego and regal config + tempDir := t.TempDir() + + files := map[string]string{ + ".regal/config.yaml": `rules: + idiomatic: + directory-package-mismatch: + level: error + exclude-test-suffix: false +`, } - // create the initial state needed for the regal config root detection - for file := range initialState { - fileDir := filepath.Dir(file) + for f, fc := range files { + err := os.MkdirAll(filepath.Dir(filepath.Join(tempDir, f)), 0o755) + if err != nil { + t.Fatalf("failed to create directory %s: %s", filepath.Dir(filepath.Join(tempDir, f)), err) + } - err := os.MkdirAll(fileDir, 0o755) + err = os.WriteFile(filepath.Join(tempDir, f), []byte(fc), 0o600) if err != nil { - t.Fatalf("unexpected error: %v", err) + t.Fatalf("failed to write file %s: %s", f, err) } + } - err = os.WriteFile(file, []byte(""), 0o600) + // set up the server and client connections + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ls := NewLanguageServer(&LanguageServerOptions{ + ErrorLog: newTestLogger(t), + }) + + go ls.StartConfigWorker(ctx) + go ls.StartTemplateWorker(ctx) + + receivedMessages := make(chan []byte, 10) + clientHandler := func(_ context.Context, _ *jsonrpc2.Conn, req *jsonrpc2.Request) (result any, err error) { + bs, err := json.MarshalIndent(req.Params, "", " ") if err != nil { - t.Fatalf("unexpected error: %v", err) + t.Fatalf("failed to marshal params: %s", err) } + + receivedMessages <- bs + + return struct{}{}, nil } - fileURI := uri.FromPath(clients.IdentifierGeneric, filePath) + connServer, connClient, cleanup := createConnections(ctx, ls.Handle, clientHandler) + defer cleanup() + + ls.SetConn(connServer) + + // 1. Client sends initialize request + request := types.InitializeParams{ + RootURI: fileURIScheme + tempDir, + ClientInfo: types.Client{Name: "go test"}, + } - s.cache.SetFileContents(fileURI, "") + var response types.InitializeResult - newContents, err := s.templateContentsForFile(fileURI) + err = connClient.Call(ctx, "initialize", request, &response) if err != nil { - t.Fatalf("unexpected error: %v", err) + t.Fatalf("failed to send initialize request: %s", err) } - if newContents != "package foo.bar\n\nimport rego.v1\n" { - t.Fatalf("unexpected contents: %v", newContents) + // 2. Client sends initialized notification no response to the call is + // expected + err = connClient.Call(ctx, "initialized", struct{}{}, nil) + if err != nil { + t.Fatalf("failed to send initialized notification: %s", err) + } + + // 3. wait for the server to load it's config + timeout := time.NewTimer(defaultTimeout) + select { + case <-timeout.C: + t.Fatalf("timed out waiting for server to load config") + default: + for { + time.Sleep(100 * time.Millisecond) + + if ls.loadedConfig != nil { + break + } + } + } + + // 4. Touch the new file on disk + newFilePath := filepath.Join(tempDir, "foo/bar/policy_test.rego") + newFileURI := uri.FromPath(clients.IdentifierGeneric, newFilePath) + expectedNewFileURI := uri.FromPath(clients.IdentifierGeneric, filepath.Join(tempDir, "foo/bar_test/policy_test.rego")) + + err = os.MkdirAll(filepath.Dir(newFilePath), 0o755) + if err != nil { + t.Fatalf("failed to create directory %s: %s", filepath.Dir(newFilePath), err) + } + + err = os.WriteFile(newFilePath, []byte(""), 0o600) + if err != nil { + t.Fatalf("failed to write file %s: %s", newFilePath, err) + } + + // 5. Client sends workspace/didCreateFiles notification + err = connClient.Call(ctx, "workspace/didCreateFiles", types.WorkspaceDidCreateFilesParams{ + Files: []types.WorkspaceDidCreateFilesParamsCreatedFile{ + {URI: newFileURI}, + }, + }, nil) + if err != nil { + t.Fatalf("failed to send didChange notification: %s", err) + } + + // 6. Validate that the client received a workspace edit + timeout = time.NewTimer(3 * time.Second) + defer timeout.Stop() + + expectedMessage := fmt.Sprintf(`{ + "edit": { + "documentChanges": [ + { + "edits": [ + { + "newText": "package foo.bar_test\n\nimport rego.v1\n", + "range": { + "end": { + "character": 0, + "line": 0 + }, + "start": { + "character": 0, + "line": 0 + } + } + } + ], + "textDocument": { + "uri": "%[1]s", + "version": null + } + }, + { + "kind": "rename", + "newUri": "%[2]s", + "oldUri": "%[1]s", + "options": { + "ignoreIfExists": false, + "overwrite": false + } + }, + { + "kind": "delete", + "options": { + "ignoreIfNotExists": true, + "recursive": true + }, + "uri": "file://%[3]s/foo/bar" + } + ] + }, + "label": "Template new Rego file" +}`, newFileURI, expectedNewFileURI, tempDir) + + for { + var success bool + select { + case msg := <-receivedMessages: + t.Log("received message:", string(msg)) + + expectedLines := strings.Split(expectedMessage, "\n") + gotLines := strings.Split(string(msg), "\n") + + if len(gotLines) != len(expectedLines) { + t.Logf("expected message to have %d lines, got %d", len(expectedLines), len(gotLines)) + + continue + } + + allLinesMatch := true + + for i, expected := range expectedLines { + if gotLines[i] != expected { + t.Logf("expected message line %d to be:\n%s\ngot\n%s", i, expected, gotLines[i]) + + allLinesMatch = false + } + } + + if allLinesMatch { + success = true + } + case <-timeout.C: + t.Log("never received expected message", expectedMessage) + + t.Fatalf("timed out waiting for expected message to be sent") + } + + if success { + break + } } } diff --git a/internal/lsp/types/types.go b/internal/lsp/types/types.go index 1087a032..c96b86c9 100644 --- a/internal/lsp/types/types.go +++ b/internal/lsp/types/types.go @@ -243,6 +243,17 @@ type RenameFile struct { AnnotationIdentifier *string `json:"annotationId,omitempty"` } +type DeleteFileOptions struct { + Recursive bool `json:"recursive"` + IgnoreIfNotExists bool `json:"ignoreIfNotExists"` +} + +type DeleteFile struct { + Kind string `json:"kind"` // must always be "delete" + URI string `json:"uri"` + Options *DeleteFileOptions `json:"options,omitempty"` +} + // WorkspaceRenameEdit is a WorkspaceEdit that is used for renaming files. // Perhaps we should use generics and a union type here instead. type WorkspaceRenameEdit struct { diff --git a/internal/util/util.go b/internal/util/util.go index c5e0d11f..e0e7cd8c 100644 --- a/internal/util/util.go +++ b/internal/util/util.go @@ -159,3 +159,89 @@ func DeleteEmptyDirs(dir string) error { return nil } + +// DirCleanUpPaths will, for a given target file, list all the dirs that would +// be empty if the target file was deleted. +func DirCleanUpPaths(target string, preserve []string) ([]string, error) { + dirs := make([]string, 0) + + preserveDirs := make(map[string]struct{}) + + for _, p := range preserve { + for { + preserveDirs[p] = struct{}{} + + p = filepath.Dir(p) + + if p == "." || p == "/" { + break + } + + if _, ok := preserveDirs[p]; ok { + break + } + } + } + + dir := filepath.Dir(target) + + for { + // check if we reached the preserved dir + _, ok := preserveDirs[dir] + if ok { + break + } + + parts := strings.Split(dir, string(filepath.Separator)) + if len(parts) == 1 { + break + } + + info, err := os.Stat(dir) + if err != nil { + return nil, fmt.Errorf("failed to stat directory %s: %w", dir, err) + } + + if !info.IsDir() { + return nil, fmt.Errorf("expected directory, got file %s", dir) + } + + files, err := os.ReadDir(dir) + if err != nil { + return nil, fmt.Errorf("failed to read directory %s: %w", dir, err) + } + + empty := true + + for _, file := range files { + // exclude the target + abs := filepath.Join(dir, file.Name()) + if abs == target { + continue + } + + // exclude any other marked dirs + if file.IsDir() && len(dirs) > 0 { + if dirs[len(dirs)-1] == abs { + continue + } + } + + empty = false + + break + } + + if !empty { + break + } + + dirs = append(dirs, dir) + + fmt.Fprintln(os.Stderr, "added", dir) + + dir = filepath.Dir(dir) + } + + return dirs, nil +} diff --git a/internal/util/util_test.go b/internal/util/util_test.go index 66ebbea3..37decfdb 100644 --- a/internal/util/util_test.go +++ b/internal/util/util_test.go @@ -1,6 +1,12 @@ package util -import "testing" +import ( + "os" + "path/filepath" + "slices" + "strings" + "testing" +) func TestFindClosestMatchingRoot(t *testing.T) { t.Parallel() @@ -48,3 +54,96 @@ func TestFindClosestMatchingRoot(t *testing.T) { }) } } + +func TestDirCleanUpPaths(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + State map[string]string + DeleteTarget string + AdditionalPreserveTargets []string + Expected []string + }{ + "simple": { + DeleteTarget: "foo/bar.rego", + State: map[string]string{ + "foo/bar.rego": "package foo", + }, + Expected: []string{"foo"}, + }, + "not empty": { + DeleteTarget: "foo/bar.rego", + State: map[string]string{ + "foo/bar.rego": "package foo", + "foo/baz.rego": "package foo", + }, + Expected: []string{}, + }, + "all the way up": { + DeleteTarget: "foo/bar/baz/bax.rego", + State: map[string]string{ + "foo/bar/baz/bax.rego": "package baz", + }, + Expected: []string{"foo/bar/baz", "foo/bar", "foo"}, + }, + "almost all the way up": { + DeleteTarget: "foo/bar/baz/bax.rego", + State: map[string]string{ + "foo/bar/baz/bax.rego": "package baz", + "foo/bax.rego": "package foo", + }, + Expected: []string{"foo/bar/baz", "foo/bar"}, + }, + "with preserve targets": { + DeleteTarget: "foo/bar/baz/bax.rego", + AdditionalPreserveTargets: []string{ + "foo/bar/baz_test/bax.rego", + }, + State: map[string]string{ + "foo/bar/baz/bax.rego": "package baz", + "foo/bax.rego": "package foo", + }, + // foo/bar is not deleted because of the preserve target + Expected: []string{"foo/bar/baz"}, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + tempDir := t.TempDir() + + for k, v := range test.State { + err := os.MkdirAll(filepath.Dir(filepath.Join(tempDir, k)), 0o755) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + err = os.WriteFile(filepath.Join(tempDir, k), []byte(v), 0o600) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + } + + expected := make([]string, len(test.Expected)) + for i, v := range test.Expected { + expected[i] = filepath.Join(tempDir, v) + } + + additionalPreserveTargets := []string{tempDir} + for i, v := range test.AdditionalPreserveTargets { + additionalPreserveTargets[i] = filepath.Join(tempDir, v) + } + + got, err := DirCleanUpPaths(filepath.Join(tempDir, test.DeleteTarget), additionalPreserveTargets) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !slices.Equal(got, expected) { + t.Fatalf("expected\n%v\ngot:\n%v", strings.Join(expected, "\n"), strings.Join(got, "\n")) + } + }) + } +} diff --git a/pkg/fixer/fixes/directorypackagemismatch.go b/pkg/fixer/fixes/directorypackagemismatch.go index fd6069bd..61935bf4 100644 --- a/pkg/fixer/fixes/directorypackagemismatch.go +++ b/pkg/fixer/fixes/directorypackagemismatch.go @@ -82,6 +82,10 @@ func getPackagePathDirectory(fc *FixCandidate, config *config.Config) (string, e } func shouldExcludeTestSuffix(config *config.Config) bool { + if config == nil { + return true + } + if category, ok := config.Rules["idiomatic"]; ok { if rule, ok := category["directory-package-mismatch"]; ok { if exclude, ok := rule.Extra["exclude-test-suffix"].(bool); ok {