WIP: Add a better example for file upload

[barrachri]

What do these changes do?

Improve docs.

I think most of the time when you upload a file you also have related fields.

This PR add a general case to the docs., where you have a text field and file field.

Are there changes in behavior for the user?

No.

Related issue number

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the changes folder
  • name it <issue_id>.<type> for example (588.bug)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

[codecov-io]

Codecov Report

Merging #2401 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2401   +/-   ##
=======================================
  Coverage   97.19%   97.19%           
=======================================
  Files          39       39           
  Lines        8159     8159           
  Branches     1419     1419           
=======================================
  Hits         7930     7930           
  Misses         99       99           
  Partials      130      130

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fa7c1f...56395ed. Read the comment docs.

[asvetlov]

Hmm. The code is definitely robust but the snippet goes to very low details maybe.
@kxepal please review.

[barrachri]

Thanks @asvetlov.

Do you think is too verbose or more comments/explanations outside of the code would improve this?

[fafhrd91]

I’d add note in file operations, in general case file operations block

[kxepal]

Example is indeed better, but I'm not sure if we should have complicated things on our docs. As more code we have in docs as more hard keep it valid. Eventually, we cannot automatically test it to make sure that doc examples are correct.

I think we should keep example in docs simple and clean, while more complicated stuff keep in examples/ directory. This code at least is possible to test.

[kxepal]

I think this is a bad pattern to recommend. You should have this condition on router level.

[kxepal]

page is not defined.

[kxepal]

@fafhrd91
Good point. However, that's not the only edge case for this example.

[barrachri]

So:

• Add a note that file operations generally block.

@kxepal a small note in the official docs completed with a link to a good example in /examples would be better?

[asvetlov]

I think we need the docs change but it should look like:

    async def store_mp3_handler(request):

        reader = await request.multipart()

        # /!\ Don't forget to validate your inputs /!\

        field = await reader.next()
        assert field.name == 'name'
        name = await field.read(decode=True)

        field = await reader.next()
        assert field.name == 'mp3'
        filename = field.filename
        # You cannot rely on Content-Length if transfer is chunked.
        size = 0
        with open(os.path.join('/spool/yarrr-media/mp3/', filename), 'wb') as f:
            while True:
                chunk = await field.read_chunk()  # 8192 bytes by default.
                if not chunk:
                    break
                size += len(chunk)
                f.write(chunk)

        return web.Response(text='{} sized of {} successfully stored'
                                 ''.format(filename, size))

The key is using field.name and field.read().
Maybe we need process field.get_charset() too.

@kxepal does it make sense?

[kxepal]

@barrachri
Yes.

@asvetlov
That's nice. I believe you would like to use name instead of filename on file open line. Otherwise it's unused and may confuse.

[asvetlov]

I have no strong opinion.
My intention is showing all useful field methods and attributes

[barrachri]

The current example and @asvetlov's are very clear in showing how you can chunk files.

The problem is that usually in other frameworks you don't loop or iterate on an object to get your params and the same is also true for aiohttp, with Request.query and data = await request.post().

My problem was that you jump from this example

    mp3 = data['mp3']

    # .filename contains the name of the file in string format.
    filename = mp3.filename

    # .file contains the actual file data that needs to be stored somewhere.
    mp3_file = data['mp3'].file

    content = mp3_file.read()

to this example

async def store_mp3_handler(request):

    reader = await request.multipart()

    # /!\ Don't forget to validate your inputs /!\

    mp3 = await reader.next()

    filename = mp3.filename

    # You cannot rely on Content-Length if transfer is chunked.
    size = 0
    with open(os.path.join('/spool/yarrr-media/mp3/', filename), 'wb') as f:
        while True:
            chunk = await mp3.read_chunk()  # 8192 bytes by default.
            if not chunk:
                break
            size += len(chunk)
            f.write(chunk)

    return web.Response(text='{} sized of {} successfully stored'
                             ''.format(filename, size))

and you completely miss what's going on with next because there isn't a context anymore.
The short paragraph before the example talk about the multipart reader and if you follow the link everything is discussed apart from Reading Multipart Request.

After I checked the implementation of Request.post everything was clear but I agree is too verbose to just paste the code.

I think that a simple comment will make the example more clear.
Something like this:

    async def store_mp3_handler(request):

        reader = await request.multipart()

        # /!\ Don't forget to validate your inputs /!\

        # reader.next() will `yield` the fields of your form

        field = await reader.next()
        assert field.name == 'name'
        name = await field.read(decode=True)

        field = await reader.next()
        assert field.name == 'mp3'
        filename = field.filename
        # You cannot rely on Content-Length if transfer is chunked.
        size = 0
        with open(os.path.join('/spool/yarrr-media/mp3/', filename), 'wb') as f:
            while True:
                chunk = await field.read_chunk()  # 8192 bytes by default.
                if not chunk:
                    break
                size += len(chunk)
                f.write(chunk)

        return web.Response(text='{} sized of {} successfully stored'
                                 ''.format(filename, size))

[asvetlov]

I think now is better. Please update the RP.
@kxepal ?

[barrachri]

Shall I add also an example in examples/ or just updating the PR?

[kxepal]

Let's update examples/ in another PR. It's not a blocker for docs improvement.

[asvetlov]

Thanks!

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new