Add questions and responses user endpoints #1

tenyo · 2018-09-04T13:17:27Z

This initial PR adds basic functionality for vending questions/answers and calculating data types and risk level based on a response.
Configuration is exclusively via JSON files in S3.

fishnix · 2018-09-04T14:10:08Z

libs/s3/objects.go

+			log.Println(err.Error(), input)
+		}
+		return nil, err
+	}


This is a little convoluted, maybe something more like

if err != nil { log.Println(err.Error(), input) if aerr, ok := err.(awserr.Error); ok && aerr == s3.ErrCodeNoSuchKey { return []byte{}, err } return nil, err }

fishnix · 2018-09-04T14:13:04Z

libs/s3/objects.go

+			log.Println(err.Error(), input)
+		}
+		return nil, err
+	}


fishnix · 2018-09-04T14:16:28Z

actions/app.go

-		app.GET("/", HomeHandler)
+		// initialize S3 client session
+		S3 = s3.NewSession(os.Getenv("S3_API_KEY"), os.Getenv("S3_API_SECRET"), os.Getenv("S3_REGION"))
+		S3.Bucket = os.Getenv("S3_BUCKET")


This is fine, but why not have the single constructor that also takes the bucket? Or pass the bucket as parameters to the methods that use the client?

fishnix · 2018-09-04T14:17:43Z

actions/app.go

+		S3 = s3.NewSession(os.Getenv("S3_API_KEY"), os.Getenv("S3_API_SECRET"), os.Getenv("S3_REGION"))
+		S3.Bucket = os.Getenv("S3_BUCKET")
+
+		app.GET("/v1/proctor/ping", PingPong)


This is part of the API group actually -- I only split mine in Tweaser because there was different auth middleware

fishnix · 2018-09-04T14:38:47Z

actions/risklevels.go

+}
+
+// RiskLevelsList is a versioned collection of RiskLevels
+type RiskLevelsList struct {


This should just be RiskLevels

fishnix · 2018-09-04T14:43:32Z

actions/risklevels.go

+	}
+
+	return d, nil
+}


I think this stuff needs to be abstracted into an interface the more I think about it. Let's just pull these few methods out into a helpers package for now? Then you can call helpers.LoadRiskLevels(version)

fishnix · 2018-09-04T14:50:24Z

actions/risklevels.go

+
+// loadRisklevels loads the risk levels json from S3 and returns a slice of bytes
+func loadRiskLevels(version string) ([]byte, error) {
+	if len(version) == 0 {


I think this should just be if version == ""

fishnix · 2018-09-04T14:52:24Z

actions/risklevels.go

+		}
+	}
+
+	log.Println("Loading risk levels version " + version)


log.Printf("Loading risk levels version %s", verison) 😄

heh, still getting used to Printf 😃

fishnix · 2018-09-04T14:53:39Z

actions/risklevels.go

+
+	log.Println("Loading risk levels version " + version)
+
+	d, err := S3.GetObject("risklevels/" + version + "/risklevels.json")


path := fmt.Sprintf("risklevels/%s/risklevels.json", version) d, err := S3.GetObject(path)

fishnix · 2018-09-04T14:58:32Z

actions/risklevels.go

+			return []byte{}, errors.New("Object not found in S3")
+		}
+		return nil, errors.New("Unable to get object from S3")
+	}


I wonder if we are over processing the error states... do you really want to throw a 500 and a stack trace if you get an error getting the object? Or would it be better to just always return []byte{}, err ? This code gets a lot simpler in the latter case (and more Go-y I think).

if err != nil { return []byte{}, err }

I wanted to return a 404 if object doesn't exist, I think that makes more sense than 500

im saying if you get an error from that method, just always return a 404

fishnix · 2018-09-04T15:00:07Z

actions/versions.go

+		versions = append(versions, s[len(s)-1])
+	}
+	return versions, nil
+}


Shouldn't this just be a method on RiskLevels?

It's also used for getting the question versions.

Ah! perfect! your first interface! 😆

fishnix · 2018-09-04T15:04:18Z

actions/versions.go

+	if len(vl) == 0 {
+		return ""
+	}
+	sort.Strings(vl)


I'm not sure this works... I think you need to convert these to numbers, no? What about the slice:

["0.1", "0.2", "0.3", ....., "0.20, 0.21", ...., "0.30", "0.31"]

also your comment needs quotes around the version numbers if they are strings 😸

good point, I had the version as a float initially but then changed it to string to make it more flexible. i've gotta fix that sort algo.

