Skip to content

Commit

Permalink
Prevent code actions on read only sources.
Browse files Browse the repository at this point in the history 
Code actions cannot be performed on read only sources, so filter them
out of proposals.
  • Loading branch information
@andrewL-avlq @mickaelistria
andrewL-avlq authored and mickaelistria committed Mar 30, 2023
1 parent 12464f8 commit e49962d
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 10 deletions.
47 changes: 40 additions & 7 deletions org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/codeactions/CodeActionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.eclipse.core.resources.IMarker;
import org.eclipse.core.resources.IProject;
import org.eclipse.core.resources.IResource;
import org.eclipse.core.resources.ResourceAttributes;
import org.eclipse.core.runtime.CoreException;
import org.eclipse.core.runtime.NullProgressMonitor;
import org.eclipse.lsp4e.LanguageServerPlugin;
Expand Down Expand Up @@ -172,6 +173,33 @@ public void testCodeActionLiteralWorkspaceEdit() throws CoreException {
}
}

@Test
public void testNoCodeActionOnReadOnlySource() throws CoreException {
IProject p = TestUtils.createProject(getClass().getSimpleName() + System.currentTimeMillis());
IFile f = TestUtils.createUniqueTestFile(p, "error");
f.setResourceAttributes(new ResourceAttributes() {
@Override
public boolean isReadOnly() {
return true;
}
});

TextEdit tEdit = new TextEdit(new Range(new Position(0, 0), new Position(0, 5)), "fixed");
WorkspaceEdit wEdit = new WorkspaceEdit(Collections.singletonMap(f.getLocationURI().toString(), Collections.singletonList(tEdit)));
CodeAction codeAction = new CodeAction("fixme");
codeAction.setEdit(wEdit);
MockLanguageServer.INSTANCE.setCodeActions(Collections.singletonList(Either.forRight(codeAction)));
MockLanguageServer.INSTANCE.setDiagnostics(Collections.singletonList(
new Diagnostic(new Range(new Position(0, 0), new Position(0, 5)), "error", DiagnosticSeverity.Error, null)));
AbstractTextEditor editor = (AbstractTextEditor)TestUtils.openEditor(f);
try {
assertDiagnostics(f, "error", "fixme", false);
} finally {
editor.close(false);
p.delete(true, new NullProgressMonitor());
}
}

@Test
public void testCodeActionLiteralWithClientCommand() throws CoreException {
IProject p = TestUtils.createProject(getClass().getSimpleName() + System.currentTimeMillis());
Expand Down Expand Up @@ -200,7 +228,7 @@ public void testCodeActionWorkspaceEditlWithDifferentURI() throws CoreException
IFile sourceFile = TestUtils.createUniqueTestFile(p, "error");
IFile targetFile = TestUtils.createUniqueTestFile(p, "fixme");

// create a diagnostic on the sourceFile with a code action
// create a diagnostic on the sourceFile with a code action
// that changes the targetFile
TextEdit tEdit = new TextEdit(new Range(new Position(0, 0), new Position(0, 5)), "fixed");
WorkspaceEdit wEdit = new WorkspaceEdit(Collections.singletonMap(targetFile.getLocationURI().toString(), Collections.singletonList(tEdit)));
Expand All @@ -214,10 +242,10 @@ public void testCodeActionWorkspaceEditlWithDifferentURI() throws CoreException
AbstractTextEditor activeEditor = null;
try {
IMarker m = assertDiagnostics(sourceFile, "error", "fixme");

// Apply and check the resolution
assertResolution(targetFile, m, "fixed");

// Double check that the editor is opened and active for the targetFile
// as result of the resolution
IEditorPart activeEditorPart = TestUtils.getActiveEditor();
Expand All @@ -232,8 +260,12 @@ public void testCodeActionWorkspaceEditlWithDifferentURI() throws CoreException
p.delete(true, new NullProgressMonitor());
}
}

public static IMarker assertDiagnostics(IFile f, String markerMessage, String resolutionLabel) throws CoreException {
return assertDiagnostics(f, markerMessage, resolutionLabel, true);
}

