Fix LSP exceptions serializing ResponseErrors with Uris
The ResponseError.data field was typed `dynamic` (as in the LSP spec) but this allows us to accidentally put non-serializable classes in it which will result in server errors when we try to send them back.
This change forces them to Strings and it's up to the code constructing the error to convert any additional data to a string first.
In order to catch this in tests, the mocks now force serialisation of all types flowing from the server to the client (this already happened for client to server).
Change-Id: I5d7f322e1a4296b1479468e36d81daebdbb4ab52
Reviewed-on: https://dart-review.googlesource.com/c/89511
Commit-Queue: Danny Tuppeny <dantup@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
diff --git a/pkg/analysis_server/lib/lsp_protocol/protocol_generated.dart b/pkg/analysis_server/lib/lsp_protocol/protocol_generated.dart
index af2a5e1..8991e2f 100644
--- a/pkg/analysis_server/lib/lsp_protocol/protocol_generated.dart
+++ b/pkg/analysis_server/lib/lsp_protocol/protocol_generated.dart
@@ -6674,9 +6674,9 @@
/// A number indicating the error type that occurred.
final ErrorCodes code;
- /// A Primitive or Structured value that contains additional information about
- /// the error. Can be omitted.
- final D data;
+ /// A string that contains additional information about the error. Can be
+ /// omitted.
+ final String data;
/// A string providing a short description of the error.
final String message;
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/simple_edit_handler.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/simple_edit_handler.dart
index 24c7b7d..64c5877 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/simple_edit_handler.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/simple_edit_handler.dart
@@ -47,7 +47,7 @@
return error(
ServerErrorCodes.ClientFailedToApplyEdit,
'Client failed to apply workspace edit for $commandName',
- editResponse.error,
+ editResponse.error.toString(),
);
}
@@ -64,7 +64,7 @@
return error(
ServerErrorCodes.ClientFailedToApplyEdit,
'Client failed to apply workspace edit for $commandName',
- workspaceEdit,
+ workspaceEdit.toString(),
);
}
}
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart b/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
index 5125008..78684ec 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
@@ -32,7 +32,7 @@
mixin Handler<P, R> {
LspAnalysisServer server;
- ErrorOr<R> error<R>(ErrorCodes code, String message, Object data) =>
+ ErrorOr<R> error<R>(ErrorCodes code, String message, String data) =>
new ErrorOr<R>.error(new ResponseError(code, message, data));
ErrorOr<R> failure<R>(ErrorOr<dynamic> error) =>
@@ -93,7 +93,7 @@
}
ErrorOr<Object> failure<Object>(ErrorCodes code, String message,
- [Object data]) =>
+ [String data]) =>
new ErrorOr<Object>.error(new ResponseError(code, message, data));
/// Handle the given [message]. If the [message] is a [RequestMessage], then the
diff --git a/pkg/analysis_server/lib/src/lsp/mapping.dart b/pkg/analysis_server/lib/src/lsp/mapping.dart
index 3f1e394..edf1327 100644
--- a/pkg/analysis_server/lib/src/lsp/mapping.dart
+++ b/pkg/analysis_server/lib/src/lsp/mapping.dart
@@ -256,7 +256,7 @@
return new ErrorOr<String>.error(new ResponseError(
lsp.ServerErrorCodes.InvalidFilePath,
'URI was not a valid file:// URI',
- uri));
+ uri.toString()));
}
try {
return new ErrorOr<String>.success(uri.toFilePath());
@@ -266,7 +266,7 @@
return new ErrorOr<String>.error(new ResponseError(
lsp.ServerErrorCodes.InvalidFilePath,
'File URI did not contain a valid file path',
- uri));
+ uri.toString()));
}
}
@@ -458,7 +458,7 @@
? lsp.ServerErrorCodes.ClientServerInconsistentState
: lsp.ServerErrorCodes.InvalidFileLineCol,
'Invalid line number',
- pos.line));
+ pos.line.toString()));
}
// TODO(dantup): Is there any way to validate the character? We could ensure
// it's less than the offset of the next line, but that would only work for
diff --git a/pkg/analysis_server/test/mocks.dart b/pkg/analysis_server/test/mocks.dart
index 286da86..4ee6655 100644
--- a/pkg/analysis_server/test/mocks.dart
+++ b/pkg/analysis_server/test/mocks.dart
@@ -8,6 +8,7 @@
import 'package:analysis_server/lsp_protocol/protocol_generated.dart' as lsp;
import 'package:analysis_server/lsp_protocol/protocol_generated.dart';
import 'package:analysis_server/lsp_protocol/protocol_special.dart' as lsp;
+import 'package:analysis_server/lsp_protocol/protocol_special.dart';
import 'package:analysis_server/protocol/protocol.dart';
import 'package:analysis_server/protocol/protocol_generated.dart';
import 'package:analysis_server/src/analysis_server.dart';
@@ -98,6 +99,13 @@
}
}
+ /// Run the object through JSON serialisation to catch any
+ /// issues like fields that are unserialisable types. This is used for
+ /// messages going server-to-client.
+ void ensureMessageCanBeJsonSerialized(ToJsonable message) {
+ jsonEncode(message.toJson());
+ }
+
@override
void listen(void Function(lsp.Message message) onMessage,
{Function onError, void Function() onDone}) {
@@ -110,6 +118,9 @@
if (_closed.isCompleted) {
return;
}
+
+ ensureMessageCanBeJsonSerialized(notification);
+
_serverToClient.add(notification);
}
@@ -118,7 +129,9 @@
if (_closed.isCompleted) {
return;
}
+
notification = _convertJson(notification, lsp.NotificationMessage.fromJson);
+
_clientToServer.add(notification);
}
@@ -128,6 +141,9 @@
if (_closed.isCompleted) {
return;
}
+
+ ensureMessageCanBeJsonSerialized(request);
+
_serverToClient.add(request);
}
@@ -141,7 +157,9 @@
if (_closed.isCompleted) {
throw new Exception('sendLspRequest after connection closed');
}
+
request = _convertJson(request, lsp.RequestMessage.fromJson);
+
// Wrap send request in future to simulate WebSocket.
new Future(() => _clientToServer.add(request));
return waitForResponse(request);
@@ -153,6 +171,9 @@
if (_closed.isCompleted) {
return;
}
+
+ ensureMessageCanBeJsonSerialized(response);
+
// Wrap send response in future to simulate WebSocket.
new Future(() => _serverToClient.add(response));
}
@@ -162,7 +183,9 @@
if (_closed.isCompleted) {
return;
}
+
response = _convertJson(response, lsp.ResponseMessage.fromJson);
+
_clientToServer.add(response);
}
diff --git a/pkg/analysis_server/test/tool/lsp_spec/typescript_test.dart b/pkg/analysis_server/test/tool/lsp_spec/typescript_test.dart
index abf1791..f100429 100644
--- a/pkg/analysis_server/test/tool/lsp_spec/typescript_test.dart
+++ b/pkg/analysis_server/test/tool/lsp_spec/typescript_test.dart
@@ -102,7 +102,7 @@
test('parses an interface with type args', () {
final String input = '''
-interface ResponseError<D> {
+interface MyInterface<D> {
data?: D;
}
''';
diff --git a/pkg/analysis_server/tool/lsp_spec/typescript.dart b/pkg/analysis_server/tool/lsp_spec/typescript.dart
index 6ff12a9..afa5b76 100644
--- a/pkg/analysis_server/tool/lsp_spec/typescript.dart
+++ b/pkg/analysis_server/tool/lsp_spec/typescript.dart
@@ -64,6 +64,11 @@
},
"ResponseError": {
"code": "ErrorCodes",
+ // This is dynamic normally, but since this class can be serialised
+ // we will crash if it data is set to something that can't be converted to
+ // JSON (for ex. Uri) so this forces anyone setting this to convert to a
+ // String.
+ "data": "String",
},
"NotificationMessage": {
"method": "Method",
@@ -89,6 +94,21 @@
return interface != null ? interface[fieldName] : null;
}
+/// Improves comments in generated code to support where types may have been
+/// altered (for ex. with [getImprovedType] above).
+String getImprovedComment(String interfaceName, String fieldName) {
+ const Map<String, Map<String, String>> _improvedComments = {
+ "ResponseError": {
+ "data":
+ "// A string that contains additional information about the error. Can be omitted.",
+ },
+ };
+
+ final interface = _improvedComments[interfaceName];
+
+ return interface != null ? interface[fieldName] : null;
+}
+
List<String> getSpecialBaseTypes(Interface interface) {
if (interface.name == 'RequestMessage' ||
interface.name == 'NotificationMessage') {
diff --git a/pkg/analysis_server/tool/lsp_spec/typescript_parser.dart b/pkg/analysis_server/tool/lsp_spec/typescript_parser.dart
index bd303e1..2412309 100644
--- a/pkg/analysis_server/tool/lsp_spec/typescript_parser.dart
+++ b/pkg/analysis_server/tool/lsp_spec/typescript_parser.dart
@@ -306,6 +306,12 @@
type = _type(containerName, name.lexeme,
includeUndefined: canBeUndefined, improveTypes: true);
+ // Overwrite comment if we have an improved one.
+ final improvedComment = getImprovedComment(containerName, name.lexeme);
+ leadingComment = improvedComment != null
+ ? new Comment(new Token(TokenType.COMMENT, improvedComment))
+ : leadingComment;
+
// Some fields have weird comments like this in the spec:
// {@link MessageType}
// These seem to be the correct type of the field, while the field is
