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.

Sign up for GitHub

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

Rewrite the Script Engine API #2107

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Rewrite the Script Engine API #2107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 22 additions & 0 deletions phoenicis-engines/src/main/java/org/phoenicis/engines/EngineSettingScriptEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package org.phoenicis.engines;

import org.graalvm.polyglot.Value;
import org.phoenicis.scripts.engine.implementation.PhoenicisScriptEngine;
import org.phoenicis.scripts.engine.implementation.TypedScriptEngine;

public class EngineSettingScriptEngine implements TypedScriptEngine<EngineSetting> {
private final PhoenicisScriptEngine<Value> scriptEngine;

public EngineSettingScriptEngine(PhoenicisScriptEngine<Value> scriptEngine) {
super();

this.scriptEngine = scriptEngine;
}

@Override
public EngineSetting evaluate(String scriptId) {
final String script = String.format("include(\"%s\")", scriptId);

return scriptEngine.evaluate(script).newInstance().as(EngineSetting.class);
}
}
Comment on lines +7 to +22
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here the TypedScriptEngine for the EngineSetting application class

41 changes: 22 additions & 19 deletions phoenicis-engines/src/main/java/org/phoenicis/engines/EngineSettingsManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@

package org.phoenicis.engines;

import org.graalvm.polyglot.Value;
import org.phoenicis.repository.dto.ApplicationDTO;
import org.phoenicis.repository.dto.CategoryDTO;
import org.phoenicis.repository.dto.RepositoryDTO;
import org.phoenicis.scripts.engine.PhoenicisScriptEngineFactory;
import org.phoenicis.scripts.engine.implementation.PhoenicisScriptEngine;
import org.phoenicis.scripts.engine.implementation.TypedScriptEngine;
import org.phoenicis.scripts.engine.implementation.TypedScriptEngineFactory;
import org.phoenicis.scripts.exceptions.ScriptException;

