Ensure charm origins have a revision

This will be useful for a future change, since we will now be able to
use the charm origin to get a revision, rather than being force to parse
it from the charm url

The client always fills in a revision when deploying. Also the apiserver
parses from the provided charm url is a revision is not provided. This
is for compatibility with old clients. In future, we will likely enforce
an origin is provided in validation steps

Add upgrade steps which add a revision to charm origins if it doesn't
have one. Parsed from the charm url

Do the same for migration on import

As a flyby, on migrations make sure to export -1 as the revision for charm
origins without a revision, because 0 is a valid revision for local
charms.

apiserver/facades/client/application/application.go

@@ -287,6 +287,16 @@ func (api *APIBase) Deploy(args params.ApplicationsDeploy) (params.ErrorResults,
 			result.Results[i].Error = apiservererrors.ServerError(err)
 			continue
 		}
+		// Fill in the charm origin revision from the charm url if it's absent
+		if arg.CharmOrigin.Revision == nil {
+			curl, err := charm.ParseURL(arg.CharmURL)
+			if err != nil {
+				result.Results[i].Error = apiservererrors.ServerError(err)
+				continue
+			}
+			rev := curl.Revision
+			arg.CharmOrigin.Revision = &rev
+		}
 		err := deployApplication(
 			api.backend,
 			api.model,

apiserver/facades/client/application/application_unit_test.go

@@ -1405,7 +1405,7 @@ func (s *ApplicationSuite) TestDeployCharmOrigin(c *gc.C) {
 	args := params.ApplicationsDeploy{
 		Applications: []params.ApplicationDeploy{{
 			ApplicationName: "foo",
-			CharmURL:        "local:foo-0",
+			CharmURL:        "local:foo-4",
 			CharmOrigin:     &params.CharmOrigin{Source: "local", Base: params.Base{Name: "ubuntu", Channel: "20.04/stable"}},
 			NumUnits:        1,
 		}, {
@@ -1419,7 +1419,7 @@ func (s *ApplicationSuite) TestDeployCharmOrigin(c *gc.C) {
 			NumUnits: 1,
 		}, {
 			ApplicationName: "hub",
-			CharmURL:        "hub-0",
+			CharmURL:        "hub-7",
 			CharmOrigin: &params.CharmOrigin{
 				Source: "charm-hub",
 				Risk:   "stable",
@@ -1438,6 +1438,10 @@ func (s *ApplicationSuite) TestDeployCharmOrigin(c *gc.C) {
 
 	c.Assert(s.deployParams["foo"].CharmOrigin.Source, gc.Equals, corecharm.Source("local"))
 	c.Assert(s.deployParams["hub"].CharmOrigin.Source, gc.Equals, corecharm.Source("charm-hub"))
+
+	// assert revision is filled in from the charm url
+	c.Assert(*s.deployParams["foo"].CharmOrigin.Revision, gc.Equals, 4)
+	c.Assert(*s.deployParams["hub"].CharmOrigin.Revision, gc.Equals, 7)
 }
 
 func createCharmOriginFromURL(curl *charm.URL) *params.CharmOrigin {
@@ -1626,13 +1630,18 @@ func (s *ApplicationSuite) TestApplicationDeploymentRemovesPendingResourcesOnFai
 	resourcesManager.EXPECT().RemovePendingAppResources("my-app", resources).Return(nil)
 	s.backend.EXPECT().Resources().Return(resourcesManager)
 
+	rev := 8
 	results, err := s.api.Deploy(params.ApplicationsDeploy{
 		Applications: []params.ApplicationDeploy{{
-			// CharmURL is missing to ensure deployApplication fails
-			// so that we can assert pending app resources are removed
+			// CharmURL is missing & revision included to ensure deployApplication
+			// fails sufficiently far through execution so that we can assert pending
+			// app resources are removed
 			ApplicationName: "my-app",
-			CharmOrigin:     validCharmOriginForTest(),
-			Resources:       resources,
+			CharmOrigin: &params.CharmOrigin{
+				Source:   "charm-hub",
+				Revision: &rev,
+			},
+			Resources: resources,
 		}},
 	})
 	c.Assert(err, jc.ErrorIsNil)

cmd/juju/application/utils/origin.go

@@ -55,19 +55,17 @@ func DeduceOrigin(url *charm.URL, channel charm.Channel, platform corecharm.Plat
 		if channel.Branch != "" {
 			branch = &channel.Branch
 		}
-		var revision *int
-		if url.Revision != -1 {
-			revision = &url.Revision
-		}
 		origin = commoncharm.Origin{
 			Source:       commoncharm.OriginCharmHub,
-			Revision:     revision,
 			Risk:         string(channel.Risk),
 			Track:        track,
 			Branch:       branch,
 			Architecture: architecture,
 		}
 	}
+	if url.Revision != -1 {
+		origin.Revision = &url.Revision
+	}
 	if platform.OS != "" && platform.Channel != "" {
 		base, err := corebase.ParseBase(platform.OS, platform.Channel)
 		if err != nil {

state/migration_export.go

@@ -2220,7 +2220,11 @@ func (e *exporter) getCharmOrigin(doc applicationDoc, defaultArch string) (descr
 	origin := doc.CharmOrigin
 
 	// If the channel is empty, then we fall back to the Revision.
-	var revision int
+	// Set default revision to -1. This is because a revision of 0 is
+	// a valid revision for local charms which we need to be able to
+	// from. On import, in the -1 case we grab the revision by parsing
+	// the charm url.
+	revision := -1
 	if rev := origin.Revision; rev != nil {
 		revision = *rev
 	}

state/migration_import.go

@@ -1419,6 +1419,17 @@ func (i *importer) makeCharmOrigin(a description.Application) (*CharmOrigin, err
 
 	co := a.CharmOrigin()
 	rev := co.Revision()
+	// If revision is empty we export revision -1. In this case
+	// parse the revision from the url
+	// NOTE: We use <= 0 because some old versions of juju default
+	// revision to 0 when it's empty
+	if rev <= 0 {
+		curl, err := charm.ParseURL(a.CharmURL())
+		if err != nil {
+			return nil, errors.Trace(err)
+		}
+		rev = curl.Revision
+	}
 
 	var channel *Channel
 	// Only charmhub charms will have a channel. Local charms

state/migration_import_test.go

@@ -697,6 +697,30 @@ func (s *MigrationImportSuite) TestApplicationsUpdateSeriesNotPlatform(c *gc.C)
 	c.Assert(obtainedOrigin.Platform.Channel, gc.Equals, "20.04/stable")
 }
 
+func (s *MigrationImportSuite) TestApplicationCharmOriginFilledIn(c *gc.C) {
+	platform := &state.Platform{Architecture: corearch.DefaultArchitecture, OS: "ubuntu", Channel: "12.10/stable"}
+	f := factory.NewFactory(s.State, s.StatePool)
+
+	testCharm := f.MakeCharm(c, &factory.CharmParams{Revision: "8"})
+	_ = f.MakeApplication(c, &factory.ApplicationParams{
+		Charm: testCharm,
+		Name:  "mysql",
+		CharmOrigin: &state.CharmOrigin{
+			Source: "charm-hub",
+			Type:   "charm",
+			Channel: &state.Channel{
+				Risk: "edge",
+			},
+			Platform: platform,
+		},
+	})
+
+	_, newSt := s.importModel(c, s.State)
+	newApp, err := newSt.Application("mysql")
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(*newApp.CharmOrigin().Revision, gc.Equals, 8)
+}
+
 func (s *MigrationImportSuite) TestApplicationStatus(c *gc.C) {
 	cons := constraints.MustParse("arch=amd64 mem=8G")
 	platform := &state.Platform{Architecture: corearch.DefaultArchitecture, OS: "ubuntu", Channel: "12.10/stable"}

state/upgrades.go

@@ -4,6 +4,7 @@
 package state
 
 import (
+	"github.com/juju/charm/v11"
 	"github.com/juju/errors"
 	"github.com/juju/mgo/v3/bson"
 	"github.com/juju/mgo/v3/txn"
@@ -253,3 +254,42 @@ func EnsureInitalRefCountForExternalSecretBackends(pool *StatePool) error {
 	}
 	return nil
 }
+
+func EnsureApplicationCharmOriginsHaveRevisions(pool *StatePool) error {
+	return errors.Trace(runForAllModelStates(pool, func(st *State) error {
+		allApps, err := st.AllApplications()
+		if err != nil {
+			return errors.Annotate(err, "loading applications")
+		}
+
+		var ops []txn.Op
+
+		for _, app := range allApps {
+			if app.CharmOrigin().Revision != nil {
+				continue
+			}
+			curlStr, _ := app.CharmURL()
+			if curlStr == nil {
+				logger.Warningf("Application %q has no charm url", app.Name())
+				continue
+			}
+			curl, err := charm.ParseURL(*curlStr)
+			if err != nil {
+				return errors.Annotatef(err, "parsing charm url %q", *curlStr)
+			}
+			if curl.Revision == -1 {
+				logger.Warningf("Charm url %q has no revision, defaulting to -1", curl.String())
+			}
+			ops = append(ops, txn.Op{
+				C:      applicationsC,
+				Id:     app.doc.DocID,
+				Assert: bson.D{{"charm-origin.revision", bson.D{{"$exists", false}}}},
+				Update: bson.D{{"$set", bson.D{{"charm-origin.revision", curl.Revision}}}},
+			})
+		}
+		if len(ops) > 0 {
+			return errors.Trace(st.runRawTransaction(ops))
+		}
+		return nil
+	}))
+}

state/upgrades_test.go

@@ -6,6 +6,7 @@ package state
 import (
 	"sort"
 
+	"github.com/juju/charm/v11"
 	"github.com/juju/errors"
 	"github.com/juju/mgo/v3"
 	"github.com/juju/mgo/v3/bson"
@@ -370,3 +371,94 @@ func (s *upgradesSuite) TestEnsureInitalRefCountForExternalSecretBackends(c *gc.
 	expectedData.filter = bson.D{{"_id", bson.M{"$regex": "secretbackend#revisions#.*"}}}
 	s.assertUpgradedData(c, EnsureInitalRefCountForExternalSecretBackends, expectedData)
 }
+
+func (s *upgradesSuite) TestEnsureApplicationCharmOriginsHaveRevisions(c *gc.C) {
+	ch := AddTestingCharm(c, s.state, "dummy")
+	rev := 8
+	platform := Platform{OS: "ubuntu", Channel: "20.04"}
+	_ = addTestingApplication(c, addTestingApplicationParams{
+		st:   s.state,
+		name: "my-app",
+		ch:   ch,
+		origin: &CharmOrigin{
+			Source:   charm.CharmHub.String(),
+			Platform: &platform,
+			Revision: &rev,
+		},
+		numUnits: 1,
+	})
+	_ = addTestingApplication(c, addTestingApplicationParams{
+		st:   s.state,
+		name: "my-local-app",
+		ch:   ch,
+		origin: &CharmOrigin{
+			Source:   charm.Local.String(),
+			Platform: &platform,
+		},
+		numUnits: 1,
+	})
+
+	apps, err := s.state.AllApplications()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(apps, gc.HasLen, 2)
+	c.Assert(*apps[0].CharmOrigin().Revision, gc.Equals, 8)
+	c.Assert(apps[1].CharmOrigin().Revision, gc.IsNil)
+
+	chBson := bson.M{
+		"_id": s.state.docID("my-app"),
+		"charm-origin": bson.M{
+			"hash":     "",
+			"id":       "",
+			"platform": bson.M{"os": "ubuntu", "channel": "20.04"},
+			"source":   "ch",
+			"revision": 8,
+		},
+		"charmmodifiedversion": 0,
+		"charmurl":             "local:quantal/quantal-dummy-1",
+		"exposed":              false,
+		"forcecharm":           false,
+		"life":                 0,
+		"metric-credentials":   []uint8{},
+		"minunits":             0,
+		"model-uuid":           s.state.ModelUUID(),
+		"name":                 "my-app",
+		"passwordhash":         "",
+		"provisioning-state":   nil,
+		"relationcount":        0,
+		"scale":                0,
+		"subordinate":          false,
+		"unitcount":            1,
+	}
+	localChBson := bson.M{
+		"_id": s.state.docID("my-local-app"),
+		"charm-origin": bson.M{
+			"hash":     "",
+			"id":       "",
+			"platform": bson.M{"os": "ubuntu", "channel": "20.04"},
+			"source":   "local",
+			"revision": 1,
+		},
+		"charmmodifiedversion": 0,
+		"charmurl":             "local:quantal/quantal-dummy-1",
+		"exposed":              false,
+		"forcecharm":           false,
+		"life":                 0,
+		"metric-credentials":   []uint8{},
+		"minunits":             0,
+		"model-uuid":           s.state.ModelUUID(),
+		"name":                 "my-local-app",
+		"passwordhash":         "",
+		"provisioning-state":   nil,
+		"relationcount":        0,
+		"scale":                0,
+		"subordinate":          false,
+		"unitcount":            1,
+	}
+	expected := bsonMById{chBson, localChBson}
+
+	appColl, closer := s.state.db().GetRawCollection(applicationsC)
+	defer closer()
+
+	expectedData := upgradedData(appColl, expected)
+	s.assertUpgradedData(c, EnsureApplicationCharmOriginsHaveRevisions, expectedData)
+}

upgrades/backend.go

@@ -14,6 +14,7 @@ type StateBackend interface {
 	RemoveOrphanedSecretPermissions() error
 	MigrateApplicationOpenedPortsToUnitScope() error
 	EnsureInitalRefCountForExternalSecretBackends() error
+	EnsureApplicationCharmOriginsHaveRevisions() error
 }
 
 // Model is an interface providing access to the details of a model within the
@@ -43,3 +44,7 @@ func (s stateBackend) MigrateApplicationOpenedPortsToUnitScope() error {
 func (s stateBackend) EnsureInitalRefCountForExternalSecretBackends() error {
 	return state.EnsureInitalRefCountForExternalSecretBackends(s.pool)
 }
+
+func (s stateBackend) EnsureApplicationCharmOriginsHaveRevisions() error {
+	return state.EnsureApplicationCharmOriginsHaveRevisions(s.pool)
+}

upgrades/operations.go

@@ -21,6 +21,7 @@ var stateUpgradeOperations = func() []Operation {
 	steps := []Operation{
 		upgradeToVersion{version.MustParse("3.1.1"), stateStepsFor311()},
 		upgradeToVersion{version.MustParse("3.1.3"), stateStepsFor313()},
+		upgradeToVersion{version.MustParse("3.1.7"), stateStepsFor317()},
 	}
 	return steps
 }
@@ -32,6 +33,7 @@ var upgradeOperations = func() []Operation {
 	steps := []Operation{
 		upgradeToVersion{version.MustParse("3.1.1"), stepsFor311()},
 		upgradeToVersion{version.MustParse("3.1.3"), stepsFor313()},
+		upgradeToVersion{version.MustParse("3.1.7"), stepsFor317()},
 	}
 	return steps
 }

upgrades/steps_317.go

@@ -0,0 +1,20 @@
+// Copyright 2023 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package upgrades
+
+func stepsFor317() []Step {
+	return []Step{}
+}
+
+func stateStepsFor317() []Step {
+	return []Step{
+		&upgradeStep{
+			description: "ensure application charm origins have revisions",
+			targets:     []Target{DatabaseMaster},
+			run: func(context Context) error {
+				return context.State().EnsureApplicationCharmOriginsHaveRevisions()
+			},
+		},
+	}
+}

upgrades/steps_317_test.go

@@ -0,0 +1,26 @@
+// Copyright 2023 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package upgrades_test
+
+import (
+	jc "github.com/juju/testing/checkers"
+	"github.com/juju/version/v2"
+	gc "gopkg.in/check.v1"
+
+	"github.com/juju/juju/testing"
+	"github.com/juju/juju/upgrades"
+)
+
+var v317 = version.MustParse("3.1.7")
+
+type steps317Suite struct {
+	testing.BaseSuite
+}
+
+var _ = gc.Suite(&steps317Suite{})
+
+func (s *steps317Suite) TestEnsureApplicationCharmOriginsHaveRevisions(c *gc.C) {
+	step := findStateStep(c, v317, "ensure application charm origins have revisions")
+	c.Assert(step.Targets(), jc.DeepEquals, []upgrades.Target{upgrades.DatabaseMaster})
+}