search

gristlabs / grist-core

Grist is the evolution of spreadsheets.
https://www.getgrist.com
Apache License 2.0
7.27k stars 323 forks source link

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

Closed Spoffy closed 1 month ago

Spoffy commented 1 month ago

Context

Error is caused due to these steps:

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 commented 1 month ago

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 commented 1 month ago

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.

Spoffy commented 1 month ago

Added some tests that cover this change.

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