public static IMarker assertDiagnostics(IFile f, String markerMessage, String resolutionLabel, boolean resolutionExpected) throws CoreException {
waitForAndAssertCondition(2_000, () -> {
IMarker[] markers = f.findMarkers(LSPDiagnosticsToMarkers.LS_DIAGNOSTIC_MARKER_TYPE, true,
IResource.DEPTH_ZERO);
Expand All @@ -243,11 +275,12 @@ public static IMarker assertDiagnostics(IFile f, String markerMessage, String re
final IMarker m = f.findMarkers(LSPDiagnosticsToMarkers.LS_DIAGNOSTIC_MARKER_TYPE, true, IResource.DEPTH_ZERO)[0];
assertEquals(markerMessage, m.getAttribute(IMarker.MESSAGE));

waitForAndAssertCondition(2_000, () ->
assertEquals(resolutionExpected ? "Resolution not found within expected time." : "Unexpected resolution found", resolutionExpected,
waitForCondition(2_000, () ->
IDE.getMarkerHelpRegistry().hasResolutions(m) &&
// need this 2nd condition because async introduces a dummy resolution that's
// not the one we want
IDE.getMarkerHelpRegistry().getResolutions(m)[0].getLabel().equals(resolutionLabel));
IDE.getMarkerHelpRegistry().getResolutions(m)[0].getLabel().equals(resolutionLabel)));
return m;
}

Expand All @@ -274,7 +307,7 @@ public static void assertResolution(IFile targetFile, IMarker m, String newText)
}
assertTrue(editorPart instanceof AbstractTextEditor);
AbstractTextEditor editor = (AbstractTextEditor)editorPart;

waitForCondition(1_000,
() -> newText.equals(editor.getDocumentProvider().getDocument(editor.getEditorInput()).get()));
assertEquals(newText, editor.getDocumentProvider().getDocument(editor.getEditorInput()).get());
Expand Down
16 changes: 16 additions & 0 deletions org.eclipse.lsp4e/src/org/eclipse/lsp4e/LSPEclipseUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import org.eclipse.core.resources.IProject;
import org.eclipse.core.resources.IResource;
import org.eclipse.core.resources.IWorkspaceRoot;
import org.eclipse.core.resources.ResourceAttributes;
import org.eclipse.core.resources.ResourcesPlugin;
import org.eclipse.core.runtime.Adapters;
import org.eclipse.core.runtime.CoreException;
Expand Down Expand Up @@ -1341,4 +1342,19 @@ public static boolean hasCapability(final @Nullable Either<Boolean, ? extends Ob
}
return eitherCapability.isRight() || eitherCapability.getLeft();
}

public static boolean isReadOnly(final @NonNull URI uri) {
IResource resource = findResourceFor(uri);
return resource != null && isReadOnly(resource);
}

public static boolean isReadOnly(final @NonNull IDocument document) {
IFile file = getFile(document);
return file != null && isReadOnly(file);
}

public static boolean isReadOnly(final @NonNull IResource resource) {
ResourceAttributes attributes = resource.getResourceAttributes();
return attributes != null && attributes.isReadOnly();
}
}
52 changes: 50 additions & 2 deletions ...pse.lsp4e/src/org/eclipse/lsp4e/operations/codeactions/LSPCodeActionMarkerResolution.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@
import java.io.IOException;
import java.lang.reflect.Field;
import java.lang.reflect.Method;
import java.net.URI;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.Map;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
Expand Down Expand Up @@ -49,7 +50,12 @@
import org.eclipse.lsp4j.CodeActionParams;
import org.eclipse.lsp4j.Command;
import org.eclipse.lsp4j.Diagnostic;
import org.eclipse.lsp4j.ResourceOperation;
import org.eclipse.lsp4j.ServerCapabilities;
import org.eclipse.lsp4j.TextDocumentEdit;
import org.eclipse.lsp4j.TextEdit;
import org.eclipse.lsp4j.VersionedTextDocumentIdentifier;
import org.eclipse.lsp4j.WorkspaceEdit;
import org.eclipse.lsp4j.jsonrpc.messages.Either;
import org.eclipse.swt.graphics.Image;
import org.eclipse.swt.widgets.Display;
Expand Down Expand Up @@ -108,7 +114,7 @@ public IMarkerResolution[] getResolutions(IMarker marker) {
} else if (att == null) {
return new IMarkerResolution[0];
}
return ((List<Either<Command, CodeAction>>) att).stream().filter(Objects::nonNull)
return ((List<Either<Command, CodeAction>>) att).stream().filter(LSPCodeActionMarkerResolution::canPerform)
.map(command -> command.map(CommandMarkerResolution::new, CodeActionMarkerResolution::new))
.toArray(IMarkerResolution[]::new);
}
Expand Down Expand Up @@ -208,4 +214,46 @@ public boolean hasResolutions(IMarker marker) {
}
return false;
}

