Details

Reviewers

Commits

Summary

Part of ENG-3530.

Added possibility of storing blobHolder in the extra column of the uploads table.
Having this value requires the content column to be empty (for consistency - data is stored either on keyserver or Blob service).
Fetching uploads from keyserver url should return resource_unavailable error if the blobHolder is set.

Depends on D7617

Test Plan

Repeated test plan from two previous diffs, this time updated the DB columns directly instead of artifically hardcoding blob URIs.
Also tried to fetch external upload with http://KEYSERVER_ADDR/uploads/{id}/{secret} - ensured that resource_unavailable error is thrown.
DB Insertion will be tested in further diffs.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

bartek held this revision as a draft.

bartek published this revision for review.Apr 25 2023, 11:44 AM

keyserver/src/creators/upload-creator.js
48–51 ↗	(On Diff #25692)	`invariant` should be used for things that should never happen, and are a result of programmer error. Most common use case is to assert things for Flow For errors that can be caused by a malicious user, I don't think we want to use `invariant`... can you `throw new ServerError('invalid_parameters')` instead? `ServerError` is special-cased in error-handling code

keyserver/src/creators/upload-creator.js
48–51 ↗	(On Diff #25692)	`invariant` should be used for things that should never happen, and are a result of programmer error This is exactly what I'm doing here! This is a double-check for the call sites in D7619 (`uploads.js`) - where I set `new Buffer(0)`. Another (better?) approach could probably be replacing `buffer` and `blobHolder` with some union type `Buffer \| string` or even an object union: `{ type: 'blob_service' \| holder: string } \| { type: 'keyserver', content: Buffer }`

keyserver/src/creators/upload-creator.js
48–51 ↗	(On Diff #25692)	Ah! Thanks for clarifying. Yes I think it would be better to type this accurately instead of having to use an `invariant`

Replaced invariant with union type to be clear about upload storage

Nothing major here, but enough comments that I figured it would be good to do another cycle of review

keyserver/src/creators/upload-creator.js
18–26 ↗	(On Diff #25807)	Should be safe to make this read-only
29–35 ↗	(On Diff #25807)	Does it introduce type errors to make all of these read-only? It would be good to guarantee to callers of `createUploads` that the function won't mutate its inputs
54–56 ↗	(On Diff #25807)	Why not use your `makeURI` utility function here?
75 ↗	(On Diff #25807)	Can we try to avoid including `blobHolder: undefined` in the database?
keyserver/src/fetchers/upload-fetchers.js
44 ↗	(On Diff #25807)	This was a concerning line to see as the `extra` column appears to be nullable, and `JSON.parse(undefined)` will throw an exception. However, I ran a couple queries on my production keyserver and it seems like `extra` is always populated: MariaDB [comm]> SELECT id FROM uploads WHERE extra IS NULL; Empty set (0.004 sec) MariaDB [comm]> SELECT id FROM uploads WHERE extra = ""; Empty set (0.004 sec) We should probably update the database schema to indicate that this field is required. Can you create a backlog task for that?
221 ↗	(On Diff #25807)	It's a little confusing that we overload `holder` to mean two things: Raw holder Raw holder prefixed with `comm-blob-service://` It's a minor difference, so probably fine to leave as-is (especially since changing it here would require painful refactors)

This revision now requires changes to proceed.Apr 27 2023, 12:09 PM

keyserver/src/creators/upload-creator.js
29–35 ↗	(On Diff #25807)	Not sure why it wasn't read-only earlier. I'll try to fix this.
54–56 ↗	(On Diff #25807)	It's not exported from the other file
75 ↗	(On Diff #25807)	`JSON.stringify()` skips `undefined` values, but leaves `null`. This is why I set `undefined` in L53
keyserver/src/fetchers/upload-fetchers.js
44 ↗	(On Diff #25807)	Sure, will do
221 ↗	(On Diff #25807)	Yep, I realized this a bit too late when the new types including `holder` were already populated. It was my bad design idea to "temporarily" use `holder` as keyserver URI for non-blob-service uploads - this introduced the overload now. Also this is why I proposed to skip the prefix as one of the options in the "Design holder format" task.