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

Extract download type from download command #821

Merged
merged 2 commits into from
Feb 28, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
251 changes: 152 additions & 99 deletions cmd/download.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,66 +54,29 @@ func runDownload(cfg config.Config, flags *pflag.FlagSet, args []string) error {
return err
}

identifier, err := downloadIdentifier(flags)
download, err := newDownload(flags, usrCfg)
if err != nil {
return err
}
url := fmt.Sprintf("%s/solutions/%s", usrCfg.GetString("apibaseurl"), identifier)

client, err := api.NewClient(usrCfg.GetString("token"), usrCfg.GetString("apibaseurl"))
if err != nil {
return err
}

req, err := client.NewRequest("GET", url, nil)
if err != nil {
return err
}

req, err = addQueryToDownloadRequest(flags, req)
if err != nil {
return err
}
metadata := download.payload.metadata()
dir := metadata.Exercise(usrCfg.GetString("workspace")).MetadataDir()

res, err := client.Do(req)
if err != nil {
if err := os.MkdirAll(dir, os.FileMode(0755)); err != nil {
return err
}

var payload downloadPayload
defer res.Body.Close()
if err := json.NewDecoder(res.Body).Decode(&payload); err != nil {
return fmt.Errorf("unable to parse API response - %s", err)
}

if res.StatusCode == http.StatusUnauthorized {
siteURL := config.InferSiteURL(usrCfg.GetString("apibaseurl"))
return fmt.Errorf("unauthorized request. Please run the configure command. You can find your API token at %s/my/settings", siteURL)
}

if res.StatusCode != http.StatusOK {
switch payload.Error.Type {
case "track_ambiguous":
return fmt.Errorf("%s: %s", payload.Error.Message, strings.Join(payload.Error.PossibleTrackIDs, ", "))
default:
return errors.New(payload.Error.Message)
}
}

metadata := payload.metadata()
dir := metadata.Exercise(usrCfg.GetString("workspace")).MetadataDir()

if err := os.MkdirAll(dir, os.FileMode(0755)); err != nil {
if err := metadata.Write(dir); err != nil {
return err
}

err = metadata.Write(dir)
client, err := api.NewClient(usrCfg.GetString("token"), usrCfg.GetString("apibaseurl"))
if err != nil {
return err
}

for _, file := range payload.Solution.Files {
unparsedURL := fmt.Sprintf("%s%s", payload.Solution.FileDownloadBaseURL, file)
for _, file := range download.payload.Solution.Files {
unparsedURL := fmt.Sprintf("%s%s", download.payload.Solution.FileDownloadBaseURL, file)
parsedURL, err := netURL.ParseRequestURI(unparsedURL)

if err != nil {
Expand Down Expand Up @@ -176,6 +139,150 @@ func runDownload(cfg config.Config, flags *pflag.FlagSet, args []string) error {
return nil
}

type download struct {
// either/or
slug, uuid string

// user config
token, apibaseurl, workspace string
Copy link
Contributor Author

@jdsutherland jdsutherland Feb 28, 2019

Choose a reason for hiding this comment

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

I'm not sure if most would agree to save these an individual fields rather than the config object. Calling GetString throughout the system seems vulnerable to typo's where using fields has the benefit of the compiler catching it.

Copy link
Member

Choose a reason for hiding this comment

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

I actually really like having them here like that. The flags feel like they should be processed once, and that's it.


// optional
track, team string

payload *downloadPayload
}

func newDownload(flags *pflag.FlagSet, usrCfg *viper.Viper) (*download, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is currently longer than ideal. I think it makes sense to have the logic to populate the payload in the constructor though. Clients can't do anything with a download without the payload being populated and extracting it to a method doesn't seem appropriate because clients should never need to call it - it just needs to happen on creation.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it would either have to be populated in the constructor, or have a separate method that always gets called before we need those values. The second seems like an unnecessary abstraction.

var err error
d := &download{}
d.uuid, err = flags.GetString("uuid")
if err != nil {
return nil, err
}
d.slug, err = flags.GetString("exercise")
if err != nil {
return nil, err
}
d.track, err = flags.GetString("track")
if err != nil {
return nil, err
}
d.team, err = flags.GetString("team")
if err != nil {
return nil, err
}

d.token = usrCfg.GetString("token")
d.apibaseurl = usrCfg.GetString("apibaseurl")
d.workspace = usrCfg.GetString("workspace")

if err = d.needsSlugXorUUID(); err != nil {
return nil, err
}
if err = d.needsUserConfigValues(); err != nil {
return nil, err
}
if err = d.needsSlugWhenGivenTrackOrTeam(); err != nil {
return nil, err
}

client, err := api.NewClient(d.token, d.apibaseurl)
if err != nil {
return nil, err
}

req, err := client.NewRequest("GET", d.url(), nil)
if err != nil {
return nil, err
}
d.buildQueryParams(req.URL)

res, err := client.Do(req)
if err != nil {
return nil, err
}
defer res.Body.Close()

if err := json.NewDecoder(res.Body).Decode(&d.payload); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible (or useful) to use the error parser?

cli/cmd/cmd.go

Line 78 in c3525cf

func decodedAPIError(resp *http.Response) error {

I cherry-picked your commit here #814

If so, let's do that as the next PR (I'm going to merge this one, as it looks great).

return nil, fmt.Errorf("unable to parse API response - %s", err)
}

if res.StatusCode == http.StatusUnauthorized {
return nil, fmt.Errorf(
"unauthorized request. Please run the configure command. You can find your API token at %s/my/settings",
config.InferSiteURL(d.apibaseurl),
)
}
if res.StatusCode != http.StatusOK {
switch d.payload.Error.Type {
case "track_ambiguous":
return nil, fmt.Errorf(
"%s: %s",
d.payload.Error.Message,
strings.Join(d.payload.Error.PossibleTrackIDs, ", "),
)
default:
return nil, errors.New(d.payload.Error.Message)
}
}

return d, nil
}

func (d download) url() string {
id := "latest"
if d.uuid != "" {
id = d.uuid
}
return fmt.Sprintf("%s/solutions/%s", d.apibaseurl, id)
}

func (d download) buildQueryParams(url *netURL.URL) {
query := url.Query()
if d.slug != "" {
query.Add("exercise_id", d.slug)
if d.track != "" {
query.Add("track_id", d.track)
}
if d.team != "" {
query.Add("team_id", d.team)
}
}
url.RawQuery = query.Encode()
}

// needsSlugXorUUID checks the presence of slug XOR uuid.
func (d download) needsSlugXorUUID() error {
if d.slug != "" && d.uuid != "" || d.uuid == d.slug {
return errors.New("need an --exercise name or a solution --uuid")
}
return nil
}

// needsUserConfigValues checks the presence of required values from the user config.
func (d download) needsUserConfigValues() error {
Copy link
Contributor Author

@jdsutherland jdsutherland Feb 28, 2019

Choose a reason for hiding this comment

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

In cmd/cmd we have func validateUserConfig(cfg *viper.Viper) but coupling to an outside validation seems like a bad idea so that's why it's duplicated here.

errMsg := "missing required user config: '%s'"
if d.token == "" {
return fmt.Errorf(errMsg, "token")
}
if d.apibaseurl == "" {
return fmt.Errorf(errMsg, "apibaseurl")
}
if d.workspace == "" {
return fmt.Errorf(errMsg, "workspace")
}
return nil
}

// needsSlugWhenGivenTrackOrTeam ensures that track/team arguments are also given with a slug.
// (track/team meaningless when given a uuid).
func (d download) needsSlugWhenGivenTrackOrTeam() error {
if (d.team != "" || d.track != "") && d.slug == "" {
return errors.New("--track or --team requires --exercise (not --uuid)")
}
return nil
}

type downloadPayload struct {
Solution struct {
ID string `json:"id"`
Expand Down Expand Up @@ -223,60 +330,6 @@ func (dp downloadPayload) metadata() workspace.ExerciseMetadata {
}
}

// downloadIdentifier is the variable for the URI to initiate an exercise download.
func downloadIdentifier(flags *pflag.FlagSet) (string, error) {
uuid, err := flags.GetString("uuid")
if err != nil {
return "", err
}
slug, err := flags.GetString("exercise")
if err != nil {
return "", err
}
if uuid != "" && slug != "" || uuid == slug {
return "", errors.New("need an --exercise name or a solution --uuid")
}

identifier := "latest"
if uuid != "" {
identifier = uuid
}
return identifier, nil
}

func addQueryToDownloadRequest(flags *pflag.FlagSet, req *http.Request) (*http.Request, error) {
uuid, err := flags.GetString("uuid")
if err != nil {
return req, err
}
slug, err := flags.GetString("exercise")
if err != nil {
return req, err
}
track, err := flags.GetString("track")
if err != nil {
return req, err
}

team, err := flags.GetString("team")
if err != nil {
return req, err
}

if uuid == "" {
q := req.URL.Query()
q.Add("exercise_id", slug)
if track != "" {
q.Add("track_id", track)
}
if team != "" {
q.Add("team_id", team)
}
req.URL.RawQuery = q.Encode()
}
return req, nil
}

func setupDownloadFlags(flags *pflag.FlagSet) {
flags.StringP("uuid", "u", "", "the solution UUID")
flags.StringP("track", "t", "", "the track ID")
Expand Down