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

add an /applications/details endpoint which corrects problems with /fetch #967

Merged
merged 2 commits into from Jun 29, 2018

Conversation

Projects
None yet
4 participants
@ahgittin
Copy link
Contributor

commented Jun 1, 2018

and adds some new features:

noticed that fetch actually recurses through all children entities which was never the intention and is wasteful at scale. also for usability it would be nice to include tags and all/regex sensors and config values. this adds support for that.

i've deprecated the existing /fetch call but left its behaviour unchanged. have also expanded description of /list endpoint.

to make this easier i've promoted getAll() from SensorSupportInternal. previously we had no way on the public API to list all sensors for an entity.
it is still @beta.

add an /applications/details endpoint which corrects problems with /f…
…etch and adds new features

noticed that fetch actually recurses through all children entities which was never the intention and is wasteful at scale.
also for usability it would be nice to include tags and all/regex sensors and config values.  this adds support for that.

i've deprecated the existing `/fetch` call but left its behaviour unchanged.  have also expanded description of `/list` endpoint.

to make this easier i've promoted `getAll()` from SensorSupportInternal. previously we had no way on the public API to list all sensors for an entity.
it is still @beta.
@aledsage
Copy link
Contributor

left a comment

Looks good, but a few comments - primarily about (explaining) the implementation.

)
@Deprecated
/** @deprecated since 1.0.0 use {@link #details(String, String, int)} */

This comment has been minimized.

Copy link
@aledsage

aledsage Jun 4, 2018

Contributor

