Extend backend "ClientTypes" by StachuDotNet · Pull Request #4542 · darklang/dark

StachuDotNet · 2022-10-20T15:23:26Z

Changelog:

Internal improvements:
- Extracted most client-facing types to central "ClientTypes" project to allow for safe adjustments of those types without leaking into our domain logic

Our backend passes data back and forth with our client in a few different ways:

via API request/response payloads
via WASM-compiled code used to support "Analysis"
via ApiServer/UI.fs which injects JSON into ui.html dynamically
via Pusher.com 'pushes'

Presently, all of this communication is done via JSON payloads, serialized largely by our Prelude.Vanilla System.Text.Json serializer. The types in fsharp-backend corresponding to those JSON payloads are spread throughout our codebase, resulting in us serializing "internal" types.

That said, there's been some effort in the past to create "client types" in a ClientTypes project - the idea is to 'map' between internal types and external types, such that we may safely update either without breaking the communication between the Client and the Server. Generally, I've seen this idea referred to as "DTOs - Data Transfer Objects." Our ClientType definitions/setup is currently quite thin, and only covers Analysis and several APIs.

As is, it's unsafe to make many sorts of adjustments to our internal F# types (e.g. removing or renaming fields), as the serialized payload wouldn't be parse-able in our ReScript code, or vice versa.

The intent of this PR is to consolidate most (if not all) of the types used in client-server communication, to reduce/remove the current risks involved in updating internal types.

The specific cause of this work being done is an ongoing effort to "rebrand" our "Patterns" as "Match Patterns," as we'd like to shortly implement "let patterns" and so our current 'branding' would be confusing. Specifically, these existing "patterns" are serialized in a style of "PBlank," (corresponding to a 'blank' match pattern) and we'd like them to instead be serialized as "MPBlank". Our client is set up to handle this change, (our Match Pattern deserializer is able to parse both MPBlank and PBlank) but in order to finish this work, we need to be able to safely pass the new "MPBlank"-style payloads from client to our backend. Our backend's "AddOpV1" API endpoint request payload includes Match Patterns, and the types referenced are "internal" (not client types).

So, the motivation behind this PR is to ensure all types referenced by the "AddOpV1" are ClientTypes, where we may safely have both "MPBlank" and "PBlank"-style definitions for these patterns, and translate those appropriately to a single "MPBlank" case in our "internal" types. (so the rename doesn't bleed into our domain logic). We could have gotten around this by writing a specialized serializer for the MatchPattern type, but moving everything to ClientTypes seems more beneficial long-term.

This work involved:

breaking down the ClientTypes project into multiple modules (RuntimeTypes and AnalysisTypes)
cloning many internal types (anything referenced by the noted means of client-server communication) into ClientTypes
ProgramTypes, Ops, Worker, Secrets, etc,
migrating all existing serializable types to ClientTypes
- API requests/responses, Pusher.com payloads, ui.html-injected data
creating additional projects to convert/map between ClientTypes and internal types
- ClientTypes2ExecutionTypes handles conversions for our core types defined in LibExecution
- ClientTypes2BackendTypes handles types elsewhere, namely in LibBackend which is responsible for the surrounding infrastructure
along the way, adjust/correct our testing around stable serializations of these types
- Serialization.Tests.fs: reference the new types in tests
- TestJsonEncoding.res: mostly, file names have simply adjusted due to updates in Serialization.Tests.fs

At this point, the PR is aimed to be a no-op; no functional changes. As far as I can tell, that is the case.

I have some outstanding questions/concerns/todos:

given these refactors, it's not clear to me where Json.Vanilla.allow<CTApi.Ops.AddOpV1.Request> "ApiServer.AddOps" calls should be made, to explicitly allow serializations of various types. Previously these types were generally 'registered' in the assembly which needed them, but I've been starting to think that this might make more sense in an init within ClientTypes itself
I'm not sure if I need to add a ClientTypes.Init.init call to DataTests, ExecHost, and CronChecker projects.
Serialization.Tests.fs is feeling a bit unwieldy - maybe we should break this down into 2 files, one for client-server serializations, and one for DB-bound serializations
it feels a bit awkward/confusing to me right now that many of the static "Values" in that file are "client types" and many are "internal" types that we map to "serialized types". Maybe it was a mistake for me to update those static Values to ClientTypes?

This is a bit non-exhaustive - there are some tests around our Vanilla serialize still in Serialization.Tests.fs that reference types outside of ClientTypes. Likely in another PR, I need to figure out where these are used and what to do about them.

StachuDotNet · 2022-10-24T20:59:35Z

    LibService.Init.init name
    LibExecution.Init.init ()
    Telemetry.Console.loadTelemetry name Telemetry.DontTraceDBQueries
+    //? ClientTypes.Init.init name


TODO: figure out if this should be removed/uncommented.

StachuDotNet · 2022-10-24T20:59:40Z

      (LibBackend.Init.init LibBackend.Init.WaitForDB name).Result
    let result = (run options).Result
    LibService.Init.shutdown name
+    //? ClientTypes.Init.init name


TODO: figure out if this should be removed/uncommented.

StachuDotNet · 2022-10-24T21:00:28Z

  LibService.Telemetry.Console.loadTelemetry
    "DataTests"
    LibService.Telemetry.DontTraceDBQueries
+  //? ClientTypes.Init.init name


TODO: figure out if this should be removed/uncommented.

StachuDotNet · 2022-10-24T21:01:42Z

This is ready for an initial review. I tried my best to break the commits down well and add appropriate commentary.
That said, it's a rather large PR, so happy to 1:1 on this.

pbiggar

This looks great.

My one suggestion would be to make the testing more thorough - we currently don't have full end-to-end analysis including the conversion functions, and there's so much conversion that typos are easy to sneak in (both now and in the future).

pbiggar · 2022-10-25T17:01:52Z

@@ -1,7 +0,0 @@
-{


Unclear why this is gone.

We don't yet handle the case of the Pusher.com AddOpV1 payload being too big to push to the client.

let tooBigPayload = { tlids = tlids } |> Json.Vanilla.serialize // CLEANUP: when changes are too big, notify the client to reload them. We'll // have to add support to the client before enabling this. The client would // reload after this. // push canvasID "addOpTooBig" tooBigPayload

is the code before changes, and we have the equivalent (just swallow the issue) in the new code.

Since we don't yet serialize it, I figured we didn't need to register the serializer, nor test for how it serializes.

pbiggar · 2022-10-25T17:31:23Z

-      v<LibExecution.ProgramTypes.Oplist> "complete" testOplist
-      v<LibExecution.ProgramTypes.Handler.T> "simple" testHttpHandler
+      v<LibExecution.ProgramTypes.Oplist> "complete" ProgramTypes.testOplist
+      v<LibExecution.ProgramTypes.Handler.T> "simple" ProgramTypes.testHttpHandler


Surely this isn't allowed serialize anymore?

It seems they are.

LibExecution.ProgramTypes.Handler.T is used in a canvas-clone test, and the .allow is registered in Tests.fs just for that

LibExecution.ProgramTypes.Oplist is used in Canvas's loadJsonFromDisk

As I look through other non-ClientTypes types, I seem to find similar edge cases.

pbiggar · 2022-10-25T17:33:14Z

-            userTypes = testUserTypes
-            packageFns = [ testPackageFn ]
+            userFns =
+              List.map CT2Program.UserFunction.toCT ProgramTypes.testUserFunctions


It seems the tests here are sometimes for ClientTypes and sometimes for converted types. It feels like there should be tests for roundtrips from real types, right? Maybe also roundtrips from ClientTypes?

pbiggar

This looks fantastic, nice job!

A couple of small suggestions and we should be good to merge.

pbiggar · 2022-11-01T16:41:14Z

  }
 }

