launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/juju-core/mpv-storage into lp:~maas-maintainers/juju-core/maas-provider-skeleton with lp:~jtv/juju-core/mpv-storage-list as a prerequisite.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/juju-core/mpv-storage/+merge/151705

This does not include file deletion yet; that part is still forthcoming.  There are tests to express what we expect from file deletion, and they compile & fail as expected at this stage, but I commented them out for now.  The next branch will re-enable them and then satisfy them.  The List() implementation was split off into a separate branch, lp:~jtv/gomaasapi/mpv-storage-list

After discussion with Julian, this storage implementation is without real support for slashes in filenames.  We don't know yet whether that is actually needed, and to implement it would require changes in both MAAS and gomaasapi.


Jeroen
-- 
https://code.launchpad.net/~jtv/juju-core/mpv-storage/+merge/151705
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/juju-core/mpv-storage into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

=== modified file 'environs/maas/storage.go'
--- environs/maas/storage.go	2013-03-05 09:18:27 +0000
+++ environs/maas/storage.go	2013-03-05 09:18:27 +0000
@@ -1,7 +1,11 @@
 package maas
 
 import (
+	"bytes"
+	"encoding/base64"
+	"fmt"
 	"io"
+	"io/ioutil"
 	"launchpad.net/gomaasapi"
 	"launchpad.net/juju-core/environs"
 	"net/url"
@@ -52,8 +56,50 @@
 	}
 }
 
-func (*maasStorage) Get(name string) (io.ReadCloser, error) {
-	panic("Not implemented.")
+// addressFileObject creates a MAASObject pointing to a given file.
+// Takes out a lock on the storage object to get a consistent view.
+func (stor *maasStorage) addressFileObject(name string) gomaasapi.MAASObject {
+	stor.Lock()
+	defer stor.Unlock()
+	return stor.maasClientUnlocked.GetSubObject(name)
+}
+
+// retrieveFileObject retrieves the information of the named file, including
+// its download URL and its contents, as a MAASObject.
+//
+// This may return many different errors, but specifically, it returns
+// environs.NotFoundError if the file did not exist.
+//
+// The function takes out a lock on the storage object.
+func (stor *maasStorage) retrieveFileObject(name string) (gomaasapi.MAASObject, error) {
+	obj, err := stor.addressFileObject(name).Get()
+	if err != nil {
+		noObj := gomaasapi.MAASObject{}
+		serverErr, ok := err.(gomaasapi.ServerError)
+		if ok && serverErr.StatusCode == 404 {
+			msg := fmt.Errorf("file '%s' not found", name)
+			return noObj, environs.NotFoundError{msg}
+		}
+		msg := fmt.Errorf("could not access file '%s': %v", name, err)
+		return noObj, msg
+	}
+	return obj, nil
+}
+
+func (stor *maasStorage) Get(name string) (io.ReadCloser, error) {
+	fileObj, err := stor.retrieveFileObject(name)
+	if err != nil {
+		return nil, err
+	}
+	data, err := fileObj.GetField("content")
+	if err != nil {
+		return nil, fmt.Errorf("could not extract file content for %s: %v", name, err)
+	}
+	buf, err := base64.StdEncoding.DecodeString(data)
+	if err != nil {
+		return nil, fmt.Errorf("bad data in file '%s': %v", name, err)
+	}
+	return ioutil.NopCloser(bytes.NewReader(buf)), nil
 }
 
 // extractFilenames returns the filenames from a "list" operation on the
@@ -92,12 +138,35 @@
 	return snapshot.extractFilenames(obj)
 }
 
