Skip to content
Merged
52 changes: 52 additions & 0 deletions github/repos_releases.go
Original file line number Diff line number Diff line change
Expand Up @@ -488,3 +488,55 @@ func (s *RepositoriesService) UploadReleaseAsset(ctx context.Context, owner, rep
}
return asset, resp, nil
}

// UploadReleaseAssetFromRelease uploads an asset using the UploadURL that's embedded
// in a RepositoryRelease object.
//
// This is a convenience wrapper that extracts the release.UploadURL (which is usually
// templated like "https://uploads.github.com/.../assets{?name,label}") and uploads
// the provided data (reader + size) using the existing upload helpers.
Comment on lines +492 to +497
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add a new example that demonstrates how to use this wrapper?

//
// GitHub API docs: https://docs.github.com/rest/releases/assets#upload-a-release-asset
//
//meta:operation POST /repos/{owner}/{repo}/releases/{release_id}/assets
func (s *RepositoriesService) UploadReleaseAssetFromRelease(ctx context.Context, release *RepositoryRelease, opts *UploadOptions, reader io.Reader, size int64) (*ReleaseAsset, *Response, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why we need to pass the entire RepositoryRelease object if we can simply pass uploadURL. Like this:

func (s *RepositoriesService) UploadReleaseAssetFromRelease(ctx context.Context, uploadURL string, opts *UploadOptions, reader io.Reader, size int64) (*ReleaseAsset, *Response, error) {

Copy link

Choose a reason for hiding this comment

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

It would then mean that the consumer needs to know that the upload URL needs to be extracted from a RepositoryRelease object, requiring knowledge from behind the curtain.

The implemented way makes it trivial for users - they know that they need to supply a release, and probably already have one at hand from creating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed the final updates. absolute upload URLs are preserved, template trimming is correct, media-type handling now matches UploadReleaseAsset, and tests fully cover all paths. Everything passes locally. Let me know if you'd like any further tweaks!

if release == nil || release.UploadURL == nil {
return nil, nil, errors.New("release UploadURL must be provided")
}
if reader == nil {
return nil, nil, errors.New("reader must be provided")
}
if size < 0 {
return nil, nil, errors.New("size must be >= 0")
}

// Strip URI-template portion (e.g. "{?name,label}") if present.
uploadURL := *release.UploadURL
if idx := strings.Index(uploadURL, "{"); idx != -1 {
uploadURL = uploadURL[:idx]
}

// IMPORTANT: preserve absolute upload URLs. Do NOT convert to relative paths.
// addOptions will append name/label query params (same behavior as UploadReleaseAsset).
u, err := addOptions(uploadURL, opts)
if err != nil {
return nil, nil, err
}

mediaType := defaultMediaType
if opts != nil && opts.MediaType != "" {
mediaType = opts.MediaType
}

req, err := s.client.NewUploadRequest(u, reader, size, mediaType)
if err != nil {
return nil, nil, err
}

asset := new(ReleaseAsset)
resp, err := s.client.Do(ctx, req, asset)
if err != nil {
return nil, resp, err
}
return asset, resp, nil
}
203 changes: 203 additions & 0 deletions github/repos_releases_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -930,3 +930,206 @@ func TestGenerateNotesOptions_Marshal(t *testing.T) {

testJSONMarshal(t, u, want)
}

func TestRepositoriesService_UploadReleaseAssetFromRelease(t *testing.T) {
t.Parallel()

var (
defaultUploadOptions = &UploadOptions{Name: "n"}
defaultExpectedFormValue = values{"name": "n"}
mediaTypeTextPlain = "text/plain; charset=utf-8"
)

client, mux, _ := setup(t)

// Use the same endpoint path used in other release asset tests.
mux.HandleFunc("/repos/o/r/releases/1/assets", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "POST")
testHeader(t, r, "Content-Type", mediaTypeTextPlain)
testHeader(t, r, "Content-Length", "12")
testFormValues(t, r, defaultExpectedFormValue)
testBody(t, r, "Upload me !\n")

fmt.Fprint(w, `{"id":1}`)
})

body := []byte("Upload me !\n")
reader := bytes.NewReader(body)
size := int64(len(body))

// Provide a templated upload URL like GitHub returns.
uploadURL := "/repos/o/r/releases/1/assets{?name,label}"
release := &RepositoryRelease{
UploadURL: &uploadURL,
}

ctx := t.Context()
asset, _, err := client.Repositories.UploadReleaseAssetFromRelease(ctx, release, defaultUploadOptions, reader, size)
if err != nil {
t.Fatalf("Repositories.UploadReleaseAssetFromRelease returned error: %v", err)
}
want := &ReleaseAsset{ID: Ptr(int64(1))}
if !cmp.Equal(asset, want) {
t.Fatalf("Repositories.UploadReleaseAssetFromRelease returned %+v, want %+v", asset, want)
}
}

func TestRepositoriesService_UploadReleaseAssetFromRelease_AbsoluteTemplate(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)

mux.HandleFunc("/repos/o/r/releases/1/assets", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "POST")
// Expect name query param created by addOptions after trimming template.
if got := r.URL.Query().Get("name"); got != "abs.txt" {
t.Errorf("Expected name query param 'abs.txt', got %q", got)
}
fmt.Fprint(w, `{"id":1}`)
})