Signature of link is wrong: should be {@link #details(String, boolean, String, String, int)}`

This comment has been minimized.

Copy link
@ahgittin

ahgittin Jun 12, 2018

Author Contributor

agree, done

@@ -106,7 +112,16 @@
@Context
private UriInfo uriInfo;

private EntityDetail fromEntity(Entity entity) {
private EntitySummary fromEntity(Entity entity, boolean includeTags, int detailDepth, List<String> extraSensorGlobs, List<String> extraConfigGlobs) {
if (detailDepth==0) {

This comment has been minimized.

Copy link
@aledsage

aledsage Jun 4, 2018

Contributor

If detailDepth==0, it will ignore the extraSensorGlobs and extraConfigGlobs. That doesn't feel intuitive - is it deliberate? I interpreted "with references to children but not their details" as meaning we'd see the entity ids, but we wouldn't have a record for them.

The deliberate behaviour for negative depth is also not intuitive (i.e. will contradict what a maintainer thinks 'depth' means) and subtle.

This comment has been minimized.

Copy link
@ahgittin

ahgittin Jun 12, 2018

Author Contributor

see below

List<EntityDetail> entitySummaries = Lists.newArrayList();
for (Entity application : mgmt().getApplications()) {
entitySummaries.add(addSensors(fromEntity(application), application, extraSensors));
entitySummaries.add(addSensorsByName((EntityDetail)fromEntity(application, false, -1, null, null), application, extraSensors));

This comment has been minimized.

Copy link
@aledsage

aledsage Jun 4, 2018

Contributor

It looks like this is doing something sneaky (passing in depth -1, so the recursion will never terminate early by reaching depth 0). If I'm reading that right, it really needs to be documented. Someone could easily change the behaviour of fromEntity in the future without realising that someone was relying on negative depth to mean infinite!

This comment has been minimized.

Copy link
@ahgittin

ahgittin Jun 12, 2018

Author Contributor

see below

Map<String, EntitySummary> entitySummaries = MutableMap.of();
if (includeAllApps) {
for (Entity application : mgmt().getApplications()) {
entitySummaries.put(application.getId(), fromEntity(application, true, depth, extraSensorGlobs, extraSensorGlobs));

This comment has been minimized.

Copy link
@aledsage

aledsage Jun 4, 2018

Contributor

Should guard this with Entitlements.isEntitled for each of the apps.

Are we happy to assume that if you can see an entity, then you should be allowed to see all of that entity's children?

This comment has been minimized.

Copy link
@ahgittin

ahgittin Jun 12, 2018

Author Contributor

good spot, done, and for descendants that should also have a guard. both done.

String name = kv.getKey().getName();
if (extraSensorGlobs.stream().anyMatch(sn -> WildcardGlobs.isGlobMatched(sn, name))) {
sv = resolving(sv).preferJson(true).asJerseyOutermostReturnValue(false).raw(true).context(entity).immediately(true).timeout(Duration.ZERO).resolve();
sensors.put(name, sv);

This comment has been minimized.

Copy link
@aledsage

aledsage Jun 4, 2018

Contributor

Should this be guarded with Entitlements.isEntitled(mgmt().getEntitlementManager(), Entitlements.SEE_SENSOR, new EntityAndItem<String>(entity, name))

This comment has been minimized.

Copy link
@ahgittin

ahgittin Jun 12, 2018

Author Contributor

another good spot, all sensors and configs should be guarded, done.

Object v = entity.config().get(key);
if (v!=null) {
v = resolving(v).preferJson(true).asJerseyOutermostReturnValue(false).raw(true).context(entity).immediately(true).timeout(Duration.ZERO).resolve();
configs.put(key.getName(), v);

This comment has been minimized.

Copy link
@aledsage

aledsage Jun 4, 2018

Contributor

Should this be guarded with Entitlements.isEntitled(mgmt().getEntitlementManager(), Entitlements.SEE_CONFIG, new EntityAndItem<String>(entity, key.getName()))

This comment has been minimized.

Copy link
@ahgittin

ahgittin Jun 12, 2018

Author Contributor

done

@geomacy
Copy link
Contributor

left a comment

Looks useful, one thing to fix and one suggestion below.

Map<String, EntitySummary> entitySummaries = MutableMap.of();
if (includeAllApps) {
for (Entity application : mgmt().getApplications()) {
entitySummaries.put(application.getId(), fromEntity(application, true, depth, extraSensorGlobs, extraSensorGlobs));

This comment has been minimized.

Copy link
@geomacy

geomacy Jun 4, 2018

Contributor

fromEntity(application, true, depth, extraSensorGlobs, extraSensorGlobs)) needs to be
... extraSensorGlobs, extraConfigGlobs)).

at the moment when you request sensors you get results including empty config object, but when you request config you don't get either sensors or config.

This comment has been minimized.

Copy link
@ahgittin

ahgittin Jun 12, 2018

Author Contributor

good spot, fixed

Entity entity = mgmt().getEntityManager().getEntity(entityId.trim());
while (entity != null && !entitySummaries.containsKey(entity.getId())) {
if (Entitlements.isEntitled(mgmt().getEntitlementManager(), Entitlements.SEE_ENTITY, entity)) {
entitySummaries.put(entity.getId(), fromEntity(entity, true, depth, extraSensorGlobs, extraSensorGlobs));

This comment has been minimized.

Copy link
@geomacy

geomacy Jun 4, 2018

Contributor

same as above

This comment has been minimized.

Copy link
@ahgittin

ahgittin Jun 12, 2018

Author Contributor

done

entity.getDisplayName(),
entity.getEntityType().getName(),
entity.getCatalogItemId(),
MutableMap.of("self", EntityTransformer.entityUri(entity, ui.getBaseUriBuilder())) );

This comment has been minimized.

Copy link
@geomacy

geomacy Jun 4, 2018

Contributor

An observation - not something necessary but I'd suggest let's add the self link into the entity in all cases; this would be more consistent and would mean you could always just use the link if you wanted it, rather than have to check whether it is there and calculate it if not. It would just mean an extra parameter in the entity detail constructor at 161 below.

This comment has been minimized.

Copy link
@ahgittin

ahgittin Jun 12, 2018

Author Contributor

good call, agree

@geomacy

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2018

Might be worth adding some unit tests for this

@ahgittin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2018

Thanks @aledsage @geomacy . Great comments.

I'll add tests and push all changes addressed except depth.

On maxDepth, it's a not uncommon pattern that <0 is infinite, and 0 could to me logically mean either no detail at level 0, or yes detail at level 0 but 0 additional levels. I've gone with the former semantics as strictly more useful (in terms of functionality). The fact that the default is 1 and this is explained I think is sufficient.

Agree both are a little subtle but there is not an elegant way I can see to make it clearer; it involves additional fields with some redundancy.

If you feel strongly happy to go with an alternate API that preserves functionality -- please suggest -- however this as is my preferred.

address code review comments -
- entitlements check
- test for details endpoint including depth
- better comments on depth
- include 'self' link for entity details
- fix bug where config wasn't returned
@ahgittin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2018

tests added and all comments addressed except for depth as noted (which now has improved docs/comments)

@geomacy
Copy link
Contributor

left a comment

Looks good to me will leave it to @aledsage to re-review

@asfgit asfgit merged commit 475c439 into apache:master Jun 29, 2018

1 check passed

Jenkins: brooklyn-server-pull-request SUCCESS 5006 tests run, 0 skipped, 0 failed.
Details

asfgit pushed a commit that referenced this pull request Jun 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.