Skip to content

Conversation

@angelsk
Copy link
Contributor

@angelsk angelsk commented Sep 30, 2022

When the bucket exists then error is null which causes this code to fail with an error TypeError: Cannot read properties of null (reading 'code').

We are using "@google-cloud/storage": "^6.5.2", but there's no harm in checking if a variable is not null before trying to access a property.

This is with the latest "tus-node-server": "^0.8.0", release

When the bucket exists then `error` is `null` which causes this code to fail with an error  `TypeError: Cannot read properties of null (reading 'code')`.

We are using `"@google-cloud/storage": "^6.5.2",` but there's no harm in checking if a variable is not null before trying to access a property.
@angelsk angelsk changed the title Fix issue where there is no error Fix issue where there is no error checking bucket exists in GCSDataStore Sep 30, 2022
@angelsk
Copy link
Contributor Author

angelsk commented Sep 30, 2022

Sorry, I don't know why the test runs are failing - seems to be a timeout issue. Don't know how quick releases are made - this seems to be a fairly well maintained product :). But I am going to sadly have to downgrade the version until this can be patched coz my docker won't run.

Hope this helps

@mitjap
Copy link
Collaborator

mitjap commented Sep 30, 2022

Thank you for this fix. This was recently changed. I wonder how that went past our tests. I recommend we also take a look at tests to see why this didn't trigger any errors.

Yeah, CI is grumpy sometimes :)

@angelsk
Copy link
Contributor Author

angelsk commented Sep 30, 2022

Thank you for this fix. This was recently changed. I wonder how that went past our tests. I recommend we also take a look at tests to see why this didn't trigger any errors.

Yeah, CI is grumpy sometimes :)

It happens in the constructor - is that mocked somehow in the tests?

Or maybe the bucket it's checking in the tests always throws this 403 error?

@mitjap
Copy link
Collaborator

mitjap commented Sep 30, 2022

I guess your docker does not start because you have installed uncaughtException listener (https://nodejs.org/api/process.html#event-uncaughtexception) that terminates your application.

Thrown errors are unhandled and silently ignored. Intended behavior can not work as bucket existence can not be asynchronously checked in constructor. I recommend that we print warning message with logger and continue. Test should handle uncaughtExceptions and fail.

@angelsk
Copy link
Contributor Author

angelsk commented Sep 30, 2022

I guess your docker does not start because you have installed uncaughtException listener (https://nodejs.org/api/process.html#event-uncaughtexception) that terminates your application.

Thrown errors are unhandled and silently ignored. Intended behavior can not work as bucket existence can not be asynchronously checked in constructor. I recommend that we print warning message with logger and continue. Test should handle uncaughtExceptions and fail.

Yes we have an unhandledRejection listener. Which is probably why all the mocha tests were passing but the API fell over in docker.

@angelsk
Copy link
Contributor Author

angelsk commented Sep 30, 2022

Are you able to handle the test part of this? I'm a PHP dev by trade, learning JS and no idea how to approach this beyond I found the error and think I fixed it :)

@mitjap
Copy link
Collaborator

mitjap commented Sep 30, 2022

Sure. We'll handle the rest.

@mitjap
Copy link
Collaborator

mitjap commented Sep 30, 2022

Tests handle unhandled errors everywhere else but not in this function. Not sure why is this so. This behavior is also inconsistent with S3 which reports error when uploading a file. I'll make a separate issue to discuss this and accept this PR.

@mitjap mitjap merged commit 9eeb26c into tus:master Sep 30, 2022
@angelsk angelsk deleted the patch-1 branch October 3, 2022 09:17
@mitjap
Copy link
Collaborator

mitjap commented Oct 3, 2022

@angelsk Version 0.8.1 is released. It should fix your issues.

Murderlon added a commit that referenced this pull request Oct 3, 2022
* 1.x:
  0.8.1
  Fix validation of upload-length header in patch handler (#303)
  Bump @google-cloud/storage from 6.4.1 to 6.5.2 (#304)
  Bump aws-sdk from 2.1206.0 to 2.1227.0 (#305)
  Revert "Bump tus-js-client from 2.3.1 to 3.0.0" (#302)
  Emit upload complete event when using creation-with-upload extension (#301)
  GCS: fix error handling when checking for bucket existence (#299)
@angelsk
Copy link
Contributor Author

angelsk commented Oct 4, 2022

@angelsk Version 0.8.1 is released. It should fix your issues.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants