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

fix: nesting-tracking in resolver aborting too eagerly #10554

Merged
merged 2 commits into from Jan 24, 2022

Conversation

steven-noorbergen
Copy link
Contributor

Closes: #10549

This is a minor change to the resolver that removes a false-positive nesting depth error.

When importing a file from a js function (using resolveVariable), all variables in that file are resolved in a loop. The loop passes a nest depth of one lower, but did so by decrementing the loop counter on each iteration. This means that once you hit your 10th variable, it aborts because it thinks you've nested to deep (while in fact you're still only one level deep).

Effectively this changes --nestTracker to nestTracker - 1, so that the actual depth is kept, allowing for normal behavior.

I also added a test case for this particular use-case. Note that while the test case does file inclusion inside the file-included file, it affects any variable resolving, but I did not want to mess with the test file too much.

@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

Merging #10554 (4948190) into master (1729e0f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10554   +/-   ##
=======================================
  Coverage   85.82%   85.83%           
=======================================
  Files         335      335           
  Lines       13942    13942           
=======================================
+ Hits        11966    11967    +1     
+ Misses       1976     1975    -1     
Impacted Files Coverage Δ
lib/configuration/variables/resolve.js 98.05% <100.00%> (+0.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1729e0f...4948190. Read the comment docs.

@pgrzesik
Copy link
Contributor

Hello @steven-xaroth - we plan to release new major version very soon and for now we're "freezing" merging new features/fixes to the current master (v2) branch. We will let you know after v3 release, unfortunately there might be some changes needed on your side.

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@steven-xaroth that's a great find! Big thanks for proposing that PR

@medikoo
Copy link
Contributor

medikoo commented Jan 24, 2022

@pgrzesik I think it's an important bug fix, that justifies another v2 release before v3. I propose to release it, also when I'll have #10551 done

@medikoo medikoo merged commit 8db03c9 into serverless:master Jan 24, 2022
@pgrzesik
Copy link
Contributor

Thanks @medikoo 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File lookup with >9 lookups inside causes Excessive variables nest depth error
3 participants