dmitri.shuralyov.com/service/change/...

gerritapi: use project~id change ID format

Use the more explicit project~id change ID format. It's not deprecated,
and lets us detect and return a not exist error when the project doesn't
match the CL.

Previously, just the numeric change ID was used. That format is
deprecated, and doesn't tell us when the project doesn't match the CL.

References:

• https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id
• https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#change.api.allowedIdentifier
dmitshur committed 6 years ago commit 7c09d163af9b65a0f95bf6dc84a4acf9fdd82fc5
Collapse all
gerritapi/gerritapi.go
@@ -2,10 +2,11 @@
package gerritapi

import (
	"context"
	"fmt"
	"net/http"
	"os"
	"sort"
	"strings"
	"unicode"

@@ -26,32 +27,35 @@ func NewService(client *gerrit.Client) change.Service {
type service struct {
	cl     *gerrit.Client
	domain string
}

func (s service) List(ctx context.Context, rs string, opt change.ListOptions) ([]change.Change, error) {
	project := project(rs)
func (s service) List(ctx context.Context, repo string, opt change.ListOptions) ([]change.Change, error) {
	project := project(repo)
	var query string
	switch opt.Filter {
	case change.FilterOpen:
		query = fmt.Sprintf("project:%s status:open", project)
	case change.FilterClosedMerged:
		// "status:closed" is equivalent to "(status:abandoned OR status:merged)".
		query = fmt.Sprintf("project:%s status:closed", project)
	case change.FilterAll:
		query = fmt.Sprintf("project:%s", project)
	}
	cs, _, err := s.cl.Changes.QueryChanges(&gerrit.QueryChangeOptions{
	cs, resp, err := s.cl.Changes.QueryChanges(&gerrit.QueryChangeOptions{
		QueryOptions: gerrit.QueryOptions{
			Query: []string{query},
			Limit: 25,
		},
		ChangeOptions: gerrit.ChangeOptions{
			AdditionalFields: []string{"DETAILED_ACCOUNTS", "MESSAGES"},
		},
	})
	if err != nil {
		if resp != nil && resp.StatusCode == http.StatusNotFound {
			return nil, os.ErrNotExist
		}
		return nil, err
	}
	var is []change.Change
	for _, chg := range *cs {
		if chg.Status == "DRAFT" {
@@ -76,15 +80,19 @@ func (s service) List(ctx context.Context, rs string, opt change.ListOptions) ([
func (s service) Count(_ context.Context, repo string, opt change.ListOptions) (uint64, error) {
	// TODO.
	return 0, nil
}

func (s service) Get(ctx context.Context, _ string, id uint64) (change.Change, error) {
	chg, _, err := s.cl.Changes.GetChange(fmt.Sprint(id), &gerrit.ChangeOptions{
func (s service) Get(ctx context.Context, repo string, id uint64) (change.Change, error) {
	project := project(repo)
	chg, resp, err := s.cl.Changes.GetChange(fmt.Sprintf("%s~%d", project, id), &gerrit.ChangeOptions{
		AdditionalFields: []string{"DETAILED_ACCOUNTS", "MESSAGES", "ALL_REVISIONS"},
	})
	if err != nil {
		if resp != nil && resp.StatusCode == http.StatusNotFound {
			return change.Change{}, os.ErrNotExist
		}
		return change.Change{}, err
	}
	if chg.Status == "DRAFT" {
		return change.Change{}, os.ErrNotExist
	}
@@ -113,16 +121,20 @@ func state(status string) change.State {
	default:
		panic("unreachable")
	}
}

func (s service) ListCommits(ctx context.Context, _ string, id uint64) ([]change.Commit, error) {
	chg, _, err := s.cl.Changes.GetChange(fmt.Sprint(id), &gerrit.ChangeOptions{
func (s service) ListCommits(ctx context.Context, repo string, id uint64) ([]change.Commit, error) {
	project := project(repo)
	chg, resp, err := s.cl.Changes.GetChange(fmt.Sprintf("%s~%d", project, id), &gerrit.ChangeOptions{
		AdditionalFields: []string{"DETAILED_ACCOUNTS", "ALL_REVISIONS"},
		//AdditionalFields: []string{"ALL_REVISIONS", "ALL_COMMITS"}, // TODO: Consider using git committer/author instead...
	})
	if err != nil {
		if resp != nil && resp.StatusCode == http.StatusNotFound {
			return nil, os.ErrNotExist
		}
		return nil, err
	}
	if chg.Status == "DRAFT" {
		return nil, os.ErrNotExist
	}
@@ -137,23 +149,30 @@ func (s service) ListCommits(ctx context.Context, _ string, id uint64) ([]change
		}
	}
	return commits, nil
}

func (s service) GetDiff(ctx context.Context, _ string, id uint64, opt *change.GetDiffOptions) ([]byte, error) {
func (s service) GetDiff(ctx context.Context, repo string, id uint64, opt *change.GetDiffOptions) ([]byte, error) {
	project := project(repo)
	switch opt {
	case nil:
		diff, _, err := s.cl.Changes.GetPatch(fmt.Sprint(id), "current", nil)
		diff, resp, err := s.cl.Changes.GetPatch(fmt.Sprintf("%s~%d", project, id), "current", nil)
		if err != nil {
			if resp != nil && resp.StatusCode == http.StatusNotFound {
				return nil, os.ErrNotExist
			}
			return nil, err
		}
		return []byte(*diff), nil
	default:
		chg, _, err := s.cl.Changes.GetChange(fmt.Sprint(id), &gerrit.ChangeOptions{
		chg, resp, err := s.cl.Changes.GetChange(fmt.Sprintf("%s~%d", project, id), &gerrit.ChangeOptions{
			AdditionalFields: []string{"ALL_REVISIONS"},
		})
		if err != nil {
			if resp != nil && resp.StatusCode == http.StatusNotFound {
				return nil, os.ErrNotExist
			}
			return nil, err
		}
		if chg.Status == "DRAFT" {
			return nil, os.ErrNotExist
		}
@@ -166,11 +185,11 @@ func (s service) GetDiff(ctx context.Context, _ string, id uint64, opt *change.G
		case 1:
			base = ""
		default:
			base = fmt.Sprint(r.Number - 1)
		}
		files, _, err := s.cl.Changes.ListFiles(fmt.Sprint(id), opt.Commit, &gerrit.FilesOptions{
		files, _, err := s.cl.Changes.ListFiles(fmt.Sprintf("%s~%d", project, id), opt.Commit, &gerrit.FilesOptions{
			Base: base,
		})
		if err != nil {
			return nil, err
		}
@@ -179,11 +198,11 @@ func (s service) GetDiff(ctx context.Context, _ string, id uint64, opt *change.G
			sortedFiles = append(sortedFiles, file)
		}
		sort.Strings(sortedFiles)
		var diff string
		for _, file := range sortedFiles {
			diffInfo, _, err := s.cl.Changes.GetDiff(fmt.Sprint(id), opt.Commit, file, &gerrit.DiffOptions{
			diffInfo, _, err := s.cl.Changes.GetDiff(fmt.Sprintf("%s~%d", project, id), opt.Commit, file, &gerrit.DiffOptions{
				Base:    base,
				Context: "5",
			})
			if err != nil {
				return nil, err
@@ -231,18 +250,22 @@ func (s service) GetDiff(ctx context.Context, _ string, id uint64, opt *change.G
		}
		return []byte(diff), nil
	}
}

func (s service) ListTimeline(ctx context.Context, _ string, id uint64, opt *change.ListTimelineOptions) ([]interface{}, error) {
func (s service) ListTimeline(ctx context.Context, repo string, id uint64, opt *change.ListTimelineOptions) ([]interface{}, error) {
	// TODO: Pagination. Respect opt.Start and opt.Length, if given.

	chg, _, err := s.cl.Changes.GetChangeDetail(fmt.Sprint(id), nil)
	project := project(repo)
	chg, resp, err := s.cl.Changes.GetChangeDetail(fmt.Sprintf("%s~%d", project, id), nil)
	if err != nil {
		if resp != nil && resp.StatusCode == http.StatusNotFound {
			return nil, os.ErrNotExist
		}
		return nil, err
	}
	comments, _, err := s.cl.Changes.ListChangeComments(fmt.Sprint(id))
	comments, _, err := s.cl.Changes.ListChangeComments(fmt.Sprintf("%s~%d", project, id))
	if err != nil {
		return nil, err
	}
	var timeline []interface{}
	timeline = append(timeline, change.Comment{ // CL description.