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

Introduced a plugin-API and show usage with rhino-xml #1699

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions rhino-xml/src/main/java/module-info.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,7 @@

requires transitive org.mozilla.rhino;
requires transitive java.xml;

provides org.mozilla.javascript.Plugin with
org.mozilla.javascript.xmlimpl.XmlPlugin;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package org.mozilla.javascript.xmlimpl;

import org.mozilla.javascript.CompilerEnvirons;
import org.mozilla.javascript.Context;
import org.mozilla.javascript.LazilyLoadedCtor;
import org.mozilla.javascript.Plugin;
import org.mozilla.javascript.ScriptableObject;

/**
* Registers the XML objects in the scope.
*
* @author Roland Praml, Foconis Analytics GmbH
*/
public class XmlPlugin implements Plugin {

@Override
public String getName() {
return "xml";
}

@Override
public void initSafeStandardObjects(Context cx, ScriptableObject scope, boolean sealed) {
if (cx.hasFeature(Context.FEATURE_E4X)) {
String xmlImpl = XMLLibImpl.class.getName();
new LazilyLoadedCtor(scope, "XML", xmlImpl, sealed, true);
new LazilyLoadedCtor(scope, "XMLList", xmlImpl, sealed, true);
new LazilyLoadedCtor(scope, "Namespace", xmlImpl, sealed, true);
new LazilyLoadedCtor(scope, "QName", xmlImpl, sealed, true);
}
}

@Override
public void initCompilerEnvirons(Context cx, CompilerEnvirons compilerEnvirons) {
if (cx.hasFeature(Context.FEATURE_E4X)) {
compilerEnvirons.setXmlAvailable(true);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
org.mozilla.javascript.xmlimpl.XmlPlugin
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

package org.mozilla.javascript.tests;

import static org.junit.jupiter.api.Assertions.assertEquals;

import org.junit.Test;
import org.mozilla.javascript.Context;
import org.mozilla.javascript.ContextFactory;
import org.mozilla.javascript.Scriptable;

/**
* Test if XML is present. This test exists in "rhino-xml" where XML is present and in "rhino",
* where XML is not on classpath.
*
* @author Roland Praml
*/
public class XMLPresentTest {

@Test
public void testXMLPresent() {
try (Context cx = ContextFactory.getGlobal().enterContext()) {
Scriptable scope = cx.initStandardObjects();
Object result =
cx.evaluateString(
scope, "new XML('<a></a>').toXMLString();", "source", 1, null);
assertEquals("<a/>", Context.toString(result));
}
}
}
2 changes: 2 additions & 0 deletions rhino/src/main/java/module-info.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,6 @@
requires java.compiler;
requires jdk.dynalink;
requires transitive java.desktop;

uses org.mozilla.javascript.Plugin;
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ public void initFromContext(Context cx) {
allowMemberExprAsFunctionName = cx.hasFeature(Context.FEATURE_MEMBER_EXPR_AS_FUNCTION_NAME);
strictMode = cx.hasFeature(Context.FEATURE_STRICT_MODE);
warningAsError = cx.hasFeature(Context.FEATURE_WARNING_AS_ERROR);
xmlAvailable = cx.hasFeature(Context.FEATURE_E4X);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to remove all XML-Lib dependency to rhino-xml. There is still org.mozilla.javascript.xml.XMLLib and org.mozilla.javascript.xml.XMLObject in rhino base module. They are a bit difficult to remove


optimizationLevel = cx.getOptimizationLevel();

Expand All @@ -42,6 +41,7 @@ public void initFromContext(Context cx) {

// Observer code generation in compiled code :
generateObserverCount = cx.isGenerateObserverCount();
cx.getFactory().getPlugins().forEach(plugin -> plugin.initCompilerEnvirons(cx, this));
}

public final ErrorReporter getErrorReporter() {
Expand Down
17 changes: 2 additions & 15 deletions rhino/src/main/java/org/mozilla/javascript/Context.java
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,9 @@ public class Context implements Closeable {
* {@link #VERSION_DEFAULT} or is at least {@link #VERSION_1_6}.
*
* @since 1.6 Release 1
* @deprecated This flag may be removed later.
*/
public static final int FEATURE_E4X = 6;
@Deprecated public static final int FEATURE_E4X = 6;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is a fine change and we should do it, but I still don't think that we should deprecate this flag until it really does something. IMO, deprecating this flag is like saying that we're going to deprecate the E4X feature and I don't think that we're doing that.

Copy link
Contributor Author

@rPraml rPraml Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is still a misunderstanding here...

until it really does something

the flag currently does something, it controls, if E4X is enabled, when the plugin is present. So currently, you have to enable the flag (which is done by default for >= 1.6) and ensure, that the plugin is in classpath. I see these options:

add xml-plugin to rhino-all and keep the flag

If someone uses rhino-all as dependency, he will get the plugin automatically and the flag behaves like before. This solution provides maximum backward compatibility (and this is how it is implemented now, but it is annotated as deprecated)

remove the flag completely

As the flag is enabled by default (at least for languageVersion >= 1.6), we could move that check to the plugin. So if someone has decided to overrride FEATURE_E4X in his factory, this will be a breaking change, as code will no longer compile.

something between the 2 solutions

deprecate the flag - with a clear hint, that disabling E4X (that is the purpose that the flag currently has IMHO) with the flag will be removed in the future and should be controlled with the plugin mechanism.


/**
* Control if dynamic scope should be used for name access. If hasFeature(FEATURE_DYNAMIC_SCOPE)
Expand Down Expand Up @@ -2226,20 +2227,6 @@ public boolean hasFeature(int featureIndex) {
return f.hasFeature(this, featureIndex);
}

/**
* Returns an object which specifies an E4X implementation to use within this <code>Context
* </code>. Note that the XMLLib.Factory interface should be considered experimental.
*
* <p>The default implementation uses the implementation provided by this <code>Context</code>'s
* {@link ContextFactory}.
*
* @return An XMLLib.Factory. Should not return <code>null</code> if {@link #FEATURE_E4X} is
* enabled. See {@link #hasFeature}.
*/
public XMLLib.Factory getE4xImplementationFactory() {
return getFactory().getE4xImplementationFactory();
}

/**
* Get threshold of executed instructions counter that triggers call to <code>
* observeInstructionCount()</code>. When the threshold is zero, instruction counting is
Expand Down
74 changes: 38 additions & 36 deletions rhino/src/main/java/org/mozilla/javascript/ContextFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@

import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.ServiceLoader;

/**
* Factory class that Rhino runtime uses to create new {@link Context} instances. A <code>
Expand Down Expand Up @@ -114,6 +118,25 @@ public class ContextFactory {
private volatile Object listeners;
private boolean disabledListening;
private ClassLoader applicationClassLoader;
private final List<Plugin> plugins;

/** returns a list of plugins found by the ServiceLoader */
public static List<Plugin> getDefaultPluginsFromServiceLoader() {
List<Plugin> result = new ArrayList<Plugin>();
ServiceLoader.load(Plugin.class)
.forEach(
plugin -> {
String disabled =
SecurityUtilities.getSystemProperty(
"rhino.plugin." + plugin.getName() + ".disabled");
if ("1".equals(disabled) || "true".equals(disabled)) {
// the plugin is disabled
} else {
result.add(plugin);
}
});
return result;
}

/** Listener of {@link Context} creation and release events. */
public interface Listener {
Expand All @@ -127,6 +150,16 @@ public interface Listener {
public void contextReleased(Context cx);
}

/** Constructs a new ContextFactory with the plugins found by serviceLoader. */
public ContextFactory() {
this(getDefaultPluginsFromServiceLoader());
}

/** Constructs a new ContextFactory with a given list of plugins. */
public ContextFactory(List<Plugin> plugins) {
this.plugins = Collections.unmodifiableList(plugins);
}

/**
* Get global ContextFactory.
*
Expand Down Expand Up @@ -295,42 +328,6 @@ protected boolean hasFeature(Context cx, int featureIndex) {
throw new IllegalArgumentException(String.valueOf(featureIndex));
}

private static boolean isDom3Present() {
Class<?> nodeClass = Kit.classOrNull("org.w3c.dom.Node");
if (nodeClass == null) return false;
// Check to see whether DOM3 is present; use a new method defined in
// DOM3 that is vital to our implementation
try {
nodeClass.getMethod("getUserData", String.class);
return true;
} catch (NoSuchMethodException e) {
return false;
}
}

/**
* Provides a default {@link org.mozilla.javascript.xml.XMLLib.Factory XMLLib.Factory} to be
* used by the <code>Context</code> instances produced by this factory. See {@link
* Context#getE4xImplementationFactory} for details.
*
* <p>May return null, in which case E4X functionality is not supported in Rhino.
*
* <p>The default implementation now prefers the DOM3 E4X implementation.
*/
protected org.mozilla.javascript.xml.XMLLib.Factory getE4xImplementationFactory() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed both checks. If rhino-xml is on classpath, we should also have org.w3c.dom.Node.
Can re-add this check, if wanted.

Note: Kit.classOrNull can be very slow, if class is not found, as every check a ClassNotFoundException happens

// Must provide default implementation, rather than abstract method,
// so that past implementors of ContextFactory do not fail at runtime
// upon invocation of this method.
// Note that the default implementation returns null if we
// neither have XMLBeans nor a DOM3 implementation present.

if (isDom3Present()) {
return org.mozilla.javascript.xml.XMLLib.Factory.create(
"org.mozilla.javascript.xmlimpl.XMLLibImpl");
}
return null;
}

/**
* Create class loader for generated classes. This method creates an instance of the default
* implementation of {@link GeneratedClassLoader}. Rhino uses this interface to load generated
Expand Down Expand Up @@ -552,4 +549,9 @@ public final void exit() {
public final Context enterContext(Context cx) {
return Context.enter(cx, this);
}

/** Returns a list of plugins. */
public List<Plugin> getPlugins() {
return plugins;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public LazilyLoadedCtor(
this(scope, propertyName, className, sealed, false);
}

LazilyLoadedCtor(
public LazilyLoadedCtor(
ScriptableObject scope,
String propertyName,
String className,
Expand Down
21 changes: 21 additions & 0 deletions rhino/src/main/java/org/mozilla/javascript/Plugin.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package org.mozilla.javascript;

/**
* Plugins may be loaded with the serviceLocator.
*
* @author Roland Praml, Foconis Analytics GmbH
*/
public interface Plugin {

/** The name of the plugin. */
String getName();

/** Initializes the safe standard objects. */
default void initSafeStandardObjects(Context cx, ScriptableObject scope, boolean sealed) {}

/** Initializes the (unsafe) standard objects. */
default void initStandardObjects(Context cx, ScriptableObject scope, boolean sealed) {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHECKME: Do we need the difference between safe and unsafe objects in the long term?


/** Initialize the compiler environmnt. */
default void initCompilerEnvirons(Context cx, CompilerEnvirons compilerEnvirons) {}
}
14 changes: 5 additions & 9 deletions rhino/src/main/java/org/mozilla/javascript/ScriptRuntime.java
Original file line number Diff line number Diff line change
Expand Up @@ -192,21 +192,14 @@ public static ScriptableObject initSafeStandardObjects(
NativeJavaObject.init(scope, sealed);
NativeJavaMap.init(scope, sealed);

boolean withXml =
cx.hasFeature(Context.FEATURE_E4X) && cx.getE4xImplementationFactory() != null;

// define lazy-loaded properties using their class name
new LazilyLoadedCtor(
scope, "RegExp", "org.mozilla.javascript.regexp.NativeRegExp", sealed, true);
new LazilyLoadedCtor(
scope, "Continuation", "org.mozilla.javascript.NativeContinuation", sealed, true);

if (withXml) {
String xmlImpl = cx.getE4xImplementationFactory().getImplementationClassName();
new LazilyLoadedCtor(scope, "XML", xmlImpl, sealed, true);
new LazilyLoadedCtor(scope, "XMLList", xmlImpl, sealed, true);
new LazilyLoadedCtor(scope, "Namespace", xmlImpl, sealed, true);
new LazilyLoadedCtor(scope, "QName", xmlImpl, sealed, true);
for (Plugin plugin : cx.getFactory().getPlugins()) {
plugin.initSafeStandardObjects(cx, scope, sealed);
}

if (((cx.getLanguageVersion() >= Context.VERSION_1_8)
Expand Down Expand Up @@ -303,6 +296,9 @@ public static ScriptableObject initStandardObjects(
Context cx, ScriptableObject scope, boolean sealed) {
ScriptableObject s = initSafeStandardObjects(cx, scope, sealed);

for (Plugin plugin : cx.getFactory().getPlugins()) {
plugin.initStandardObjects(cx, s, sealed);
}
new LazilyLoadedCtor(
s, "Packages", "org.mozilla.javascript.NativeJavaTopPackage", sealed, true);
new LazilyLoadedCtor(
Expand Down
22 changes: 0 additions & 22 deletions rhino/src/main/java/org/mozilla/javascript/xml/XMLLib.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,28 +15,6 @@
public abstract class XMLLib {
private static final Object XML_LIB_KEY = new Object();

/**
* An object which specifies an XMLLib implementation to be used at runtime.
*
* <p>This interface should be considered experimental. It may be better (and certainly more
* flexible) to write an interface that returns an XMLLib object rather than a class name, for
* example. But that would cause many more ripple effects in the code, all the way back to
* {@link ScriptRuntime}.
*/
public abstract static class Factory {

public static Factory create(final String className) {
return new Factory() {
@Override
public String getImplementationClassName() {
return className;
}
};
}

public abstract String getImplementationClassName();
}

public static XMLLib extractFromScopeOrNull(Scriptable scope) {
ScriptableObject so = ScriptRuntime.getLibraryScopeOrNull(scope);
if (so == null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

package org.mozilla.javascript.tests;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.fail;

import org.junit.Test;
import org.mozilla.javascript.Context;
import org.mozilla.javascript.ContextFactory;
import org.mozilla.javascript.EcmaError;
import org.mozilla.javascript.Scriptable;

/**
* Test if XML is present. This test exists in "rhino-xml" where XML is present and in "rhino",
* where XML is not on classpath.
*
* @author Roland Praml
*/
public class XMLPresentTest {

@Test
public void testXMLPresent() {
try (Context cx = ContextFactory.getGlobal().enterContext()) {
Scriptable scope = cx.initStandardObjects();
Object result =
cx.evaluateString(
scope, "new XML('<a></a>').toXMLString();", "source", 1, null);
fail("not expected");
} catch (EcmaError e) {
assertEquals("ReferenceError: \"XML\" is not defined. (source#1)", e.getMessage());
}
}
}
Loading