Store ghc version for each snippet #24

trevorsibanda · 2023-03-01T18:02:43Z

Proposed changes

Move Contents out of DB and rename to Paste
Add ghcVersion field to Paste
Alter paste table to include ghcVersion column
Modify play.mustache and play-index.ts
Add db migration script

Contributor checklist

[ x] My PR is related to Save GHC version in stored pastes #15, if applicable
[x ] I have read and understood the CONTRIBUTING guide

tomsmeding · 2023-03-01T19:09:54Z

Haven't built or tested locally, but from reading the code, this looks good so far. :) Thanks!

tomsmeding · 2023-03-02T12:44:53Z

Something that I thought of (not sure if this is still applicable after your last commit): I'm not sure I'd like the string "default" to ever appear in a Version. This is kind of in-band signalling of an unusual value, which I usually prefer to model using an ADT, i.e. Maybe Version in this case. In the database, the same idea would apply by making the ghcVersion column nullable, and storing NULL whenever there is no version specified.

trevorsibanda · 2023-03-03T09:07:42Z

I've made the ghcVersion column nullable and left it empty if it's not a supported version.

play-haskell-server/src/DB.hs

Co-authored-by: Piotr Lewandowski <[email protected]>

piotr-lewandowski

Honestly, can't really say much about the code, but you requested a review.

Anyway, thanks for the fix :)

tomsmeding · 2023-03-17T07:53:19Z

Oh oops -- apparently I do not get notifications of PRs being set to "ready for review", and I'd missed most of what happened here completely. Sorry for the silence.

I'll try to review this either today or early next week.

tomsmeding

Disclaimer: I didn't test yet! Just looked at the code in the PR here.

Mostly looks good, couple of small things. Perhaps you can have a look at those? If you have no time any more I can also address them if you want.

tomsmeding · 2023-03-17T08:12:29Z

play-haskell-server/src/DB.hs

@@ -71,6 +69,7 @@ schemaVersion :: Int
   ,"CREATE TABLE pastes (\n\
    \    id INTEGER PRIMARY KEY NOT NULL, \n\
    \    key BLOB NOT NULL, \n\
+    \    ghcVersion TEXT, \n\


please add an explicit NULL after TEXT

tomsmeding · 2023-03-17T08:12:46Z

play-haskell-server/play.mustache

+preload_ghc_version = {{&version}};
+{{/version}}
+{{^preload_ghc_version}}
+preload_ghc_version = "default";


I don't like the in-band signalling here very much. I think it would be better if the "default" signal was null instead. null here would correspond to NULL in the database.

tomsmeding · 2023-03-17T08:13:20Z

play-haskell-server/migrate_db_5_6.sh

+
+UPDATE meta SET version = 6;
+
+ALTER TABLE pastes ADD COLUMN ghcVersion TEXT;


Also add an explicit NULL here after TEXT

tomsmeding · 2023-03-17T08:19:47Z

play-haskell-server/static/play-index.ts

 const snippet = preload_script != null ? preload_script : example_snippets[Math.floor(Math.random() * example_snippets.length)];
+if (preload_ghc_version == "default") {
+	preload_ghc_version = defaultGHCversion
+}


When switching away from "default", this whole if-statement can go :)

tomsmeding · 2023-03-17T08:28:14Z

play-haskell-server/src/Play.hs