import java.util.List;
import java.util.Map;
Expand All @@ -35,49 +35,52 @@
* Manages the engine settings
*/
public class EngineSettingsManager {
private final PhoenicisScriptEngineFactory phoenicisScriptEngineFactory;
private final TypedScriptEngineFactory typedScriptEngineFactory;
private final ExecutorService executorService;

/**
* Constructor
*
* @param phoenicisScriptEngineFactory The used script engine factory
* @param executorService The executor service to allow for parallelization
* @param typedScriptEngineFactory The used script engine factory
* @param executorService The executor service to allow for parallelization
*/
public EngineSettingsManager(PhoenicisScriptEngineFactory phoenicisScriptEngineFactory,
ExecutorService executorService) {
public EngineSettingsManager(TypedScriptEngineFactory typedScriptEngineFactory,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This class shows an example how I intend to use the script engine in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it mean that we cannot access e.g. applications and engines via the same script engine?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. We will need to provide a dedicated TypedScriptEngine implementation for every application type (i.e. Installer, Engine, etc)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Understood. So this is also because we want to keep the APIs in the modules and not centralize them in phoenicis-scripts (we discussed this somewhere else).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this is also because we want to keep the APIs in the modules and not centralize them in phoenicis-scripts (we discussed this somewhere else).

No the reason why I want to implement a dedicated TypedScriptEngine class for every application type is because it divides the functionality better. It follows the approach divide and conquer in that we implement independent behavior in different modules/classes. For example what have the processing of Installer script and Verb script in common except that they are both executed with graaljs and use the module system? Therefore we divide the corresponding functionality in different classes.

I still plan to move all application types (and ideally also the TypeScriptEngine implementations) to the phoenicis-scripts module. I didn't do this yet because I think the main changes of this PR are easier comprehensible if I leave as much of the related refactoring as possible for later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still plan to move all application types

I thought @qparis doesn't like this?

ExecutorService executorService) {
super();

this.phoenicisScriptEngineFactory = phoenicisScriptEngineFactory;
this.typedScriptEngineFactory = typedScriptEngineFactory;
this.executorService = executorService;
}

/**
* Fetches the available engine settings
*
* @param repositoryDTO The repository containing the engine settings
* @param callback The callback which recieves the found engine settings
* @param callback The callback which recieves the found engine settings
* @param errorCallback The callback which will be executed if an error occurs
*/
public void fetchAvailableEngineSettings(RepositoryDTO repositoryDTO,
Consumer<Map<String, List<EngineSetting>>> callback, Consumer<Exception> errorCallback) {
Consumer<Map<String, List<EngineSetting>>> callback, Consumer<Exception> errorCallback) {
executorService.execute(() -> {
final List<SettingConfig> configurations = fetchSettingConfigurations(repositoryDTO);

// the script engine needs to be created inside the correct thread otherwise GraalJS throws an error
final PhoenicisScriptEngine phoenicisScriptEngine = phoenicisScriptEngineFactory.createEngine();
final TypedScriptEngine<EngineSetting> phoenicisScriptEngine = typedScriptEngineFactory.createScriptEngine(EngineSetting.class);

final Map<String, List<EngineSetting>> result = configurations.stream()
.collect(Collectors.groupingBy(
configuration -> configuration.engineId,
Collectors.mapping(configuration -> {
final String include = String.format("include(\"engines.%s.settings.%s\");",
configuration.engineId, configuration.settingId);
final String scriptId = String.format("engines.%s.settings.%s", configuration.engineId, configuration.settingId);

final Value settingClass = (Value) phoenicisScriptEngine.evalAndReturn(include,
errorCallback);
try {
return phoenicisScriptEngine.evaluate(scriptId);
} catch (ScriptException se) {
errorCallback.accept(se);

return settingClass.newInstance().as(EngineSetting.class);
// rethrow exception
throw se;
}
}, Collectors.toList())));

callback.accept(result);
Expand Down Expand Up @@ -122,7 +125,7 @@ private List<SettingConfig> fetchSettingConfigurations(RepositoryDTO repositoryD
.collect(Collectors.toList());
}

private class EngineInformation {
private static class EngineInformation {
public final String engineId;

public final ApplicationDTO application;
Expand All @@ -133,7 +136,7 @@ private EngineInformation(String engineId, ApplicationDTO application) {
}
}

private class SettingConfig {
private static class SettingConfig {
public final String engineId;

public final String settingId;
Expand Down
16 changes: 14 additions & 2 deletions phoenicis-engines/src/main/java/org/phoenicis/engines/EnginesConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@

package org.phoenicis.engines;

import org.graalvm.polyglot.Value;
import org.phoenicis.configuration.PhoenicisGlobalConfiguration;
import org.phoenicis.multithreading.MultithreadingConfiguration;
import org.phoenicis.scripts.ScriptsConfiguration;
import org.phoenicis.scripts.engine.PhoenicisScriptEngineFactory;
import org.phoenicis.scripts.engine.implementation.TypedScriptEngine;
import org.phoenicis.scripts.engine.implementation.TypedScriptEngineFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
Expand All @@ -45,8 +49,16 @@ public EnginesManager enginesSource() {

@Bean
public EngineSettingsManager engineSettingsManager() {
return new EngineSettingsManager(scriptsConfiguration.graalScriptEngineFactory(),
multithreadingConfiguration.scriptExecutorService());
final PhoenicisScriptEngineFactory<Value> scriptEngineFactory = scriptsConfiguration.graalScriptEngineFactory();
final TypedScriptEngineFactory factory = new TypedScriptEngineFactory() {
@Override
public <T> TypedScriptEngine<T> createScriptEngine(Class<T> type) {
//noinspection unchecked
return (TypedScriptEngine<T>) new EngineSettingScriptEngine(scriptEngineFactory.createEngine());
}
};

return new EngineSettingsManager(factory, multithreadingConfiguration.scriptExecutorService());
}

@Bean
Expand Down
12 changes: 7 additions & 5 deletions phoenicis-scripts/src/main/java/org/phoenicis/scripts/ScriptsConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@
import org.phoenicis.multithreading.MultithreadingConfiguration;
import org.phoenicis.repository.RepositoryConfiguration;
import org.phoenicis.scripts.engine.PhoenicisScriptEngineFactory;
import org.phoenicis.scripts.interpreter.PhoenicisScriptInterpreter;
import org.phoenicis.scripts.engine.ScriptEngineType;
import org.phoenicis.scripts.engine.injectors.*;
import org.phoenicis.scripts.interpreter.BackgroundScriptInterpreter;
import org.phoenicis.scripts.interpreter.PhoenicisScriptInterpreter;
import org.phoenicis.scripts.interpreter.ScriptFetcher;
import org.phoenicis.scripts.interpreter.ScriptInterpreter;
import org.phoenicis.scripts.wizard.WizardConfiguration;
Expand All @@ -34,7 +34,7 @@
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Import;

import java.util.Arrays;
import java.util.List;

@Configuration
@Import(WizardConfiguration.class)
Expand All @@ -52,10 +52,12 @@ public class ScriptsConfiguration {
private MultithreadingConfiguration multithreadingConfiguration;

@Bean
public PhoenicisScriptEngineFactory graalScriptEngineFactory() {
return new PhoenicisScriptEngineFactory(ScriptEngineType.GRAAL, Arrays.asList(new ScriptUtilitiesInjector(),
public PhoenicisScriptEngineFactory<org.graalvm.polyglot.Value> graalScriptEngineFactory() {
final List<EngineInjector<org.graalvm.polyglot.Value>> injectors = List.of(new ScriptUtilitiesInjector(),
new BeanInjector(applicationContext), new SetupWizardInjector(wizardConfiguration.setupWizardFactory()),
new IncludeInjector(scriptFetcher()), new LocalisationInjector()));
new IncludeInjector(scriptFetcher()), new LocalisationInjector());

return new PhoenicisScriptEngineFactory<>(ScriptEngineType.GRAAL, injectors);
}

@Bean
Expand Down
16 changes: 9 additions & 7 deletions ...icis-scripts/src/main/java/org/phoenicis/scripts/engine/PhoenicisScriptEngineFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,24 @@

package org.phoenicis.scripts.engine;

import org.phoenicis.scripts.engine.injectors.EngineInjector;
import org.phoenicis.scripts.engine.implementation.PhoenicisScriptEngine;
import org.phoenicis.scripts.engine.injectors.EngineInjector;

import java.util.List;

public class PhoenicisScriptEngineFactory {
private final ScriptEngineType type;
private final List<EngineInjector> engineInjectors;
public class PhoenicisScriptEngineFactory<T> {
private final ScriptEngineType<T> type;
private final List<EngineInjector<T>> engineInjectors;

public PhoenicisScriptEngineFactory(ScriptEngineType<T> type, List<EngineInjector<T>> engineInjectors) {
super();

public PhoenicisScriptEngineFactory(ScriptEngineType type, List<EngineInjector> engineInjectors) {
this.type = type;
this.engineInjectors = engineInjectors;
}

public PhoenicisScriptEngine createEngine() {
final PhoenicisScriptEngine phoenicisScriptEngine = type.createScriptEngine();
public PhoenicisScriptEngine<T> createEngine() {
final PhoenicisScriptEngine<T> phoenicisScriptEngine = this.type.createScriptEngine();

engineInjectors.forEach(engineInjector -> engineInjector.injectInto(phoenicisScriptEngine));

Expand Down
48 changes: 12 additions & 36 deletions phoenicis-scripts/src/main/java/org/phoenicis/scripts/engine/ScriptEngineType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,46 +1,22 @@
package org.phoenicis.scripts.engine;

import org.graalvm.polyglot.Value;
import org.phoenicis.scripts.engine.implementation.GraalEngineType;
import org.phoenicis.scripts.engine.implementation.PhoenicisScriptEngine;
import org.phoenicis.scripts.engine.implementation.PolyglotScriptEngine;

import java.util.Map;

/**
* The supported script engine types
* A script engine type supported by Phoenicis
*
* @param <T> The internal script result type used by the script engine
*/
public enum ScriptEngineType {
GRAAL("graal.js") {
@Override
public PhoenicisScriptEngine createScriptEngine() {
return new PolyglotScriptEngine("js",
Map.of("js.nashorn-compat", "true",
"js.experimental-foreign-object-prototype", "true"));
}
};
public interface ScriptEngineType<T> {
// convenience instance
ScriptEngineType<Value> GRAAL = new GraalEngineType();

/**
* The name of the script engine type
*/
private final String name;

/**
* Constructor
* Creates a new script engine
*
* @param name The name of the script engine type
* @return The created script engine
*/
ScriptEngineType(String name) {
this.name = name;
}

/**
* Creates a new instance of the {@link ScriptEngineType}
*
* @return The new instance of the {@link ScriptEngineType}
*/
public abstract PhoenicisScriptEngine createScriptEngine();

@Override
public String toString() {
return this.name;
}
}
PhoenicisScriptEngine<T> createScriptEngine();
}
20 changes: 20 additions & 0 deletions ...is-scripts/src/main/java/org/phoenicis/scripts/engine/implementation/GraalEngineType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package org.phoenicis.scripts.engine.implementation;

import org.graalvm.polyglot.Value;
import org.phoenicis.scripts.engine.ScriptEngineType;

import java.util.Map;

public class GraalEngineType implements ScriptEngineType<Value> {
@Override
public PhoenicisScriptEngine<Value> createScriptEngine() {
return new PolyglotScriptEngine("js",
Map.of("js.nashorn-compat", "true",
"js.experimental-foreign-object-prototype", "true"));
}

@Override
public String toString() {
return "graal.js";
}
}
Comment on lines +8 to +20
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here the graal.js specific implementation of the ScriptEngineType

40 changes: 21 additions & 19 deletions ...ipts/src/main/java/org/phoenicis/scripts/engine/implementation/PhoenicisScriptEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,25 @@

package org.phoenicis.scripts.engine.implementation;

import com.google.common.util.concurrent.Runnables;

import java.io.InputStreamReader;
import java.util.function.Consumer;

public interface PhoenicisScriptEngine {
void eval(InputStreamReader inputStreamReader, Consumer<Exception> errorCallback);

void eval(String script, Runnable doneCallback, Consumer<Exception> errorCallback);

default void eval(String script, Consumer<Exception> errorCallback) {
eval(script, Runnables.doNothing(), errorCallback);
}

Object evalAndReturn(String line, Consumer<Exception> errorCallback);

void put(String name, Object object, Consumer<Exception> errorCallback);

void addErrorHandler(Consumer<Exception> errorHandler);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I'm removing the error handlers contained in the script engine we need to provide them nearer to where they are needed in specific try/catch blocks. I think this is a better approach because it is currently very hard to know what error handlers are called for a script because they are often specified in an entirely different code region.

/**
* A script engine used by Phoenicis
*
* @param <R> The internal script result type
*/
public interface PhoenicisScriptEngine<R> {
/**
* Sets a global value for the given key (variable) in the script context
*
* @param key The variable name in the script context
* @param value The corresponding value
*/
void put(String key, Object value);

/**
* Evaluates the given script and returns its result
*
* @param script The script
* @return The result of the evaluated script
*/
R evaluate(String script);
}
Comment on lines +21 to 42
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A script engine now provides a blocking method evaluate which returns the result of the evaluated script in a blocking manner. The result is of a generic type which depends on the concrete script interpreter used underneath (e.g. Value for Graal)

Loading