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

Avoid use of multipart.Part.FileName() to fix files API on Go 1.17 #64

Merged
merged 3 commits into from Aug 19, 2021

Conversation

benhoyt
Copy link
Contributor

@benhoyt benhoyt commented Aug 17, 2021

In Go 1.17 they changed part.FileName() to pass the result through filepath.Base() before returning, stripping off the directory component which is important for Pebble's files API. See https://golang.org/doc/go1.17#mime/multipart and golang/go@784ef4c.

Note that ReadForm() used in the tests also calls part.FileName() so we have to roll that ourselves too.

Incidentally, we'd had some discussion about this in the original PR: #25 (comment)

I had thought about using the filename field earlier, but originally decided against it because I know some clients strip off the path part of it (per the "SHOULD NOT" warning in RFC section 2.3 that you've quoted). However, Go does not, and we'll have a lot of control over the Python client (there's no multipart parser in the stdlib) so I think it's okay. I've updated the code to use "filename" -- it is simpler and cleaner.

Oh well, it's a fairly easy fix. :-)

This commit also runs Go 1.17's version of "go fmt", which adds some //go:build lines.

In Go 1.17 they changed part.FileName() to pass the result through
filepath.Base() before returning, stripping off the directory component
which is important for Pebble's files API. See:
https://golang.org/doc/go1.17#mime/multipart
golang/go@784ef4c

Note that ReadForm used in the tests also calls part.FileName(), so we
have to roll that ourselves too.

Incidentally, we'd had some discussion about this in the original PR:
canonical#25 (comment)

> I had thought about using the filename field earlier, but originally
> decided against it because I know some clients strip off the path part
> of it (per the "SHOULD NOT" warning in RFC section 2.3 that you've
> quoted). However, Go does not, and we'll have a lot of control over
> the Python client (there's no multipart parser in the stdlib) so I
> think it's okay. I've updated the code to use "filename" -- it is
> simpler and cleaner.

Oh well, it's a fairly easy fix. :-)

This commit also runs Go 1.17's version of "go fmt", which adds some
"//go:build" lines.
@@ -1,4 +1,5 @@
// -*- Mode: Go; indent-tabs-mode: t -*-
//go:build arm || 386
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are due to the new //go:build style build constraints being introduced (and go fmt adds them in Go 1.17). See https://go.googlesource.com/proposal/+/master/design/draft-gobuild.md

c.Assert(err, IsNil)
b, err := ioutil.ReadAll(f)
if part.FormName() != "files" {
Copy link
Member

Choose a reason for hiding this comment

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

This seems a significant difference in the looping. In the former we were iterating over everything in 'files' now we seem to be iterating just over the multipart but then not iterating over files.

(The old form looked like the data was something like:

{'blah': ?,
'files': [content, content, content],
}

But the new way we are treating it seems more like:
['blah', {'files': content}, {'files': content'}, {'files': content}]

I'm assuming that the test suite is passing so I'm wrong about something (I seem to recall some Python HTTP classes that would let you treat repeated headers as a single list.)

Copy link
Contributor Author

@benhoyt benhoyt Aug 18, 2021

Choose a reason for hiding this comment

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

Yeah, I think you're assuming something wrong. The raw read-files response (before and after) is like so:

--01234567890123456789012345678901
Content-Disposition: form-data; name="files"; filename="/root/hi.txt"

Hello
--01234567890123456789012345678901
Content-Disposition: form-data; name="files"; filename="/root/bye.txt"

Bye bye
--01234567890123456789012345678901
Content-Disposition: form-data; name="files"; filename="/root/foobar.txt"

Foo
Bar
--01234567890123456789012345678901
Content-Disposition: form-data; name="response"

{
    "action": "read",
    "files": [
        {"path": "/root/hi.txt"},
        {"path": "/root/bye.txt"},
        {"path": "/root/foobar.txt"}
    ]
}
--01234567890123456789012345678901--

In the old code, we called multipart.Reader.ReadForm to loop over the multipart sections (files and response). In the new code, we do that loop ourselves and don't call ReadForm at all -- either way it loops over the files first, and then grabs the response at the end. Hope that helps.

@benhoyt benhoyt merged commit 9dd207b into canonical:master Aug 19, 2021
@benhoyt benhoyt deleted the go-1.17-fixes branch August 19, 2021 07:54
@benhoyt
Copy link
Contributor Author

benhoyt commented Aug 19, 2021

@niemeyer I merged this relatively trivial fix with two approvals, but let me know if you have any concerns.

benhoyt added a commit to benhoyt/juju that referenced this pull request Oct 1, 2021
More specifically, these PRs:

* canonical/pebble#59 Add GPLv3 license (to match source header and snapd)
* canonical/pebble#64 Avoid use of multipart.Part.FileName() to fix files API on Go 1.17
* canonical/pebble#63 Add wait-change API endpoint and client function
* canonical/pebble#65 Make Pebble "go vet" clean
* canonical/pebble#66 Remove a bunch of snapd references
* canonical/pebble#67 Add API section to README; add HACKING.md; remove Makefile
* canonical/pebble#60 Add Replan operation to restart changed services.
* canonical/pebble#61 One-shot commands (pebble exec)
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.

None yet

3 participants