-func (*maasStorage) URL(name string) (string, error) {
-	panic("Not implemented.")
+func (stor *maasStorage) URL(name string) (string, error) {
+	fileObj, err := stor.retrieveFileObject(name)
+	if err != nil {
+		return "", err
+	}
+	uri, err := fileObj.GetField("anon_resource_uri")
+	if err != nil {
+		msg := fmt.Errorf("could not get file's download URL (may be an outdated MAAS): %s", err)
+		return "", msg
+	}
+
+	partialURL, err := url.Parse(uri)
+	if err != nil {
+		return "", err
+	}
+	fullURL := fileObj.URL().ResolveReference(partialURL)
+	return fullURL.String(), nil
 }
 
-func (*maasStorage) Put(name string, r io.Reader, length int64) error {
-	panic("Not implemented.")
+func (stor *maasStorage) Put(name string, r io.Reader, length int64) error {
+	data, err := ioutil.ReadAll(io.LimitReader(r, length))
+	if err != nil {
+		return err
+	}
+	params := url.Values{"filename": {name}}
+	files := map[string][]byte{"file": data}
+	snapshot := stor.getSnapshot()
+	_, err = snapshot.maasClientUnlocked.CallPostFiles("add", params, files)
+	return err
 }
 
 func (*maasStorage) Remove(name string) error {

=== modified file 'environs/maas/storage_test.go'
--- environs/maas/storage_test.go	2013-03-05 09:18:27 +0000
+++ environs/maas/storage_test.go	2013-03-05 09:18:27 +0000
@@ -1,10 +1,15 @@
 package maas
 
 import (
+	"bytes"
+	"encoding/base64"
+	"io/ioutil"
 	. "launchpad.net/gocheck"
 	"launchpad.net/gomaasapi"
 	"launchpad.net/juju-core/environs"
 	"math/rand"
+	"net/http"
+	"net/url"
 	"sync"
 )
 
@@ -52,6 +57,90 @@
 	c.Check(snapshot.Mutex, Equals, sync.Mutex{})
 }
 
+func (s *StorageSuite) TestGetRetrievesFile(c *C) {
+	const filename = "stored-data"
+	storage := s.makeStorage("get-retrieves-file")
+	file := s.fakeStoredFile(storage, filename)
+	base64Content, err := file.GetField("content")
+	c.Assert(err, IsNil)
+	content, err := base64.StdEncoding.DecodeString(base64Content)
+	c.Assert(err, IsNil)
+
+	reader, err := storage.Get(filename)
+	c.Assert(err, IsNil)
+	defer reader.Close()
+
+	buf, err := ioutil.ReadAll(reader)
+	c.Assert(err, IsNil)
+	c.Check(len(buf), Equals, len(content))
+	c.Check(buf, DeepEquals, content)
+}
+
+func (s *StorageSuite) TestRetrieveFileObjectReturnsFileObject(c *C) {
+	const filename = "myfile"
+	stor := s.makeStorage("rfo-test")
+	file := s.fakeStoredFile(stor, filename)
+	fileURI, err := file.GetField("anon_resource_uri")
+	c.Assert(err, IsNil)
+	fileContent, err := file.GetField("content")
+	c.Assert(err, IsNil)
+
+	obj, err := stor.retrieveFileObject(filename)
+	c.Assert(err, IsNil)
+
+	uri, err := obj.GetField("anon_resource_uri")
+	c.Assert(err, IsNil)
+	c.Check(uri, Equals, fileURI)
+	content, err := obj.GetField("content")
+	c.Check(content, Equals, fileContent)
+}
+
+func (s *StorageSuite) TestRetrieveFileObjectReturnsNotFound(c *C) {
+	stor := s.makeStorage("rfo-test")
+	_, err := stor.retrieveFileObject("nonexistent-file")
+	c.Assert(err, NotNil)
+	c.Check(err, FitsTypeOf, environs.NotFoundError{})
+}
+
+func (s *StorageSuite) TestRetrieveFileObjectEscapesName(c *C) {
+	const filename = "#a?b c&d%e!"
+	data := []byte("File contents here")
+	stor := s.makeStorage("rfo-test")
+	err := stor.Put(filename, bytes.NewReader(data), int64(len(data)))
+	c.Assert(err, IsNil)
+
+	obj, err := stor.retrieveFileObject(filename)
+	c.Assert(err, IsNil)
+
+	base64Content, err := obj.GetField("content")
+	c.Assert(err, IsNil)
+	content, err := base64.StdEncoding.DecodeString(base64Content)
+	c.Assert(err, IsNil)
+	c.Check(content, DeepEquals, data)
+}
+
+func (s *StorageSuite) TestFileContentsAreBinary(c *C) {
+	const filename = "myfile.bin"
+	data := []byte{0, 1, 255, 2, 254, 3}
+	stor := s.makeStorage("binary-test")
+
+	err := stor.Put(filename, bytes.NewReader(data), int64(len(data)))
+	c.Assert(err, IsNil)
+	file, err := stor.Get(filename)
+	c.Assert(err, IsNil)
+	content, err := ioutil.ReadAll(file)
+	c.Assert(err, IsNil)
+
+	c.Check(content, DeepEquals, data)
+}
+
+func (s *StorageSuite) TestGetReturnsNotFoundErrorIfNotFound(c *C) {
+	const filename = "lost-data"
+	storage := NewStorage(s.environ)
+	_, err := storage.Get(filename)
+	c.Assert(err, FitsTypeOf, environs.NotFoundError{})
+}
+
 func (s *StorageSuite) TestListReturnsEmptyIfNoFilesStored(c *C) {
 	storage := NewStorage(s.environ)
 	listing, err := storage.List("")
@@ -120,3 +209,148 @@
 	c.Assert(err, IsNil)
 	c.Check(listing, DeepEquals, []string{"a/b/c/d"})
 }
+
+// getFileAtURL requests, and returns, the file at the given URL.
+func getFileAtURL(fileURL string) ([]byte, error) {
+	request, err := http.NewRequest("GET", fileURL, nil)
+	if err != nil {
+		return nil, err
+	}
+	client := http.Client{}
+	response, err := client.Do(request)
+	if err != nil {
+		return nil, err
+	}
+	body, err := ioutil.ReadAll(response.Body)
+	if err != nil {
+		return nil, err
+	}
+	return body, nil
+}
+
+func (s *StorageSuite) TestURLReturnsURLCorrespondingToFile(c *C) {
+	const filename = "my-file.txt"
+	storage := NewStorage(s.environ).(*maasStorage)
+	file := s.fakeStoredFile(storage, filename)
+	// The file contains an anon_resource_uri, which lacks a network part
+	// (but will probably contain a query part).  anonURL will be the
+	// file's full URL.
+	anonURI, err := file.GetField("anon_resource_uri")
+	c.Assert(err, IsNil)
+	parsedURI, err := url.Parse(anonURI)
+	c.Assert(err, IsNil)
+	anonURL := storage.maasClientUnlocked.URL().ResolveReference(parsedURI)
+	c.Assert(err, IsNil)
+
+	fileURL, err := storage.URL(filename)
+	c.Assert(err, IsNil)
+
+	c.Check(fileURL, NotNil)
+	c.Check(fileURL, Equals, anonURL.String())
+}
+
+func (s *StorageSuite) TestPutStoresRetrievableFile(c *C) {
+	const filename = "broken-toaster.jpg"
+	contents := []byte("Contents here")
+	length := int64(len(contents))
+	storage := NewStorage(s.environ)
+
+	err := storage.Put(filename, bytes.NewReader(contents), length)
+
+	reader, err := storage.Get(filename)
+	c.Assert(err, IsNil)
+	defer reader.Close()
+
+	buf, err := ioutil.ReadAll(reader)
+	c.Assert(err, IsNil)
+	c.Check(buf, DeepEquals, contents)
+}
+
+func (s *StorageSuite) TestPutOverwritesFile(c *C) {
+	const filename = "foo.bar"
+	storage := NewStorage(s.environ)
+	s.fakeStoredFile(storage, filename)
+	newContents := []byte("Overwritten")
+
+	err := storage.Put(filename, bytes.NewReader(newContents), int64(len(newContents)))
+	c.Assert(err, IsNil)
+
+	reader, err := storage.Get(filename)
+	c.Assert(err, IsNil)
+	defer reader.Close()
+
+	buf, err := ioutil.ReadAll(reader)
+	c.Assert(err, IsNil)
+	c.Check(len(buf), Equals, len(newContents))
+	c.Check(buf, DeepEquals, newContents)
+}
+
+func (s *StorageSuite) TestPutStopsAtGivenLength(c *C) {
+	const filename = "xyzzyz.2.xls"
+	const length = 5
+	contents := []byte("abcdefghijklmnopqrstuvwxyz")
+	storage := NewStorage(s.environ)
+
+	err := storage.Put(filename, bytes.NewReader(contents), length)
+	c.Assert(err, IsNil)
+
+	reader, err := storage.Get(filename)
+	c.Assert(err, IsNil)
+	defer reader.Close()
+
+	buf, err := ioutil.ReadAll(reader)
+	c.Assert(err, IsNil)
+	c.Check(len(buf), Equals, length)
+}
+
+func (s *StorageSuite) TestPutToExistingFileTruncatesAtGivenLength(c *C) {
+	const filename = "a-file-which-is-mine"
+	oldContents := []byte("abcdefghijklmnopqrstuvwxyz")
+	newContents := []byte("xyz")
+	storage := NewStorage(s.environ)
+	err := storage.Put(filename, bytes.NewReader(oldContents), int64(len(oldContents)))
+	c.Assert(err, IsNil)
+
+	err = storage.Put(filename, bytes.NewReader(newContents), int64(len(newContents)))
+	c.Assert(err, IsNil)
+
+	reader, err := storage.Get(filename)
+	c.Assert(err, IsNil)
+	defer reader.Close()
+
+	buf, err := ioutil.ReadAll(reader)
+	c.Assert(err, IsNil)
+	c.Check(len(buf), Equals, len(newContents))
+	c.Check(buf, DeepEquals, newContents)
+}
+
+// These tests not satisfied yet.  Coming in next branch!
+/*
+func (s *StorageSuite) TestRemoveDeletesFile(c *C) {
+	const filename = "doomed.txt"
+	storage := NewStorage(s.environ)
+	s.fakeStoredFile(storage, filename)
+
+	err := storage.Remove(filename)
+	c.Assert(err, IsNil)
+
+	_, err = storage.Get(filename)
+	c.Assert(err, FitsTypeOf, environs.NotFoundError{})
+
+	listing, err := storage.List(filename)
+	c.Assert(err, IsNil)
+	c.Assert(listing, DeepEquals, []string{})
+}
+
+func (s *StorageSuite) TestRemoveIsIdempotent(c *C) {
+	const filename = "half-a-file"
+	storage := NewStorage(s.environ)
+	s.fakeStoredFile(storage, filename)
+
+	err := storage.Remove(filename)
+	c.Assert(err, IsNil)
+
+	err = storage.Remove(filename)
+	c.Assert(err, IsNil)
+}
+*/