Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LSP autogenerated contracts POC, DO NOT MERGE #1623

Draft
wants to merge 8 commits into
base: nade-dex-lsp
Choose a base branch
from

Conversation

sfc-gh-jchernandez
Copy link

@sfc-gh-jchernandez sfc-gh-jchernandez commented Sep 24, 2024

Best reviewed commit by commit, to focus on not-autogenerated code

Goal

Explore a way to better manage LSP api contracts which have to be honored by both client (VSC extenstion) and server (LSP server in cli project)

By having a single source of truth for the contracts, we can have both projects import the autogenerated entities, which are strongly typed. This would ensure we detect unexpected issues if the contract evolution breaks the existing code (either in server or client side)

Non goal

Deal with how to wire and setup the source of truth contracts, autogeneration and import into cli and vsc extension.

  • Where would the source of truth yaml live?
  • Diff projects? npm & pip packages to be imported? etc

Important note

See the current appVersionCreate raw payload to flow between the VSC extension and LSP server WIP implementation.

{
      "connection": {
          "temporary_connection": true,
          "account": "myaccount",
          "user": "myuser",
          // etc.
      },
      "project_path": "/home/me/dev/myapp",
      "env": {
          "USER": "myuser",
      },
      "kwargs": {
          "patch": 5,
          "version": "v1",
      },
      "args": [],  // or omit
  }

In this POC, I took the liberty of changing this payload with a better encapsulation and separation of concerns, it would look like:

{
    "context": {
        "connection": {
            "account": "myaccount",
            "user": "myuser"
        },
        "project_path": "/home/me/dev/myapp",
        "env": {
            "USER": "myuser",
        },
    },
    "cmd": {
        "open_input_1": "foo",
        "open_input_2": "faa"
    }
}

@sfc-gh-jchernandez sfc-gh-jchernandez changed the base branch from main to nade-dex-lsp September 24, 2024 20:18
@sfc-gh-tkojima
Copy link

Nice! I'm onboard with open-api in general. We have also starting using it in coldbrew in snapps.

It'd be nice to see how the generated types will be used, it's a bit hard to visualize without trying it out.

It seems like we'd have a single request type though and the actual command we want to run would be passed in via a type key. If we could it would be great to move the command name out of the object since LSP supports the the request/command type as a top level arg. Then we could achieve the same type-safe ergonomics as the snowflake-sql LSP requests.

Maybe we can repurpose the path and use it as the top-level key

i.e.

https://github.com/snowflakedb/snowflake-sql-language-server/blob/main/src/requests.ts#L52

import {
  AnalyzeSetupScript,
} from '@snowflake/snowflake-sql-language-server/dist/requests';

...

const res = await client.sendRequest(AnalyzeSetupScript, {
  setupScriptIdentifier: textDocumentIdentifier,
  packageName,
});

...

@sfc-gh-jchernandez
Copy link
Author

sfc-gh-jchernandez commented Sep 25, 2024

If we could it would be great to move the command name out of the object since LSP supports the the request/command type as a top level arg

Yes, that property is redundant, however having it inside the cmd "payload" would make that payload self contained. That is, if you get your hands on that payload, you do not need anything outside it to deserialize it to its mapped entity.

You are right in that we can do without it, specially given that today, a single LSP transaction payload has a 1:1 relationship with a cmd entity inside it.

In the structure of the payload I showed, there are two separate entities that might need to be deserialized separately. The context and the cmd. For the context, I did not go that far, but we could choose to do something similar, and have it contain a discriminator property ("context_type") to distinguish about different types of contexts. For some transactions, the connection might not make sense, for others, the project path might not make sense.

All this above is the way I think, a 100% we could keep these discriminators out, and rely on the top level LSP "command" property. Yes the inner payloads would not be self-contained with regards to deserialization, but we can always introduce this later.

@sfc-gh-jchernandez
Copy link
Author

It'd be nice to see how the generated types will be used, it's a bit hard to visualize without trying it out.

100%, I should have gone that extra mile to show how these autogenerated entities could be used from python and ts, let me spend a little time today, I will add another commit.


@workspace_command(server, "poc_cmd")
def poc_cmd(input: CmdPocInput) -> CmdPocOutput:
return CmdPocOutput(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the default type checking behavior in pycharm and visual studio DOES NOT seem to complain about type violations (the whole point of this).

I got it to behave as expected in pycharm after installing the pydantic plugin. To make sure bugs are detected, we would want to stricter type validation (if not there yet during "build" time) using mypy or equivalent.

Screenshot 2024-09-25 at 7 33 28 PM


from pydantic import BaseModel, ConfigDict, StrictStr
from typing import Any, ClassVar, Dict, List, Optional
from snowflake.cli.plugins.lsp.models.connection import Connection
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is properly wired within the project thanks to -p packageName=snowflake.cli.plugins.lsp

@@ -47,30 +34,51 @@ def workspace_command(
(if required) as well as ensuring arguments are in the required format.
"""
def _decorator(func):
"""
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retrieve type hints in the method

@sfc-gh-jchernandez
Copy link
Author

@sfc-gh-tkojima I have not been able to find any breakthrough regarding being able to wire the openapi paths with the LSP server, but I spent 30 min looking. If it is possible, it might in the fringes, LSP I think rides on JSON-RPC which is not RESTful (see thread here), openapi was thought for http REST

I went ahead and showcased in python (will leave ts to the imagination) on how I imagine the plugin definitions being cleanly defined with strong typing (see poc_cmd) of input and output models.

The inner framework code is obscure and a bit nasty, but should not need to change, and we could test it thoroughly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants