s3: fix handling of NULL blob.contentType values

[alxndrsn]

Ref #1351

This aligns handling of NULL values for blob."contentType" between postgres-stored and s3-stored blobs.

[lognaturel]

This seems like a reasonable fix for now and we can take some time to consider what an improvement to both cases would look like.

[ktuite]

I was able to locally reproduce the error TypeError: response header response-content-type should be of type "string" (with a dev server connected to s3) and then verify that this fixes it. Checking out the related PRs now.

[ktuite]

The only other thing I was wondering about was if it was possible/worth checking this in the integration tests

    it.only('should get back null content-type when blob is on s3', testService(async (service, { Blobs, all }) => {
      global.s3.enableMock();
      const asAlice = await service.login('alice');

      await asAlice.post('/v1/projects/1/forms?publish=true')
        .set('Content-Type', 'application/xml')
        .send(testData.forms.binaryType)
        .expect(200);

      await asAlice.post('/v1/projects/1/forms/binaryType/submissions')
        .send(testData.instances.binaryType.both)
        .set('Content-Type', 'text/xml')
        .expect(200);

      await asAlice.post('/v1/projects/1/forms/binaryType/submissions/both/attachments/my_file1.mp4')
        .send('testvideo')
        .set('Content-Type', '') // set empty content type
        .expect(200);

      const blobs = await all(sql`select id, "contentType", md5 from blobs`);
      console.log(blobs);

      await Blobs.s3UploadPending();

      await asAlice.get('/v1/projects/1/forms/binaryType/submissions/both/attachments/my_file1.mp4')
        .expect(307)
        .then(({ headers, body }) => {
          console.log("headers", headers);
          console.log("body", body);
        });
    }));

but the bug fix is in lib/external/s3.js getRespHeaders which is just a utility of the s3 external utility and isn't part of s3 the mock.

If we go down a longer adventure of setting content types more appropriately, we would probably want to test it more here, but for now, this patch is good.

[alxndrsn]

The only other thing I was wondering about was if it was possible/worth checking this in the integration tests

    it.only('should get back null content-type when blob is on s3', testService(async (service, { Blobs, all }) => {
      global.s3.enableMock();
      const asAlice = await service.login('alice');

      await asAlice.post('/v1/projects/1/forms?publish=true')
        .set('Content-Type', 'application/xml')
        .send(testData.forms.binaryType)
        .expect(200);

      await asAlice.post('/v1/projects/1/forms/binaryType/submissions')
        .send(testData.instances.binaryType.both)
        .set('Content-Type', 'text/xml')
        .expect(200);

      await asAlice.post('/v1/projects/1/forms/binaryType/submissions/both/attachments/my_file1.mp4')
        .send('testvideo')
        .set('Content-Type', '') // set empty content type
        .expect(200);

      const blobs = await all(sql`select id, "contentType", md5 from blobs`);
      console.log(blobs);

      await Blobs.s3UploadPending();

      await asAlice.get('/v1/projects/1/forms/binaryType/submissions/both/attachments/my_file1.mp4')
        .expect(307)
        .then(({ headers, body }) => {
          console.log("headers", headers);
          console.log("body", body);
        });
    }));

but the bug fix is in lib/external/s3.js getRespHeaders which is just a utility of the s3 external utility and isn't part of s3 the mock.

If we go down a longer adventure of setting content types more appropriately, we would probably want to test it more here, but for now, this patch is good.

Can you clarify what you're suggesting testing here? Is it the Content-Type header of the 307 redirect?