Redesign stores for better separation of concerns

[mitjap]

Datastores's responsibility should not be handling of http requests but working with readable streams and File class. This PR is just a first step towards this goal, it is probably not working and tests fail miserably. I'd like to get your opinion about these changes before I spend too much time.

Related to #68.

[Acconut]

This looks very interesting, thank you! However, would you mind explaining what the actual changes are? Then I can try to provide some valuable feedback :)

[mitjap]

Every store has identical code for reading and validating headers, generating file name and checking for errors. I think this is not the responsibility of a store, so I moved all this to PostHandler.

Another change was to move event emitting code from stores to PostHandler (or any other handler). This is also to reduce code duplication different stores implementations.

I also propose to remove tus extensions from stores. This is something I think TusServer should be responsible of. For example if store implements create function than server should send creation extension.

[Acconut]

I agree, those are very sensible changes and mimic a similar data storage interface as present in tusd! They will definitely make it easier to develop new tus features independent of the storage layer. Very forward thinking of you, thank you very much!

[mitjap]

@Murderlon I think this PR is still worth working on. I can do some more work but my main issue is that this change breaks almost all existing tests. Is this something you could help me with?

[Murderlon]

@Murderlon I think this PR is still worth working on. I can do some more work but my main issue is that this change breaks almost all existing tests. Is this something you could help me with?

Currently the tests seem to fail and something unrelated, it can't decrypt the secret. I'll look into that. If you want to test locally, you will need credentials for S3 and GCS. If that's not possible for you, I can help with the tests at a later stage. I do agree with the proposed changes and discussions from this PR 👍

[Murderlon]

@mitjap I fixed the tests in #267. If you merge and resolve conflicts we can start to take a look at the tests.

[mitjap]

I've made rough changes so that you can have better understanding of what changes I propose. Main goal is still to simplify stores. Store's only responsibility now is to implement create, write, delete and getOffset functions and nothing more. Ideally I would also separate configstore for metadata but this can be left for next PR :) After this PR is finished stores can be easily extracted to separate packages/repositories.

[Murderlon]

Can you mark as ready for review when it's ready? I'm also surprised to see this PR still doesn't have access to GH secrets after my PR 🤔

[mitjap]

I think this is ready for review. All tests are now passing.

[Murderlon]

If we merge now then the tests finally run properly from forks (tested in your other PR)

[mitjap]

@Murderlon I think this PR is now ready for some serious real world testing.

On the side note: CI test on this PR is running tests against master branch.

[Murderlon]

@Murderlon I think this PR is now ready for some serious real world testing.

Perfect 👌

On the side note: CI test on this PR is running tests against master branch.

Fixed and merged into this branch. Tests are passing.

[mitjap]

I'm not really sure why this last test is failing. It works for me with node 14 and 16. Also the code looks like it should be rejected as expected.

   1) S3DataStore
       write
         should reject when stream is destroyed:
     Error: Timeout of 15000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/tus-node-server/tus-node-server/test/Test-S3DataStore.js)
      at listOnTimeout (internal/timers.js:557:17)
      at processTimers (internal/timers.js:500:7)

[Murderlon]

@mitjap any chance you'll have time for the final tiny steps to get this merged? I'm thinking of starting with the TypeScript rewrite and such after this PR to avoid merge conflicts.

[Murderlon]

I'm a bit confused why CI runs successfully on Node 16.x but not on 14.x? I don't see any errors about using something unsupported.

[mitjap]

I'm a bit confused why CI runs successfully on Node 16.x but not on 14.x? I don't see any errors about using something unsupported.

I skipped GCS test and all other tests work. https://github.com/tus/tus-node-server/actions/runs/3122672586/jobs/5064763568

Are credentials still valid? Is there some firewall preventing CI communication with GCS? Tests fail with Retry limit exceeded which is thrown from @google-cloud.

[Murderlon]

Are credentials still valid? Is there some firewall preventing CI communication with GCS? Tests fail with Retry limit exceeded which is thrown from @google-cloud.

Testing locally with npm run test:local, which uses the unencrypted version of keyfile.json.gpg (same as in CI), works for me. This makes me think something did in fact change in this PR which makes the tests fail.

I'm very appreciative for your efforts in this PR but I think skipping all GCS tests is not an acceptable solution. I'll checkout this PR locally and see what I can do.

[Murderlon]

The tests in this branch also run successfully for me locally and other PRs also still work the GCS tests 🤔. Bit of a mystery.

[Murderlon]

Things work again 🤷‍♂️

[mitjap]

I was not suggesting to leave test ignored, just wanted to see that other tests pass and that GCS is to blame. But I guess everything works as expected after some magic :)

Very happy to see this merged.