Backend produces messages inconsistent with protocol definition

[kazcw]

According the protocol, the tagValues and defaultValue fields of a SuggestionEntryArgument may have values or may be omitted, but must not be null. The current LS (8054ff7) may provide null for these fields; here's a (formatted) excerpt from a search/suggestionsDatabaseUpdates message:

{ 
  "type": "Add", 
  "id": 13260, 
  "suggestion": { 
    "type": "constructor", 
    "module": "Standard.Image.Histogram", 
    "name": "Value", 
    "arguments": [ 
      { 
        "name": "channel", 
        "reprType": "Standard.Base.Any.Any", 
        "isSuspended": false, 
        "hasDefault": false, 
        "defaultValue": null, 
        "tagValues": null 
      }, 
      { 
        "name": "data", 
        "reprType": "Standard.Base.Any.Any", 
        "isSuspended": false, 
        "hasDefault": false, 
        "defaultValue": null, 
        "tagValues": null 
      } 
    ], 
    "returnType": "Standard.Image.Histogram.Histogram", 
    "documentation": " PRIVATE\nThe histogram of a single image channel.\n\nArguments:\n- channel: The channel in the image for which this is a histogram.\n- data: The histogram data.", 
    "annotations": [], 
    "reexports": [] 
  } 
}

Proximal cause

When encoding, .dropNullValues from circe is applied to the top level of the object; however, it is non-recursive.

enso/engine/language-server/src/main/scala/org/enso/languageserver/search/SearchProtocol.scala

Line 117 in 8054ff7

.dropNullValues

Solution

circe has a deepDropNullValues that might be an appropriate replacement.

Root cause

Currently, the protocol types are defined in three places: As Typescript code blocks in the Markdown protocol specification, with circe in the backend, and with zod in the frontend. Consistency of the protocol used by the backend and frontend is dependent on the backend and frontend developers carefully ensuring the respective protocol definitions are consistent with the protocol specification. Subtle differences, such as in the distinction between null or an omitted field, are hard to avoid and are likely to cause bugs at runtime.

Solution

To ensure backend and frontend are using the same protocol (and simplify making protocol changes), we could unify the protocol definitions:

• Specify the protocol as a JSON Schema.
• Use json-schema-to-zod to generate the frontend type definitions.
• Use endpoints4s Circe to generate the backend type definitions.

[farmaazon]

The question is how much do we want to invest in JSON-RPC LS protocol before ditching it in favor of a new API based on YDoc AST.

[hubertp]

The question is how much do we want to invest in JSON-RPC LS protocol before ditching it in favor of a new API based on YDoc AST.

Yeap, my thought exactly.

I'm happy to implement the null fix but investing more into circe-based infra is the opposite direction I wanted to go. It also has a number of performance issues (during startup) and we were slowly moving away to other libraries, see for example #10326.
Having said that, a single JSON schema and generating protocol from it for different implementations is a good idea, no matter what tech we pick. So I would split this issue into two, if you don't mind, with a lower priority for the latter.