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

getting-started/bookshelf: initial rewrite #951

Merged
merged 5 commits into from Sep 10, 2019

Conversation

@tbpg
Copy link
Contributor

commented Aug 22, 2019

Areas to review:

  • Should this be more than one package? I moved everything to package main.
  • Is the design easy to understand? Should we move files/packages around?
  • func NewBookshelf is a little weird to me. Is there a better way to write it?
  • Should we use Bookshelf.logWriter, is there a better way to do it, or should we just print logs when testing?

Summary of changes:

  • Switch to GAE Standard (go112).
  • Close() returns an error.
  • Booskshelf methods take a context.
  • Remove Datastore and replace it with Firestore.
  • Remove auth/login.
  • Remove databases other than Firestore and Memory.
  • Remove GCE and GKE deployments.
  • Add a go.mod and go.sum.
  • Move webtest into this directory to avoid depending on golang-samples.
  • Move all Go code into a root directory package main.
  • Make handlers methods on Bookshelf, rather than standalone functions.
  • Always initialize storage (see config.go) and give runtime error on
    image upload.
  • Firestore doc IDs stored as part of the Book in Firestore, rather than
    assigned during editing and listing.
getting-started/bookshelf: initial rewrite
* Switch to GAE Standard (go112).
* Close() returns an error.
* Booskshelf methods take a context.
* Remove Datastore and replace it with Firestore.
* Remove auth/login.
* Remove databases other than Firestore and Memory.
* Remove GCE and GKE deployments.
* Add a go.mod and go.sum.
* Move webtest into this directory to avoid depending on golang-samples.
* Move all Go code into a root directory package main.
* Make handlers methods on Bookshelf, rather than standalone functions.
* Always initialize storage (see config.go) and give runtime error on
  image upload.
* Firestore doc IDs stored as part of the Book in Firestore, rather than
  assigned during editing and listing.

@tbpg tbpg requested review from broady and jadekler Aug 22, 2019

@googlebot googlebot added the cla: yes label Aug 22, 2019

@tbpg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

cc @dmitshur for review (can't add as a PR reviewer).

@dmitshur
Copy link

left a comment

The package code feels small and comprehensive, without too many extra things at the same time (compared to before). Overall, this looks like a nice simplification and general direction to move in. 👍

I don't see any major issue or things to point out.

One option that is perhaps worth considering (and maybe you already have considered it) is to factor out the two memorydb and firestoredb implementations into standalone packages. The main package would continue to define and use the interface, but the implementation can live separately.

However, I don't think it would neccessarily be better. It feels like it would be roughly the same, maybe slightly worse (because the implementation code is quite short and simple). So having it all in one package is good too.

I left minor comments for a few trivial things.


require (
cloud.google.com/go v0.44.3
github.com/gofrs/uuid v3.2.0+incompatible

This comment has been minimized.

Copy link
@dmitshur

dmitshur Aug 22, 2019

Perhaps consider switching to another version that isn't incompatible.

For example, github.com/gofrs/uuid/v3 v3.1.2 or github.com/google/uuid v1.1.1.

This comment has been minimized.

Copy link
@tbpg

tbpg Aug 23, 2019

Author Contributor

Unfortunately, that version also doesn't work. So, sticking with incompatible.

}

// Ensure firestoreDB conforms to the BookDatabase interface.
var _ BookDatabase = &firestoreDB{}

This comment has been minimized.

Copy link
@dmitshur

dmitshur Aug 22, 2019

This is completely okay as is, but consider moving it to a _test.go file since it's not a piece of code that is needed for functionality of the app.

Doing so causes a failure for *firestoreDB to implement the BookDatabase interface to be reported when you do go test rather than go build, which feels more fitting IMO.

This comment has been minimized.

Copy link
@tbpg

tbpg Aug 23, 2019

Author Contributor

I think I like it in here - when firestoreDB stops implementing BookDatabase, you know right away in your editor, unlike when it's in the test file (for VS Code, at least).

This comment has been minimized.

Copy link
@broady

broady Sep 9, 2019

Contributor

Won't it fail for go test, too? Package must compile for the tests to run.

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Yes, it also fails for go test.

// Use gorilla/mux for rich routing.
// See http://www.gorillatoolkit.org/pkg/mux
// See http://www.gorillatoolkit.org/pkg/mux.

This comment has been minimized.

Copy link
@dmitshur

dmitshur Aug 22, 2019

s,http://,https://,

This comment has been minimized.

Copy link
@tbpg

tbpg Aug 23, 2019

Author Contributor

Done.

_, err = topic.Publish(ctx, &pubsub.Message{Data: b}).Get(ctx)
log.Printf("Published update to Pub/Sub for Book ID %d: %v", bookID, err)
}