fishnix · 2018-09-04T15:08:12Z

actions/questions.go

+}
+
+// QuestionsList is a versioned collection of questions
+type QuestionsList struct {


Questions

fishnix · 2018-09-04T15:26:22Z

actions/questions.go

+	}
+
+	var ql *QuestionsList
+	if err := json.Unmarshal(q, &ql); err != nil {


This is a double pointer

fishnix · 2018-09-04T15:30:11Z

Nice start! 🥂

fishnix · 2018-09-06T17:15:08Z

actions/versions.go

+// latestVersion gets a slice of versions and returns the latest version as a string
+// e.g. ["0.1", "0.10", "1.0"] or ["0.1.0", "0.10.1", "1.0.2"]
+// any number of minor/patch versions will work as long as all versions are consistent
+// i.e. this is not allowed ["0.1", "0.10.0", "1.0"] and the function will return ""


fishnix · 2018-09-06T17:23:59Z

libs/helpers/helpers.go

+// LatestVersion gets a slice of versions and returns the latest version as a string
+// e.g. ["0.1", "0.10", "1.0"] or ["0.1.0", "0.10.1", "1.0.2"]
+// any number of minor/patch versions will work as long as all versions are consistent
+// i.e. this is not allowed ["0.1", "0.10.0", "1.0"] and the function will return ""


🤷‍♂️

fishnix · 2018-09-06T17:41:56Z

libs/helpers/helpers.go

+// e.g. ["0.1", "0.10", "1.0"] or ["0.1.0", "0.10.1", "1.0.2"]
+// any number of minor/patch versions will work as long as all versions are consistent
+// i.e. this is not allowed ["0.1", "0.10.0", "1.0"] and the function will return ""
+func LatestVersion(vl []string) string {


probably should return error too so you can bail from your errors below more gracefully

fishnix · 2018-09-06T17:42:46Z

libs/helpers/helpers.go

+// i.e. this is not allowed ["0.1", "0.10.0", "1.0"] and the function will return ""
+func LatestVersion(vl []string) string {
+	if len(vl) == 0 {
+		return ""


should this actually return an error?

fishnix · 2018-09-06T18:53:08Z

libs/helpers/helpers.go

+	}
+
+	return strings.Join(ss[len(ss)-1], ".")
+}


I didn't make it take arbitrary length strings, although that should be easy.... this is quite a bit faster

➜ tmp go test -bench=. -benchtime=20s goos: darwin goarch: amd64 BenchmarkLatestVersionT-8 10000000 3858 ns/op BenchmarkLatestVersionC-8 10000000 2741 ns/op BenchmarkLatestVersionN-8 10000000 2436 ns/op

func LVS(vl []string) string { if len(vl) == 0 { return "" } // initialize latest with zeros latest := []int{} for i := 0; i < len(strings.Split(vl[0], ".")); i++ { latest = append(latest, 0) } // loop over list and compare each to latest for _, sv := range vl { current := strings.Split(sv, ".") if len(current) == 0 { log.Printf("Error: bad current version") return "" } si := []int{} for _, s := range current { i, err := strconv.Atoi(s) if err != nil { log.Println("Failed to parse", current) return "" } si = append(si, i) } if len(latest) != len(si) { log.Println("Error format mismatch for", sv) return "" } for i, siv := range si { if siv > latest[i] { latest = si break } } } l := []byte{} for _, i := range latest { l = strconv.AppendInt(l, int64(i), 10) } return string(l) }

hmm, yeah, no need to sort all the versions if we only need to pick the latest one

fishnix · 2018-09-06T19:45:18Z

models/responses.go

+
+// Responses is a collection of responses to questions submitted by the client
+type Responses struct {
+	List              map[string]string `json:"responses"`


this is kinda strange -- to have the JSON key be responses

why? have any suggestions?

fishnix · 2018-09-06T19:52:25Z

models/risklevels.go

+	}
+
+	return nil
+}


I'm thinking that these Load() methods should actually be on the S3 client and you can pass a pointer to the struct that should be filled from the storage location. Later, this could become an interface for a storage construct.

I think it will have to be an interface, otherwise how are we going to pass a pointer to different structs?

fishnix · 2018-09-06T20:31:57Z

lookin good! 👍

tenyo · 2018-09-07T13:19:57Z

I've added an S3 Load method that takes any struct which satisfies the Loader interface.

fishnix · 2018-09-07T14:17:23Z

libs/s3/objects.go

+	}
+
+	return nil
+}