-      else do body <- readRequestBody (fromIntegral @Int @Word64 maxSaveFileSize)
-              let body' = BSL.toStrict body
-              let contents = Contents [(Nothing, body')] Nothing Nothing
+      then lift $ httpError 429 "Please slow down a bit, you're rate limited"


when isSpam $ do lift $ httpError 429 "Please slow down a bit, you're rate limited" exitEarly () postdata <- ...

tomsmeding · 2023-03-17T08:32:47Z

play-haskell-server/src/Play.hs

+                _ -> do lift (httpError 400 "Invalid JSON")
+                        exitEarly ()
+              versions <- liftIO (WP.getAvailableVersions (ctxPool ctx))
+              let version' = fromMaybe defaultGHCVersion $ Just <$> L.find (==version) versions


So this evaluates to fromMaybe Nothing (fmap Just x), which is equivalent to x. I get that you want to abstract defaultGHCVersion out, but since the intuitive semantics of Maybe correspond quite nicely with what is meant here ("Nothing indicates no version specified"), an alternative is to embrace the Maybe, remove defaultGHCVersion (which doesn't really yield a default version, it yields no version which is typically interpreted as meaning some default version), and simplify this to L.find (==version) versions. Which, by the way, is a nice trick.

tomsmeding · 2023-03-17T18:34:58Z

play-haskell-server/src/Play.hs

-              let contents = Contents [(Nothing, body')] Nothing Nothing
+      then lift $ httpError 429 "Please slow down a bit, you're rate limited"
+      else do postdata <- getRequestBodyEarlyExit maxSaveFileSize "Program too large"
+              ClientSavePasteReq{csprCode =code, csprVersion = version} <- case J.decodeStrict' postdata of


missing space after =

tomsmeding · 2023-03-17T18:37:01Z

play-haskell-server/static/play-index.ts

-			const readable = versions[i] in ghcReadableVersion ? ghcReadableVersion[versions[i]] : versions[i];
-			opt.textContent = "GHC " + readable;
-			if (versions[i] == defaultGHCversion) opt.setAttribute("selected", "");
+			let readable = versions[i] in ghcReadableVersion ? ghcReadableVersion[versions[i]] : versions[i];


No reason for readable to become non-const, right?

tomsmeding · 2023-03-17T18:37:36Z

play-haskell-server/static/play-index.ts

+			if (versions[i] === defaultGHCversion) {
+				verAnnotation = "(Default) "
+			}
+			opt.textContent = verAnnotation + "GHC " + readable;


I think I prefer the annotation to come at the end, so GHC <version> (Default). Then the entries in the list all start in a uniform way (with GHC ).

tomsmeding · 2023-03-17T20:16:20Z

play-haskell-server/src/Pages.hs

+
+versionToMustache :: Maybe Version -> Mustache.Value
+versionToMustache = \case
+  Just ( Version v) -> toMustache v


Superfluous space after (.
Also, versionToMustache = mixinMaybeNull (\(Version v) -> v) if I'm not mistaken.

Store ghc version for each snippet

012d46a

Accept json body for save

9f6c98d

ghcVersion DB field should be nullable

13c300a

trevorsibanda marked this pull request as ready for review March 3, 2023 09:05

trevorsibanda changed the title ~~WIP: Store ghc version for each snippet~~ Store ghc version for each snippet Mar 3, 2023

piotr-lewandowski reviewed Mar 4, 2023

View reviewed changes

play-haskell-server/src/DB.hs Outdated Show resolved Hide resolved

Apply formatting play-haskell-server/src/DB.hs

c1f82b9

Co-authored-by: Piotr Lewandowski <[email protected]>

trevorsibanda requested a review from piotr-lewandowski March 15, 2023 09:52

piotr-lewandowski approved these changes Mar 16, 2023

View reviewed changes

tomsmeding requested changes Mar 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store ghc version for each snippet #24

Store ghc version for each snippet #24

trevorsibanda commented Mar 1, 2023

tomsmeding commented Mar 1, 2023

tomsmeding commented Mar 2, 2023

trevorsibanda commented Mar 3, 2023

piotr-lewandowski left a comment

tomsmeding commented Mar 17, 2023

tomsmeding left a comment

tomsmeding Mar 17, 2023

tomsmeding Mar 17, 2023

tomsmeding Mar 17, 2023

tomsmeding Mar 17, 2023

tomsmeding Mar 17, 2023

tomsmeding Mar 17, 2023

tomsmeding Mar 17, 2023

tomsmeding Mar 17, 2023

tomsmeding Mar 17, 2023

tomsmeding Mar 17, 2023


		UPDATE meta SET version = 6;

		ALTER TABLE pastes ADD COLUMN ghcVersion TEXT;

Store ghc version for each snippet #24

Are you sure you want to change the base?

Store ghc version for each snippet #24

Conversation

trevorsibanda commented Mar 1, 2023

Proposed changes

Contributor checklist

tomsmeding commented Mar 1, 2023

tomsmeding commented Mar 2, 2023

trevorsibanda commented Mar 3, 2023

piotr-lewandowski left a comment

Choose a reason for hiding this comment

tomsmeding commented Mar 17, 2023

tomsmeding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment