Remote SOQL Errors - Language Server Part #93
Conversation
code0100fun
requested review from
amphro,
dehru,
jgellin-sf,
jonnyhork,
mysticflute,
praksb and
tmock12
as code owners
jgellin-sf
left a comment
jgellin-sf
left a comment
There was a problem hiding this comment.
Generally, the code looks good. However, I have a couple of portability concerns with the custom messages. I'd prefer to see a gate on custom messages established by a client capability that gets set on initialize. There is probably no short-term runway to a non-vscode client for this language server, but I don't want to start digging a tech debt hole right off the bat.
| const diagnostics: Diagnostic[] = []; | ||
| const response = (await connection.sendRequest( | ||
| RequestTypes.RunQuery, | ||
| appendLimit0(textDocument.getText()) | ||
| )) as RunQueryResponse; | ||
| if (response.error) { | ||
| diagnostics.push({ | ||
| severity: DiagnosticSeverity.Error, | ||
| range: | ||
| extractErrorRange(response.error.message) || | ||
| documentRange(textDocument), | ||
| message: response.error.message, | ||
| source: 'soql', | ||
| }); | ||
| } |
There was a problem hiding this comment.
This request is a bit concerning from a portability standpoint. I'm not sure what happens if we are connected to a (non-VS Code) client that doesn't understand this request. Does it never get answered (not too good)? Does it return an error that is not a query error (we need to handle that)? Does the request throw (this would be good)? Is there a timeout (not too good)?
If this language server is going to be useful outside of our current VS Code implementation, we should know when the client can handle this type of request.
Typically this is done in the client capabilities object on the initialize request. https://microsoft.github.io/language-server-protocol/specifications/specification-current/#initialize
Maybe we can extend that object with a custom property that indicates we can send a RunQuery request.
|
|
||
| // Create a connection for the server, using Node's IPC as a transport. | ||
| let connection = createConnection(ProposedFeatures.all); | ||
| connection.sendNotification('soql/validate', 'createConnection'); |
There was a problem hiding this comment.
Since we are not expecting any response, it's no big deal if the client doesn't understand this custom message.
Codecov Report
@@ Coverage Diff @@
## develop #93 +/- ##
===========================================
+ Coverage 94.09% 94.16% +0.06%
===========================================
Files 33 33
Lines 847 874 +27
Branches 174 181 +7
===========================================
+ Hits 797 823 +26
- Misses 48 49 +1
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
code0100fun
force-pushed
the
chase/soql-remote-errors
branch
from
fb9767a to
244c2ca
Compare
jgellin-sf
left a comment
jgellin-sf
left a comment
There was a problem hiding this comment.
Thanks for the changes.
code0100fun
force-pushed
the
chase/soql-remote-errors
branch
from
448fe93 to
608cfc7
Compare
| params.initializationOptions?.runQueryValidation || false; | ||
| runQueryValidation = QueryValidationFeature.hasRunQueryValidation( | ||
| params.capabilities | ||
| ); |
There was a problem hiding this comment.
@jgellin-sf what do you think about this for a capability check? The client and server share the feature file for setting/checking the flag.
| "engines": { | ||
| "node": "*" | ||
| }, | ||
| "main": "lib/index.js", |
There was a problem hiding this comment.
This allows the client to access the exported types and feature from the package
* Add debounce dep to language-server * Call runQuery on clients when SOQL query text changes * Gate runQuery request with runQueryValidation flag * Convert runQuery flag to client feature
* model WHERE clause (#68) * Remote SOQL Errors - Language Server Part (#93) * remove code builder (#92) * Move from antlr4JS to antlr4TS (#96) * SOQL code completion for SELECT fields (#99) * publish language server 0.2.9 (#102) * Fix LSP dependency (#104) * Disabled 'Running...' button while query running (#106) * Add basic code-completion for ORDER BY and other improvements (#107) * Dehru and JG telemetry - Send GDPR clean data to telemetry when errors/unsupported syntax present (#110) * Disable WHERE in model (#113) * Bump versions for publishing (#114) * Code-completion for WHERE clause expressions (#112) Co-authored-by: Jonathan Gellin <jgellin@salesforce.com> Co-authored-by: jgellin-sf <55159130+jgellin-sf@users.noreply.github.com> Co-authored-by: Chase McCarthy <charles.mccarthy@heroku.com> Co-authored-by: Jonny Hork <jhork@salesforce.com> Co-authored-by: Fernando Dobladez <fernandodobladez@salesforce.com> Co-authored-by: Jonathan Gellin <jgellin@salesforce.com>
2 participants
W-7977000
What does this PR do?
The SOQL language server triggers a
LIMIT 0query execution on the SOQL language client (viasendRequest). After validating the response the language server adds diagnostic messages to the client connection viasendDiagnostics.