Re: Running the Javascript rules

[jesus rodriguez]

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 
> 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
> *            Pools to choose from.
> * @return First pool in the list. (default behavior)
> */
> private Pool selectBestPoolDefault(List<Pool> pools) {
>
> 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
> http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
> 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