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 ImportFile() to send a multipart form request #1215

Closed
wants to merge 2 commits into from

Conversation

dheitman42
Copy link

Update ImportFile() to send a multipart form request, add missing "Name" parameter to ImportFileOptions.

@dheitman42
Copy link
Author

I'll test this more thoroughly after the weekend.

@dheitman42
Copy link
Author

Note this is intended to resolve #636

} else {
_ = writer.WriteField(strings.ToLower(key), fmt.Sprintf("%v", value))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

No clue what you are trying to do here, but this doesn't look right to me. Can you please elaborate on this approach?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, we're creating a multipart form, creating the file component with CreateFormFile(), copying the file contents to that part, then adding each of the fields from the ImportFileOptions struct as additional field data in the multipart form. That's the intent, anyway. I'll admit to being n00bish with golang, so maybe this isn't the proper way to do the struct-to-string-map conversion, but that's the basic idea.

Copy link
Author

Choose a reason for hiding this comment

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

Dunno if that answered your question, happy to clarify further if need be.

Copy link
Contributor

Choose a reason for hiding this comment

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

@svanharmelen this is how you do a multipart form upload: https://gist.github.com/mattetti/5914158/f4d1393d83ebedc682a3c8e7bdc6b49670083b84

In the api doc it is described as:

To upload a file from your file system, use the --form argument. This causes cURL to post data using the header Content-Type: multipart/form-data.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dheitman42 I tried out your pull request and I got an internal server error. I tested our server successfully with the following code though:

func TestHelloWorld(t *testing.T) {
	extraParams := map[string]string{
		"namespace": "go",
		"path":      "test",
	}

	request, err := newfileUploadRequest("https://gitlab.example.com/api/v4/projects/import", extraParams, "file", "/tmp/file.tar.gz")
	require.NoError(t, err)
	request.Header.Set("Authorization", "Bearer "+gitlabTkn)
	client := &http.Client{}
	resp, err := client.Do(request)
        require.NoError(t, err)
}


func newfileUploadRequest(uri string, params map[string]string, paramName, path string) (*http.Request, error) {
	file, err := os.Open(path)
	if err != nil {
		return nil, err
	}
	fileContents, err := ioutil.ReadAll(file)
	if err != nil {
		return nil, err
	}
	fi, err := file.Stat()
	if err != nil {
		return nil, err
	}
	file.Close()

	body := new(bytes.Buffer)
	writer := multipart.NewWriter(body)
	part, err := writer.CreateFormFile(paramName, fi.Name())
	if err != nil {
		return nil, err
	}

	if _, err := part.Write(fileContents); err != nil {
		return nil, err
	}

	for key, val := range params {
		_ = writer.WriteField(key, val)
	}
	err = writer.Close()
	if err != nil {
		return nil, err
	}

	req, err := http.NewRequest("POST", uri, body)
	if err != nil {
		return nil, err
	}
	req.Header.Set("Content-Type", writer.FormDataContentType())

	return req, nil
}

Copy link
Author

Choose a reason for hiding this comment

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

Strange, I'm using the exact code from my MR in production, just verified it's working. I'll peek at your version when I have a spare moment (so, probably don't hold your breath :D )

Copy link
Member

Choose a reason for hiding this comment

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

See my latest commit to this PR:

By using query.Values we are using the URL tags and are omitting any empty values. This seems to be inline with what the API expects. Could you both this this version @dheitman42 and @zbindenren?

Copy link
Contributor

Choose a reason for hiding this comment

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

@svanharmelen I am still getting a http 500 for my test:

	is, _, err := dst.ProjectImportExport.ImportFile(&gitlab.ImportFileOptions{
		Namespace: gitlab.String("linux/go"),
		Path:      gitlab.String("flash"),
		File:      gitlab.String("flash.tar.gz"),
	})
	if err != nil {
		fmt.Println("error", err)
		fmt.Println("body", err)
	}
go test -count=1 -v -run 'TestHelloWorld$' ./.
=== RUN   TestHelloWorld
error POST https://gitlab.pnet.ch/api/v4/projects/import: 500 failed to parse unknown error format
body POST https://gitlab.pnet.ch/api/v4/projects/import: 500 failed to parse unknown error format
    main_test.go:82: 
                Error Trace:    main_test.go:82
                Error:          Received unexpected error:
                                POST https://gitlab.pnet.ch/api/v4/projects/import: 500 failed to parse unknown error format
                Test:           TestHelloWorld
--- FAIL: TestHelloWorld (12.38s)
FAIL
FAIL    it.pnet.ch/golang/gitmig        12.388s
FAIL

By using `query.Values` we are using the URL tags and are omitting any
empty values. This seems to be inline with what the API expects. Could
you both this this version @dheitman42 and @zbindenren?
@svanharmelen
Copy link
Member

This should be fixed by PR #1318 which also adds some additional file upload improvements. Please open an issue if you have any issues with the new code. Thanks for the PR and the discussions!

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