Fix HubL from tag nested relative import resolution
#1248
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Resolves HubL nested relative import failures where FromTag wasn't properly managing the currentPathStack used by RelativePathResolver for path resolution. Root cause: FromTag only managed fromStack for cycle detection but not currentPathStack for path resolution, causing nested imports to resolve relative paths from the wrong context. Changes: - FromTag now properly pushes/pops currentPathStack during template processing - Added @VisibleForTesting annotations for comprehensive test coverage - Maintains backwards compatibility and cycle detection functionality Tests added: - itResolvesNestedRelativeImports: Core nested import scenario - itMaintainsPathStackIntegrity: Stack management verification - itWorksWithIncludeAndFromTogether: Tag interaction compatibility - itResolvesUpAndAcrossDirectoryPaths: Complex navigation patterns - itResolvesOriginalErrorCasePaths: Exact build failure reproduction 🤖 Generated with [Claude Code](https://claude.ai/code)
Extends the nested relative import fix to eager execution mode. EagerFromTag had the same issue as FromTag - it wasn't managing currentPathStack for relative path resolution. Changes: - EagerFromTag now properly pushes/pops currentPathStack during template processing - Removed eager mode bypasses from tests since both modes now work correctly - Added specific test for eager mode nested relative imports This ensures consistent behavior between regular and eager execution modes for nested HubL relative imports. 🤖 Generated with [Claude Code](https://claude.ai/code)
Split large test methods that tested multiple concerns into focused, single-responsibility tests for better maintainability and clarity: FromTagTest: - Split itResolvesAbsolutePathsWithNestedRelativeImports into 3 tests: - itResolvesProjectsAbsolutePathsWithNestedRelativeImports - itResolvesHubspotAbsolutePathsWithNestedRelativeImports - itResolvesMixedAbsoluteAndRelativeImports ImportTagTest: - Split itResolvesAbsolutePathsWithProjectPrefixes into 2 tests: - itResolvesProjectsAbsolutePaths - itResolvesHubspotAbsolutePaths - Added 5 additional focused test methods for comprehensive coverage: - itResolvesNestedRelativeImports - itMaintainsPathStackIntegrityWithImport - itWorksWithIncludeAndImportTogether - itResolvesUpAndAcrossDirectoryPaths All 40 tests pass, maintaining comprehensive coverage while improving test isolation, clarity, and maintainability. 🤖 Generated with [Claude Code](https://claude.ai/code)
- Replace JUnit assertTrue() with AssertJ assertions for consistency - Remove redundant test methods between FromTagTest and ImportTagTest: - Removed itMaintainsPathStackIntegrityWithImport (duplicate logic) - Removed itWorksWithIncludeAndImportTogether (covered by FromTagTest) - Maintain comprehensive coverage with 38 focused tests (14 + 24) - All tests pass with improved maintainability and reduced duplication 🤖 Generated with [Claude Code](https://claude.ai/code)
The getFromStack() method was made public with @VisibleForTesting but was never actually used in our tests or implementation. The CallStack.size() method is the one that's actually needed and used via getCurrentPathStack().size(). This keeps the API surface minimal and only exposes what's necessary.
Instead of adding a new public size() method to CallStack, use the existing public peek() method to verify stack integrity. This achieves the same test coverage (ensuring push/pop operations are balanced) without expanding the public API surface. The test now verifies that the top path on the stack remains the same before and after rendering, which is equivalent to checking stack size but uses only existing public methods.
jasmith-hs
reviewed
Jun 10, 2025
Comment on lines
119
to
160
| jinjava.setResourceLocator( | ||
| new ResourceLocator() { | ||
| private final RelativePathResolver relativePathResolver = | ||
| new RelativePathResolver(); | ||
| private final java.util.Map<String, String> templates = | ||
| new java.util.HashMap<>() { | ||
| { | ||
| put( | ||
| "level0.jinja", | ||
| "{% from 'level1/nested.jinja' import macro1 %}{{ macro1() }}" | ||
| ); | ||
| put( | ||
| "level1/nested.jinja", | ||
| "{% from '../level1/deeper/macro.jinja' import macro2 %}{% macro macro1() %}L1:{{ macro2() }}{% endmacro %}" | ||
| ); | ||
| put( | ||
| "level1/deeper/macro.jinja", | ||
| "{% from '../../utils/helper.jinja' import helper %}{% macro macro2() %}L2:{{ helper() }}{% endmacro %}" | ||
| ); | ||
| put("utils/helper.jinja", "{% macro helper() %}HELPER{% endmacro %}"); | ||
| } | ||
| }; | ||
|
|
||
| @Override | ||
| public String getString( | ||
| String fullName, | ||
| Charset encoding, | ||
| JinjavaInterpreter interpreter | ||
| ) throws IOException { | ||
| String template = templates.get(fullName); | ||
| if (template == null) { | ||
| throw new IOException("Template not found: " + fullName); | ||
| } | ||
| return template; | ||
| } | ||
|
|
||
| @Override | ||
| public Optional<LocationResolver> getLocationResolver() { | ||
| return Optional.of(relativePathResolver); | ||
| } | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
This test setup is quite verbose and has a lot of duplicate boilerplate. Could you either use a testing pattern that's already in use or make this a reusable pattern?
Contributor
Author
There was a problem hiding this comment.
You're absolutely right, it is. I created a new helper method to build the resource locator, which simplified things a lot. Let me know if you think it needs more work.
Comment on lines
88
to
92
| interpreter | ||
| .getContext() | ||
| .getCurrentPathStack() | ||
| .push(templateFile, tagNode.getLineNumber(), tagNode.getStartPosition()); | ||
|
|
Contributor
There was a problem hiding this comment.
You could use ImportTag#parseTemplateAsNode
Contributor
Author
There was a problem hiding this comment.
Great point. Updated to do so and moved context stack popping into the finally block to match ImportTag.
jasmith-hs
reviewed
Jun 11, 2025
| } finally { | ||
| interpreter.getContext().popFromStack(); | ||
| interpreter.getContext().getCurrentPathStack().pop(); | ||
| interpreter.getContext().getImportPathStack().pop(); |
Contributor
There was a problem hiding this comment.
This is popping the wrong stack. It should be popping the fromStack
| } finally { | ||
| interpreter.getContext().popFromStack(); | ||
| interpreter.getContext().getCurrentPathStack().pop(); | ||
| interpreter.getContext().getImportPathStack().pop(); |
Contributor
There was a problem hiding this comment.
This is popping the wrong stack. It should be popping the fromStack
jasmith-hs
approved these changes
Jun 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug with relative imports using the
fromtag.This issue surfaced when investigating a customer issue with HubL + React themes where they had a theme structure similar to this one:
With this import flow:
Which, when built, results in these validation errors:
The relevant line of which being:
Where it's looking for
hubl-modules/icons/icons.hubl.htmlinstead ofpartials/atoms/icons/icons.hubl.htmlbecause it's evaluating the relative path against the module instead of thelink.hubl.htmlfile imported by said module.