Marcy #16
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,85 +1,230 @@ | ||
| package controllers | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "regexp" | ||
| "strconv" | ||
|
|
||
| "acme-books/models" | ||
| "cloud.google.com/go/datastore" | ||
| "github.com/go-martini/martini" | ||
| ) | ||
|
|
||
| type LibraryController struct { | ||
| ctx context.Context | ||
| client *datastore.Client | ||
| } | ||
|
|
||
| func NewLibraryController() *LibraryController { | ||
| lc := new(LibraryController) | ||
| lc.ctx = context.Background() | ||
| lc.client, _ = datastore.NewClient(lc.ctx, "acme-books") | ||
|
|
||
| books := []models.Book{ | ||
| {Id: 1, Author: "George Orwell", Title: "1984", Borrowed: false}, | ||
| {Id: 2, Author: "George Orwell", Title: "Animal Farm", Borrowed: false}, | ||
| {Id: 3, Author: "Robert Jordan", Title: "Eye of the world", Borrowed: false}, | ||
| {Id: 4, Author: "Various", Title: "Collins Dictionary", Borrowed: false}, | ||
| } | ||
|
|
||
| var keys []*datastore.Key | ||
|
|
||
| for _, book := range books { | ||
| keys = append(keys, datastore.IDKey("Book", book.Id, nil)) | ||
| } | ||
|
|
||
| if _, err := lc.client.PutMulti(lc.ctx, keys, books); err != nil { | ||
| fmt.Println(err) | ||
| } | ||
|
|
||
| return lc | ||
| } | ||
| func (lc LibraryController) Close() error { | ||
| return lc.client.Close() | ||
| } | ||
|
|
||
| func (lc LibraryController) GetByKey(params martini.Params, w http.ResponseWriter) { | ||
|
|
||
| id, err := strconv.Atoi(params["id"]) | ||
| if handleNoneNilErr(err, w, http.StatusBadRequest) { | ||
| return | ||
| } | ||
|
|
||
| key := datastore.IDKey("Book", int64(id), nil) | ||
| var book models.Book | ||
| err = lc.client.Get(lc.ctx, key, &book) | ||
| if handleNoneNilErr(err, w, http.StatusInternalServerError) { | ||
| return | ||
| } | ||
|
|
||
| writeJson(book, w) | ||
| } | ||
|
|
||
| func (lc LibraryController) ListAll(r *http.Request, w http.ResponseWriter) { | ||
| filter := r.URL.Query().Get("q") | ||
|
|
||
| var output []models.Book | ||
| query := datastore.NewQuery("Book") | ||
|
|
||
| if filter != "" { | ||
| splitIndices := regexp.MustCompile("(<=|<|>=|>|=)").FindStringIndex(filter) | ||
| if len(splitIndices) < 2 { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
| sepIndex := splitIndices[1] | ||
| fieldName := filter[:splitIndices[0]] | ||
| filterStr := filter[:sepIndex] | ||
|
|
||
| metaQuery := datastore.NewQuery("__property__") | ||
| type Prop struct { | ||
| Reps []string `datastore:"property_representation"` | ||
| } | ||
| var props []Prop | ||
|
|
||
| keys, err := lc.client.GetAll(lc.ctx, metaQuery, &props) | ||
| if handleNoneNilErr(err, w, http.StatusBadRequest) { | ||
| return | ||
| } | ||
|
|
||
| var columnKind string | ||
| ok := false | ||
| for i, k := range keys { | ||
| if k.Name == fieldName { | ||
| columnKind = props[i].Reps[0] | ||
| ok = true | ||
| break | ||
| } | ||
| } | ||
| if !ok { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| var filterVal interface{} | ||
| switch columnKind { | ||
| case "BOOLEAN": | ||
| filterVal, err = strconv.ParseBool(filter[sepIndex:]) | ||
|
There was a problem hiding this comment. Be careful with these methods. If this failed, then filterVal would be false/0 (the default's in go for bool and int is false/0, not nil), and err would be an error. But you aren't checking the err before running the filter, so you wouldn't get the results you expect at this point |
||
| case "INT64": | ||
| filterVal, err = strconv.Atoi(filter[sepIndex:]) | ||
| default: | ||
| filterVal = filter[sepIndex:] | ||
| } | ||
|
|
||
| query = query.Filter(filterStr, filterVal) | ||
|
|
||
| } | ||
|
|
||
| query = query.Order("Id") | ||
| it := lc.client.Run(lc.ctx, query) | ||
|
|
||
| for { | ||
| var b models.Book | ||
| _, err := it.Next(&b) | ||
| if err != nil { | ||
| fmt.Println(err) | ||
| break | ||
| } | ||
| output = append(output, b) | ||
| } | ||
|
|
||
| writeJson(output, w) | ||
| } | ||
|
|
||
| func (lc LibraryController) Borrow(params martini.Params, w http.ResponseWriter) { | ||
| lc.SetBorrowed(params, w, true) | ||
| return | ||
| } | ||
|
|
||
| func (lc LibraryController) Return(params martini.Params, w http.ResponseWriter) { | ||
| lc.SetBorrowed(params, w, false) | ||
| return | ||
| } | ||
| func (lc LibraryController) SetBorrowed(params martini.Params, w http.ResponseWriter, borrowed bool) { | ||
| id, err := strconv.Atoi(params["id"]) | ||
| if handleNoneNilErr(err, w, http.StatusBadRequest) { | ||
| return | ||
| } | ||
| key := datastore.IDKey("Book", int64(id), nil) | ||
|
|
||
| var book models.Book | ||
| err = lc.client.Get(lc.ctx, key, &book) | ||
| if handleNoneNilErr(err, w, http.StatusBadRequest) { | ||
| return | ||
| } | ||
| if book.Borrowed == borrowed { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
| book.Borrowed = borrowed | ||
| _, err = lc.client.Mutate(lc.ctx, datastore.NewUpdate(key, &book)) | ||
| if handleNoneNilErr(err, w, http.StatusBadRequest) { | ||
| return | ||
| } | ||
| w.WriteHeader(http.StatusNoContent) | ||
| return | ||
| } | ||
|
|
||
| func (lc LibraryController) New(r *http.Request, w http.ResponseWriter) { | ||
| var book models.Book | ||
| body := new(bytes.Buffer) | ||
|
|
||
| _, err := io.Copy(body, r.Body) | ||
| if handleNoneNilErr(err, w, http.StatusBadRequest) { | ||
| return | ||
| } | ||
|
|
||
| err = json.Unmarshal(body.Bytes(), &book) | ||
| if handleNoneNilErr(err, w, http.StatusBadRequest) { | ||
| return | ||
| } | ||
|
|
||
| key := datastore.IncompleteKey("Book", nil) | ||
| key, err = lc.client.Put(lc.ctx, key, &book) | ||
| if handleNoneNilErr(err, w, http.StatusBadRequest) { | ||
| return | ||
| } | ||
|
|
||
| type bookWithKey struct { | ||
| Key string | ||
| models.Book | ||
| } | ||
| amendedBook := *new(bookWithKey) | ||
| amendedBook.Key = (*key).String() | ||
| amendedBook.Book = book | ||
|
|
||
| jsonStr, err := json.MarshalIndent(amendedBook, "", " ") | ||
| if handleNoneNilErr(err, w, http.StatusBadRequest) { | ||
| return | ||
| } | ||
|
|
||
| w.WriteHeader(http.StatusOK) | ||
| _, err = w.Write(jsonStr) | ||
| handleNoneNilErr(err, w, http.StatusInternalServerError) | ||
| } | ||
|
|
||
| func writeJson(item interface{}, w http.ResponseWriter) { | ||
| jsonStr, err := json.MarshalIndent(item, "", " ") | ||
|
|
||
| if handleNoneNilErr(err, w, http.StatusInternalServerError) { | ||
| return | ||
| } | ||
|
|
||
| w.WriteHeader(http.StatusOK) | ||
| _, err = w.Write(jsonStr) | ||
| handleNoneNilErr(err, w, http.StatusInternalServerError) | ||
| } | ||
|
|
||
| func handleNoneNilErr(err error, w http.ResponseWriter, httpResponseStatusCode int) bool { | ||
| if err != nil { | ||
| fmt.Println(err) | ||
| w.WriteHeader(httpResponseStatusCode) | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be wary of over engineering. This is an impressive bit of code, but it would leave you open to running queries like
borrowed > falsewhich doesn't really make much sense. Sometimes simpler is better 😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just seen the commit message for
3. An incredibly over-engineered way to do Filtering😄 . As an aside, what would you have done to simplify this? Ie lock it down so you could only filter on the title or author. And it was a string contains match?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The quickest way to modify the method to only allow strings would be to return a 400 in place of converting the filter string to bool or int.
The simplest code though would be to remove the type checking completely, since before that was added the queries would return nothing if the column type wasn't string (as it would be like doing
WHERE Id = "3", which doesn't match the int 3), but that doesn't allow any kind of handling, it just returns empty.The main reason it's so complicated is because I didn't want to redefine the book model anywhere, so I had to make it dynamically find the type to convert the parameter correctly. If I threw out that requirement (since it's not actually in the exercise), I could just hard code a switch-case statement to check which name is in "fieldName", then either convert based on type or essentially make a passlist that only allows author and title. That would cut 47 lines down to like 12 and be less over-engineered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very sensible answer! How about the match being a contains match, rather than an equals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats fine. Datastore doesn't support contains (or case in-sensitive matches amongst other things). So it was a bit of a trick question (sorry about that)
You would most likely do a Get all from the datastore, then https://pkg.go.dev/strings#Contains check each one in turn. If we know its going to be a small list (100's) and not run often (less than once a minute say) then we would be ok with this brute force approach. If it was more performant, then indexes or tech like big query/elastic search might be more approriate