Running the Javascript rules

Jesus Rodriguez jesusr at redhat.com
Tue Mar 16 13:54:28 UTC 2010


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 at 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
> +     *            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
> 

-- 
jesus m. rodriguez          | jesusr at redhat.com
principal software engineer | irc: zeus
red hat systems management  | 919.754.4413 (w)
rhce # 805008586930012      | 919.623.0080 (c)
+---------------------------------------------+
|   "Those who cannot remember the past       |
|    are condemned to repeat it."             |
|                        -- George Santayana  |
+---------------------------------------------+




More information about the candlepin mailing list