Merge lp:~rharding/charmworld/fix-revisionless-store_url into lp:charmworld

[Richard Harding]

Allow revisionless cs: urls to be valid and found.

- Normally a full qualified store_url has a version on the end. Some users
(Jorge) remove those to create bundles that stay up to date with charms vs
locked.
- This supports that by performing a mongo regex vs exact match query when no
revision is found on the charm store urls.

https://codereview.appspot.com/19990043/

R=bac, benji

Allow revisionless cs: urls to be valid and found.

- Normally a full qualified store_url has a version on the end. Some users
(Jorge) remove those to create bundles that stay up to date with charms vs
locked.
- This supports that by performing a mongo regex vs exact match query when no
revision is found on the charm store urls.

https://codereview.appspot.com/19990043/

[Richard Harding (rharding)]

Reviewers: mp+193336_code.launchpad.net,

Message:
Please take a look.

Description:
Allow revisionless cs: urls to be valid and found.

- Normally a full qualified store_url has a version on the end. Some
users
(Jorge) remove those to create bundles that stay up to date with charms
vs
locked.
- This supports that by performing a mongo regex vs exact match query
when no
revision is found on the charm store urls.

https://code.launchpad.net/~rharding/charmworld/fix-revisionless-store_url/+merge/193336

(do not edit description out of merge proposal)

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

Affected files (+24, -1 lines):
   A [revision details]
   M charmworld/models.py
   M charmworld/tests/test_models.py

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-20131030201737-gz0c5vg8fgoy12ly
+New revision: <email address hidden>

Index: charmworld/models.py
=== modified file 'charmworld/models.py'
--- charmworld/models.py 2013-10-24 17:24:43 +0000
+++ charmworld/models.py 2013-10-30 20:59:40 +0000
@@ -1627,7 +1627,16 @@
      charm_query = None
      if (charm_description.store_url):
          # The description includes a cs: charm store url, use that.
- charm_query = {'store_url': charm_description.store_url}
+ # That cs url may or may not have a revision in it. If not, use a
+ # regex query to find charms regardless of revision.
+ if charm_description.revision:
+ charm_query = {'store_url': charm_description.store_url}
+ else:
+ charm_query = {
+ 'store_url': {
+ "$regex": charm_description.store_url
+ }
+ }
      elif charm_description.branch:
          # The description includes a branch_spec to search for.
          charm_query = {u'branch_spec': charm_description.branch}

Index: charmworld/tests/test_models.py
=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py 2013-10-24 18:23:37 +0000
+++ charmworld/tests/test_models.py 2013-10-30 20:59:40 +0000
@@ -2150,6 +2150,18 @@
          self.assertEqual(1, charm['store_data']['revision'])
          self.assertEqual(1, charm['revision'])

+ def test_resolve_by_store_url_sans_version(self):
+ data = {
+ 'charm': 'cs:precise/wikipedia'
+ }
+ charm_description = BundledCharmDescription('wiki', data)
+ charm = resolve_charm_from_description(self.db, charm_description)
+ self.assertIsNotNone(charm)
+ self.assertEqual('~charmers/precise/wikipedia/3', charm['_id'])
+ self.assertEqual('wikipedia', charm['name'])
+ self.assertEqual(3, charm['store_data']['revision'])
+ self.assertEqual(3, charm['revision'])
+
      def test_resolve_by_name_and_series(self):
          data = {
              'charm': 'wikipedia'

[Brad Crittenden (bac)]

LGTM

https://codereview.appspot.com/19990043/diff/1/charmworld/models.py
File charmworld/models.py (right):

https://codereview.appspot.com/19990043/diff/1/charmworld/models.py#newcode1637
charmworld/models.py:1637: "$regex": charm_description.store_url
Perhaps a comment here explaining why only the most recent revisions is
found due to code elsewhere would be good.

https://codereview.appspot.com/19990043/

[Benji York (benji)]

LGTM once the comments have been considered.

https://codereview.appspot.com/19990043/diff/1/charmworld/models.py
File charmworld/models.py (right):

https://codereview.appspot.com/19990043/diff/1/charmworld/models.py#newcode1630
charmworld/models.py:1630: # That cs url may or may not have a revision
in it. If not, use a
It took me a second to figure out what "cs" was. That suggests
expanding it to "charmstore" would help a less-involved reader.

https://codereview.appspot.com/19990043/diff/1/charmworld/models.py#newcode1637
charmworld/models.py:1637: "$regex": charm_description.store_url
Do we know that the regex query will return the latest version?

Is there something like a $prefix operator that we could use instead.
If not, we should meditate upon whether or not a store URL could be
interpreted as a regex. For example, if a dot were allowed in the name
it would match any character, potentially returning the wrong data.

Once that meditation has been done (or if it has already been done), a
comment noting the potential complications and that they have been
considered would be good.

https://codereview.appspot.com/19990043/

[lp:~rharding/charmworld/fix-revisionless-store_url]

438. By Richard Harding on 2013-10-31

Update docs and regex

[Juju Gui Bot (juju-gui-bot)]

review: Approve (continuous-integration)

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-10-24 17:24:43 +0000
+++ charmworld/models.py	2013-10-31 11:26:12 +0000
@@ -1608,6 +1608,10 @@
         if self.store_url:
             thoughts.append(
                 'Looking for store url: ' + self.store_url)
+            if not self.revision:
+                thoughts.append(
+                    'Regex search for store_url since no revision found: ' +
+                    self.store_url)
         elif self.branch:
             thoughts.append(
                 'Looking for branch: ' + self.branch)
@@ -1627,7 +1631,22 @@
     charm_query = None
     if (charm_description.store_url):
         # The description includes a cs: charm store url, use that.
-        charm_query = {'store_url': charm_description.store_url}
+        # That charmstore (cs:) url may or may not have a revision in it. If
+        # not, use a regex query to find charms regardless of revision.
+        if charm_description.revision:
+            charm_query = {'store_url': charm_description.store_url}
+        else:
+            # Charm names and series which make up a store_url are garunteed
+            # to be url safe. They also cannot have a '.' in them due to
+            # MongoDB limitations. This means we should be safe from .?&+.
+            # Someone could have a charm with []() in it, but it's pretty
+            # unlikely.
+            regex_query = "^%s-\d+" % charm_description.store_url
+            charm_query = {
+                'store_url': {
+                    "$regex": regex_query
+                }
+            }
     elif charm_description.branch:
         # The description includes a branch_spec to search for.
         charm_query = {u'branch_spec': charm_description.branch}

=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py	2013-10-24 18:23:37 +0000
+++ charmworld/tests/test_models.py	2013-10-31 11:26:12 +0000
@@ -2150,6 +2150,18 @@
         self.assertEqual(1, charm['store_data']['revision'])
         self.assertEqual(1, charm['revision'])
 
+    def test_resolve_by_store_url_sans_version(self):
+        data = {
+            'charm': 'cs:precise/wikipedia'
+        }
+        charm_description = BundledCharmDescription('wiki', data)
+        charm = resolve_charm_from_description(self.db, charm_description)
+        self.assertIsNotNone(charm)
+        self.assertEqual('~charmers/precise/wikipedia/3', charm['_id'])
+        self.assertEqual('wikipedia', charm['name'])
+        self.assertEqual(3, charm['store_data']['revision'])
+        self.assertEqual(3, charm['revision'])
+
     def test_resolve_by_name_and_series(self):
         data = {
             'charm': 'wikipedia'