REST API for bundles, types, and subtypes by ahgittin · Pull Request #810 · apache/brooklyn-server

ahgittin · 2017-09-07T16:05:51Z

also tweaks to other classes to make them more usable, eg VersionedName comparables and bundle removal giving an OsgiBundleInstallationResult

test in BundleAndTypeAndSubtypeResourcesTest based on CatalogResourceTest, basically giving parity in terms of functionality and test coverage

also tweaks to other classes to make them more usable, eg VersionedName comparables and bundle removal giving an OsgiBundleInstallationResult test in BundleAndTypeAndSubtypeResourcesTest based on CatalogResourceTest, basically giving parity in terms of functionality and test coverage

ahgittin · 2017-09-07T16:18:05Z

BrooklynCatalog api tidy minor conflict

tbouron · 2017-09-15T10:11:17Z


+/** @deprecated since 0.12.0 use /bundle, /type, and /subtype */
+// but we will probably keep this around for a while as many places use it
+@Deprecated


I strongly disagree with deprecating the entire /catalog endpoint. I think we should rather deprecate /catalog/(entities|applications|policies|enrichers) and add /bundle, /type and /subtype within /catalog as we have a "catalog" of bundles, types, etc. (that is effectivement what you did with the /type endpoint, by duplicating /application, /entities, etc) Promoting them to the root level feel way too loose.

Also, what is the difference between type and subtype? It's not clear to me and it looks like a subtype is just a subset of type. If that is a case, why bother having a endpoint for this? Cannot we just have a filter on the /type endpoint?

Regarding the bundle additions, the POST /catalog/create can already deal with bundles so I would just reuse that. For the deletion, I would use the DELETE /catalog/bundles/<symbolicName>/<version>.

Good points @tbouron - discussed on ML

update: i think this is how we are going to do it now

m4rkmckenna · 2017-09-20T10:47:50Z

+    @JsonInclude(value=Include.NON_EMPTY)
+    private Set<String> aliases;
+    @JsonInclude(value=Include.NON_EMPTY)
+    private Set<Object> supertypes;


This should be Set<String>

agree, done

m4rkmckenna · 2017-09-20T11:04:01Z

