Gzip package documentation when publishing to Pursuit #659

pete-murphy · 2023-09-04T16:31:03Z

Fixes #642.

pete-murphy · 2023-09-04T16:33:50Z

app/src/App/Effect/Pursuit.purs

-    result <- Run.liftAff $
+    result <- Run.liftAff do
+      gzipped <- Gzip.compress (Argonaut.stringify payload)
+      body <- liftEffect (Buffer.toString UTF8 gzipped)


I haven't tested this. I've tested sending a Buffer as request body directly without converting to string, but that's not currently supported in the upstream types for RequestBody purescript-web/purescript-web-fetch#15.

Just passing Buffer directly gets this to pass the integration tests (e231be6). Before that, was seeing "Not in GZIP format" errors

registry # [ 43.420167] registry-server-init[910]: Pushing to Pursuit... registry # [ 43.625450] registry-server-init[910]: [ERROR] Pursuit publishing failed with status 500 and body registry # [ 43.626327] registry-server-init[910]: { registry # [ 43.626781] registry-server-init[910]: "cause1":"java.util.zip.ZipException: Not in GZIP format", registry # [ 43.627595] registry-server-init[910]: "servlet":"com.github.tomakehurst.wiremock.servlet.WireMockHandlerDispatchingServlet-2dca0d64", registry # [ 43.629109] registry-server-init[910]: "cause0":"java.lang.RuntimeException: java.util.zip.ZipException: Not in GZIP format", registry # [ 43.634155] registry-server-init[910]: "message":"java.lang.RuntimeException: java.util.zip.ZipException: Not in GZIP format", registry # [ 43.635515] registry-server-init[910]: "url":"/packages", registry # [ 43.636876] registry-server-init[910]: "status":"500" registry # [ 43.637356] registry-server-init[910]: } registry # [ 43.637757] registry-server-init[910]: [ERROR] Could not publish your package to Pursuit because an error was encountered (cc: @purescript/packaging): Expected to receive a 201 status from Pursuit, but received 500 instead.

The upstream is actually https://github.com/rowtype-yoga/purescript-fetch-core, and we can more easily make changes to that library. fetch relies on fetch-core instead of web-fetch (until one day we merge them into js-fetch).

Regardless — I think your Gzip newtype workaround is a proper solution. Good intuition!

foreign/test/Foreign/Gzip.purs

pete-murphy · 2023-09-04T16:58:45Z

foreign/test/Foreign/Gzip.purs

-  Spec.it "Compresses a string with gzip" do
-    tmp <- Tmp.mkTmpDir
-    let
-      file = Path.concat [ tmp, "out.gz" ]
-      contents = "<contents>"
-    buffer <- Gzip.compress contents
-    FS.Aff.writeFile file buffer
-    result <- _.result =<< Execa.execa "gzcat" [ file ] identity
-    (_.stdout <$> result) `Assert.shouldEqual` Right contents
+  Spec.describe "compress" do
+    Spec.it "Compresses a string with gzip" do
+      tmp <- Tmp.mkTmpDir
+      let
+        file = Path.concat [ tmp, "out.gz" ]
+        contents = "<contents>"
+      buffer <- Gzip.compress contents
+      FS.Aff.writeFile file buffer
+      result <- _.result =<< Execa.execa "gzcat" [ file ] identity
+      (_.stdout <$> result) `Assert.shouldEqual` Right contents


Odd, without the describe wrapper, the Gzip test results are lumped in with the FastGlob results.

Before:

Foreign » FastGlob ✓︎ Prevents directory traversal ✓︎ Prevents symlink directory traversal ✓︎ Prevents traversal to a non-existing file ✓︎ Compresses a string with gzip

After:

Foreign » FastGlob ✓︎ Prevents directory traversal ✓︎ Prevents symlink directory traversal ✓︎ Prevents traversal to a non-existing file Foreign » Gzip » compress ✓︎ Compresses a string with gzip