// http://blog.golang.org/error-handling-and-go

This comment has been minimized.

Copy link
@dmitshur

dmitshur Aug 22, 2019

s,http://,https://,

This comment has been minimized.

Copy link
@tbpg

tbpg Aug 23, 2019

Author Contributor

Done.

nextID: 1,
}
}

// Close closes the database.
func (db *memoryDB) Close() {
func (db *memoryDB) Close(ctx context.Context) error {

This comment has been minimized.

Copy link
@dmitshur

dmitshur Aug 22, 2019

The parameter ctx is unused, so it'd be slightly better to leave it out, as you have already done in the firestoreDB.Close method.

This comment has been minimized.

Copy link
@tbpg

tbpg Aug 23, 2019

Author Contributor

Done.

@tbpg
Copy link
Contributor Author

left a comment

Thanks!

}

// Ensure firestoreDB conforms to the BookDatabase interface.
var _ BookDatabase = &firestoreDB{}

This comment has been minimized.

Copy link
@tbpg

tbpg Aug 23, 2019

Author Contributor

I think I like it in here - when firestoreDB stops implementing BookDatabase, you know right away in your editor, unlike when it's in the test file (for VS Code, at least).

nextID: 1,
}
}

// Close closes the database.
func (db *memoryDB) Close() {
func (db *memoryDB) Close(ctx context.Context) error {

This comment has been minimized.

Copy link
@tbpg

tbpg Aug 23, 2019

Author Contributor

Done.


require (
cloud.google.com/go v0.44.3
github.com/gofrs/uuid v3.2.0+incompatible

This comment has been minimized.

Copy link
@tbpg

tbpg Aug 23, 2019

Author Contributor

Unfortunately, that version also doesn't work. So, sticking with incompatible.

// Use gorilla/mux for rich routing.
// See http://www.gorillatoolkit.org/pkg/mux
// See http://www.gorillatoolkit.org/pkg/mux.

This comment has been minimized.

Copy link
@tbpg

tbpg Aug 23, 2019

Author Contributor

Done.

_, err = topic.Publish(ctx, &pubsub.Message{Data: b}).Get(ctx)
log.Printf("Published update to Pub/Sub for Book ID %d: %v", bookID, err)
}

// http://blog.golang.org/error-handling-and-go

This comment has been minimized.

Copy link
@tbpg

tbpg Aug 23, 2019

Author Contributor

Done.

nextID int64 // next ID to assign to a book.
books map[int64]*Book // maps from Book ID to Book.
nextID int64 // next ID to assign to a book.
books map[string]*Book // maps from Book ID to Book.
}

func newMemoryDB() *memoryDB {

This comment has been minimized.

Copy link
@broady

broady Sep 9, 2019

Contributor

memoryDB is not used anymore either

This comment has been minimized.

Copy link
@broady

broady Sep 9, 2019

Contributor

but maybe it should be (for tests? see comment below)

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Added to tests - see other comment.

// bookFromRequest retrieves a book from the database given a book ID in the
// URL's path.
func (b *Bookshelf) bookFromRequest(r *http.Request) (*Book, error) {
id := mux.Vars(r)["id"]

This comment has been minimized.

Copy link
@broady

broady Sep 9, 2019

Contributor

valid if empty string?

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

No. Good catch. Checked.

}
book.ID = id

if err := b.DB.UpdateBook(r.Context(), book); err != nil {

This comment has been minimized.

Copy link
@broady

broady Sep 9, 2019

Contributor

ctx := r.Context() at top of each function

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Done.

}

// Don't log anything during testing.
log.SetOutput(ioutil.Discard)

This comment has been minimized.

Copy link
@broady

broady Sep 9, 2019

Contributor

maybe capture output in a buffer and print only if tests failed? probably doesn't matter

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Will leave as-is, but we can change if if/when this fails and we need debugging.


func TestMain(m *testing.M) {
projectID := os.Getenv("GOLANG_SAMPLES_FIRESTORE_PROJECT")
if projectID == "" {
fmt.Fprintln(os.Stderr, "GOLANG_SAMPLES_FIRESTORE_PROJECT not set. Skipping.")

This comment has been minimized.

Copy link
@broady

broady Sep 9, 2019

Contributor

use in-memory DB for tests, firestore for e2e tests?

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Done. It ended up changing the API quite a bit -- now pass a BookDatabase to NewBookshelf. It makes sense for testing, but is a slightly awkward API.

I updated this to always test the in memory database and only test the Firestore one if it's able to.

cookieStore.Options = &sessions.Options{
HttpOnly: true,
var err error
b.DB, err = configureFirestoreDB(projectID)

This comment has been minimized.

Copy link
@broady

broady Sep 9, 2019

Contributor

I think configureFirestoreDB can be inlined now.

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Done.

// gsutil defacl set public-read gs://my-project_bucket
// replacing my-project with your project ID.
b.StorageBucketName = projectID + "_bucket"
b.StorageBucket, err = configureStorage(b.StorageBucketName)

This comment has been minimized.

Copy link
@broady

broady Sep 9, 2019

Contributor

and configureStorage (inline)

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Done.

}

// uploadFileFromForm uploads a file if it's present in the "image" form field.
func (b *Bookshelf) uploadFileFromForm(r *http.Request) (url string, err error) {

This comment has been minimized.

Copy link
@broady

broady Sep 9, 2019

Contributor

pass in ctx explicitly rather than getting it from r.

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Done.

@tbpg
Copy link
Contributor Author

left a comment

Thanks! Please take another look.

cookieStore.Options = &sessions.Options{
HttpOnly: true,
var err error
b.DB, err = configureFirestoreDB(projectID)

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Done.

// gsutil defacl set public-read gs://my-project_bucket
// replacing my-project with your project ID.
b.StorageBucketName = projectID + "_bucket"
b.StorageBucket, err = configureStorage(b.StorageBucketName)

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Done.

}

// Ensure firestoreDB conforms to the BookDatabase interface.
var _ BookDatabase = &firestoreDB{}

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Yes, it also fails for go test.

// bookFromRequest retrieves a book from the database given a book ID in the
// URL's path.
func (b *Bookshelf) bookFromRequest(r *http.Request) (*Book, error) {
id := mux.Vars(r)["id"]

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

No. Good catch. Checked.

}

// uploadFileFromForm uploads a file if it's present in the "image" form field.
func (b *Bookshelf) uploadFileFromForm(r *http.Request) (url string, err error) {

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Done.

}
book.ID = id

if err := b.DB.UpdateBook(r.Context(), book); err != nil {

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Done.

}

// Don't log anything during testing.
log.SetOutput(ioutil.Discard)

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Will leave as-is, but we can change if if/when this fails and we need debugging.


func TestMain(m *testing.M) {
projectID := os.Getenv("GOLANG_SAMPLES_FIRESTORE_PROJECT")
if projectID == "" {
fmt.Fprintln(os.Stderr, "GOLANG_SAMPLES_FIRESTORE_PROJECT not set. Skipping.")

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Done. It ended up changing the API quite a bit -- now pass a BookDatabase to NewBookshelf. It makes sense for testing, but is a slightly awkward API.

I updated this to always test the in memory database and only test the Firestore one if it's able to.

getting-started/bookshelf/config.go Outdated Show resolved Hide resolved
nextID int64 // next ID to assign to a book.
books map[int64]*Book // maps from Book ID to Book.
nextID int64 // next ID to assign to a book.
books map[string]*Book // maps from Book ID to Book.
}

func newMemoryDB() *memoryDB {

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Added to tests - see other comment.

@@ -229,6 +247,7 @@ func (b *Bookshelf) createHandler(w http.ResponseWriter, r *http.Request) *appEr

// updateHandler updates the details of a given book.
func (b *Bookshelf) updateHandler(w http.ResponseWriter, r *http.Request) *appError {
ctx := r.Context()
id := mux.Vars(r)["id"]

This comment has been minimized.

Copy link
@broady

broady Sep 10, 2019

Contributor

check empty ID here too

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Done.

@broady
broady approved these changes Sep 10, 2019
tbpg added 2 commits Sep 10, 2019
@tbpg
Copy link
Contributor Author

left a comment

Thanks!

@@ -229,6 +247,7 @@ func (b *Bookshelf) createHandler(w http.ResponseWriter, r *http.Request) *appEr

// updateHandler updates the details of a given book.
func (b *Bookshelf) updateHandler(w http.ResponseWriter, r *http.Request) *appError {
ctx := r.Context()
id := mux.Vars(r)["id"]

This comment has been minimized.

Copy link
@tbpg

tbpg Sep 10, 2019

Author Contributor

Done.

@tbpg tbpg merged commit 0e0de93 into master Sep 10, 2019

2 checks passed

Kokoro CI Kokoro CI build successful
Details
cla/google All necessary CLAs are signed

@tbpg tbpg deleted the bookshelf branch Sep 10, 2019

tbpg added a commit that referenced this pull request Sep 11, 2019
tbpg added a commit that referenced this pull request Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.