Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the bundles-containing API endpoint. #78

Merged
merged 10 commits into from
Aug 21, 2014
3 changes: 3 additions & 0 deletions internal/charmstore/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,9 @@ func bundleCharms(data *charm.BundleData) ([]*charm.Reference, error) {
return nil, errgo.Mask(err)
}
urlMap[url.String()] = url
// Also add the corresponding base URL.
base := baseURL(url)
urlMap[base.String()] = base
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just:

urlMap[base.String() = baseURL(url)

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow this, we need to define base to be able to do base.String().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Ignore me :-)

}
urls := make([]*charm.Reference, 0, len(urlMap))
for _, url := range urlMap {
Expand Down
28 changes: 17 additions & 11 deletions internal/v4/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,13 @@ func New(store *charmstore.Store) http.Handler {
"manifest": h.entityHandler(h.metaManifest, "blobname"),
"archive-upload-time": h.entityHandler(h.metaArchiveUploadTime, "uploadtime"),
"charm-related": h.entityHandler(h.metaCharmRelated, "charmprovidedinterfaces", "charmrequiredinterfaces"),
"bundles-containing": h.entityHandler(h.metaBundlesContaining),

// endpoints not yet implemented - use SingleIncludeHandler for the time being.
"color": router.SingleIncludeHandler(h.metaColor),
"bundles-containing": router.SingleIncludeHandler(h.metaBundlesContaining),
"revision-info": router.SingleIncludeHandler(h.metaRevisionInfo),
"extra-info": router.SingleIncludeHandler(h.metaExtraInfo),
"extra-info/": router.SingleIncludeHandler(h.metaExtraInfoWithKey),
"color": router.SingleIncludeHandler(h.metaColor),
"revision-info": router.SingleIncludeHandler(h.metaRevisionInfo),
"extra-info": router.SingleIncludeHandler(h.metaExtraInfo),
"extra-info/": router.SingleIncludeHandler(h.metaExtraInfoWithKey),
},
}, h.resolveURL)
return h
Expand Down Expand Up @@ -160,6 +160,18 @@ func preferredURL(url0, url1 *charm.Reference) bool {
return ltsReleases[url0.Series]
}

// stringToBool returns false if value is "0" or empty, true if value is "1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseBool would be a slightly more conventional name for this.
Perhaps follow the wording from strconv.ParseBool?