Maybe this is a bug in purescript-spec? I opened this issue purescript-spec/purescript-spec#144

Hm — seems like it. I think what you've done in this PR is fine (adding the top describe).

app/src/Fetch/Retry.purs

flip111 · 2023-09-04T21:01:25Z

Hey i reviewed your PR looks good to me. There is an opportunity to tweak the compression algorithm if needed. The gzip function takes a second parameter with options and you can set the compression level there with other parameters https://nodejs.org/api/zlib.html#zlibgzipbuffer-options-callback

pete-murphy · 2023-09-04T21:20:05Z

Hey i reviewed your PR looks good to me. There is an opportunity to tweak the compression algorithm if needed. The gzip function takes a second parameter with options and you can set the compression level there with other parameters https://nodejs.org/api/zlib.html#zlibgzipbuffer-options-callback

Thanks! I have no experience optimizing compression so I have no idea what would be the best settings here and would be open to recommendations. I'm hoping that since (I think) there haven't been any issues publishing via Pulp using the default settings for zlib.createGzip, we might be safe using the defaults here 🤞 .

thomashoneyman · 2023-09-05T00:20:04Z

I'm sorry for my slow reviews this week — I'm only somewhat available, and will be fully back online next week. I'm going to take a quick look through your PR but don't have the time right now to test it out. I promise to get this over the line by next week, however, should it appear to have no issues. Thanks for taking the time to work on this!

thomashoneyman

This is a nice, small patch! It looks totally fine to me module some minor review comments. I might be able to test it tomorrow, in which case I'll get it merged. Thanks!

thomashoneyman · 2023-09-05T00:22:06Z

app/src/Fetch/Retry.purs

@@ -22,16 +22,16 @@ data RetryRequestError
  | StatusError Response

 withRetryRequest
-  :: forall input output thruIn thruOut headers
-   . Union input thruIn (HighlevelRequestOptions headers String)
+  :: forall input output thruIn thruOut headers @body


Is this VTA actually used? I don't see it in the body of the function.

Before these changes, the body option of withRetryRequest was fixed to String, but I've replaced the definition of fetch below with fetchBody from the purescript-fetch library.

