Fixes an import error when using S3/Minio with no redis #1224

Spoffy · 2024-09-24T20:48:49Z

Context

Error is caused due to these steps:

File is uploaded to Home server and attempts to import
Import ends up in claimDocument in HostedStorageManager
Tries to read doc metadata from DocWorkerMap, gets 'unknown' as md5 hash
Thinks local doc is out of date and erases it.
Downloads a non-existent file from S3, so import fails as it has no data.

Proposed solution

This fixes it by checking for DummyDocWorker's special 'unknown' MD5, forcing an S3 check.

Related issues

https://community.getgrist.com/t/no-metadata-for-imported-grist-document/6029/32

Has this been tested?

Yes locally, but we don't have a CI/CD setup for S3 with no redis currently.

paulfitz · 2024-09-25T14:50:24Z

Yes locally, but we don't have a CI/CD setup for S3 with no redis currently.

Hmm, I think it may be possible to repeat the HostedStorageManager tests with REDIS_URL or TEST_REDIS_URL turned off, in which case S3/minio would still be used but Redis would be unavailable. But maybe it wouldn't even have caught this problem...

paulfitz · 2024-09-25T15:29:16Z

Yes locally, but we don't have a CI/CD setup for S3 with no redis currently.

Hmm, I think it may be possible to repeat the HostedStorageManager tests with REDIS_URL or TEST_REDIS_URL turned off, in which case S3/minio would still be used but Redis would be unavailable. But maybe it wouldn't even have caught this problem...

This does seem doable, but the tests in the file don't exercise importing so they pass without this fix.

Hmm test/nbrowser/Importer.ts fails quickly if run again S3 and not Redis, then succeeds if your fix is applied.

It is maybe a bit excessive to run a whole new copy of all browser tests under this condition, but @Spoffy do you think it would be worth adding a clause in .github/workflows/main.yml to add test/nbrowser/Importer.ts to one of the runs, running it with S3 and with/without Redis? Or maybe there is a unit-like test that needs adding to HostedStorageManager, and have that do a run (say of cache or minio) without redis.

Error is caused due to these steps: - File is uploaded to Home server and attempts to import - Import ends up in `claimDocument` in `HostedStorageManager` - Tries to read doc metadata from DocWorkerMap, gets 'unknown' as md5 hash - Thinks local doc is out of date and erases it. - Downloads a non-existent file from S3, so import fails as it has no data. This fixes it by checking for DummyDocWorker's special 'unknown' MD5, forcing an S3 check.

Spoffy · 2024-09-30T12:56:22Z

Added some tests that cover this change.

I also altered the constructors for HostedMetadataManager and HostedStorageManager to simplify mocking when testing.

paulfitz

Thanks for the test! Some small comments.

paulfitz · 2024-09-30T12:59:00Z

app/server/lib/DocManager.ts

@@ -638,6 +638,7 @@ export class DocManager extends EventEmitter {
        throw new Error('Grist docs must be uploaded individually');
      }
      const first = uploadInfo.files[0].origName;
+      log.debug(`DocManager._doImportDoc: Received doc with name ${first}`);


Is there any context that can be added here? For example, what user this is on behalf of, the team site, anything like that. Otherwise this isn't very helpful in prod, though I can see how it could be useful to you while you work on this change. Admittedly your log messages are consistent with the style in other parts of DocManager, but for new messages it would be great to get closer to what new messages in ActiveDoc do, see uses of this.getLogMeta(docSession) there.

I think my main goal for leaving this in, was to know when a file was traveling along the import route.

I'll see if I can workspace / org / userId context? Up to you if this is worth having in.

The thing is without the context these kinds of logs are mostly useless in prod because they are mixed in with tons of parallel activity. Some kind of context can help you filter, or piece a narrative together. Still, not going to insist on it.

paulfitz · 2024-09-30T13:00:06Z

app/server/lib/HostedMetadataManager.ts

 import log from 'app/server/lib/log';

+export type SetDocsMetadataFunc = (metadata: { [p: string]: DocumentMetadata }) => Promise<any>;


Can you add a comment on what this type is for.

Added a comment, and slightly renamed the type for a bit more clarity.

paulfitz · 2024-09-30T13:03:03Z

app/server/lib/HostedStorageManager.ts

@@ -52,6 +52,11 @@ function checkValidDocId(docId: string): void {
  }
 }