// parseBool returns the boolean value represented by the string.
// It accepts "1" or "0". Any other value returns an error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// an error otherwise. This can be useful to convert url.Values data.
func stringToBool(value string) (bool, error) {
switch value {
case "0", "":
return false, nil
case "1":
return true, nil
}
return false, errgo.Newf(`value must be either "0", "1" or empty, but %q passed`, value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errgo.Newf(`unexpected bool value %q (must be "0" or "1")`, value)

?

(probably not worth mentioning that it can be empty - that's just to allow
it to be omitted)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

var errNotImplemented = errgo.Newf("method not implemented")

// GET search[?text=text][&autocomplete=1][&filter=value…][&limit=limit][&include=meta]
Expand Down Expand Up @@ -299,12 +311,6 @@ func (h *handler) metaStats(id *charm.Reference, path string, method string, fla
return nil, errNotImplemented
}

// GET id/meta/bundles-containing[?include=meta[&include=meta…]]
// http://tinyurl.com/oqc386r
func (h *handler) metaBundlesContaining(id *charm.Reference, path string, method string, flags url.Values) (interface{}, error) {
return nil, errNotImplemented
}

// GET id/meta/revision-info
// http://tinyurl.com/q6xos7f
func (h *handler) metaRevisionInfo(id *charm.Reference, path string, method string, flags url.Values) (interface{}, error) {
Expand Down
42 changes: 37 additions & 5 deletions internal/v4/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,13 @@ func storeURL(path string) string {
type metaEndpointExpectedValueGetter func(*charmstore.Store, *charm.Reference) (interface{}, error)

type metaEndpoint struct {
name string
exclusive int
bundleOnly bool
get metaEndpointExpectedValueGetter

// The name of the meta endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding these comments. I'd be tempted to stick to the usual style,
with white space between the entries. Here are some suggestions
for slightly different phrasing:

// name names the meta endpoint.
name string

// exclusive specifies whether the endpoint is
// valid for charms only (charmOnly), bundles only (bundleOnly)
// or to both (zero).
exclusive int

// get returns the expected data for the endpoint.
get metaEndpointExpectedValueGetter

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

name string
// Whether the endpoint applies only to charms (charmOnly),
// to bundles (bundleOnly) or to both (empty field).
exclusive int
// A function returning the expected data for the endpoint.
get metaEndpointExpectedValueGetter
// checkURL holds one URL to sanity check data against.
checkURL string
// assertCheckData holds a function that will be used to check that
Expand Down Expand Up @@ -147,6 +149,36 @@ var metaEndpoints = []metaEndpoint{{
c.Assert(response.UploadTime, gc.Not(jc.Satisfies), time.Time.IsZero)
c.Assert(response.UploadTime.Location(), gc.Equals, time.UTC)
},
}, {
name: "charm-related",
exclusive: charmOnly,
get: func(store *charmstore.Store, url *charm.Reference) (interface{}, error) {
// The charms we use for those tests are not related each other.
// Charm relations are independently tested in relations_test.go.
if url.Series == "bundle" {
return nil, nil
}
return &params.RelatedResponse{}, nil
},
checkURL: "cs:precise/wordpress-23",
assertCheckData: func(c *gc.C, data interface{}) {
_ = data.(*params.RelatedResponse)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c.Assert(data, gc.FitsTypeOf, (*params.RelatedResponse)(nil))

?

I'm assuming the (non-EndpointGet) tests fail if you change the
get function to return nil always, or &params.RelatedResponse{} always.

If you haven't checked that, please do, just for peace of mind.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I already checked that.
Added the data assertion.

},
}, {
name: "bundles-containing",
exclusive: charmOnly,
get: func(store *charmstore.Store, url *charm.Reference) (interface{}, error) {
// The charms we use for those tests are not included in any bundle.
// Charm/bundle relations are tested in relations_test.go.
if url.Series == "bundle" {
return nil, nil
}
return []*params.MetaAnyResponse{}, nil
},
checkURL: "cs:precise/wordpress-23",
assertCheckData: func(c *gc.C, data interface{}) {
_ = data.([]*params.MetaAnyResponse)
},
}}

// TestEndpointGet tries to ensure that the endpoint
Expand Down
80 changes: 80 additions & 0 deletions internal/v4/relations.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ import (
// GET id/meta/charm-related[?include=meta[&include=meta…]]
// http://tinyurl.com/q7vdmzl
func (h *handler) metaCharmRelated(entity *mongodoc.Entity, id *charm.Reference, path, method string, flags url.Values) (interface{}, error) {
if id.Series == "bundle" {
return nil, nil
}

// If the charm does not define any relation we can just return without
// hitting the db.
if len(entity.CharmProvidedInterfaces)+len(entity.CharmRequiredInterfaces) == 0 {
Expand Down Expand Up @@ -132,3 +136,79 @@ func (h *handler) getRelatedIfaceResponses(
}
return responses, nil
}

// GET id/meta/bundles-containing[?include=meta[&include=meta…]][&any-series=1][&any-revision=1]
// http://tinyurl.com/oqc386r
func (h *handler) metaBundlesContaining(entity *mongodoc.Entity, id *charm.Reference, path, method string, flags url.Values) (interface{}, error) {
if id.Series == "bundle" {
return nil, nil
}

// Validate the URL query values.
anySeries, err := stringToBool(flags.Get("any-series"))
if err != nil {
return nil, badRequestf(err, "invalid value for any-series")
}
anyRevision, err := stringToBool(flags.Get("any-revision"))
if err != nil {
return nil, badRequestf(err, "invalid value for any-revision")
}

// Mutate the reference so that it represents a base URL if required.
searchId := *id
if anySeries || anyRevision {
searchId.Revision = -1
searchId.Series = ""
}

// Retrieve the bundles containing the resulting charm id.
var entities []mongodoc.Entity
if err := h.store.DB.Entities().
Find(bson.D{{"bundlecharms", &searchId}}).
Select(bson.D{{"_id", 1}, {"bundlecharms", 1}}).
All(&entities); err != nil {
return nil, errgo.Notef(err, "cannot retrieve the related bundles")
}

// Further filter the entities if required.
if anySeries != anyRevision {
predicate := func(e *mongodoc.Entity) bool {
for _, charmId := range e.BundleCharms {
if charmId.Name == id.Name &&
charmId.User == id.User &&
(anySeries || charmId.Series == id.Series) &&
(anyRevision || charmId.Revision == id.Revision) {
return true
}
}
return false
}
entities = filterEntities(entities, predicate)
}

// Prepare and return the response.
response := make([]*params.MetaAnyResponse, 0, len(entities))
includes := flags["include"]
for _, e := range entities {
meta, err := h.GetMetadata(e.URL, includes)
if err != nil {
return nil, errgo.Notef(err, "cannot retrieve bundle metadata")
}
response = append(response, &params.MetaAnyResponse{
Id: e.URL,
Meta: meta,
})
}
return response, nil
}

// filterEntities filters the given entities based on the given predicate.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if this said how it filters the entities.

// filterEntities returns a slice containing all the entities
// for which the given predicate returns true.

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

func filterEntities(entities []mongodoc.Entity, predicate func(*mongodoc.Entity) bool) []mongodoc.Entity {
results := make([]mongodoc.Entity, 0, len(entities))
for _, entity := range entities {
if predicate(&entity) {
results = append(results, entity)
}
}
return results
}
Loading