static boolean canPerform(Either<Command, CodeAction> command) {
if (command == null) {
return false;
}
if (command.isLeft()) {
return true;
}
CodeAction action = command.getRight();
if (action.getDisabled() != null) {
return false;
}
WorkspaceEdit edit = action.getEdit();
if (edit == null) {
return true;
}
List<Either<TextDocumentEdit, ResourceOperation>> documentChanges = edit.getDocumentChanges();
if (documentChanges != null) {
for (Either<TextDocumentEdit, ResourceOperation> change : documentChanges) {
if (change.isLeft()) {
TextDocumentEdit textedit = change.getLeft();
VersionedTextDocumentIdentifier id = textedit.getTextDocument();
URI uri = URI.create(id.getUri());
if (uri != null && LSPEclipseUtils.isReadOnly(uri)) {
return false;
}
}
}
} else {
Map<String, List<TextEdit>> changes = edit.getChanges();
if (changes != null) {
for (java.util.Map.Entry<String, List<TextEdit>> textEdit : changes.entrySet()) {
URI uri = URI.create(textEdit.getKey());
if (uri != null && LSPEclipseUtils.isReadOnly(uri)) {
return false;
}
}
}
}

return true;
}
}
3 changes: 2 additions & 1 deletion ...lsp4e/src/org/eclipse/lsp4e/operations/codeactions/LSPCodeActionQuickAssistProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ public boolean canAssist(IQuickAssistInvocationContext invocationContext) {

try {
CompletableFuture<List<Either<Command, CodeAction>>> anyActions = executor.collectAll(ls -> ls.getTextDocumentService().codeAction(params)).thenApply(s -> s.stream().flatMap(List::stream).collect(Collectors.toList()));
if (anyActions.get(200, TimeUnit.MILLISECONDS).isEmpty()) {
if (anyActions.get(200, TimeUnit.MILLISECONDS).stream().filter(LSPCodeActionMarkerResolution::canPerform).collect(Collectors.toList()).isEmpty()) {
return false;
}
} catch (InterruptedException | ExecutionException | TimeoutException e) {
Expand Down Expand Up @@ -162,6 +162,7 @@ public ICompletionProposal[] computeQuickAssistProposals(IQuickAssistInvocationC
futures = executor.computeAll((w, ls) -> ls.getTextDocumentService()
.codeAction(params)
.thenAccept(actions -> LanguageServers.streamSafely(actions)
.filter(LSPCodeActionMarkerResolution::canPerform)
.forEach(action -> processNewProposal(invocationContext, new CodeActionCompletionProposal(action, w)))));

CompletableFuture<?> aggregateFutures = CompletableFuture
Expand Down
1 change: 1 addition & 0 deletions org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/codeactions/LSPCodeActionsMenu.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ public void widgetSelected(SelectionEvent e) {
proposal.apply(document);
}
});
item.setEnabled(LSPCodeActionMarkerResolution.canPerform(command));
}
}
}
Expand Down

0 comments on commit e49962d

Please sign in to comment.