+export interface HostedStorageCallbacks {


Can you add a comment on what this interface is for.

I added comments to the individual functions, as I was struggling to come up with a comment for this that wasn't just a paraphrased explanation of dependency injection. Open to suggestions on this!

It would be good to add the comment though.

I don't understand why dbManager is not enough. What other things can change that behavior (with links).

paulfitz · 2024-09-30T13:04:05Z

test/server/lib/HostedStorageManager.ts

@@ -33,6 +33,7 @@ import {createTmpDir, getGlobalPluginManager} from 'test/server/docTools';
 import {EnvironmentSnapshot, setTmpLogLevel, useFixtureDoc} from 'test/server/testUtils';
 import {waitForIt} from 'test/server/wait';
 import uuidv4 from "uuid/v4";
+import {IDocWorkerMap} from "app/server/lib/DocWorkerMap";


As much as possible, try to order imports alphabetically, and use the local quote style (I realize it is already inconsistent in this file)

paulfitz · 2024-09-30T13:06:33Z

test/server/lib/HostedStorageManager.ts

+      tmpDir = await createTmpDir();
+      oldEnv = new EnvironmentSnapshot();
+      // Disable Redis
+      delete process.env.REDIS_URL;


There's also a TEST_REDIS_URL I see mentioned in code, might that need wiping too?

That should be fine - TEST_REDIS_URL seems to be used explicitly by other tests to configure where to connect to.

All we care about here is making sure we get DummyDocWorker out of getDocWorkerMap, which removing process.env.REDIS_URL is sufficient to do. 🙂

I'm afraid it gets set in test/testrun.sh in SaaS tests :( but yes it would be ok to deal with that if tests arrive there and are failing.

paulfitz · 2024-09-30T13:18:05Z

test/server/lib/HostedStorageManager.ts

+
+      const creator = create?.getStorageOptions?.('minio')?.create;
+      if (!creator) {
+        return this.skip();


Hmm this won't skip if minio is not configured. What do you think about something like:

const storage = create?.getStorageOptions?.('minio'); const creator = storage?.create; if (!creator || !storage?.check?.()) {

paulfitz

Thanks @Spoffy

paulfitz self-requested a review September 25, 2024 12:10

Spoffy added 4 commits September 28, 2024 10:52

Tightens HostedStorageManager dependencies for easier testing

5f27f03

Adds test to check imported files aren't wiped

7a3d9e6

Adds test for fetching remote docs with minio and no redis

189aea5

Spoffy force-pushed the spoffy/fix-import-error-s3-no-redis branch from 5680d1f to 189aea5 Compare September 28, 2024 12:15

paulfitz reviewed Sep 30, 2024

View reviewed changes

Spoffy added 3 commits October 1, 2024 19:21

Renames and adds comment explaining SetDocsMetadataFunc

2dfaf4c

Adds explanatory comments for HostedStorageManager callbacks

d3caaad

Skips MinIO without redis checks if MinIO not configured

3199444

Spoffy requested a review from paulfitz October 1, 2024 18:23

paulfitz approved these changes Oct 3, 2024

View reviewed changes

Spoffy merged commit a5c9a49 into main Oct 7, 2024
11 checks passed

Spoffy deleted the spoffy/fix-import-error-s3-no-redis branch October 7, 2024 04:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes an import error when using S3/Minio with no redis #1224

Fixes an import error when using S3/Minio with no redis #1224

Spoffy commented Sep 24, 2024

paulfitz commented Sep 25, 2024

paulfitz commented Sep 25, 2024

Spoffy commented Sep 30, 2024

paulfitz left a comment

paulfitz Sep 30, 2024

Spoffy Oct 1, 2024

paulfitz Oct 3, 2024

paulfitz Sep 30, 2024

Spoffy Oct 1, 2024

paulfitz Sep 30, 2024

Spoffy Oct 1, 2024

berhalak Oct 8, 2024

paulfitz Sep 30, 2024

paulfitz Sep 30, 2024

Spoffy Oct 1, 2024

paulfitz Oct 3, 2024

paulfitz Sep 30, 2024

paulfitz left a comment

		import log from 'app/server/lib/log';

		export type SetDocsMetadataFunc = (metadata: { [p: string]: DocumentMetadata }) => Promise<any>;

Fixes an import error when using S3/Minio with no redis #1224

Fixes an import error when using S3/Minio with no redis #1224

Conversation

Spoffy commented Sep 24, 2024

Context

Proposed solution

Related issues

Has this been tested?

paulfitz commented Sep 25, 2024

paulfitz commented Sep 25, 2024

Spoffy commented Sep 30, 2024

paulfitz 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulfitz left a comment

Choose a reason for hiding this comment