+@Consumes(MediaType.APPLICATION_JSON)
+@Produces(MediaType.APPLICATION_JSON)
+public interface SubtypeApi {
+


DRY just add the below filters to the TypeApi as a query param

Discussed on ML

m4rkmckenna · 2017-09-20T11:07:22Z

+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonInclude.Include;
+
+public class TypeDetail extends TypeSummary {


Public api ... add java doc

m4rkmckenna · 2017-09-20T11:07:37Z

+import com.fasterxml.jackson.annotation.JsonInclude.Include;
+import com.google.common.collect.ComparisonChain;
+
+public class TypeSummary implements Comparable<TypeSummary> {


m4rkmckenna · 2017-09-20T11:14:29Z

+    @ApiOperation(value = "Get summaries for all versions and instances of a given type or alias, with best match first", 
+            response = TypeSummary.class,
+            responseContainer = "List")
+    public List<TypeSummary> listVersions(


The response type seems odd to me ... I would expect it to only return 1 summary

If aliases can be re used i think this is a big mistake

@m4rkmckenna I think it's a list because it's giving you info about each version (e.g. an entry for v1.0.0 and another for v1.1.0).

Interesting point about reusing aliases. Hopefully that's not the case!

yes this is for all verisons

m4rkmckenna · 2017-09-20T11:19:12Z

+        String versions,
+        @ApiParam(name = "regex", value = "Regular expression to search for")
+        @QueryParam("regex") @DefaultValue("") String regex,
+        @ApiParam(name = "fragment", value = "Substring case-insensitive to search for")


Is regex and fragment not a duplication

"some-fragment" == ".*some-fragment.*"

Not if some-fragment contains a . or other special character. Globs are more user friendly and search friendly. (This is borrowed from elsewhere in API incidentally where we do the same thing and there have been times I've been grateful for the easier fragment.)

No strong feelings, but I wonder if we should err towards the smaller api (simpler to support and for someone to get their head around). You can achieve everything with regex. A UI could offer a fragment search on top of that, escaping things as needed.

Could do with more description of what the fragment will search (think it's just the name or alias?).

Somewhere in the existing /v1/catalog api the fragment will search the entire yaml plan - that's surprising, something useful, and sometimes annoying when it returns a bunch of seemingly unrelated things!

fragment is v useful as regex-escaping is tedious (esp for case insensitive!). it could be called glob to be clearer. agree we should say what is searched, will do that.

(change to glob could be /v2beta item)

m4rkmckenna · 2017-09-20T11:20:10Z

+        String supertype,
+        @ApiParam(name = "versions", value = "Whether to list 'latest' of each symbolic-name or 'all' versions", required = false, defaultValue = "latest")
+        @QueryParam("versions")
+        String versions,


Should be singular

I think plural as version=all doesn't make sense and it implies wanting to do things like version=1.0. It could be boolean allVersions but my thinking was in future this might take a range.

aledsage · 2017-09-20T11:33:43Z

TL;DR: I propose we split this PR into two. The /bundles part we can merge pretty quickly. However, the /types and /subtypes is too controversial - it probably deserves a /v2/ of the REST api.

I don't want to lose the word "catalog" in the REST api - it's so good at getting across the meaning for users! The alternative /type is just not as good, in my opinion.

The multiple endpoints of /types and /subtypes is confusing. I'd model the latter as just a filter on /type. It would be better achieved with an additional query parameter rather than a separate endpoint.

If designing a /v2 REST api, we could use /catalog instead of /type. However, it will likely take a while to get to a stable and good /v2 api. There are other cleanup/improvements we should probably do to the REST api if we're releasing a new version of it (e.g. exclude the deprecated stuff, get rid of /locations but figure out if we really need to support locations from brooklyn.properties, find out from the community about other inconsistencies or hard-to-understand parts of the api).

The meaning of GET /subtypes/application looks completely different from GET /catalog/applications. The latter retrieves the catalog items marked as template, but the new api returns everything that implements Application. Perhaps this is an opportunity to get rid of the "entity" versus "template" distinction (at least in the REST api). The original meaning/intent of "template" has entirely changed / been misused! I believe it was originally intended as a partially-complete YAML blueprint that someone would retrieve via the REST api, and then modify. They'd then POST their .yaml file to deploy their app. It has now been used as the list of apps to include in a "quick launch" view. If designing a new /v2 api, I'd explicitly support a "quick launch" list and would get rid of the "template" versus "application" versus "entity" distinction in the REST api (anywhere you can use an entity, you can use an app; anywhere you need an app then a non-app entity will be auto-wrapped in a basic-app).

ahgittin · 2017-09-20T14:45:45Z

As per ML:

Aled, Thomas, Mark - thanks for the good feedback.

All - There are some good suggestions which I agree deserve discussion. Specific points.

(1) Should /types and /bundles be top-level or under a /catalog prefix ? Thomas suggested the latter which also fits. My reason for doing top-level is simply that most REST APIs these days seem to have more top-level resources. /catalog is not necessary and in some ways it's cleaner having separate endpoints. On the other hand the /catalog prefix is nice to orient consumers, as Aled says: bundles and types on their own aren't as self-evident. And it means we could continue to have POST /catalog as the main way to install.

(2) Should we deprecate /catalog ? Thomas suggests maybe not yet. I think we should as equivalent functionality and more is available at the new endpoints. (Though if we go with (1), obviously we wouldn't deprecate the whole endpoint, just the old calls.). Also worth noting: I don't think we should remove the deprecated endpoint anytime soon however.

(3) Should we switch to /v2 ? Aled has suggested we do as the generic types support is a significant shift from the old more restrictive catalog. I don't think we should yet, however: I'd prefer to make that move when we are ready to remove all old endpoints so v2 is clutter-free. To the extent v2 can look like a subset of v1 we make life easier for users and ourselves. Also there is significant work to add a /v2 endpoint and I don't think there is benefit yet to justify this work.

(4) Should /subtypes be an endpoint peer of /types ? It has been noted the same functionality as /subtypes/entity could be done with /types?super=entity (with appropriate query paramter). My reasoning for making it a separate endpoint is that this is an activity I expect people will want to do a lot, avoiding the query parameter makes for a slightly nicer API. But it is repetition in the code.

(There are some other minor issues but I think I agree with the points made there.)

My answers:

(1) slight preference for the /catalog prefix
(2) strong pref to deprecate old calls - they are redundant and multiple is confusing
(3) strong pref to stay with /v1 for now
(4) slight pref for explicit [/catalog]/subtypes endpoint

ahgittin · 2017-09-20T14:52:55Z

Regarding other point about template v application - should that be addressed in this PR @aledsage or could it be a subsequent one?

aledsage · 2017-09-20T15:59:14Z

For template versus application, we can defer that (as long as we're not making pain for ourselves in this api; I don't think we are). Good to separate things into more PRs. UIs that want to show a quick launch will continue to use /v1/catalog/application, which will not be supported by the new endpoint(s).

aledsage · 2017-09-21T17:19:52Z

+    }
+
+    private static <T extends TypeSummary> T embellish(T result, RegisteredType item, boolean detail, BrooklynRestResourceUtils b, UriBuilder ub) {
+        result.getExtraFields().put("links", makeLinks(item, ub));


This use of getXyz().put(key, val) is a really bad pattern, especially when there is a setExtraField(key, val) method you can call instead.

It makes it a lot less clear to people supporting the code how the BundleSummary is populated.

aledsage · 2017-09-21T17:26:18Z

+            for (Effector<?> x: type.getEffectors())
+                effectors.add(EffectorTransformer.effectorSummaryForCatalog(x));
+
+            result.getExtraFields().put("config", config);


Where do you imagine we'll document the names "config", "sensors" and "effectors" (or do people have to discover it by making some live calls and figuring out what looks to be there consistently for everything of a given type)?

Where would we document the type of the values for something like an effector?

I think the looseness won't be an issue for Brooklyn developers (because we can easily navigate the code base to find the EffectorTransformer, when working on the CLI or UI).

However, I think it makes it feel more scary for a user of Brooklyn who is thinking of writing an integration with our REST api. They won't know what fields to expect. They can make a few calls and then guess at what is optional versus mandatory, and then code as defensively as possible in case their guess was wrong.

for now there is no doc for extra info for specific types. agree this is a gap but it is a lot more classes and maintenance to have all that detail completely type specific. i'm quite sure run-time discoverability for extra fields will be sufficient for a while -- and it's not at all uncommon in pragmatic REST APIs i have used -- and when users start wanting more precision then i think it's worth doing the extra work (and preference for extra fields added in this class with docs to say they are only for selected types, rather than lots of subtypes).

aledsage · 2017-09-21T17:29:12Z

+    @ApiOperation(value = "List bundles registered in the system including their types", 
+            response = BundleSummary.class,
+            responseContainer = "List")
+    public List<BundleSummary> list(


Feels like this will return a huge amount of data (giving all the types that are inside the bundle).

Do we want a way to just list the bundles? I think we want to have a BundleSummary and a BundleDetail, where only the latter includes all the types. The list method could include support for ?detail=true if we think that's important for a UI to retrieve everything it needs to show in one go.

aledsage · 2017-09-21T17:36:46Z

+            @ApiResponse(code = 400, message = "Error processing the given archive, or the catalog.bom is invalid"),
+            @ApiResponse(code = 201, message = "Catalog items added successfully")
+    })
+    public Response createAutodetecting(


I lean towards not doing this - instead rely on Content-Type: ... being supplied.

I say this because of the annoying behaviour in the existing api for things like POST /v1/catalog. If you post an invalid .bom file, it first tries to parse it as a .bom which fails, and then tries to parse it as a different format. The error that gets returned is often misleading (e.g. it gives you details of the second attempt, rather than the first attempt). I've hit this problem while trying to help real newbee (but smart) users of Brooklyn.

Looking at this new code, it would be quite easy to lose the error message that tells you your yaml was invalid, with it instead telling you that it's not a zip file.

What does this really buy us? The UI and CLI will always supply it. If using curl then you have to type a little bit more, but I think that's fine.

this is all being removed as we will just have POST /catalog -- but i agree with you

aledsage · 2017-09-21T17:37:45Z

+            return data;
+        }
+    }
+    private final TypeImplementationPlanSummary plan;


I'd make this non-final, so that json-deserialization doesn't have to do nasty stuff.

aledsage · 2017-09-21T17:44:00Z

+
+    @Override
+    public BundleInstallationRestResult remove(String symbolicName, String version, Boolean force) {
+        ManagedBundle b = lookup(symbolicName, version);


Really want an entitlements check (as per your previous TODOs). Otherwise the presence of the /bundles endpoint will be a horrible backdoor for allowing a user to damage other people's apps.

agree, as above

aledsage · 2017-09-21T17:49:58Z

+        String versions,
+        @ApiParam(name = "regex", value = "Regular expression to search for")
+        @QueryParam("regex") @DefaultValue("") String regex,
+        @ApiParam(name = "fragment", value = "Substring case-insensitive to search for")


No strong feelings, but I wonder if we should err towards the smaller api (simpler to support and for someone to get their head around). You can achieve everything with regex. A UI could offer a fragment search on top of that, escaping things as needed.

aledsage · 2017-09-21T17:57:14Z

+        String versions,
+        @ApiParam(name = "regex", value = "Regular expression to search for")
+        @QueryParam("regex") @DefaultValue("") String regex,
+        @ApiParam(name = "fragment", value = "Substring case-insensitive to search for")


Could do with more description of what the fragment will search (think it's just the name or alias?).

Somewhere in the existing /v1/catalog api the fragment will search the entire yaml plan - that's surprising, something useful, and sometimes annoying when it returns a bunch of seemingly unrelated things!

aledsage · 2017-09-21T17:58:05Z

+    @ApiOperation(value = "Get summaries for all versions and instances of a given type or alias, with best match first", 
+            response = TypeSummary.class,
+            responseContainer = "List")
+    public List<TypeSummary> listVersions(


@m4rkmckenna I think it's a list because it's giving you info about each version (e.g. an entry for v1.0.0 and another for v1.1.0).

aledsage · 2017-09-21T17:58:43Z

+    @ApiOperation(value = "Get summaries for all versions and instances of a given type or alias, with best match first", 
+            response = TypeSummary.class,
+            responseContainer = "List")
+    public List<TypeSummary> listVersions(


Interesting point about reusing aliases. Hopefully that's not the case!

still to do: supertypes as query param, and restructure to hang both of catalog (remove POST /catalog/bundles)

ahgittin · 2017-09-22T12:13:04Z

all updated as discussed, running tests now.

misc notes:

entitlements reuses see_catalog_item for now, for bundles and types; that should be tidied up at some point
POST /catalog/bundles has a different return type to POST /catalog so i've kept that; in v2 we should switch
create autodetecting removed from /catalog/bundles

move /bundles and /types under /catalog, remove subtype, remove autodetect create in POST /catalog/bundles

aledsage

@ahgittin LGTM. A few minor comments for you to read through (your call if/when anything needs done), then +1 to merging.

aledsage · 2017-09-22T21:27:19Z


    private static class AnySuperTypeMatches implements Predicate<RegisteredType> {
-        private final Predicate<Class<?>> filter;
+        private final Predicate<Object> filter;


Is this definitely not persisted anywhere? If it is, then changing the generics could cause weird errors later.

didn't think generics would affect persistence, but i've restored it and made a new class, just in case.

also removed inner classes elsewhere here where they might be returned to the caller.

aledsage · 2017-09-22T21:38:08Z

 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

+import com.google.common.base.Function;


I only just noticed that a bunch of this code is in core/mgmt/ha. Why is that? I'd have thought it would equally apply to non-ha (e.g. persisting your state locally, and then restarting Brooklyn).

indeed, longstanding misplacement. would be good to move OsgiManager and the related classes.

aledsage · 2017-09-22T21:43:30Z


+    /** @deprecated since 0.13.0 delete the bundle via DELETE /catalog/bundles/xxx */
+    // but we will probably keep this around for a while as many places use it
+    @Deprecated


I'm still uncomfortable about the way we're handling our versioning, and with deprecating /v1/catalog methods. To me, the old /catalog is the "v1 way of doing it". Once we do a properly versioned /v2 then it will be clear that v1 is deprecated (and we'll document it as such).

But we can go with this for now.

For future work, it would be great if we can get a process in place by which we version the api in a sensible way: to help people understand what they are using, what is replacing it, and how/when it will be deprecated.

A big problem with @Deprecated here is that I don't think an api user is told about it at all (it's not even in the swagger), so from the perspective of a rest-api-user it's as though it's not deprecated.

aledsage · 2017-09-22T21:51:09Z

-import com.google.common.collect.ImmutableMap;
-
-public class EnricherConfigSummary extends ConfigSummary {
+// TODO remove? this class has no value over its super


Agreed - I think we can safely delete it. The json returned by AdjunctConfigSummary will be identical to EnricherConfigSummary etc. But we can do that in a future PR.

Not sure what the difference is between EntityConfigSummary and AdjunctConfigSummary (it has "pinned" and "constraints"). Adjuncts can have constraints, I believe. For "pinned", the CatalogConfig javadoc says "a pinned configuration means that the config key will always be displayed when presenting as editable in the catalog", so I think that also applies to adjuncts (well, most adjuncts and eventually all).

agree, there is no reason to have more than one ConfigSummary

aledsage · 2017-09-22T22:00:59Z

+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
+
+public class BundleAndTypeResourcesTest extends BrooklynRestResourceTest {


Great tests!

Class is getting pretty big. Maybe worth splitting into separate classes for bundle and type tests (or separating out the invalid jar/bundle/bom tests). But leave as-is for now.

ahgittin · 2017-09-25T09:20:41Z

addressed comments - tidied RegisteredTypePredicates for persistence, and deprecated all pointless descendants of ConfigSummary

ahgittin force-pushed the bundle-rest-api branch from 8355661 to 71f43d5 Compare September 7, 2017 16:13

ahgittin force-pushed the bundle-rest-api branch from 71f43d5 to 9b33703 Compare September 7, 2017 16:15

Merge branch 'master' into bundle-rest-api

1cd6bf4

BrooklynCatalog api tidy minor conflict

ahgittin mentioned this pull request Sep 13, 2017

REST API for accessing adjuncts (including highlights) #821

Merged

tbouron reviewed Sep 15, 2017

View reviewed changes

m4rkmckenna requested changes Sep 20, 2017

View reviewed changes

This was referenced Sep 20, 2017

REST call to get activities on an adjunct #836

Merged

Improve Brooklyn-management of OSGi-already-managed bundles #842

Merged

aledsage suggested changes Sep 21, 2017

View reviewed changes

ahgittin added 2 commits September 22, 2017 10:20

Merge branch 'master' into bundle-rest-api

831db4a

address most PR comments

206f744

still to do: supertypes as query param, and restructure to hang both of catalog (remove POST /catalog/bundles)

ahgittin added 2 commits September 22, 2017 13:28

remaining PR comments

05ad45e

move /bundles and /types under /catalog, remove subtype, remove autodetect create in POST /catalog/bundles

deprecation is since 0.13.0 now (12 is out)

8e69330

ahgittin force-pushed the bundle-rest-api branch from 412ce6c to 8e69330 Compare September 22, 2017 12:28

aledsage approved these changes Sep 22, 2017

View reviewed changes

address PR comments, mainly simplify ConfigSummary

54fb9c6

asfgit merged commit 54fb9c6 into apache:master Sep 25, 2017

asfgit pushed a commit that referenced this pull request Sep 25, 2017

This closes #810

05ae90a

Conversation

ahgittin commented Sep 7, 2017

Uh oh!

ahgittin commented Sep 7, 2017

Uh oh!

tbouron Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aledsage commented Sep 20, 2017

Uh oh!

ahgittin commented Sep 20, 2017

Uh oh!

ahgittin commented Sep 20, 2017

Uh oh!

aledsage commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tbouron Sep 15, 2017 •

edited

Loading