Bundle type endpoint #927

ahgittin · 2018-01-05T13:17:48Z

It was noted that if the same type is defined in two different bundles, the API does not let you distinguish between the two. This adds /catalog/bundles/{symbolicName}/{version}/types/{typeSymbolicName}/{typeVersion} to resolve that. (It also adds the two immediate ancestor paths, with ../types listing types in the bundle and ../types/{typeSymbolicName} giving TypeDetail assuming typeVersion==version).

It also adds a bunch of tests for behaviour when adding the same type ID:

testAddSameTypeTwiceInSameBundle_SilentlyDeduped()
testAddSameTypeTwiceInSameBundleDifferentDisplayName_LastOneWins()
testAddSameTypeTwiceInSameBundleDifferentDefinition_Disallowed()
testAddSameTypeTwiceInDifferentBundleDifferentDefinition_Disallowed()
testAddSameTypeTwiceInDifferentBundleSameDefinition_AllowedAndApiMakesTheDifferentOnesClear()

the final test above fails after the first commit but is fixed by the second commit here, by changing the type's self link to use the unambiguous bundles/.../types/... endpoint.

and fixes test for self link distinguishing between same type id in different bundles

tbouron

This LGTM @ahgittin, we were definitely missing those endpoints!

I just have a few comments/questions (see below) + one request: we can request the latest type by passing latest as the type's version (it also works like that for the all old catalog endpoints) Could you also add that to the lookup method for bundles?

tbouron · 2018-01-08T10:32:28Z

rest/rest-api/src/main/java/org/apache/brooklyn/rest/api/BundleApi.java

+        String version,
+        @ApiParam(name = "typeSymbolicName", value = "Type name to query", required = true)
+        @PathParam("typeSymbolicName")
+        String typeSymbolicName);


I'm not sure to understand the difference between this endpoint and the one below (i.e. /{symbolicName}/{version}/types/{typeSymbolicName}/{typeVersion}).

It seems to that that it will be the same as "/{symbolicName}/{version}/types/{typeSymbolicName}/latest so probably not needed?

it isn't latest - it is for case where type version = bundle version. i think as that should be the normal case we should have an endpoint which doesn't require repeating version.

Hum I get your PoV. I believe it's better to specify everything but why not, and it's using the same codepath so I'm fine with that

tbouron · 2018-01-08T10:34:31Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/BundleResource.java

+    @Override
+    public List<TypeSummary> getTypes(String symbolicName, String version) {
+        ManagedBundle b = lookup(symbolicName, version);
+        return TypeTransformer.bundleDetails(brooklyn(), b, ui.getBaseUriBuilder(), mgmt()).getTypes();


Don't we need an entitlement check on the bundle here?

lookup does the bundle entitlement check doesn't it?

Indeed, I did check that but forgot about it, ignore me then.

tbouron · 2018-01-08T10:59:17Z

...t-resources/src/test/java/org/apache/brooklyn/rest/resources/BundleAndTypeResourcesTest.java

+    private final static boolean DISALLOW_DIFFERENCES_IN_SAME_TYPE_ID_FROM_DIFFERENT_BUNDLES = true;
+
+    @Test
+    public void testAddSameTypeTwiceInDifferentBundleDifferentDefinition_Disallowed() throws Exception {


I don't understand why this case is not allowed. Could you elaborate on this please @ahgittin ?

it might be allowed in future, it certainly can be behind the scenes, but it can cause oddness as it is then ambiguous what type:version would mean if there are two different implementations. so there is strong argument to discourage/prevent it.

however in a multi-tenant world different users might not be allowed to see other bundles, so user 1 and user 2 might add the same type:version where they aren't allowed to see the other (and in fact shouldn't be allowed to know that someone else has defined that type:version), so in that case we would allow this. (we'd weaken restrictions to allow it, maybe e.g. user 2 can add type:version so long as he can't see any differing type:version.) but it might mean that a user 3 who is allowed to see bundles from both user 1 and user 2 is then able to see differing definition, which is not ideal and potentially confusing but i think least bad option. cc @aledsage ?

(NB longer term discussion, not part of this PR which just clarifies and tests current behaviour in this regard)

Ah got it, makes sense

ahgittin · 2018-01-08T11:59:43Z

+1 to supporting latest on bundle version -- i'll add that. i think all other comments addressed. latest bundle support could be new PR if you're happy to merge this now @tbouron .

tbouron · 2018-01-08T12:09:50Z

Yep, the latest for bundles is not in the scope of this PR so I'm happy if there is another one for this.

Merging

tbouron · 2018-01-08T15:51:59Z

Actually @ahgittin, it seems the new endpoint is not available: I get a 404 and it does not appear on the swagger page. Not sure what's going on here

ahgittin · 2018-01-08T16:04:51Z

v strange. are you sure it was built correctly @tbouron ? it maybe had a mvn clean install pull down a slightly-old snapshot jar? adding a new *Resource class needs updates in a few places but adding methods to the *Api classes is usually straightforward.

tbouron · 2018-01-08T16:08:18Z

I did but looks like I messed up my env somehow. Everything is fine now, sorry for the noise @ahgittin

ahgittin added 2 commits January 5, 2018 12:24

rest tests for same types added

2b5e55a

adds bundle-specific type endpoint

93b2b99

and fixes test for self link distinguishing between same type id in different bundles

tbouron reviewed Jan 8, 2018

View reviewed changes

asfgit merged commit 93b2b99 into apache:master Jan 8, 2018

asfgit pushed a commit that referenced this pull request Jan 8, 2018

This closes #927

792f4dd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bundle type endpoint #927

Bundle type endpoint #927

ahgittin commented Jan 5, 2018

tbouron left a comment

tbouron Jan 8, 2018

ahgittin Jan 8, 2018

tbouron Jan 8, 2018

tbouron Jan 8, 2018

ahgittin Jan 8, 2018

tbouron Jan 8, 2018

tbouron Jan 8, 2018

ahgittin Jan 8, 2018

tbouron Jan 8, 2018

ahgittin commented Jan 8, 2018

tbouron commented Jan 8, 2018

tbouron commented Jan 8, 2018 •

edited

ahgittin commented Jan 8, 2018

tbouron commented Jan 8, 2018

Bundle type endpoint #927

Bundle type endpoint #927

Conversation

ahgittin commented Jan 5, 2018

tbouron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahgittin commented Jan 8, 2018

tbouron commented Jan 8, 2018

tbouron commented Jan 8, 2018 • edited

ahgittin commented Jan 8, 2018

tbouron commented Jan 8, 2018

tbouron commented Jan 8, 2018 •

edited