From 0744ad1d814afa0ece9472d3bd5379fe106208ce Mon Sep 17 00:00:00 2001 From: Jack Smith <72623970+jasmith-hs@users.noreply.github.com> Date: Tue, 10 Sep 2024 11:57:52 -0400 Subject: [PATCH] Revert "Wrap the current path around reconstructed macro functions" --- .../java/com/hubspot/jinjava/Jinjava.java | 8 ---- .../lib/fn/eager/EagerMacroFunction.java | 46 +------------------ .../java/com/hubspot/jinjava/EagerTest.java | 11 +---- .../jinjava/ExpectedTemplateInterpreter.java | 38 ++------------- .../com/hubspot/jinjava/FullSnippetsTest.java | 15 +----- ...rred-fromed-macro-in-output.expected.jinja | 2 +- ...ed-imported-macro-in-output.expected.jinja | 2 +- .../dir1/macro.jinja | 4 -- .../dir2/imported.jinja | 1 - .../test.expected.jinja | 11 ----- .../test.jinja | 2 - .../from-tag-doesnt-steal-extends/base.jinja | 3 -- .../something-to-import.jinja | 1 - .../test.expected.jinja | 1 - .../from-tag-doesnt-steal-extends/test.jinja | 7 --- 15 files changed, 12 insertions(+), 140 deletions(-) delete mode 100644 src/test/resources/eager/wraps-current-path-around-macro/dir1/macro.jinja delete mode 100644 src/test/resources/eager/wraps-current-path-around-macro/dir2/imported.jinja delete mode 100644 src/test/resources/eager/wraps-current-path-around-macro/test.expected.jinja delete mode 100644 src/test/resources/eager/wraps-current-path-around-macro/test.jinja delete mode 100644 src/test/resources/snippets/from-tag-doesnt-steal-extends/base.jinja delete mode 100644 src/test/resources/snippets/from-tag-doesnt-steal-extends/something-to-import.jinja delete mode 100644 src/test/resources/snippets/from-tag-doesnt-steal-extends/test.expected.jinja delete mode 100644 src/test/resources/snippets/from-tag-doesnt-steal-extends/test.jinja diff --git a/src/main/java/com/hubspot/jinjava/Jinjava.java b/src/main/java/com/hubspot/jinjava/Jinjava.java index 7079616bd..21179b86d 100644 --- a/src/main/java/com/hubspot/jinjava/Jinjava.java +++ b/src/main/java/com/hubspot/jinjava/Jinjava.java @@ -35,7 +35,6 @@ import com.hubspot.jinjava.lib.fn.ELFunctionDefinition; import com.hubspot.jinjava.lib.tag.Tag; import com.hubspot.jinjava.loader.ClasspathResourceLocator; -import com.hubspot.jinjava.loader.RelativePathResolver; import com.hubspot.jinjava.loader.ResourceLocator; import de.odysseus.el.ExpressionFactoryImpl; import de.odysseus.el.misc.TypeConverter; @@ -245,10 +244,6 @@ public RenderResult renderForResult( } else { context = new Context(copyGlobalContext(), bindings, renderConfig.getDisabled()); } - Object currentPath = context.get(RelativePathResolver.CURRENT_PATH_CONTEXT_KEY); - if (currentPath != null) { - context.getCurrentPathStack().pushWithoutCycleCheck(currentPath.toString(), 0, 0); - } JinjavaInterpreter interpreter = globalConfig .getInterpreterFactory() @@ -295,9 +290,6 @@ public RenderResult renderForResult( ); } finally { globalContext.reset(); - if (currentPath != null) { - context.getCurrentPathStack().pop(); - } JinjavaInterpreter.popCurrent(); } } diff --git a/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java b/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java index 4dbc553cc..9e805daa3 100644 --- a/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java +++ b/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java @@ -1,7 +1,5 @@ package com.hubspot.jinjava.lib.fn.eager; -import static com.hubspot.jinjava.loader.RelativePathResolver.CURRENT_PATH_CONTEXT_KEY; - import com.google.common.annotations.Beta; import com.hubspot.jinjava.el.ext.AstMacroFunction; import com.hubspot.jinjava.el.ext.DeferredInvocationResolutionException; @@ -15,7 +13,6 @@ import com.hubspot.jinjava.lib.fn.MacroFunction; import com.hubspot.jinjava.lib.tag.MacroTag; import com.hubspot.jinjava.lib.tag.eager.EagerExecutionResult; -import com.hubspot.jinjava.lib.tag.eager.importing.EagerImportingStrategyFactory; import com.hubspot.jinjava.objects.serialization.PyishObjectMapper; import com.hubspot.jinjava.tree.Node; import com.hubspot.jinjava.util.EagerContextWatcher; @@ -83,11 +80,6 @@ public Object doEvaluate( () -> getEvaluationResultDirectly(argMap, kwargMap, varArgs, interpreter), interpreter ); - return wrapCurrentPathSetting( - interpreter, - importFile, - result.asTemplateString() - ); } return result.asTemplateString(); } finally { @@ -129,45 +121,12 @@ public Object doEvaluate( .put( tempVarName, new MacroFunctionTempVariable( - wrapCurrentPathSetting( - interpreter, - Optional - .ofNullable(localContextScope.get(Context.IMPORT_RESOURCE_PATH_KEY)) - .map(Object::toString), - prefixToPreserveState + eagerExecutionResult.asTemplateString() - ) + prefixToPreserveState + eagerExecutionResult.asTemplateString() ) ); throw new DeferredInvocationResolutionException(tempVarName); } - return wrapCurrentPathSetting( - interpreter, - Optional - .ofNullable(localContextScope.get(Context.IMPORT_RESOURCE_PATH_KEY)) - .map(Object::toString), - eagerExecutionResult.getResult().toString(true) - ); - } - - private static String wrapCurrentPathSetting( - JinjavaInterpreter interpreter, - Optional importFile, - String resultToWrap - ) { - if (!importFile.isPresent()) { - return resultToWrap; - } - final String initialPathSetter = - EagerImportingStrategyFactory.getSetTagForCurrentPath(interpreter); - final String newPathSetter = EagerReconstructionUtils.buildBlockOrInlineSetTag( - CURRENT_PATH_CONTEXT_KEY, - importFile.get(), - interpreter - ); - if (initialPathSetter.equals(newPathSetter)) { - return resultToWrap; - } - return newPathSetter + resultToWrap + initialPathSetter; + return eagerExecutionResult.getResult().toString(true); } private String getEvaluationResultDirectly( @@ -353,7 +312,6 @@ private boolean differentMacroWithSameNameExists(JinjavaInterpreter interpreter) return false; } MacroFunction mostRecent = context.getGlobalMacro(getName()); - //noinspection ErrorProne if (this != mostRecent) { return true; } diff --git a/src/test/java/com/hubspot/jinjava/EagerTest.java b/src/test/java/com/hubspot/jinjava/EagerTest.java index 7db5c58f9..76d5f170f 100644 --- a/src/test/java/com/hubspot/jinjava/EagerTest.java +++ b/src/test/java/com/hubspot/jinjava/EagerTest.java @@ -625,7 +625,7 @@ public void itDefersMacroInIf() { @Test public void itPutsDeferredImportedMacroInOutput() { - expectedTemplateInterpreter.assertExpectedOutputNonIdempotent( + expectedTemplateInterpreter.assertExpectedOutput( "puts-deferred-imported-macro-in-output" ); } @@ -643,7 +643,7 @@ public void itPutsDeferredImportedMacroInOutputSecondPass() { @Test public void itPutsDeferredFromedMacroInOutput() { - expectedTemplateInterpreter.assertExpectedOutputNonIdempotent( + expectedTemplateInterpreter.assertExpectedOutput( "puts-deferred-fromed-macro-in-output" ); } @@ -1597,11 +1597,4 @@ public void prepareContext(Context context) { "keeps-meta-context-variables-through-import/test" ); } - - @Test - public void itWrapsCurrentPathAroundMacro() { - expectedTemplateInterpreter.assertExpectedOutputNonIdempotent( - "wraps-current-path-around-macro/test" - ); - } } diff --git a/src/test/java/com/hubspot/jinjava/ExpectedTemplateInterpreter.java b/src/test/java/com/hubspot/jinjava/ExpectedTemplateInterpreter.java index 0c9be20bf..0694a47ac 100644 --- a/src/test/java/com/hubspot/jinjava/ExpectedTemplateInterpreter.java +++ b/src/test/java/com/hubspot/jinjava/ExpectedTemplateInterpreter.java @@ -15,7 +15,6 @@ public class ExpectedTemplateInterpreter { private Jinjava jinjava; private JinjavaInterpreter interpreter; private String path; - private boolean sensibleCurrentPath = false; public ExpectedTemplateInterpreter( Jinjava jinjava, @@ -27,26 +26,6 @@ public ExpectedTemplateInterpreter( this.path = path; } - public static ExpectedTemplateInterpreter withSensibleCurrentPath( - Jinjava jinjava, - JinjavaInterpreter interpreter, - String path - ) { - return new ExpectedTemplateInterpreter(jinjava, interpreter, path, true); - } - - private ExpectedTemplateInterpreter( - Jinjava jinjava, - JinjavaInterpreter interpreter, - String path, - boolean sensibleCurrentPath - ) { - this.jinjava = jinjava; - this.interpreter = interpreter; - this.path = path; - this.sensibleCurrentPath = sensibleCurrentPath; - } - public String assertExpectedOutput(String name) { String template = getFixtureTemplate(name); String output = JinjavaInterpreter.getCurrent().render(template); @@ -137,13 +116,6 @@ public String assertExpectedNonEagerOutput(String name) { public String getFixtureTemplate(String name) { try { - if (sensibleCurrentPath) { - JinjavaInterpreter - .getCurrent() - .getContext() - .getCurrentPathStack() - .push(String.format("%s/%s.jinja", path, name), 0, 0); - } return Resources.toString( Resources.getResource(String.format("%s/%s.jinja", path, name)), StandardCharsets.UTF_8 @@ -155,12 +127,10 @@ public String getFixtureTemplate(String name) { private String expected(String name) { try { - return Resources - .toString( - Resources.getResource(String.format("%s/%s.expected.jinja", path, name)), - StandardCharsets.UTF_8 - ) - .replaceAll("\\\\\n\\s*", ""); + return Resources.toString( + Resources.getResource(String.format("%s/%s.expected.jinja", path, name)), + StandardCharsets.UTF_8 + ); } catch (IOException e) { throw new RuntimeException(e); } diff --git a/src/test/java/com/hubspot/jinjava/FullSnippetsTest.java b/src/test/java/com/hubspot/jinjava/FullSnippetsTest.java index e033118ad..06fca73db 100644 --- a/src/test/java/com/hubspot/jinjava/FullSnippetsTest.java +++ b/src/test/java/com/hubspot/jinjava/FullSnippetsTest.java @@ -39,7 +39,7 @@ public String getString( JinjavaInterpreter interpreter ) throws IOException { return Resources.toString( - Resources.getResource(relativePathResolver.resolve(fullName, interpreter)), + Resources.getResource(String.format("tags/macrotag/%s", fullName)), StandardCharsets.UTF_8 ); } @@ -66,11 +66,7 @@ public Optional getLocationResolver() { ); interpreter = new JinjavaInterpreter(parentInterpreter); expectedTemplateInterpreter = - ExpectedTemplateInterpreter.withSensibleCurrentPath( - jinjava, - interpreter, - "snippets" - ); + new ExpectedTemplateInterpreter(jinjava, interpreter, "snippets"); localContext = interpreter.getContext(); JinjavaInterpreter.pushCurrent(interpreter); @@ -105,11 +101,4 @@ public void itUsesLowerScopeValueInMacroEvaluation() { "uses-lower-scope-value-in-macro-evaluation" ); } - - @Test - public void itFromTagDoesntStealExtends() { - expectedTemplateInterpreter.assertExpectedOutput( - "from-tag-doesnt-steal-extends/test" - ); - } } diff --git a/src/test/resources/eager/puts-deferred-fromed-macro-in-output.expected.jinja b/src/test/resources/eager/puts-deferred-fromed-macro-in-output.expected.jinja index fcf340374..be059f126 100644 --- a/src/test/resources/eager/puts-deferred-fromed-macro-in-output.expected.jinja +++ b/src/test/resources/eager/puts-deferred-fromed-macro-in-output.expected.jinja @@ -1 +1 @@ -{% set myname = deferred + 3 %}{% set __macro_getPath_331491059_temp_variable_1__ %}{% set current_path = 'simple-with-call.jinja' %}Hello {{ myname }}{% set current_path = '' %}{% endset %}{% print __macro_getPath_331491059_temp_variable_1__ %} \ No newline at end of file +{% set myname = deferred + 3 %}{% set __macro_getPath_331491059_temp_variable_1__ %}Hello {{ myname }}{% endset %}{% print __macro_getPath_331491059_temp_variable_1__ %} diff --git a/src/test/resources/eager/puts-deferred-imported-macro-in-output.expected.jinja b/src/test/resources/eager/puts-deferred-imported-macro-in-output.expected.jinja index fcf340374..fde94f4f8 100644 --- a/src/test/resources/eager/puts-deferred-imported-macro-in-output.expected.jinja +++ b/src/test/resources/eager/puts-deferred-imported-macro-in-output.expected.jinja @@ -1 +1 @@ -{% set myname = deferred + 3 %}{% set __macro_getPath_331491059_temp_variable_1__ %}{% set current_path = 'simple-with-call.jinja' %}Hello {{ myname }}{% set current_path = '' %}{% endset %}{% print __macro_getPath_331491059_temp_variable_1__ %} \ No newline at end of file +{% set myname = deferred + 3 %}{% set __macro_getPath_331491059_temp_variable_1__ %}Hello {{ myname }}{% endset %}{% print __macro_getPath_331491059_temp_variable_1__ %} \ No newline at end of file diff --git a/src/test/resources/eager/wraps-current-path-around-macro/dir1/macro.jinja b/src/test/resources/eager/wraps-current-path-around-macro/dir1/macro.jinja deleted file mode 100644 index 099408892..000000000 --- a/src/test/resources/eager/wraps-current-path-around-macro/dir1/macro.jinja +++ /dev/null @@ -1,4 +0,0 @@ -{% macro foo_importer() -%} - {%- import "../../../eager/wraps-current-path-around-macro/dir2/imported.jinja" -%} - {%- print foo -%} -{%- endmacro %} \ No newline at end of file diff --git a/src/test/resources/eager/wraps-current-path-around-macro/dir2/imported.jinja b/src/test/resources/eager/wraps-current-path-around-macro/dir2/imported.jinja deleted file mode 100644 index 27547dad8..000000000 --- a/src/test/resources/eager/wraps-current-path-around-macro/dir2/imported.jinja +++ /dev/null @@ -1 +0,0 @@ -{% set foo = deferred %} \ No newline at end of file diff --git a/src/test/resources/eager/wraps-current-path-around-macro/test.expected.jinja b/src/test/resources/eager/wraps-current-path-around-macro/test.expected.jinja deleted file mode 100644 index 1bc819fcf..000000000 --- a/src/test/resources/eager/wraps-current-path-around-macro/test.expected.jinja +++ /dev/null @@ -1,11 +0,0 @@ -{% set __macro_foo_importer_1564912668_temp_variable_0__ %}\ - {% set current_path = '../../eager/wraps-current-path-around-macro/dir1/macro.jinja' %}\ - {% do %}\ - {% set current_path = '../../eager/wraps-current-path-around-macro/dir2/imported.jinja' %}\ - {% set foo = deferred %}\ - {% set current_path = '../../eager/wraps-current-path-around-macro/dir1/macro.jinja' %}\ - {% enddo %}\ - {% print foo %}\ - {% set current_path = '' %}\ -{% endset %}\ -{% print __macro_foo_importer_1564912668_temp_variable_0__ %} \ No newline at end of file diff --git a/src/test/resources/eager/wraps-current-path-around-macro/test.jinja b/src/test/resources/eager/wraps-current-path-around-macro/test.jinja deleted file mode 100644 index d05b5b2ef..000000000 --- a/src/test/resources/eager/wraps-current-path-around-macro/test.jinja +++ /dev/null @@ -1,2 +0,0 @@ -{% from "../../eager/wraps-current-path-around-macro/dir1/macro.jinja" import foo_importer -%} -{% print foo_importer() %} diff --git a/src/test/resources/snippets/from-tag-doesnt-steal-extends/base.jinja b/src/test/resources/snippets/from-tag-doesnt-steal-extends/base.jinja deleted file mode 100644 index 4dec9a793..000000000 --- a/src/test/resources/snippets/from-tag-doesnt-steal-extends/base.jinja +++ /dev/null @@ -1,3 +0,0 @@ -{% block content %} -yo -{% endblock %} \ No newline at end of file diff --git a/src/test/resources/snippets/from-tag-doesnt-steal-extends/something-to-import.jinja b/src/test/resources/snippets/from-tag-doesnt-steal-extends/something-to-import.jinja deleted file mode 100644 index fbce8d547..000000000 --- a/src/test/resources/snippets/from-tag-doesnt-steal-extends/something-to-import.jinja +++ /dev/null @@ -1 +0,0 @@ -{% set foo = 'hi' %} \ No newline at end of file diff --git a/src/test/resources/snippets/from-tag-doesnt-steal-extends/test.expected.jinja b/src/test/resources/snippets/from-tag-doesnt-steal-extends/test.expected.jinja deleted file mode 100644 index b2feaee55..000000000 --- a/src/test/resources/snippets/from-tag-doesnt-steal-extends/test.expected.jinja +++ /dev/null @@ -1 +0,0 @@ -

Test

diff --git a/src/test/resources/snippets/from-tag-doesnt-steal-extends/test.jinja b/src/test/resources/snippets/from-tag-doesnt-steal-extends/test.jinja deleted file mode 100644 index 5b8981ae3..000000000 --- a/src/test/resources/snippets/from-tag-doesnt-steal-extends/test.jinja +++ /dev/null @@ -1,7 +0,0 @@ -{% extends './base.jinja' %} -{% from './something-to-import.jinja' import foo %} -{{ foo }} -Don't show me! -{% block content %} -

Test

-{% endblock %} \ No newline at end of file