From 1b0b930bb3c2f595d3a55bc8370c0bdc594b9c01 Mon Sep 17 00:00:00 2001 From: Lars Marius Garshol Date: Mon, 28 Jan 2019 14:13:34 +0100 Subject: [PATCH] Allow global variables in modules to use context --- .../com/schibsted/spt/data/jslt/Parser.java | 1 + .../spt/data/jslt/impl/ExpressionImpl.java | 24 ++++++------ .../spt/data/jslt/impl/JstlFile.java | 4 +- .../spt/data/jslt/impl/ParseContext.java | 29 +++++++++----- .../schibsted/spt/data/jslt/impl/Scope.java | 17 --------- .../spt/data/jslt/parser/ParserImpl.java | 12 ++---- .../resources/function-declaration-tests.yaml | 38 +++++++++++++++++++ .../module-also-with-global-var.jslt | 6 +++ .../module-as-function-with-global.jslt | 4 ++ .../module-imports-other-module.jslt | 5 +++ .../resources/module-with-global-var-dot.jslt | 5 +++ .../resources/module-with-global-var.jslt | 4 +- 12 files changed, 100 insertions(+), 49 deletions(-) create mode 100644 src/test/resources/module-also-with-global-var.jslt create mode 100644 src/test/resources/module-as-function-with-global.jslt create mode 100644 src/test/resources/module-imports-other-module.jslt create mode 100644 src/test/resources/module-with-global-var-dot.jslt diff --git a/src/main/java/com/schibsted/spt/data/jslt/Parser.java b/src/main/java/com/schibsted/spt/data/jslt/Parser.java index 245a8e54..559a3550 100644 --- a/src/main/java/com/schibsted/spt/data/jslt/Parser.java +++ b/src/main/java/com/schibsted/spt/data/jslt/Parser.java @@ -187,6 +187,7 @@ public Parser withNamedModules(Map thisModules) { */ public Expression compile() { ParseContext ctx = new ParseContext(functions, source, resolver, modules, + new ArrayList(), new PreparationContext()); return ParserImpl.compileExpression(ctx, new JsltParser(reader)); } diff --git a/src/main/java/com/schibsted/spt/data/jslt/impl/ExpressionImpl.java b/src/main/java/com/schibsted/spt/data/jslt/impl/ExpressionImpl.java index d8dcd23c..97b5fffc 100644 --- a/src/main/java/com/schibsted/spt/data/jslt/impl/ExpressionImpl.java +++ b/src/main/java/com/schibsted/spt/data/jslt/impl/ExpressionImpl.java @@ -35,7 +35,7 @@ public class ExpressionImpl implements Expression { private Map functions; private ExpressionNode actual; private int stackFrameSize; - private JsonNode[] globalStackFrame; + private JstlFile[] fileModules; // contains the mapping from external parameters (variables set from // outside at query-time) to slots, so that we can put the @@ -79,12 +79,15 @@ public JsonNode apply(Scope scope, JsonNode input) { if (input == null) input = NullNode.instance; - // if imported modules have global variables we need to set those - // before we start - if (globalStackFrame != null) - scope.insertModuleGlobals(globalStackFrame); + // evaluate lets in global modules + if (fileModules != null) { + for (int ix = 0; ix < fileModules.length; ix++) + fileModules[ix].evaluateLetsOnly(scope, input); + } + // evaluate own lets NodeUtils.evalLets(scope, input, lets); + return actual.apply(scope, input); } @@ -112,10 +115,8 @@ public void prepare(PreparationContext ctx) { * ExpressionImpl is a module. Called once during compilation. * The values are then remembered forever. */ - public void evaluateLetsOnly(Scope scope) { - // the context node is null: all references to it in modules are - // verboten, anyway - NodeUtils.evalLets(scope, null, lets); + public void evaluateLetsOnly(Scope scope, JsonNode input) { + NodeUtils.evalLets(scope, input, lets); } public void optimize() { @@ -149,7 +150,8 @@ public int getStackFrameSize() { return stackFrameSize; } - public void setInitialScope(Scope startScope) { - this.globalStackFrame = startScope.getGlobalStackFrame(); + public void setGlobalModules(List fileModules) { + this.fileModules = new JstlFile[fileModules.size()]; + this.fileModules = fileModules.toArray(this.fileModules); } } diff --git a/src/main/java/com/schibsted/spt/data/jslt/impl/JstlFile.java b/src/main/java/com/schibsted/spt/data/jslt/impl/JstlFile.java index 1dca5f0f..8a44d472 100644 --- a/src/main/java/com/schibsted/spt/data/jslt/impl/JstlFile.java +++ b/src/main/java/com/schibsted/spt/data/jslt/impl/JstlFile.java @@ -64,7 +64,7 @@ public JsonNode call(JsonNode input, JsonNode[] arguments) { return body.apply(arguments[0]); } - public void evaluateLetsOnly(Scope scope) { - body.evaluateLetsOnly(scope); + public void evaluateLetsOnly(Scope scope, JsonNode input) { + body.evaluateLetsOnly(scope, input); } } diff --git a/src/main/java/com/schibsted/spt/data/jslt/impl/ParseContext.java b/src/main/java/com/schibsted/spt/data/jslt/impl/ParseContext.java index 0264b704..acd9723c 100644 --- a/src/main/java/com/schibsted/spt/data/jslt/impl/ParseContext.java +++ b/src/main/java/com/schibsted/spt/data/jslt/impl/ParseContext.java @@ -16,6 +16,7 @@ package com.schibsted.spt.data.jslt.impl; import java.util.Map; +import java.util.List; import java.util.HashMap; import java.util.ArrayList; import java.util.Collection; @@ -39,10 +40,18 @@ public class ParseContext { */ private String source; /** - * Imported modules listed under their prefixes. + * Imported modules listed under their prefixes. This is scoped per + * source file, since each has a different name-module mapping. */ private Map modules; - private Collection funcalls; // delayed function resolution + /** + * Tracks all loaded JSLT files. Shared between all contexts. + */ + private List files; + /** + * Function expressions, used for delayed name-to-function resolution. + */ + private Collection funcalls; private ParseContext parent; private ResourceResolver resolver; /** @@ -57,6 +66,7 @@ public class ParseContext { public ParseContext(Collection extensions, String source, ResourceResolver resolver, Map namedModules, + List files, PreparationContext preparationContext) { this.extensions = extensions; this.functions = new HashMap(); @@ -64,6 +74,7 @@ public ParseContext(Collection extensions, String source, functions.put(func.getName(), func); this.source = source; + this.files = files; this.funcalls = new ArrayList(); this.modules = new HashMap(); this.resolver = resolver; @@ -75,7 +86,7 @@ public ParseContext(Collection extensions, String source, public ParseContext(String source) { this(Collections.EMPTY_SET, source, new ClasspathResourceResolver(), - new HashMap(), new PreparationContext()); + new HashMap(), new ArrayList(), new PreparationContext()); } public void setParent(ParseContext parent) { @@ -165,11 +176,11 @@ public ResourceResolver getResolver() { return resolver; } - public Scope evaluateGlobalModuleVariables(int stackFrameSize) { - Scope scope = Scope.getRoot(stackFrameSize); - for (Module m : modules.values()) - if (m instanceof JstlFile) - ((JstlFile) m).evaluateLetsOnly(scope); - return scope; + public List getFiles() { + return files; + } + + public void registerJsltFile(JstlFile file) { + files.add(file); } } diff --git a/src/main/java/com/schibsted/spt/data/jslt/impl/Scope.java b/src/main/java/com/schibsted/spt/data/jslt/impl/Scope.java index 2c4f887c..11ed842f 100644 --- a/src/main/java/com/schibsted/spt/data/jslt/impl/Scope.java +++ b/src/main/java/com/schibsted/spt/data/jslt/impl/Scope.java @@ -66,21 +66,4 @@ public void setValue(int slot, JsonNode value) { else localStackFrames.peek()[slot] = value; } - - public void insertModuleGlobals(JsonNode[] globals) { - for (int ix = 0; ix < globals.length; ix++) - if (globals[ix] != null) - globalStackFrame[ix] = globals[ix]; - } - - public JsonNode[] getGlobalStackFrame() { - return globalStackFrame; - } - - public boolean hasGlobalValuesSet() { - for (int ix = 0; ix < globalStackFrame.length; ix++) - if (globalStackFrame[ix] != null) - return true; - return false; - } } diff --git a/src/main/java/com/schibsted/spt/data/jslt/parser/ParserImpl.java b/src/main/java/com/schibsted/spt/data/jslt/parser/ParserImpl.java index 1556bfb1..e5a14908 100644 --- a/src/main/java/com/schibsted/spt/data/jslt/parser/ParserImpl.java +++ b/src/main/java/com/schibsted/spt/data/jslt/parser/ParserImpl.java @@ -52,14 +52,9 @@ public static Expression compileExpression(ParseContext ctx, JsltParser parser) try { parser.Start(); ExpressionImpl expr = compile(ctx, (SimpleNode) parser.jjtree.rootNode()); - - // we need to evaluate the global variables in all the modules, - // if there are any, once and for all, and remember their values - Scope scope = ctx.evaluateGlobalModuleVariables(expr.getStackFrameSize()); - if (scope.hasGlobalValuesSet()) - expr.setInitialScope(scope); - + expr.setGlobalModules(ctx.getFiles()); return expr; + } catch (ParseException e) { throw new JsltException("Parse error: " + e.getMessage(), makeLocation(ctx, e.currentToken)); @@ -72,7 +67,7 @@ private static ExpressionImpl compileImport(Collection functions, ParseContext parent, String jslt) { try (Reader reader = parent.getResolver().resolve(jslt)) { - ParseContext ctx = new ParseContext(functions, jslt, parent.getResolver(), parent.getNamedModules(), parent.getPreparationContext()); + ParseContext ctx = new ParseContext(functions, jslt, parent.getResolver(), parent.getNamedModules(), parent.getFiles(), parent.getPreparationContext()); ctx.setParent(parent); return compileModule(ctx, new JsltParser(reader)); } catch (IOException e) { @@ -552,6 +547,7 @@ private static void processImports(ParseContext ctx, SimpleNode parent) { JstlFile file = doImport(ctx, source, node, prefix); ctx.registerModule(prefix, file); ctx.addDeclaredFunction(prefix, file); + ctx.registerJsltFile(file); } } } diff --git a/src/test/resources/function-declaration-tests.yaml b/src/test/resources/function-declaration-tests.yaml index 4f14fb5b..0f06b073 100644 --- a/src/test/resources/function-declaration-tests.yaml +++ b/src/test/resources/function-declaration-tests.yaml @@ -124,6 +124,30 @@ tests: m:variable($global) output: "\"variable is global\"" + # in this one we don't define the global variable in the query + - + input: {} + query: > + import "module-with-global-var.jslt" as m + m:variable("variable is") + output: "\"variable is global\"" + + - + input: {} + query: > + import "module-as-function-with-global.jslt" as m + m("variable is") + output: "\"variable is global\"" + + - + input: {} + query: > + import "module-with-global-var.jslt" as m + import "module-also-with-global-var.jslt" as m2 + let global = "variable is" + m:variable($global) + " " + m2:andfunc($global) + " " + $global + output: "\"variable is global and variable is variable is\"" + - input: {} query: > @@ -145,6 +169,20 @@ tests: m:foo(2) output: 6 + - + input: "{\"foo\" : 22}" + query: > + import "module-with-global-var-dot.jslt" as m + m:foo() + output: 22 + + - + input: "{\"foo\" : 22}" + query: > + import "module-imports-other-module.jslt" as m + m:foo() + output: 22 + - input: {} query: > diff --git a/src/test/resources/module-also-with-global-var.jslt b/src/test/resources/module-also-with-global-var.jslt new file mode 100644 index 00000000..892f5a0e --- /dev/null +++ b/src/test/resources/module-also-with-global-var.jslt @@ -0,0 +1,6 @@ + +// Test that we can use global variables in modules +def andfunc(text) + $global + $text + +let global = "and " diff --git a/src/test/resources/module-as-function-with-global.jslt b/src/test/resources/module-as-function-with-global.jslt new file mode 100644 index 00000000..aac8e7d7 --- /dev/null +++ b/src/test/resources/module-as-function-with-global.jslt @@ -0,0 +1,4 @@ + +let global = "global" + +. + " " + $global diff --git a/src/test/resources/module-imports-other-module.jslt b/src/test/resources/module-imports-other-module.jslt new file mode 100644 index 00000000..a56fa4c5 --- /dev/null +++ b/src/test/resources/module-imports-other-module.jslt @@ -0,0 +1,5 @@ + +import "module-with-global-var-dot.jslt" as m + +def foo() + m:foo() \ No newline at end of file diff --git a/src/test/resources/module-with-global-var-dot.jslt b/src/test/resources/module-with-global-var-dot.jslt new file mode 100644 index 00000000..26af82a9 --- /dev/null +++ b/src/test/resources/module-with-global-var-dot.jslt @@ -0,0 +1,5 @@ + +let foo = .foo + +def foo() + $foo \ No newline at end of file diff --git a/src/test/resources/module-with-global-var.jslt b/src/test/resources/module-with-global-var.jslt index ba99a10f..89226d83 100644 --- a/src/test/resources/module-with-global-var.jslt +++ b/src/test/resources/module-with-global-var.jslt @@ -1,6 +1,6 @@ +let global = "global" + // Test that we can use global variables in modules def variable(text) $text + " " + $global - -let global = "global"