body := []byte("Upload me !\n")
reader := bytes.NewReader(body)
size := int64(len(body))

// Build an absolute URL using the test client's BaseURL.
absoluteUploadURL := client.BaseURL.String() + "repos/o/r/releases/1/assets{?name,label}"
release := &RepositoryRelease{UploadURL: &absoluteUploadURL}

opts := &UploadOptions{Name: "abs.txt"}
ctx := t.Context()
asset, _, err := client.Repositories.UploadReleaseAssetFromRelease(ctx, release, opts, reader, size)
if err != nil {
t.Fatalf("UploadReleaseAssetFromRelease returned error: %v", err)
}
want := &ReleaseAsset{ID: Ptr(int64(1))}
if !cmp.Equal(asset, want) {
t.Fatalf("UploadReleaseAssetFromRelease returned %+v, want %+v", asset, want)
}
}

func TestRepositoriesService_UploadReleaseAssetFromRelease_NilRelease(t *testing.T) {
t.Parallel()
client, _, _ := setup(t)

body := []byte("Upload me !\n")
reader := bytes.NewReader(body)
size := int64(len(body))

ctx := t.Context()
_, _, err := client.Repositories.UploadReleaseAssetFromRelease(ctx, nil, &UploadOptions{Name: "n"}, reader, size)
if err == nil {
t.Fatal("expected error for nil release, got nil")
}

const methodName = "UploadReleaseAssetFromRelease"
testBadOptions(t, methodName, func() (err error) {
_, _, err = client.Repositories.UploadReleaseAssetFromRelease(ctx, nil, &UploadOptions{Name: "n"}, reader, size)
return err
})
}

func TestRepositoriesService_UploadReleaseAssetFromRelease_NilReader(t *testing.T) {
t.Parallel()
client, _, _ := setup(t)

uploadURL := "/repos/o/r/releases/1/assets{?name,label}"
release := &RepositoryRelease{UploadURL: &uploadURL}

ctx := t.Context()
_, _, err := client.Repositories.UploadReleaseAssetFromRelease(ctx, release, &UploadOptions{Name: "n"}, nil, 12)
if err == nil {
t.Fatal("expected error when reader is nil")
}

const methodName = "UploadReleaseAssetFromRelease"
testBadOptions(t, methodName, func() (err error) {
_, _, err = client.Repositories.UploadReleaseAssetFromRelease(ctx, release, &UploadOptions{Name: "n"}, nil, 12)
return err
})
}

func TestRepositoriesService_UploadReleaseAssetFromRelease_NegativeSize(t *testing.T) {
t.Parallel()
client, _, _ := setup(t)

uploadURL := "/repos/o/r/releases/1/assets{?name,label}"
release := &RepositoryRelease{UploadURL: &uploadURL}

body := []byte("Upload me !\n")
reader := bytes.NewReader(body)

ctx := t.Context()
_, _, err := client.Repositories.UploadReleaseAssetFromRelease(ctx, release, &UploadOptions{Name: "n"}, reader, -1)
if err == nil {
t.Fatal("expected error when size is negative")
}
}

func TestRepositoriesService_UploadReleaseAssetFromRelease_NoOpts(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)

// No opts: we just assert that the handler is hit and body is as expected.
mux.HandleFunc("/repos/o/r/releases/1/assets", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "POST")
testBody(t, r, "Upload me !\n")
fmt.Fprint(w, `{"id":1}`)
})

body := []byte("Upload me !\n")
reader := bytes.NewReader(body)
size := int64(len(body))

uploadURL := "/repos/o/r/releases/1/assets{?name,label}"
release := &RepositoryRelease{UploadURL: &uploadURL}

ctx := t.Context()
asset, _, err := client.Repositories.UploadReleaseAssetFromRelease(ctx, release, nil, reader, size)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
want := &ReleaseAsset{ID: Ptr(int64(1))}
if !cmp.Equal(asset, want) {
t.Fatalf("Repositories.UploadReleaseAssetFromRelease returned %+v, want %+v", asset, want)
}

const methodName = "UploadReleaseAssetFromRelease"
testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
got, resp, err := client.Repositories.UploadReleaseAssetFromRelease(ctx, release, nil, reader, size)
if got != nil {
t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
}
return resp, err
})
}

func TestRepositoriesService_UploadReleaseAssetFromRelease_WithMediaType(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)

// Expect explicit media type to be used.
mux.HandleFunc("/repos/o/r/releases/1/assets", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "POST")
testHeader(t, r, "Content-Type", "image/png")
fmt.Fprint(w, `{"id":1}`)
})

body := []byte("Binary!")
reader := bytes.NewReader(body)
size := int64(len(body))

uploadURL := "/repos/o/r/releases/1/assets{?name,label}"
release := &RepositoryRelease{UploadURL: &uploadURL}

opts := &UploadOptions{Name: "n", MediaType: "image/png"}

ctx := t.Context()
asset, _, err := client.Repositories.UploadReleaseAssetFromRelease(ctx, release, opts, reader, size)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
want := &ReleaseAsset{ID: Ptr(int64(1))}
if !cmp.Equal(asset, want) {
t.Fatalf("UploadReleaseAssetFromRelease returned %+v, want %+v", asset, want)
}
}
Loading