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.

Sign up for GitHub

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

Jump to bottom

Fix large file uploads #219

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Fix large file uploads #219

merged 2 commits into from
Aug 24, 2023

Conversation

lzaoral
Copy link
Contributor

@lzaoral lzaoral commented Aug 22, 2023

Resolves: #216

@lzaoral lzaoral mentioned this pull request Aug 22, 2023
Closed
rohanpm
rohanpm reviewed Aug 23, 2023
View reviewed changes
kobo/django/upload/models.py
@@ -25,7 +26,8 @@ class FileUpload(models.Model):
owner = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE)
name = models.CharField(max_length=255)
checksum = models.CharField(max_length=255)
size = models.PositiveIntegerField()
# models.PositiveBigIntegerField would be even better but it was introduced only in Django 3.1
size = models.BigIntegerField(validators=[MinValueValidator(0)])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea what happens with this code if the migration hasn't been applied? Can a model field declared as BigInteger successfully read/write on the table (assuming values are "small") if the migration is ignored?

Basically the same concern/question as #211 (comment) .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, that the situation here is different than in #211 (comment). This migration does not create a new field, it just extends the range of an exiting one.

I've tested this code without applied migrations on a local instance of OpenScanHub and it works as expected, small files still work fine and large files raise a database error:

  1. Small file:
$ osh/client/osh-cli mock-build --tarball-build-script=test ./small
Task info: http://0.0.0.0:8000/task/2/
Watching tasks (this may be safely interrupted)...
2 MockBuild: FREE
--> FREE: 1 [total: 1]
...
  1. Large file:
$ osh/client/osh-cli mock-build --tarball-build-script=test ./large
...
xmlrpc.client.Fault: <Fault 1: Traceback (most recent call last):
  File "/usr/lib/python3.6/site-packages/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
psycopg2.DataError: integer out of range
...

@lzaoral
django: use BigIntegerField for file upload sizes
ac2dfad 
Otherwise, we cannot log uploads that are larger than 2 GB.

Resolves: release-engineering#216
@lzaoral
client: allow file upload size to be sent a string
f277aae 
... for uploads which sizes exceed the xmlrpc.client.MAXINT limit for int
values.  This change should not be backwards incompatible.  Old client
with newer hub should work seamlessly.  And new hub with older client
would crash with the following traceback anyway.

Fixes the following traceback:
```
$ truncate -s 2G test
$ osh/client/osh-cli mock-build --tarball-build-script=test ./test
Traceback (most recent call last):
  File "/src/osh/client/osh-cli", line 79, in <module>
    main()
  File "/src/osh/client/osh-cli", line 72, in main
    parser.run()
  File "/src/kobo/kobo/cli.py", line 296, in run
    cmd.run(*cmd_args, **cmd_kwargs)
  File "/src/osh/client/commands/cmd_diff_build.py", line 157, in run
    target_dir, self.parser)
  File "/src/osh/client/commands/shortcuts.py", line 145, in upload_file
    return hub.upload_file(os.path.expanduser(srpm), target_dir)
  File "/src/kobo/kobo/client/__init__.py", line 473, in upload_file
    upload_id, upload_key = self.upload.register_upload(os.path.basename(file_name), checksum, fsize, target_dir)
  File "/usr/lib64/python3.6/xmlrpc/client.py", line 1112, in __call__
    return self.__send(self.__name, args)
  File "/usr/lib64/python3.6/xmlrpc/client.py", line 1446, in __request
    allow_none=self.__allow_none).encode(self.__encoding, 'xmlcharrefreplace')
  File "/usr/lib64/python3.6/xmlrpc/client.py", line 971, in dumps
    data = m.dumps(params)
  File "/usr/lib64/python3.6/xmlrpc/client.py", line 502, in dumps
    dump(v, write)
  File "/usr/lib64/python3.6/xmlrpc/client.py", line 524, in __dump
    f(self, value, write)
  File "/usr/lib64/python3.6/xmlrpc/client.py", line 540, in dump_long
    raise OverflowError("int exceeds XML-RPC limits")
OverflowError: int exceeds XML-RPC limits
```

Resolves: release-engineering#216
@lzaoral
Copy link
Contributor Author

lzaoral commented Aug 23, 2023

I've pushed a new version that calls the validators after the upload_key is set. Otherwise, the code could raise an ValidationError.

@rohanpm rohanpm merged commit ac4e5dc into release-engineering:master Aug 24, 2023
15 checks passed
@lzaoral lzaoral deleted the fix-large-uploads branch August 24, 2023 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohanpm rohanpm rohanpm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

client fails to upload very large files
2 participants
@lzaoral @rohanpm