lol now you have 3 Load() methods! Make this take interface{} instead.

I assumed that wasn't gonna work without even trying it 😁 Much better now!

fishnix · 2018-09-07T15:00:36Z

libs/helpers/helpers.go

 			}
 		}
 	}
-
-	return strings.Join(ss[len(ss)-1], "."), nil
+	return strings.Trim(strings.Join(strings.Fields(fmt.Sprint(latest)), "."), "[]"), nil


a lot of slow functions chained together here... even a straight append instead is a lot better

package main import( "fmt" "strings" "strconv" "testing" ) func IntSliceString1(nums []int, separator string) string { return strings.Trim(strings.Join(strings.Fields(fmt.Sprint(nums)), separator), "[]") } func IntSliceString2(nums []int, separator string) string { if len(nums) == 0 { return "" } s := make([]string, len(nums)) for i, n := range nums { s[i] = strconv.Itoa(n) } return strings.Join(s, separator) } func IntSliceString3(nums []int, separator string) string { if len(nums) == 0 { return "" } s := []string{} for _, n := range nums { s = append(s, strconv.Itoa(n)) } return strings.Join(s, separator) } func Benchmark1(b *testing.B) { foo := []int{1,2,3,4,5,6,7,8,9,10,100,1000} for n := 0; n < b.N; n++ { IntSliceString1(foo, ".") } } func Benchmark2(b *testing.B) { foo := []int{1,2,3,4,5,6,7,8,9,10,100,1000} for n := 0; n < b.N; n++ { IntSliceString2(foo, ".") } } func Benchmark3(b *testing.B) { foo := []int{1,2,3,4,5,6,7,8,9,10,100,1000} for n := 0; n < b.N; n++ { IntSliceString3(foo, ".") } }

% ➜ tmp go test -bench=. -benchtime=20s goos: darwin goarch: amd64 Benchmark1-8 20000000 1950 ns/op Benchmark2-8 100000000 417 ns/op Benchmark3-8 50000000 727 ns/op PASS ok _/Users/fish/tmp/go/yale/tmp 120.277s

but it's a one-liner 😉

the benchmark stuff is pretty cool!

fishnix · 2018-09-07T15:02:11Z

This is gettin goooooood 🤔 😆

Tenyo Grozev added 4 commits September 4, 2018 09:06

add questions and responses user endpoints

bf55b28

fixup

08c163d

fixup

63de539

fixup

df4d9aa

fishnix reviewed Sep 4, 2018

View reviewed changes

libs/s3/objects.go Outdated

log.Println(err.Error(), input)

}

return nil, err

}

Copy link

fishnix Sep 4, 2018

Choose a reason for hiding this comment

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

same here

fishnix reviewed Sep 4, 2018

View reviewed changes

actions/questions.go Outdated

}

// QuestionsList is a versioned collection of questions

type QuestionsList struct {

Copy link

fishnix Sep 4, 2018

Choose a reason for hiding this comment

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

Questions

fishnix reviewed Sep 4, 2018

View reviewed changes

actions/questions.go Outdated

}

var ql *QuestionsList

if err := json.Unmarshal(q, &ql); err != nil {

Copy link

fishnix Sep 4, 2018

Choose a reason for hiding this comment

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

This is a double pointer

Tenyo Grozev added 4 commits September 4, 2018 22:57

fixes for PR comments

4c12853

move structs to models

8ef81d6

dep ensure

c15c8b9

dockerize

18c1d04

fishnix reviewed Sep 6, 2018

View reviewed changes

Loader interface

34bdc68

fishnix reviewed Sep 7, 2018

View reviewed changes

Tenyo Grozev added 3 commits September 7, 2018 10:22

optimize LatestVersion

99aed7b

empty interface does it

b58cb59

perms

1875558

fishnix reviewed Sep 7, 2018

View reviewed changes

Tenyo Grozev added 2 commits September 7, 2018 11:52

fixup

c1b37bf

rename confusing Response to Outcome

cf1eb68

tenyo merged commit 8978a49 into master Sep 7, 2018


		log.Println("Loading risk levels version " + version)

		d, err := S3.GetObject("risklevels/" + version + "/risklevels.json")

Add questions and responses user endpoints #1

Add questions and responses user endpoints #1

Conversation

tenyo commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishnix commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishnix commented Sep 6, 2018

tenyo commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishnix commented Sep 7, 2018