Merge lp:~cmars/juju-core/cs-resolve-notfound-fix into lp:~go-bot/juju-core/trunk

[Casey Marshall]

Fixed charm store series resolving issues.

Fixed an issue where trying to resolve series for a charm not found causes an
HTTP 502, needed a nil check.

Fixed an issue where the charm URL regex match for resolving series was
too greedy, causing it to fail to resolve mysql, for example.

Added test cases for the above.

Fixed charm store series resolving issues.

Fixed an issue where trying to resolve series for a charm not found causes an
HTTP 502, needed a nil check.

Fixed an issue where the charm URL regex match for resolving series was
too greedy, causing it to fail to resolve mysql, for example.

Added test cases for the above.

https://codereview.appspot.com/81250043/

[Casey Marshall (cmars)]

Reviewers: mp+212991_code.launchpad.net,

Message:
Please take a look.

Description:
Fixed charm store series resolving issues.

Fixed an issue where trying to resolve series for a charm not found
causes an
HTTP 502, needed a nil check.

Fixed an issue where the charm URL regex match for resolving series was
too greedy, causing it to fail to resolve mysql, for example.

Added test cases for the above.

https://code.launchpad.net/~cmars/juju-core/cs-resolve-notfound-fix/+merge/212991

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/81250043/

Affected files (+80, -4 lines):
   A [revision details]
   M store/server.go
   M store/server_test.go
   M store/store.go
   M store/store_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140325164440-iqj920tnf1v754dw
+New revision: <email address hidden>

