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

Implement server /photos routing, get/post functionality #11

Merged
merged 9 commits into from
Feb 21, 2018
Merged

Conversation

warrn
Copy link
Collaborator

@warrn warrn commented Feb 17, 2018

#3

Implemented Endpoints

  • GET /photos/ - Return array of all photo objects stored
  • POST /photos/ - Upload photo
  • GET /photos/ids/ - Return array of all photo id strings
  • GET /photos/<id>/meta/ - Return photo object for the given id
  • GET /photos/<id>/image.jpg - Return jpg image data for the given id
  • GET /photos/<id>/thumb.jpg - Return jpg thumbnail data for the given id (created on upload)

Files:

  • servers/server.go - Contains server handling and setup
  • servers/request.go - Contains request handling logic for each route
  • servers/photo.go - Contains photo metadata struct, photo processing logic
  • servers/helpers.go - Contains misc. logic

@warrn warrn requested a review from kochman February 17, 2018 02:07
@warrn warrn self-assigned this Feb 17, 2018
@warrn warrn added the server label Feb 17, 2018
@warrn warrn added this to the Sprint 1 milestone Feb 17, 2018
warrn and others added 2 commits February 16, 2018 22:39
* Add thumbnail functionality

* Restructured server, added exif parsing, added fields to Photo

* Fix exif value stripping of null character
@kochman
Copy link
Owner

kochman commented Feb 20, 2018

The packages that this depends on should be added to the vendor/vendor.json file with govendor so that we're pinned to specific versions. This can be done with govendor fetch $package for each package.

Copy link
Owner

@kochman kochman left a comment

Choose a reason for hiding this comment

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

It works! Mostly commented about some stuff to think about. I don't think anything is important enough to hold up merging, but I'll wait a bit for anyone to read them/comment.

PhotosDirectory: "/var/hotshots",
}

dir, ok := os.LookupEnv("HOTSHOTS_DIR")
Copy link
Owner

Choose a reason for hiding this comment

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

We probably want to make ListenURL configurable via env var too. Another time.

defer thumbw.Close()
defer exifw.Close()

mw := io.MultiWriter(photow, thumbw, exifw)
Copy link
Owner

Choose a reason for hiding this comment

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

This is some fancy stuff I've never seen before

go GetExif(&xif, exifr, saveErr)

wasErrorSaving := false
for saveProcesses := 0; saveProcesses < 3; saveProcesses++ {
Copy link
Owner

Choose a reason for hiding this comment

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

I think using a sync.WaitGroup here would be more idiomatic, but this does the job just fine.

Success bool
Error string
URI string
}
Copy link
Owner

Choose a reason for hiding this comment

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

We likely want to snake_case all returned JSON fields with some struct tags, but it doesn't matter all that much.

}

func (s *Server) PostPhoto(w http.ResponseWriter, r *http.Request) {
r.ParseMultipartForm(1 << 23) // 8M max memory
Copy link
Owner

@kochman kochman Feb 21, 2018

Choose a reason for hiding this comment

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

Should we up this a bit? Anything up to maybe 20–30 MB would be a reasonable size for an uploaded JPEG, and if we up this a little bit (maybe 32 MB? Go's net/http sets its defaultMaxMemory to that) then we can avoid temp files touching the disk at all. We're not RAM-constrained on the server.

func GetExif(out *exif.Exif, data io.Reader, saveErr chan error) {
x, err := exif.Decode(data)
io.Copy(ioutil.Discard, data)
if err != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

Here err is always io.EOF if there is no Exif data in the photo, which causes a POST to /photos to fail. We might want to check for that and just return an empty Exif data. This is not a dealbreaker because any reasonable camera embeds some Exif data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wasn't a requirement for our service that your image had exif data?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, maybe. That would make sense. It's a requirement now.

saveErr <- err
return
}
*out = *x
Copy link
Owner

Choose a reason for hiding this comment

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

This is kinda funky; I think the idiomatic thing to do would be to return the Exif data through a channel vs. C-style passing pointers to functions to modify them.

saveErr <- err
return
}
*r = img.Bounds()
Copy link
Owner

Choose a reason for hiding this comment

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

Kinda funky; see my comment on line 187.

return false
}
readErr := unix.Access(serv.cfg.PhotosDirectory, unix.R_OK)
writeErr := unix.Access(serv.cfg.PhotosDirectory, unix.W_OK)
Copy link
Owner

Choose a reason for hiding this comment

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

Can this be done with os.Stat()? So we're not relying on OS-specific stuff. (I may want to run Hotshots on my Windows XP SP2 box.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dear god

@kochman kochman merged commit 3aca868 into master Feb 21, 2018
@warrn warrn deleted the server-3 branch February 21, 2018 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants