Merge pull request #1067 from jack-w-shaw/JUJU-6262_fix_refresh_revision

#1067

#### Description

We are incorrectly refreshing charms. The current method queries the controller for the charm's current origin and charm url

In pylibjuju, we then just overwrite the appropriate fields. However, this leaves behind id_ and hash_

This means when we later refresh, we are always returned the most recent revision by charmhub, no matter what revision we specify.

The way we handle this in the Juju CLI is by reconstructing the charm origin using MakeOrigin:
https://github.com/juju/juju/blob/a03ade4d358b1b0f135374b15b1bcd6b46d0ba0f/cmd/juju/application/utils/origin.go#L20-L75

This ensures only the fields we expect are present.

Do something similar in pylibjuju, ensuring we behave the same as the CLI.

Fixes: #1057

#### QA Steps

Unit tests
```
tox -e unit
```

Integration tests
```
tox -e integration -- tests/integration/test_application.py::test_refresh_revision
```

Manual tests
```
$ juju bootstrap lxd lxd
$ juju add-model m
$ juju deploy opensearch --revision 90 --channel=2/edge
$ python -m asyncio
>>> from juju.model import Model
>>> m = Model()
>>> await m.connect()
>>> app = m.applicatyions["opensearch"]
>>> await app.refresh(revision=91)
>>> exit()
$ juju status
Model Controller Cloud/Region Version SLA Timestamp
m lxd1 localhost/localhost 3.6-beta1 unsupported 13:32:44+01:00

App Version Status Scale Charm Channel Rev Exposed Message
opensearch maintenance 1 opensearch 2/edge 91 no Installing OpenSearch...

Unit Workload Agent Machine Public address Ports Message
opensearch/0* maintenance executing 0 10.219.211.254 (install) Installing OpenSearch...

Machine State Address Inst id Base AZ Message
0 started 10.219.211.254 juju-56bc09-0 ubuntu@22.04 Running
```

Author: jujubot

juju/application.py

@@ -12,12 +12,13 @@
 from .bundle import get_charm_series, is_local_charm
 from .client import client
 from .errors import JujuApplicationConfigError, JujuError
-from .origin import Channel, Source
+from .origin import Channel
 from .placement import parse as parse_placement
 from .relation import Relation
 from .status import derive_status
 from .url import URL
 from .utils import block_until
+from .version import DEFAULT_ARCHITECTURE
 
 log = logging.getLogger(__name__)
 
@@ -691,13 +692,15 @@ async def refresh(
         if charm_url_origin_result.error is not None:
             err = charm_url_origin_result.error
             raise JujuError(f'{err.code} : {err.message}')
-        origin = charm_url_origin_result.charm_origin
 
+        current_origin = charm_url_origin_result.charm_origin
         if path is not None or (switch is not None and is_local_charm(switch)):
-            await self.local_refresh(origin, force, force_series,
+            await self.local_refresh(current_origin, force, force_series,
                                      force_units, path or switch, resources)
             return
 
+        origin = _refresh_origin(current_origin, channel, revision)
+
         # If switch is not None at this point, that means it's a switch to a store charm
         charm_url = switch or charm_url_origin_result.url
         parsed_url = URL.parse(charm_url)
@@ -706,20 +709,6 @@ async def refresh(
         if parsed_url.schema is None:
             raise JujuError(f'A ch: or cs: schema is required for application refresh, given : {str(parsed_url)}')
 
-        if revision is not None:
-            origin.revision = revision
-
-        # Make the source-specific changes to the origin/channel/url
-        # (and also get the resources necessary to deploy the (destination) charm -- for later)
-        origin.source = Source.CHARM_HUB.value
-        if channel:
-            ch = Channel.parse(channel).normalize()
-            origin.risk = ch.risk
-            origin.track = ch.track
-
-        charmhub = self.model.charmhub
-        charm_resources = await charmhub.list_resources(charm_name)
-
         # Resolve the given charm URLs with an optionally specified preferred channel.
         # Channel provided via CharmOrigin.
         resolved_charm_with_channel_results = await charms_facade.ResolveCharms(
@@ -761,8 +750,7 @@ async def refresh(
             else:
                 _arg_res_filenames[res] = filename_or_rev
 
-        # Already prepped the charm_resources
-        # Now get the existing resources from the ResourcesFacade
+        # Get the existing resources from the ResourcesFacade
         request_data = [client.Entity(self.tag)]
         resources_facade = client.ResourcesFacade.from_connection(self.connection)
         response = await resources_facade.ListResources(entities=request_data)
@@ -771,6 +759,9 @@ async def refresh(
             for resource in response.results[0].resources
         }
 
+        charmhub = self.model.charmhub
+        charm_resources = await charmhub.list_resources(charm_name)
+
         # Compute the difference btw resources needed and the existing resources
         resources_to_update = []
         for resource in charm_resources:
@@ -917,6 +908,20 @@ async def get_metrics(self):
         return await self.model.get_metrics(self.tag)
 
 
+def _refresh_origin(current_origin: client.CharmOrigin, channel=None, revision=None) -> client.CharmOrigin:
+    if channel is not None:
+        channel = Channel.parse(channel).normalize()
+
+    return client.CharmOrigin(
+        source=current_origin.source,
+        track=channel.track if channel else current_origin.track,
+        risk=channel.risk if channel else current_origin.risk,
+        revision=revision if revision is not None else current_origin.revision,
+        base=current_origin.base,
+        architecture=current_origin.get('architecture', DEFAULT_ARCHITECTURE),
+    )
+
+
 class ExposedEndpoint:
     """ExposedEndpoint stores the list of CIDRs and space names which should be
     allowed access to the port ranges that the application has opened for a

tests/integration/test_application.py

@@ -286,6 +286,18 @@ async def test_local_refresh():
                                             base=client.Base("20.04", "ubuntu"))
 
 
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_refresh_revision():
+    async with base.CleanModel() as model:
+        app = await model.deploy('juju-qa-test', channel="latest/stable", revision=23)
+        # NOTE: juju-qa-test revision 26 has been released to this channel
+        await app.refresh(revision=25)
+
+        charm_url = URL.parse(app.data['charm-url'])
+        assert charm_url.revision == 25
+
+
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_trusted():

tests/unit/test_application.py

@@ -6,8 +6,10 @@
 import asyncio
 
 from juju.model import Model
-from juju.application import (Application, ExposedEndpoint)
+from juju.application import Application, ExposedEndpoint, _refresh_origin
 from juju.errors import JujuError
+from juju.client import client
+from juju.origin import Source
 
 
 class TestExposeApplication(unittest.IsolatedAsyncioTestCase):
@@ -177,3 +179,54 @@ async def test_refresh_mutually_exclusive_kwargs(self, mock_conn):
 
         with self.assertRaises(ValueError):
             await app.refresh(switch="charm1", path="/path/to/charm2")
+
+    def test_refresh_origin(self):
+        current_origin = client.CharmOrigin(
+            source=str(Source.CHARM_HUB),
+            track="latest",
+            risk="stable",
+            revision=100,
+            base=client.Base("24.04", "ubuntu"),
+            architecture="amd64",
+        )
+
+        origin = _refresh_origin(current_origin, None, None)
+        self.assertEqual(origin, current_origin)
+
+        origin = _refresh_origin(current_origin, None, 101)
+        self.assertEqual(origin.revision, 101)
+        # Check source, base & arch do not change
+        self.assertEqual(origin.source, current_origin.source)
+        self.assertEqual(origin.base, current_origin.base)
+        self.assertEqual(origin.architecture, current_origin.architecture)
+
+        origin = _refresh_origin(current_origin, None, 0)
+        self.assertEqual(origin.revision, 0)
+        # Check source, base & arch do not change
+        self.assertEqual(origin.source, current_origin.source)
+        self.assertEqual(origin.base, current_origin.base)
+        self.assertEqual(origin.architecture, current_origin.architecture)
+
+        origin = _refresh_origin(current_origin, "12/edge", None)
+        self.assertEqual(origin.track, "12")
+        self.assertEqual(origin.risk, "edge")
+        # Check source, base & arch do not change
+        self.assertEqual(origin.source, current_origin.source)
+        self.assertEqual(origin.base, current_origin.base)
+        self.assertEqual(origin.architecture, current_origin.architecture)
+
+    def test_refresh_origin_drops_id_hash(self):
+        current_origin = client.CharmOrigin(
+            source=str(Source.CHARM_HUB),
+            track="latest",
+            risk="stable",
+            revision=100,
+            base=client.Base("24.04", "ubuntu"),
+            architecture="amd64",
+            id_="id",
+            hash_="hash",
+        )
+
+        origin = _refresh_origin(current_origin, None, None)
+        self.assertIsNone(origin.id_)
+        self.assertIsNone(origin.hash_)