Index: store/server.go
=== modified file 'store/server.go'
--- store/server.go 2014-03-24 15:35:41 +0000
+++ store/server.go 2014-03-25 18:57:36 +0000
@@ -117,7 +117,7 @@
     c.Revision = info.Revision()
     c.Digest = info.Digest()
    } else {
- if err == ErrNotFound {
+ if err == ErrNotFound && curl != nil {
      skey = charmStatsKey(curl, "charm-missing")
     }
     c.Errors = append(c.Errors, err.Error())

Index: store/server_test.go
=== modified file 'store/server_test.go'
--- store/server_test.go 2014-03-20 19:51:05 +0000
+++ store/server_test.go 2014-03-27 05:00:05 +0000
@@ -183,6 +183,23 @@
   s.checkCounterSum(c, []string{"charm-event", "oneiric", "mysql"}, false,
1)
  }

+func (s *StoreSuite) TestSeriesNotFound(c *gc.C) {
+ server, err := store.NewServer(s.store)
+ req, err := http.NewRequest("GET", "/charm-info?charms=cs:not-found", nil)
+ c.Assert(err, gc.IsNil)
+ rec := httptest.NewRecorder()
+ server.ServeHTTP(rec, req)
+ c.Assert(rec.Code, gc.Equals, http.StatusOK)
+
+ expected := map[string]interface{}{"cs:not-found": map[string]interface{}{
+ "revision": float64(0),
+ "errors": []interface{}{"entry not found"}}}
+ obtained := map[string]interface{}{}
+ err = json.NewDecoder(rec.Body).Decode(&obtained)
+ c.Assert(err, gc.IsNil)
+ c.Assert(obtained, gc.DeepEquals, expected)
+}
+
  // checkCounterSum checks that statistics are properly collected.
  // It retries a few times as they are generally collected in background.
  func (s *StoreSuite) checkCounterSum(c *gc.C, key []string, prefix bool,
expected int64) {

Index: store/store.go
=== modified file 'store/store.go'
--- store/store.go 2014-03-21 03:20:40 +0000
+++ store/store.go 2014-03-27 05:00:05 +0000
@@ -738,12 +738,12 @@
   session := s.session.Copy()
   defer session.Close()

- patternURL := &charm.URL{Reference: ref, Series: ".*"}
+ patternURL := &charm.URL{Reference: ref, Series: "[a-z][^/]+"}
   patternURL = patternURL.WithRevision(-1)

   charms := session.Charms()
   q := charms.Find(bson.M{
- "urls": bson.RegEx{Pattern: patternURL.String()},
+ "urls": bson.RegEx{Pattern: fmt.Sprintf("^%s$", patternURL.String())},
...

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/81250043/

=== modified file 'store/server.go'
--- store/server.go	2014-03-24 15:35:41 +0000
+++ store/server.go	2014-03-27 05:09:20 +0000
@@ -117,7 +117,7 @@
 			c.Revision = info.Revision()
 			c.Digest = info.Digest()
 		} else {
-			if err == ErrNotFound {
+			if err == ErrNotFound && curl != nil {
 				skey = charmStatsKey(curl, "charm-missing")
 			}
 			c.Errors = append(c.Errors, err.Error())

=== modified file 'store/server_test.go'
--- store/server_test.go	2014-03-20 19:51:05 +0000
+++ store/server_test.go	2014-03-27 05:09:20 +0000
@@ -183,6 +183,23 @@
 	s.checkCounterSum(c, []string{"charm-event", "oneiric", "mysql"}, false, 1)
 }
 
+func (s *StoreSuite) TestSeriesNotFound(c *gc.C) {
+	server, err := store.NewServer(s.store)
+	req, err := http.NewRequest("GET", "/charm-info?charms=cs:not-found", nil)
+	c.Assert(err, gc.IsNil)
+	rec := httptest.NewRecorder()
+	server.ServeHTTP(rec, req)
+	c.Assert(rec.Code, gc.Equals, http.StatusOK)
+
+	expected := map[string]interface{}{"cs:not-found": map[string]interface{}{
+		"revision": float64(0),
+		"errors":   []interface{}{"entry not found"}}}
+	obtained := map[string]interface{}{}
+	err = json.NewDecoder(rec.Body).Decode(&obtained)
+	c.Assert(err, gc.IsNil)
+	c.Assert(obtained, gc.DeepEquals, expected)
+}
+
 // checkCounterSum checks that statistics are properly collected.
 // It retries a few times as they are generally collected in background.
 func (s *StoreSuite) checkCounterSum(c *gc.C, key []string, prefix bool, expected int64) {

=== modified file 'store/store.go'
--- store/store.go	2014-03-21 03:20:40 +0000
+++ store/store.go	2014-03-27 05:09:20 +0000
@@ -738,12 +738,12 @@
 	session := s.session.Copy()
 	defer session.Close()
 
-	patternURL := &charm.URL{Reference: ref, Series: ".*"}
+	patternURL := &charm.URL{Reference: ref, Series: "[a-z][^/]+"}
 	patternURL = patternURL.WithRevision(-1)
 
 	charms := session.Charms()
 	q := charms.Find(bson.M{
-		"urls": bson.RegEx{Pattern: patternURL.String()},
+		"urls": bson.RegEx{Pattern: fmt.Sprintf("^%s$", patternURL.String())},
 	})
 	var cdocs []charmDoc
 	err := q.All(&cdocs)
@@ -755,7 +755,9 @@
 	seriesSet := make(map[string]bool)
 	for _, cdoc := range cdocs {
 		for _, url := range cdoc.URLs {
-			seriesSet[url.Series] = true
+			if ref == url.Reference {
+				seriesSet[url.Series] = true
+			}
 		}
 	}
 

=== modified file 'store/store_test.go'
--- store/store_test.go	2014-03-21 03:20:40 +0000
+++ store/store_test.go	2014-03-27 05:09:20 +0000
@@ -344,6 +344,9 @@
 	{"trusty", "wordpress"},
 	{"volumetric", "wordpress"},
 
+	{"precise", "mysql"},
+	{"trusty", "mysqladmin"},
+
 	{"def", "zebra"},
 	{"zef", "zebra"},
 }
@@ -373,6 +376,14 @@
 	c.Check(series[3], gc.Equals, "quantal")
 	c.Check(series[4], gc.Equals, "oneiric")
 
+	// Ensure that the full charm name matches, not just prefix
+	ref, _, err = charm.ParseReference("cs:mysql")
+	c.Assert(err, gc.IsNil)
+	series, err = s.store.Series(ref)
+	c.Assert(err, gc.IsNil)
+	c.Assert(series, gc.HasLen, 1)
+	c.Check(series[0], gc.Equals, "precise")
+
 	// No LTS, reverse alphabetical order
 	ref, _, err = charm.ParseReference("cs:zebra")
 	c.Assert(err, gc.IsNil)
@@ -383,6 +394,50 @@
 	c.Check(series[1], gc.Equals, "def")
 }
 
+var mysqlSeriesCharms = []struct {
+	fakeDigest string
+	urls       []string
+}{
+	{"533224069221503992aaa726", []string{"cs:~charmers/oneiric/mysql", "cs:oneiric/mysql"}},
+	{"533224c79221503992aaa7ea", []string{"cs:~charmers/precise/mysql", "cs:precise/mysql"}},
+	{"533223a69221503992aaa6be", []string{"cs:~bjornt/trusty/mysql"}},
+	{"533225b49221503992aaa8e5", []string{"cs:~clint-fewbar/precise/mysql"}},
+	{"5332261b9221503992aaa96b", []string{"cs:~gandelman-a/precise/mysql"}},
+	{"533226289221503992aaa97d", []string{"cs:~gandelman-a/quantal/mysql"}},
+	{"5332264d9221503992aaa9b0", []string{"cs:~hazmat/precise/mysql"}},
+	{"5332272d9221503992aaaa4d", []string{"cs:~jmit/oneiric/mysql"}},
+	{"53328a439221503992aaad28", []string{"cs:~landscape/trusty/mysql"}},
+	{"533228ae9221503992aaab96", []string{"cs:~negronjl/precise/mysql-file-permissions"}},
+	{"533228f39221503992aaabde", []string{"cs:~openstack-ubuntu-testing/oneiric/mysql"}},
+	{"533229029221503992aaabed", []string{"cs:~openstack-ubuntu-testing/precise/mysql"}},
+	{"5332291e9221503992aaac09", []string{"cs:~openstack-ubuntu-testing/quantal/mysql"}},
+	{"53327f4f9221503992aaad1e", []string{"cs:~tribaal/trusty/mysql"}},
+}
+
+func (s *StoreSuite) TestMysqlSeriesSolver(c *gc.C) {
+	for _, t := range mysqlSeriesCharms {
+		var urls []*charm.URL
+		for _, url := range t.urls {
+			urls = append(urls, charm.MustParseURL(url))
+		}
+
+		pub, err := s.store.CharmPublisher(urls, t.fakeDigest)
+		c.Assert(err, gc.IsNil)
+		c.Assert(pub.Revision(), gc.Equals, 0)
+
+		err = pub.Publish(&FakeCharmDir{})
+		c.Assert(err, gc.IsNil)
+	}
+
+	ref, _, err := charm.ParseReference("cs:mysql")
+	c.Assert(err, gc.IsNil)
+	series, err := s.store.Series(ref)
+	c.Assert(err, gc.IsNil)
+	c.Assert(series, gc.HasLen, 2)
+	c.Check(series[0], gc.Equals, "precise")
+	c.Check(series[1], gc.Equals, "oneiric")
+}
+
 func (s *StoreSuite) TestConflictingUpdate(c *gc.C) {
 	// This test checks that if for whatever reason the locking
 	// safety-net fails, adding two charms in parallel still