Because body could now be any ToRequestBody body => body, I needed to specify @String in these call sites (the compiler didn't seem to infer as String without the VTA, from what I tried)

registry-dev/app/src/App/Effect/Pursuit.purs

Line 104 in 6677362

result <- Run.liftAff $ Fetch.withRetryRequest @String url

registry-dev/app/src/App/Effect/Source.purs

Line 139 in 6677362

Run.liftAff $ Fetch.withRetryRequest @String archiveUrl {}

registry-dev/app/src/App/Effect/Storage.purs

Line 247 in 6677362

response <- Run.liftAff $ Fetch.withRetryRequest @String packageUrl {}

An alternative that comes to mind, to avoid adding those @String changes, would be keeping both fetch and fetchWithBody implementations copied from purescript-fetch and then adding a retryWithRequestBody so we don't have to update call sites of retryWithRequest

I needed to specify @String in these call sites (the compiler didn't seem to infer as String without the VTA, from what I tried)

Hm, nevermind, trying again without VTAs and type inference is working fine. I think at one point there was an explicit ToRequestBody body constraint that required the type annotation. I copied the constraint from the upstream definition of fetchBody, but from what I can tell it's not really necessary.

Removed here 0a02e14

thomashoneyman · 2023-09-05T00:27:48Z

foreign/src/Foreign/Gzip.purs

+foreign import toRequestBodyImpl :: Buffer -> RequestBody
+
+newtype Gzip = Gzip Buffer
+
+instance ToRequestBody Gzip where
+  toRequestBody (Gzip buffer) = toRequestBodyImpl buffer


I typically like to write functions like this one with unsafeCoerce (since it's a straightforward coercion) instead of a as a foreign import, because you can tell at a glance how it works instead of having to look at the FFI to see if there's anything interesting happening.

Would you mind reimplementing the foreign import as something like this?

-- could also be 'gzipToRequestBody :: Gzip -> RequestBody' and the instance: -- instance ToRequestBody Gzip where -- toRequestBody = gzipToRequestBody coerceToRequestBody :: Buffer -> RequestBody coerceToRequestBody = unsafeCoerce

Updated here 74cca55 with a link to rowtype-yoga/purescript-fetch#11

thomashoneyman · 2023-09-12T01:00:20Z

Thank you!

pete-murphy added 2 commits September 4, 2023 12:30

Add Gzip FFI module and test

c75e521

Gzip request body when publishing to Pursuit

272a2eb

pete-murphy commented Sep 4, 2023

View reviewed changes

foreign/test/Foreign/Gzip.purs Outdated Show resolved Hide resolved

Add Spec.describe block

029905a

pete-murphy commented Sep 4, 2023

View reviewed changes

pete-murphy added 3 commits September 4, 2023 13:02

Try with zcat instead of gzcat

54de9ba

WIP: Attempt overloading fetch body

e231be6

Remove unused VTA

ca71737

pete-murphy commented Sep 4, 2023

View reviewed changes

app/src/Fetch/Retry.purs Outdated Show resolved Hide resolved

pete-murphy marked this pull request as ready for review September 4, 2023 18:25

pete-murphy mentioned this pull request Sep 4, 2023

Gzip package documentation when sending to Pursuit #642

Closed

Use newtype for Gzip

6677362

thomashoneyman reviewed Sep 5, 2023

View reviewed changes

pete-murphy added 2 commits September 4, 2023 21:32

Remove unused VTA

0a02e14

Move toRequestBody coerce function out of FFI

74cca55

thomashoneyman approved these changes Sep 12, 2023

View reviewed changes

thomashoneyman merged commit 1b9fc7d into purescript:master Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gzip package documentation when publishing to Pursuit #659

Gzip package documentation when publishing to Pursuit #659

pete-murphy commented Sep 4, 2023

pete-murphy Sep 4, 2023

pete-murphy Sep 4, 2023

thomashoneyman Sep 5, 2023

thomashoneyman Sep 5, 2023

pete-murphy Sep 4, 2023 •

edited

Loading

pete-murphy Sep 4, 2023 •

edited

Loading

thomashoneyman Sep 5, 2023

flip111 commented Sep 4, 2023

pete-murphy commented Sep 4, 2023

thomashoneyman commented Sep 5, 2023

thomashoneyman left a comment

thomashoneyman Sep 5, 2023

pete-murphy Sep 5, 2023 •

edited

Loading

pete-murphy Sep 5, 2023 •

edited

Loading

pete-murphy Sep 5, 2023 •

edited

Loading

pete-murphy Sep 5, 2023

thomashoneyman Sep 5, 2023

pete-murphy Sep 5, 2023 •

edited

Loading

thomashoneyman commented Sep 12, 2023

Gzip package documentation when publishing to Pursuit #659

Gzip package documentation when publishing to Pursuit #659

Conversation

pete-murphy commented Sep 4, 2023

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

pete-murphy Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

pete-murphy Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip111 commented Sep 4, 2023

pete-murphy commented Sep 4, 2023

thomashoneyman commented Sep 5, 2023

thomashoneyman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pete-murphy Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

pete-murphy Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

pete-murphy Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pete-murphy Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

thomashoneyman commented Sep 12, 2023

pete-murphy Sep 4, 2023 •

edited

Loading

pete-murphy Sep 4, 2023 •

edited

Loading

pete-murphy Sep 5, 2023 •

edited

Loading

pete-murphy Sep 5, 2023 •

edited

Loading

pete-murphy Sep 5, 2023 •

edited

Loading

pete-murphy Sep 5, 2023 •

edited

Loading