+// TODO: these are used outside of analysis (e.g. in APIWorkers) so the type def


Unclear action this comment implies?

The WorkerState module/type isn't just used in Analysis (as implied by its location in AnalysisTypes.res).
It's also used in the "initial load" API payload, and is referenced by APIWorkers file.

Just thinking to move the module definition somewhere else, but I'm not sure where that would be.

pbiggar · 2022-11-01T16:43:49Z

-
 #nowarn "988"

+module Init =


I'm unclear the purpose of this. (Is the comment explaining it? If so, that's not clear to me)

Based on our new standard of initializing serializers in the "binary projects," it felt like an "init function" would be appropriate in the Analysis project. Since the actual initialization has to occur in LibAnalysis, I figured I'd create a little 'wrapper' to handle the initializations. It's probably redudant/useless though?

I forget why the init is in LibAnalysis actually. I'll try throwing it into Analysis and see what breaks, if anything.

Ah. Tests.fs calls on LibAnalysis.initSerializers (). I think if I try moving those to Analysis, I get some .net WASM compile issue. I should have read my own comment :)

pbiggar · 2022-11-01T16:51:09Z


-let pushNewStaticDeploy (canvasID : CanvasID) (asset : StaticAssets.StaticDeploy) =
-  push canvasID "new_static_deploy" (Json.Vanilla.serialize asset)
+  let serialized = eventSerializer event


Can you move the serialization inside the fireAndForget so that it's done in the background?

This is done

pbiggar · 2022-11-01T17:41:10Z

-    (traceID : CTA.TraceID)
-    (traceData : CTA.TraceData.T)
+    (traceID : CTAnalysis.TraceID)
+    (traceData : CTAnalysis.TraceData)


It seems odd that these take clients types for some arguments. I think if we're going to convert from clients types in one place, we should do the conversion for all the arguments in that place.

I agree. I ended up extracting this mapping out to CT2Analysis, including the bits where we map from PTs to RTs. I think this turned out much nicer.

pbiggar · 2022-11-01T17:48:25Z

@@ -0,0 +1,19 @@
+module ClientTypes.Converters
+
+module STJ =


Perhaps there's an opportunity to get rid of this. We could argue that the ClientType here is really a string and the conversion functions should do the parsing, rather than rely on this (I feel this particular thing is annoying to learn and keep track of)

I don't understand what you mean here. Could you please rephrase or briefly chat?

Never mind - I understand now.

StachuDotNet · 2022-11-02T17:18:56Z

-  Json.Vanilla.registerConverter (ClientTypes.Converters.STJ.WorkerStateConverter())
-  Json.Vanilla.allow<CTPusher.Payload.UpdateWorkerStates> "Pusher"
-
-  // allow serialization of types used in Pusher.com payloads


(apparently I had created duplicate .allow statements here for pusher.com payloads)

C_T won't work. ProgramTypes and PusherTypes, and ApiTypes and AnalysisTypes, 'collide' in abbreviation. So, let's just stick to CT___.

…sions

…inits in ExecHost

- I think maintaining a lazy list of data is clearer than a ResizeArray that is added to with a function - Some tests around the persisted output files were trying to test for multiple things at once, so I split them up - Fixes issue: multiple test cases/records were attempting to write to the same file path, namely because of type abbreviations confusing things - Adjust code commentary

StachuDotNet · 2022-11-08T15:32:41Z

This has been rebased to latest - ready for another review.

StachuDotNet force-pushed the expand-backend-clienttypes branch from c9f868a to 785949a Compare October 24, 2022 20:31