search

aml-org / als

Language Server implementation for AML and AML-defined metadata
Other
27 stars 9 forks source link

Internal server error when sending a "serialization" request #497

Closed deiteris closed 3 years ago

deiteris commented 3 years ago

Your issue may already be reported! Please search on Github issues before creating one.

When sending a request with the "serialization" method, the language server responds with internal server error.

Full trace: https://pastebin.com/fWYvRivz

Any RAML file.

Expected a response according to documentation.

llibarona commented 3 years ago

Hi @deiteris ! We will look into this error, but looking the trace I notice that the URI is sent with C:/ (upper case), while other messages use the path c:/ (lower case). I know Windows is not case sensitive in regards to its path, but URIs are, and ALS is also case sensitive. Could you try sending the same URI as the other messages in order to ensure that the problem persists? Either way we will seek to solve this RuntimeException, but the request will still not respond as you expect if the URI is not equal (case sensitive) to the one you are notifying with open/change messages

deiteris commented 3 years ago

Hi @llibarona! The problem persists even if I change the drive letter to lowercase. 

[Trace - 23:51:25] Sending request 'serialization - (7)'.
Params: {
    "uri": "file:///c:/Work/Temp/test/api.raml"
}

[Trace - 23:51:26] Received response 'serialization - (7)' in 51ms. Request failed: Internal error. (-32603).
Error data: "java.lang.RuntimeException: java.lang.reflect.InvocationTargetException\r\n\tat org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint.lambda$null$0(GenericEndpoint.java:67)\r\n\tat org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint.request(GenericEndpoint.java:120)\r\n\tat org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.handleRequest(RemoteEndpoint.java:261)\r\n\tat org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.consume(RemoteEndpoint.java:190)\r\n\tat org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.handleMessage(StreamMessageProducer.java:194)\r\n\tat org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.listen(StreamMessageProducer.java:94)\r\n\tat org.eclipse.lsp4j.jsonrpc.json.ConcurrentMessageProcessor.run(ConcurrentMessageProcessor.java:113)\r\n\tat java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)\r\n\tat java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)\r\n\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)\r\n\tat java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)\r\n\tat java.base/java.lang.Thread.run(Thread.java:834)\r\nCaused by: java.lang.reflect.InvocationTargetException\r\n\tat java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\r\n\tat java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)\r\n\tat java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)\r\n\tat java.base/java.lang.reflect.Method.invoke(Method.java:566)\r\n\tat org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint.lambda$null$0(GenericEndpoint.java:65)\r\n\t... 11 more\r\nCaused by: java.lang.NullPointerException\r\n\tat org.mulesoft.lsp.LspConversions$.textDocumentIdentifier(LspConversions.scala:281)\r\n\tat org.mulesoft.als.server.lsp4j.LspConversions$.jvmSerializationParams(LspConversions.scala:171)\r\n\tat org.mulesoft.als.server.lsp4j.TextDocumentServiceImpl.serialization(TextDocumentServiceImpl.scala:153)\r\n\t... 16 more\r\n"
llibarona commented 3 years ago

I just realized the Param is in an unexpected format. The expected Param for Serialization is:

Params: {
    "documentIdentifier": {
        "uri": "file:///c:/Work/Temp/test/api.raml"
    }
}

I will update the documentation accordingly in https://github.com/aml-org/als/blob/develop/documentation/features/custom-messages.md

Please let me know if this fixes your issue

deiteris commented 3 years ago

Oh well, I noticed in the source code that there's documentIdentifier accepted by SerializationParams, but didn't realize that this is what I am looking for -_-. Yes this solves the issue, thanks a lot. However, there're few more inconsistencies with the docs to report then:

  1. Expected result of the method call is:

    {
  "uri": "string",
  "content": "string"
}

    See resulting trace: https://pastebin.com/43Q1RX3w

  2. Methods should be named according to their actual casing, or it should be reflected in the description. First time when I tried to send request with custom Serialization or FileUsage methods, I got error that there's no such method. Switching from PascalCase to camelCase (serialization and fileUsage correspondingly) solved this, but someone else may be not as intuitive.

  3. Just like you said in your first answer: the drive letter should be in lowercase, otherwise it will cause the same internal error. This is an important thing to mention for Windows users.

llibarona commented 3 years ago

Expected result of the method call is:

Noticed! We will update accordingly!

Methods should be named according to their actual casing, or it should be reflected in the description. First time when I tried to send request with custom Serialization or FileUsage methods, I got error that there's no such method. Switching from PascalCase to camelCase (serialization and fileUsage correspondingly) solved this, but someone else may be not as intuitive.

We currently have an internal ticket to adjust this names, as of today, in JS we use PascalCase and JVM uses camelCase, but changing this should move a major in versioning, so we are saving the change for v4.0.0. But we will also update this in the documentation file!

Just like you said in your first answer: the drive letter should be in lowercase, otherwise it will cause the same internal error. This is an important thing to mention for Windows users.

We will also update the documentation accordingly!

deiteris commented 3 years ago

I am also closing this issue as it was resolved.