Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #65: ignore errors in tarballs/zipfiles #68

Merged
merged 1 commit into from Sep 2, 2020

Conversation

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Sep 2, 2020

There currently isn't really a way to talk back to the client that
they uploaded an invalid tarball/zipfile. So far this only happened
when someone uploaded a zero-byte tarball, which is pretty obvious.
Hopefully that remains this way.

The behaviour is now: if you upload an invalid tarball/zipfile,
after pressing validate, the file is no longer in the filelist.
Hopefully that gives enough clues to the user what is going on.

@LordAro
Copy link
Member

@LordAro LordAro commented Sep 2, 2020

Code is fine, but I'm not a fan of (almost) silently ignoring errors. Is there no way of communicating issues at all? How does it communicate invalid content?

@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Sep 2, 2020

How the current system is setup, there are no persistent errors. So when you validate, it checks everything again to see if it works out okay now. This is how invalid content etc is detected.

Tarballs and zipfiles are different, in that they are extracted on upload. There isn't any method of communicating any error here to the end-user.

Basically, this PR resolves the reporting to Sentry about mistakes, but doesn't fix anything towards the end-user. I am also not sure how that would work / look like.

I tried communicating back over the tus-protocol that there was an error, but it seems the javascript implementation completely ignores this, and still marks the upload as OK. I can experiment some more with this. But I am also fine closing this PR, so we can use Sentry to monitor how often people upload shitty stuff :P

@TrueBrain TrueBrain force-pushed the TrueBrain:fix_read_error branch 2 times, most recently from 6568628 to 094aa77 Sep 2, 2020
This is a bit of a hack in comparison with the normal validation
method, but these errors happen on upload, where the other errors
happen on validation.

Sadly, the tusd implementation is a bit wonky, and we cannot really
communicate back with the client when we found such error during
upload (as strictly seen the upload succeeded fine; the content
is broken). So we cannot report the error on upload, and as such
are forced to do it after upload.
@TrueBrain TrueBrain force-pushed the TrueBrain:fix_read_error branch from 094aa77 to 81583d0 Sep 2, 2020
@LordAro
LordAro approved these changes Sep 2, 2020
Copy link
Member

@LordAro LordAro left a comment

:)

@TrueBrain TrueBrain merged commit b9bc0d3 into OpenTTD:master Sep 2, 2020
5 checks passed
5 checks passed
Docker build
Details
Flake8
Details
Black
Details
Regression
Details
LGTM analysis: Python No new or fixed alerts
Details
@TrueBrain TrueBrain deleted the TrueBrain:fix_read_error branch Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.