search

juliomalegria / django-chunked-upload

Upload large files to Django in multiple chunks, with the ability to resume if the upload is interrupted.
MIT No Attribution
214 stars 71 forks source link

'user' field passed to BaseChunkedUpload causes exception #47

Closed steverecio closed 4 years ago

steverecio commented 5 years ago

In my code I've extended BaseChunkedUpload without adding a user field. If the django request is authenticated, an error is thrown after injecting a user field into the attrs dict here:

https://github.com/juliomalegria/django-chunked-upload/blob/master/chunked_upload/views.py#L184

There should be an extra condition which verifies that a user field exists on self.model before injecting it into attrs.

GitRon commented 5 years ago

@steverecio A pity that this repo seems to be abandoned...

steverecio commented 5 years ago

@GitRon Yeah maybe it should be forked

GitRon commented 5 years ago

@steverecio There are more than 30 forks unfortunately... And even if we fork, I wouldn't know how to create a new release based on a fork like in this question: https://stackoverflow.com/questions/54888750/create-pypi-package-for-fork-of-non-maintained-project

GitRon commented 5 years ago

I tried to reach @juliomalegria directly via email but no reply in three weeks. Would you be interested in making a new fork and publish it at pypi, @steverecio ?

jerinpetergeorge commented 5 years ago

@GitRon Even though I don't know much about the pypi release, I'm interested to contribute to this repo.

GitRon commented 5 years ago

@jerinpetergeorge The question is what we'll do about it. This repo is abandoned and frankly I dont have the time to care for a new repo of my own. I really tried but not going to happen any time soon :(

jerinpetergeorge commented 5 years ago

@GitRon maybe I could make a clone of this repo and can able to do some fixes and feature releases.

GitRon commented 5 years ago

@jerinpetergeorge I'd be happy to add my fix for AWS to your new project if we are going to push it to pypi. What name would be nice for the new thing? should resemble the old one somehow. and maybe we just jump to version 2.0.0? important would be to keep a changelog (in a changelog.md or just in the main Readme.md) Publishing a project is quite easy, you'll find tons of info on the internet. Nice thing: There is a test-pypi, you can see if everything looks as nice as it should. If you create a repo and give me the rights I can do the pypi-setup. Just figured out how to do this for another package.

jerinpetergeorge commented 5 years ago

@GitRon Happy to hear that!

I think eitherdjango-chunk-uploads or django-chunk-upload would be nice since those names are available in pip and it resembles with parent package/repo. What do you say?

jerinpetergeorge commented 5 years ago

@GitRon I have a second thought on the naming of the new repo/package. How about django-chunked-upload-2? Yeah, maybe it's a bit ugly. :worried:

If you create a repo and give me the rights I can do the pypi-setup.

I've already forked this project. What rights do you need? Collaborator permissions?

jerinpetergeorge commented 5 years ago

@juliomalegria @GitRon @steverecio I've created a new package named django-chunk-upload from this repository and released the Version 2.0.0--(changes). Hope it's clean and neat. If you guys found anything wrong, please let me know.

jerinpetergeorge commented 5 years ago

@steverecio I don't think your changes won't be appropriate. Because of the statement,

if hasattr(self.model, 'user'):
    ....

is more likely if True since hasattr(self.model, 'user') will be True always.

The key point is , we can't drop any field in a Abstact Model (or any Python class) .

GitRon commented 5 years ago

@jerinpetergeorge Nice! Any idea how I can create a pull request so you can add my AWS logic?

I have to admin, I have not idea but collaborator permission sounds good :)

Hm, I dont have a better idea then django-chunked-upload-2

jerinpetergeorge commented 5 years ago

@GitRon As I said, I've created a new repository, django-chunk-upload. If you wish to contribute, you can create a new issue and give a PR. Before that, you should fork that repository.

steverecio commented 5 years ago

@steverecio I don't think your changes won't be appropriate. Because of the statement,

if hasattr(self.model, 'user'):
    ....

is more likely if True since hasattr(self.model, 'user') will be True always.

The key point is , we can't drop any field in a Abstact Model (or any Python class) .

@jerinpetergeorge this change is required when you inherit directly from the BaseChunkedUpload abstract model. The abstract model has no user field. It's only ChunkedUpload that has the user field so custom models require this check.

Sorry I couldn't host the repo myself. Too busy to maintain it but happy to keep contributing. Another issue that I've been having is the md5 checksum can cause request timeouts for large files on slow filesystems. I may add another PR to keep track of the running checksum during the chunked uploads to prevent one large synchronous read at the end.

jerinpetergeorge commented 5 years ago

@steverecio Yeah... You are right. I didn't think about the BaseChunkedUpload abstract model.

If you wish to make PRs, please make it to the new repository as this repos seems abandoned.

juliomalegria commented 4 years ago

@steverecio @GitRon @jerinpetergeorge Sorry, I've been out of the loop with this package for a long time. I used to maintain it when I use it in the previous company I worked for, but I haven't had to work with it for quite some time now. I'd be happy to add you guys as collaborators of this repo if you guys are interested, as well as add you to the PyPI library. Otherwise I could link to your new repo if you have a working v2.

jerinpetergeorge commented 4 years ago

@juliomalegria Great to hear from you !! I would like to be a collaborator since this package and repository already been popular and I think people won't be much comfortable to switch the package ;) (personal opinion :)).

juliomalegria commented 4 years ago

Sure thing! I'll add you as a collaborator then. I'll try to get familiarized with this package again, but it would be great to have someone to unblock all the blocked fixes and improvements (whenever you find the time for it).

GitRon commented 4 years ago

@juliomalegria I'd like to be a collaborator as well!

@jerinpetergeorge We should then reconsider changing all the variable names. We should keep the hazzle as low as possible like in your v2 package.

jerinpetergeorge commented 4 years ago

@GitRon changing the variable name to what? I don't understand :worried: Can you elaborate little bit more?

GitRon commented 4 years ago

@jerinpetergeorge In this fork https://github.com/jerinpetergeorge/django-chunk-upload/pulls?utf8=%E2%9C%93&q= you changed the variable prefix, if I'm not mistaken. Thats what I meant. This would cause people upgrading this package with lots of stuff to change.

jerinpetergeorge commented 4 years ago

I would like to hide/archive that repository since we are planning to work on this repo and I don't think that repository has much popularity :unamused: So, we could work on this repo and variable naming won't be an issue. @GitRon

GitRon commented 4 years ago

Great! I'm with you on this one 😃

jerinpetergeorge commented 4 years ago

There are plenty of issues out there... We need to list out a few and should get work on it for our next release. @GitRon

jerinpetergeorge commented 4 years ago

fixed in #55