I added a new Constructor to JavascriptEnforcer:
public JavascriptEnforcer(DateSource dateSource, Reader rulesReader, PreEntHelper preHelper, PostEntHelper postHelper, ProductServiceAdapter prodAdapter, ScriptEngine jsEngine)
I also left the old one, but changed its implementation to call the new one: public JavascriptEnforcer(DateSource dateSource, RulesCurator rulesCurator, PreEntHelper preHelper, PostEntHelper postHelper, ProductServiceAdapter prodAdapter) {
this(dateSource, new StringReader(rulesCurator.getRules().getRules()), preHelper, postHelper, prodAdapter, new ScriptEngineManager() .getEngineByName("JavaScript")); }
However, this type of construction code is really no appropriate a class like this. Jesus mentioned that he feels the same way about ProductServiceAdapter, which I will look at in a little bit.
With those changes, I can make a stand alone unit test that runs without any Hibernate support, at least for calling public Pool selectBestPool(Consumer consumer, String productId, List<Pool> pools);
The next piece of trickiness is loading in the Javascript file. For static deployments, the usual way to find a text file like this is:
URL url = this.getClass().getClassLoader().getResource("rules/default-rules.js"); InputStreamReader inputStreamReader = new InputStreamReader(url.openStream());
Which works fine so long as the resource is on the classpath. Thus for the Junit test, I added a file CLASSPATH extension which was "target/test/resources". Since it looks like we are regenerating the .project file from git sources, this might not live across check-ins.
I'm thinkin that if our goal is to make the Javascript something that the end use is responsible for editing and deploying, we need to provide the user with a tool to go through the edit-deploy-debug cycle. Are we planning on doing that as part of the web app? Are we going to allow the user to deploy multiple .js rules files, and indicate which is the "live" one?
Both patch and html diff are are attached.
This looks good but are you using something to auto-format the code, and if so is it possible to turn it off? This code has changed and you'll likely get some conflicts, I don't think they'll be too bad but they'll definitely be worse than if the unnecessary formatting wasn't changed, similarly for anyone who gets conflicts resulting from these changes.
As for the patch itself great change, much easier to test different rules files now.
Also as Jesus pointed out yesterday the ProductServiceAdapter could probably go too, looking at how it's used it would likely require a change to the methods on the interface but nothing major.
Cheers,
Devan
On Mon, Mar 15, 2010 at 8:24 PM, Adam Young ayoung@redhat.com wrote:
I added a new Constructor to JavascriptEnforcer:
public JavascriptEnforcer(DateSource dateSource, Reader rulesReader, PreEntHelper preHelper, PostEntHelper postHelper, ProductServiceAdapter prodAdapter, ScriptEngine jsEngine)
I also left the old one, but changed its implementation to call the new one: public JavascriptEnforcer(DateSource dateSource, RulesCurator rulesCurator, PreEntHelper preHelper, PostEntHelper postHelper, ProductServiceAdapter prodAdapter) {
this(dateSource, new StringReader(rulesCurator.getRules().getRules()), preHelper, postHelper, prodAdapter, new ScriptEngineManager() .getEngineByName("JavaScript")); }
However, this type of construction code is really no appropriate a class like this. Jesus mentioned that he feels the same way about ProductServiceAdapter, which I will look at in a little bit.
With those changes, I can make a stand alone unit test that runs without any Hibernate support, at least for calling public Pool selectBestPool(Consumer consumer, String productId, List<Pool> pools);
The next piece of trickiness is loading in the Javascript file. For static deployments, the usual way to find a text file like this is:
URL url = this.getClass().getClassLoader().getResource("rules/default-rules.js"); InputStreamReader inputStreamReader = new InputStreamReader(url.openStream());
Which works fine so long as the resource is on the classpath. Thus for the Junit test, I added a file CLASSPATH extension which was "target/test/resources". Since it looks like we are regenerating the .project file from git sources, this might not live across check-ins.
I'm thinkin that if our goal is to make the Javascript something that the end use is responsible for editing and deploying, we need to provide the user with a tool to go through the edit-deploy-debug cycle. Are we planning on doing that as part of the web app? Are we going to allow the user to deploy multiple .js rules files, and indicate which is the "live" one?
Both patch and html diff are are attached.
candlepin mailing list candlepin@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/candlepin
On Mon, Mar 15, 2010 at 07:24:37PM -0400, Adam Young wrote:
I added a new Constructor to JavascriptEnforcer:
public JavascriptEnforcer(DateSource dateSource, Reader rulesReader, PreEntHelper preHelper, PostEntHelper postHelper, ProductServiceAdapter prodAdapter, ScriptEngine jsEngine)
I like it with the Reader. +1
I also left the old one, but changed its implementation to call the new one: public JavascriptEnforcer(DateSource dateSource, RulesCurator rulesCurator, PreEntHelper preHelper, PostEntHelper postHelper, ProductServiceAdapter prodAdapter) {
this(dateSource, new
StringReader(rulesCurator.getRules().getRules()), preHelper, postHelper, prodAdapter, new ScriptEngineManager() .getEngineByName("JavaScript")); }
Why can't we just whack this ctor? Is it still needed?
However, this type of construction code is really no appropriate a class like this. Jesus mentioned that he feels the same way about ProductServiceAdapter, which I will look at in a little bit.
+1
With those changes, I can make a stand alone unit test that runs without any Hibernate support, at least for calling public Pool selectBestPool(Consumer consumer, String productId, List<Pool> pools);
Very nice.
The next piece of trickiness is loading in the Javascript file. For static deployments, the usual way to find a text file like this is:
URL url =
this.getClass().getClassLoader().getResource("rules/default-rules.js"); InputStreamReader inputStreamReader = new InputStreamReader(url.openStream());
Which works fine so long as the resource is on the classpath. Thus for the Junit test, I added a file CLASSPATH extension which was "target/test/resources". Since it looks like we are regenerating the .project file from git sources, this might not live across check-ins.
IIRC I removed .project from the checkin altogether, it is now generated and no longer committed.
I'm thinkin that if our goal is to make the Javascript something that the end use is responsible for editing and deploying, we need to provide the user with a tool to go through the edit-deploy-debug cycle. Are we planning on doing that as part of the web app? Are we going to allow the user to deploy multiple .js rules files, and indicate which is the "live" one?
Not 100% sure we've decided the .js files would be user editable. I guess from an OSS project consumer they'd want to create their own rules and deploy them, so those folks will need this level of support.
Both patch and html diff are are attached.
[snip remove html diff] comments inline
From 510de733bd92039cff551140b7b1060b828f58de Mon Sep 17 00:00:00 2001
From: Adam Young ayoung@redhat.com Date: Mon, 15 Mar 2010 19:23:16 -0400 Subject: [PATCH] Added unit test for default rules, and modified classes the unit test to run outside of the container.
.../candlepin/controller/Entitler.java | 58 ++++++++------ .../candlepin/policy/js/JavascriptEnforcer.java | 84 +++++++++++-------- proxy/src/main/resources/rules/default-rules.js | 34 ++++++-- .../candlepin/policy/test/DefaultRulesTest.java | 83 +++++++++++++++++++ 4 files changed, 192 insertions(+), 67 deletions(-) create mode 100644 proxy/src/test/java/org/fedoraproject/candlepin/policy/test/DefaultRulesTest.java
diff --git a/proxy/src/main/java/org/fedoraproject/candlepin/controller/Entitler.java b/proxy/src/main/java/org/fedoraproject/candlepin/controller/Entitler.java index 5f5a060..19264b3 100644 --- a/proxy/src/main/java/org/fedoraproject/candlepin/controller/Entitler.java +++ b/proxy/src/main/java/org/fedoraproject/candlepin/controller/Entitler.java @@ -37,17 +37,17 @@ import java.util.Date;
- Entitler
*/ public class Entitler {
This one looked ok to me. [body snipped]
diff --git a/proxy/src/main/java/org/fedoraproject/candlepin/policy/js/JavascriptEnforcer.java b/proxy/src/main/java/org/fedoraproject/candlepin/policy/js/JavascriptEnforcer.java index a00e0a3..123ee74 100644 --- a/proxy/src/main/java/org/fedoraproject/candlepin/policy/js/JavascriptEnforcer.java +++ b/proxy/src/main/java/org/fedoraproject/candlepin/policy/js/JavascriptEnforcer.java @@ -14,7 +14,6 @@ */ package org.fedoraproject.candlepin.policy.js;
import org.fedoraproject.candlepin.model.Consumer; import org.fedoraproject.candlepin.model.Entitlement; import org.fedoraproject.candlepin.model.Pool; @@ -45,7 +44,7 @@ public class JavascriptEnforcer implements Enforcer {
private static Logger log = Logger.getLogger(JavascriptEnforcer.class); private DateSource dateSource;
- private RulesCurator rulesCurator;
- private ProductServiceAdapter prodAdapter; private PreEntHelper preHelper; private PostEntHelper postHelper;
@@ -55,29 +54,36 @@ public class JavascriptEnforcer implements Enforcer { private static final String PRE_PREFIX = "pre_"; private static final String POST_PREFIX = "post_"; private static final String SELECT_POOL_PREFIX = "select_pool_";
- private static final String GLOBAL_SELECT_POOL_FUNCTION = SELECT_POOL_PREFIX + "global";
private static final String GLOBAL_SELECT_POOL_FUNCTION = SELECT_POOL_PREFIX +
"global";
private static final String GLOBAL_PRE_FUNCTION = PRE_PREFIX + "global"; private static final String GLOBAL_POST_FUNCTION = POST_PREFIX + "global";
@Inject
- public JavascriptEnforcer(DateSource dateSource,
RulesCurator rulesCurator, PreEntHelper preHelper,
PostEntHelper postHelper, ProductServiceAdapter prodAdapter) {
- public JavascriptEnforcer(DateSource dateSource, RulesCurator rulesCurator,
PreEntHelper preHelper, PostEntHelper postHelper,
ProductServiceAdapter prodAdapter) {
this(dateSource, new StringReader(rulesCurator.getRules().getRules()),
preHelper, postHelper, prodAdapter, new ScriptEngineManager()
.getEngineByName("JavaScript"));
- }
I'd get rid of the above ctor altogether.
- @Inject
- public JavascriptEnforcer(DateSource dateSource, Reader rulesReader,
PreEntHelper preHelper, PostEntHelper postHelper,
ProductServiceAdapter prodAdapter, ScriptEngine jsEngine) { this.dateSource = dateSource;
this.rulesCurator = rulesCurator; this.preHelper = preHelper; this.postHelper = postHelper; this.prodAdapter = prodAdapter;
ScriptEngineManager mgr = new ScriptEngineManager();
jsEngine = mgr.getEngineByName("JavaScript");
this.jsEngine = jsEngine;
Why do we pass in the ScriptEngine? Do we intend to have the JavascriptEnforcer use a different engine aside from JavaScript? Seems odd to pass in say a Jython ScriptEngine.
I guess for "testing" you could supply a mock ScriptEngine, but this is where I find coding for testing to be bad, because we are now stating that the JavascriptEnforcer can use ANY ScriptEngine which I don't think is true.
if (jsEngine == null) {
throw new RuntimeException("No Javascript engine found");
throw new RuntimeException("No Javascript engine"); } try {
Reader reader = new StringReader(this.rulesCurator.getRules().getRules());
jsEngine.eval(reader);
this.jsEngine.eval(rulesReader); } catch (ScriptException ex) { throw new RuleParseException(ex);
@@ -90,23 +96,24 @@ public class JavascriptEnforcer implements Enforcer { runPre(preHelper, consumer, entitlementPool);
if (entitlementPool.isExpired(dateSource)) {
preHelper.getResult().addError(new ValidationError("Entitlements for " +
entitlementPool.getProductId() +
" expired on: " + entitlementPool.getEndDate()));
preHelper.getResult().addError(
new ValidationError("Entitlements for " +
entitlementPool.getProductId() + " expired on: " +
entitlementPool.getEndDate())); return preHelper; } return preHelper;
}
- private void runPre(PreEntHelper preHelper, Consumer consumer,
Pool pool) {
private void runPre(PreEntHelper preHelper, Consumer consumer, Pool pool) { Invocable inv = (Invocable) jsEngine; String productId = pool.getProductId();
// Provide objects for the script: jsEngine.put("consumer", new ReadOnlyConsumer(consumer));
jsEngine.put("product", new ReadOnlyProduct(prodAdapter.getProductById(productId)));
jsEngine.put("product", new ReadOnlyProduct(prodAdapter
.getProductById(productId))); jsEngine.put("pool", new ReadOnlyEntitlementPool(pool)); jsEngine.put("pre", preHelper);
@@ -151,12 +158,13 @@ public class JavascriptEnforcer implements Enforcer {
// Provide objects for the script: jsEngine.put("consumer", new ReadOnlyConsumer(c));
jsEngine.put("product", new ReadOnlyProduct(prodAdapter.getProductById(productId)));
jsEngine.put("product", new ReadOnlyProduct(prodAdapter
.getProductById(productId))); jsEngine.put("post", postHelper); jsEngine.put("entitlement", new ReadOnlyEntitlement(ent));
log.debug("Running post-entitlement rules for: " + c.getUuid() + " product: " +
pool.getProductId());
log.debug("Running post-entitlement rules for: " + c.getUuid() +
" product: " + pool.getProductId()); try { inv.invokeFunction(POST_PREFIX + productId);
@@ -182,13 +190,13 @@ public class JavascriptEnforcer implements Enforcer { } }
- public Pool selectBestPool(Consumer consumer, String productId, List<Pool> pools) {
public Pool selectBestPool(Consumer consumer, String productId,
List<Pool> pools) { Invocable inv = (Invocable) jsEngine; log.info("Selecting best entitlement pool for product: " + productId);
List<ReadOnlyEntitlementPool> readOnlyPools =
new LinkedList<ReadOnlyEntitlementPool>();
List<ReadOnlyEntitlementPool> readOnlyPools = new LinkedList<ReadOnlyEntitlementPool>(); for (Pool p : pools) { log.info(" " + p); readOnlyPools.add(new ReadOnlyEntitlementPool(p));
@@ -199,20 +207,23 @@ public class JavascriptEnforcer implements Enforcer {
ReadOnlyEntitlementPool result = null; try {
result = (ReadOnlyEntitlementPool) inv.invokeFunction(
SELECT_POOL_PREFIX + productId);
log.info("Excuted javascript rule: " + SELECT_POOL_PREFIX + productId);
result = (ReadOnlyEntitlementPool) inv
.invokeFunction(SELECT_POOL_PREFIX + productId);
log.info("Excuted javascript rule: " + SELECT_POOL_PREFIX +
productId); } catch (NoSuchMethodException e) { // No method for this product, try to find a global function, if // neither exists this is ok and we'll just carry on. try {
result = (ReadOnlyEntitlementPool) inv.invokeFunction(
result = (ReadOnlyEntitlementPool) inv
.invokeFunction(GLOBAL_SELECT_POOL_FUNCTION);
log.info("Excuted javascript rule: " + GLOBAL_SELECT_POOL_FUNCTION);
log.info("Excuted javascript rule: " + GLOBAL_SELECT_POOL_FUNCTION); } catch (NoSuchMethodException ex) {
log.warn("No default rule found: " + GLOBAL_SELECT_POOL_FUNCTION);
log.warn("No default rule found: " +
GLOBAL_SELECT_POOL_FUNCTION); log.warn("Resorting to default pool selection behavior."); return selectBestPoolDefault(pools); }
@@ -225,8 +236,8 @@ public class JavascriptEnforcer implements Enforcer { }
if (pools.size() > 0 && result == null) {
throw new RuleExecutionException("Rule did not select a pool for product: " +
productId);
throw new RuleExecutionException(
"Rule did not select a pool for product: " + productId); } for (Pool p : pools) {
@@ -240,8 +251,11 @@ public class JavascriptEnforcer implements Enforcer { }
/**
* Default behavior if no product specific and no global pool select rules exist.
* @param pools Pools to choose from.
* Default behavior if no product specific and no global pool select rules
* exist.
*
* @param pools
private Pool selectBestPoolDefault(List<Pool> pools) {* Pools to choose from. * @return First pool in the list. (default behavior) */
diff --git a/proxy/src/main/resources/rules/default-rules.js b/proxy/src/main/resources/rules/default-rules.js index 71de8f2..7caf73b 100644 --- a/proxy/src/main/resources/rules/default-rules.js +++ b/proxy/src/main/resources/rules/default-rules.js @@ -12,7 +12,8 @@ function virtualization_common() { return; }
- // Host must not have any guests currently (could be changed but for simplicities sake):
- // Host must not have any guests currently (could be changed but for
- // simplicities sake): if (parseInt(consumer.getFact("total_guests")) > 0) { pre.addError("rulefailed.host.already.has.guests"); }
@@ -36,16 +37,16 @@ function post_virtualization_host_platform() {
function pre_global() {
- // Support free entitlements for guests, if their parent has virt host or platform,
- // Support free entitlements for guests, if their parent has virt host or
- // platform, // and is entitled to the product the guest is requesting: if (consumer.getType() == "virt_system" && consumer.getParent() != null) {
if ((consumer.getParent().hasEntitlement("virtualization_host") ||
consumer.getParent().hasEntitlement("virtualization_host_platform")) &&
consumer.getParent().hasEntitlement(product.getLabel())) {
if ((consumer.getParent().hasEntitlement("virtualization_host") || consumer
.getParent().hasEntitlement("virtualization_host_platform"))
}&& consumer.getParent().hasEntitlement(product.getLabel())) { pre.grantFreeEntitlement();
- }
- else {
- } else { pre.checkQuantity(pool); }
} @@ -53,3 +54,22 @@ function pre_global() { function post_global() {
}
+function select_pool_global() {
- return pools.getFirst();
+}
+function select_pool_monitoring() {
- var pool;
- var poolItr;
- for (poolItr = pools.iterator(); poolItr.hasNext();) {
pool = poolItr.next();
if (pool.productId == "monitoring") {
return pool;
}
- }
- return pools.getFirst();
+} diff --git a/proxy/src/test/java/org/fedoraproject/candlepin/policy/test/DefaultRulesTest.java b/proxy/src/test/java/org/fedoraproject/candlepin/policy/test/DefaultRulesTest.java new file mode 100644 index 0000000..1af7eb8 --- /dev/null +++ b/proxy/src/test/java/org/fedoraproject/candlepin/policy/test/DefaultRulesTest.java @@ -0,0 +1,83 @@ +/**
- Copyright (c) 2009 Red Hat, Inc.
- This software is licensed to you under the GNU General Public License,
- version 2 (GPLv2). There is NO WARRANTY for this software, express or
- implied, including the implied warranties of MERCHANTABILITY or FITNESS
- FOR A PARTICULAR PURPOSE. You should have received a copy of GPLv2
- along with this software; if not, see
- Red Hat trademarks are not licensed under GPLv2. No permission is
- granted to use or replicate Red Hat trademarks that are incorporated
- in this software or its documentation.
- */
+package org.fedoraproject.candlepin.policy.test;
+import java.io.File; +import java.io.FileNotFoundException; +import java.io.FileReader; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.Reader; +import java.net.URL; +import java.util.ArrayList; +import java.util.Date; +import java.util.List;
+import javax.script.ScriptEngineManager;
+import junit.framework.Assert;
+import org.fedoraproject.candlepin.model.Consumer; +import org.fedoraproject.candlepin.model.Pool; +import org.fedoraproject.candlepin.model.RulesCurator; +import org.fedoraproject.candlepin.policy.Enforcer; +import org.fedoraproject.candlepin.policy.js.JavascriptEnforcer; +import org.fedoraproject.candlepin.policy.js.PostEntHelper; +import org.fedoraproject.candlepin.policy.js.PreEntHelper; +import org.fedoraproject.candlepin.service.ProductServiceAdapter; +import org.fedoraproject.candlepin.util.DateSource; +import org.junit.Before; +import org.junit.Test;
+import com.sun.jersey.spi.StringReader;
why are we using this StringReader?
+/**
- DefaultRulesTest
- */
+public class DefaultRulesTest {
- private Enforcer enforcer;
- private DateSource dateSource;
- private PreEntHelper preHelper;
- private PostEntHelper postHelper;
- private ProductServiceAdapter prodAdapter;
- @Before
- public void createEnforcer() throws IOException {
// InputStream inStream = this
URL url = this.getClass().getClassLoader().getResource("rules/default-rules.js");
InputStreamReader inputStreamReader = new InputStreamReader(url
.openStream());
enforcer = new JavascriptEnforcer(dateSource, inputStreamReader,
preHelper, postHelper, prodAdapter, new ScriptEngineManager()
.getEngineByName("JavaScript"));
- }
- @Test
- public void runDefaultRuels() {
Consumer consumer = new Consumer();
String productId = "Shampoo";
Pool pool = new Pool();
pool.setId(new Long(0));
pool.setProductId("default");
List<Pool> pools = new ArrayList<Pool>();
pools.add(pool);
Pool selected = enforcer.selectBestPool(consumer, productId, pools);
Assert.assertNotNull(selected);
- }
+}
1.6.6.1
candlepin@lists.fedorahosted.org