You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2016/08/15 11:57:57 UTC
[GitHub] brooklyn-server pull request #301: Test+fix DeferredSuppliers in $brooklyn:o...
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/301
Test+fix DeferredSuppliers in $brooklyn:object
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server test-and-fix-DeferredSuppliers-in-brooklynObject
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/301.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #301
----
commit c9647d717bec84b9ad8a5027fa8f50127b130e19
Author: Aled Sage <al...@gmail.com>
Date: 2016-08-15T11:57:07Z
Test+fix DeferredSuppliers in $brooklyn:object
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #301: Test+fix DeferredSuppliers in $brooklyn:o...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/301#discussion_r74754674
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ObjectsYamlTest.java ---
@@ -84,6 +94,17 @@ public void setManagementContext(ManagementContext managementContext) {
}
}
+ // TODO Copy of org.apache.brooklyn.rest.security.PasswordHasher; but this module does not
+ // have access to brooklyn-rest-resources (and would create a circular reference to do so)
+ public static class PasswordHasher {
+ public static String sha256(String salt, String password) {
+ if (salt == null) salt = "";
+ byte[] bytes = (salt + password).getBytes(Charsets.UTF_8);
+ HashCode hash = Hashing.sha256().hashBytes(bytes);
+ return hash.toString();
+ }
+ }
--- End diff --
Nice use-case, btw!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #301: Test+fix DeferredSuppliers in $brooklyn:o...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/301#discussion_r74899592
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/BrooklynDslCommon.java ---
@@ -428,28 +430,44 @@ public DslObject(
return DependentConfiguration.transformMultiple(flags, new Function<List<Object>, Object>() {
@Override
public Object apply(List<Object> input) {
- Iterator<Object> values = input.iterator();
+ Map<String, Object> resolvedFields = Maps.newLinkedHashMap();
+ Map<String, Object> resolvedConfig = Maps.newLinkedHashMap();
+ List<Object> resolvedConstructorArgs = Lists.newArrayList();
+ List<Object> resolvedFactoryMethodArgs = Lists.newArrayList();
+
+ Iterator<Object> taskValues = input.iterator();
for (String name : fields.keySet()) {
Object value = fields.get(name);
- if (value instanceof TaskAdaptable || value instanceof TaskFactory) {
- fields.put(name, values.next());
- } else if (value instanceof DeferredSupplier) {
- fields.put(name, ((DeferredSupplier<?>) value).get());
- }
+ Object resolvedValue = requiresTask(value) ? taskValues.next() : resolveValue(value);
+ resolvedFields.put(name, resolvedValue);
}
for (String name : config.keySet()) {
Object value = config.get(name);
- if (value instanceof TaskAdaptable || value instanceof TaskFactory) {
- config.put(name, values.next());
- } else if (value instanceof DeferredSupplier) {
- config.put(name, ((DeferredSupplier<?>) value).get());
- }
+ Object resolvedValue = requiresTask(value) ? taskValues.next() : resolveValue(value);
+ resolvedConfig.put(name, resolvedValue);
+ }
+ for (Object value : constructorArgs) {
+ Object resolvedValue = requiresTask(value) ? taskValues.next() : resolveValue(value);
+ resolvedConstructorArgs.add(resolvedValue);
+ }
+ for (Object value : factoryMethodArgs) {
+ Object resolvedValue = requiresTask(value) ? taskValues.next() : resolveValue(value);
+ resolvedFactoryMethodArgs.add(resolvedValue);
}
if (factoryMethodName == null) {
- return create(clazz, constructorArgs, fields, config);
+ return create(clazz, resolvedConstructorArgs, resolvedFields, resolvedConfig);
} else {
- return create(clazz, factoryMethodName, factoryMethodArgs, fields, config);
+ return create(clazz, factoryMethodName, resolvedFactoryMethodArgs, resolvedFields, resolvedConfig);
+ }
+ }
+ protected boolean requiresTask(Object value) {
+ return (value instanceof TaskAdaptable || value instanceof TaskFactory);
+ }
+ protected Object resolveValue(Object value) {
+ if (value instanceof DeferredSupplier) {
+ return ((DeferredSupplier<?>) value).get();
}
+ return value;
}
--- End diff --
yes, nice :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #301: Test+fix DeferredSuppliers in $brooklyn:o...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/301#discussion_r74905915
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/BrooklynDslCommon.java ---
@@ -47,6 +47,7 @@
import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal;
import org.apache.brooklyn.core.mgmt.persist.DeserializingClassRenamesProvider;
import org.apache.brooklyn.core.sensor.DependentConfiguration;
+import org.apache.brooklyn.util.collections.MutableList;
--- End diff --
Ha! No longer spurious!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #301: Test+fix DeferredSuppliers in $brooklyn:object
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the issue:
https://github.com/apache/brooklyn-server/pull/301
@aledsage oh, and it was written that way because that was the idiom in use at the time - `Tasks.resolveDeepValue` didn't exist I believe, but I would have used that as well if refactoring
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #301: Test+fix DeferredSuppliers in $brooklyn:o...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/301#discussion_r74754958
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/BrooklynDslCommon.java ---
@@ -428,28 +430,44 @@ public DslObject(
return DependentConfiguration.transformMultiple(flags, new Function<List<Object>, Object>() {
@Override
public Object apply(List<Object> input) {
- Iterator<Object> values = input.iterator();
+ Map<String, Object> resolvedFields = Maps.newLinkedHashMap();
+ Map<String, Object> resolvedConfig = Maps.newLinkedHashMap();
+ List<Object> resolvedConstructorArgs = Lists.newArrayList();
+ List<Object> resolvedFactoryMethodArgs = Lists.newArrayList();
+
+ Iterator<Object> taskValues = input.iterator();
for (String name : fields.keySet()) {
Object value = fields.get(name);
- if (value instanceof TaskAdaptable || value instanceof TaskFactory) {
- fields.put(name, values.next());
- } else if (value instanceof DeferredSupplier) {
- fields.put(name, ((DeferredSupplier<?>) value).get());
- }
+ Object resolvedValue = requiresTask(value) ? taskValues.next() : resolveValue(value);
+ resolvedFields.put(name, resolvedValue);
}
for (String name : config.keySet()) {
Object value = config.get(name);
- if (value instanceof TaskAdaptable || value instanceof TaskFactory) {
- config.put(name, values.next());
- } else if (value instanceof DeferredSupplier) {
- config.put(name, ((DeferredSupplier<?>) value).get());
- }
+ Object resolvedValue = requiresTask(value) ? taskValues.next() : resolveValue(value);
+ resolvedConfig.put(name, resolvedValue);
+ }
+ for (Object value : constructorArgs) {
+ Object resolvedValue = requiresTask(value) ? taskValues.next() : resolveValue(value);
+ resolvedConstructorArgs.add(resolvedValue);
+ }
+ for (Object value : factoryMethodArgs) {
+ Object resolvedValue = requiresTask(value) ? taskValues.next() : resolveValue(value);
+ resolvedFactoryMethodArgs.add(resolvedValue);
}
if (factoryMethodName == null) {
- return create(clazz, constructorArgs, fields, config);
+ return create(clazz, resolvedConstructorArgs, resolvedFields, resolvedConfig);
} else {
- return create(clazz, factoryMethodName, factoryMethodArgs, fields, config);
+ return create(clazz, factoryMethodName, resolvedFactoryMethodArgs, resolvedFields, resolvedConfig);
+ }
+ }
+ protected boolean requiresTask(Object value) {
+ return (value instanceof TaskAdaptable || value instanceof TaskFactory);
+ }
+ protected Object resolveValue(Object value) {
+ if (value instanceof DeferredSupplier) {
+ return ((DeferredSupplier<?>) value).get();
}
+ return value;
}
--- End diff --
Whoops, `fields` should come before `config`!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #301: Test+fix DeferredSuppliers in $brooklyn:o...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/301
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #301: Test+fix DeferredSuppliers in $brooklyn:object
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the issue:
https://github.com/apache/brooklyn-server/pull/301
Love it, epsecially the thorough test coverage. Just nitpicks from me really; merge away.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #301: Test+fix DeferredSuppliers in $brooklyn:o...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/301#discussion_r74754653
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ObjectsYamlTest.java ---
@@ -84,6 +94,17 @@ public void setManagementContext(ManagementContext managementContext) {
}
}
+ // TODO Copy of org.apache.brooklyn.rest.security.PasswordHasher; but this module does not
+ // have access to brooklyn-rest-resources (and would create a circular reference to do so)
+ public static class PasswordHasher {
+ public static String sha256(String salt, String password) {
+ if (salt == null) salt = "";
+ byte[] bytes = (salt + password).getBytes(Charsets.UTF_8);
+ HashCode hash = Hashing.sha256().hashBytes(bytes);
+ return hash.toString();
+ }
+ }
--- End diff --
Perfect candiate for `brooklyn-utils-common` ;o)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #301: Test+fix DeferredSuppliers in $brooklyn:object
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the issue:
https://github.com/apache/brooklyn-server/pull/301
Lovely jubbly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #301: Test+fix DeferredSuppliers in $brooklyn:o...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/301#discussion_r74754498
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/BrooklynDslCommon.java ---
@@ -428,28 +430,44 @@ public DslObject(
return DependentConfiguration.transformMultiple(flags, new Function<List<Object>, Object>() {
@Override
public Object apply(List<Object> input) {
- Iterator<Object> values = input.iterator();
+ Map<String, Object> resolvedFields = Maps.newLinkedHashMap();
+ Map<String, Object> resolvedConfig = Maps.newLinkedHashMap();
+ List<Object> resolvedConstructorArgs = Lists.newArrayList();
+ List<Object> resolvedFactoryMethodArgs = Lists.newArrayList();
+
+ Iterator<Object> taskValues = input.iterator();
for (String name : fields.keySet()) {
Object value = fields.get(name);
- if (value instanceof TaskAdaptable || value instanceof TaskFactory) {
- fields.put(name, values.next());
- } else if (value instanceof DeferredSupplier) {
- fields.put(name, ((DeferredSupplier<?>) value).get());
- }
+ Object resolvedValue = requiresTask(value) ? taskValues.next() : resolveValue(value);
+ resolvedFields.put(name, resolvedValue);
}
for (String name : config.keySet()) {
Object value = config.get(name);
- if (value instanceof TaskAdaptable || value instanceof TaskFactory) {
- config.put(name, values.next());
- } else if (value instanceof DeferredSupplier) {
- config.put(name, ((DeferredSupplier<?>) value).get());
- }
+ Object resolvedValue = requiresTask(value) ? taskValues.next() : resolveValue(value);
+ resolvedConfig.put(name, resolvedValue);
+ }
+ for (Object value : constructorArgs) {
+ Object resolvedValue = requiresTask(value) ? taskValues.next() : resolveValue(value);
+ resolvedConstructorArgs.add(resolvedValue);
+ }
+ for (Object value : factoryMethodArgs) {
+ Object resolvedValue = requiresTask(value) ? taskValues.next() : resolveValue(value);
+ resolvedFactoryMethodArgs.add(resolvedValue);
}
if (factoryMethodName == null) {
- return create(clazz, constructorArgs, fields, config);
+ return create(clazz, resolvedConstructorArgs, resolvedFields, resolvedConfig);
} else {
- return create(clazz, factoryMethodName, factoryMethodArgs, fields, config);
+ return create(clazz, factoryMethodName, resolvedFactoryMethodArgs, resolvedFields, resolvedConfig);
+ }
+ }
+ protected boolean requiresTask(Object value) {
+ return (value instanceof TaskAdaptable || value instanceof TaskFactory);
+ }
+ protected Object resolveValue(Object value) {
+ if (value instanceof DeferredSupplier) {
+ return ((DeferredSupplier<?>) value).get();
}
+ return value;
}
--- End diff --
Good catch. Lots of repetition here, however. Maybe:
```
final Iterator<Object> taskValues = input.iterator();
Function<Object, Object> resolver = new Function<Object, Object>() {
@Override public Object apply(Object value) {
Object resolvedValue = requiresTask(value) ? taskValues.next() : resolveValue(value);
return resolvedValue;
}
};
Map<String, Object> resolvedConfig = Maps.transformValues(config, resolver);
Map<String, Object> resolvedFields = Maps.transformValues(fields, resolver);
List<Object> resolvedConstructorArgs = Lists.transform(constructorArgs, resolver);
List<Object> resolvedFactoryMethodArgs = Lists.transform(factoryMethodArgs, resolver);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #301: Test+fix DeferredSuppliers in $brooklyn:object
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/301
Thanks @alasdairhodge - I'm incorporated those comments and also made a couple more tidy-ups (see commits that change it to use the `TypeResolver` utility, and that removes the strange `TypeCoercion` that I commented on previously).
Assuming you're happy with that, I'll merge once jenkins build completes.
@grkvlt I'd appreciate you taking a look as well please to let me know if I've missed something about why the code was written that way originally.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #301: Test+fix DeferredSuppliers in $brooklyn:o...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/301#discussion_r74752054
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/BrooklynDslCommon.java ---
@@ -47,6 +47,7 @@
import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal;
import org.apache.brooklyn.core.mgmt.persist.DeserializingClassRenamesProvider;
import org.apache.brooklyn.core.sensor.DependentConfiguration;
+import org.apache.brooklyn.util.collections.MutableList;
--- End diff --
Spurious.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #301: Test+fix DeferredSuppliers in $brooklyn:object
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the issue:
https://github.com/apache/brooklyn-server/pull/301
@aledsage this looks OK to me, as well. going to merge
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---