Skip to content

Commit 1ac005c

Browse files
committed
Fixes acoording to code review.
1. Use limit settings instead of constants. 2. Removed constants, moved explanation to the property initialization 3. Performe dependency depth check as last check, since it is slowest one.
1 parent fd7da14 commit 1ac005c

File tree

4 files changed

+65
-63
lines changed

4 files changed

+65
-63
lines changed

Nodejs/Product/Analysis/Analysis/AnalysisLimits.cs

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,32 @@ public AnalysisLimits() {
3434
MaxObjectLiteralProperties = 50;
3535
MaxObjectKeysTypes = 5;
3636
MaxMergeTypes = 5;
37-
NestedModulesLimit = AnalysisConstants.MaxAnalysisDepthQualty;
37+
38+
// There no practical reasons to go deeper in dependencies analysis.
39+
// Number 4 is very practical. Here the examples which hightlight idea.
40+
//
41+
// Example 1
42+
//
43+
// Level 0 - Large system. Some code should use properties of the object on level 4 here.
44+
// Level 1 - Subsystem - pass data from level 4
45+
// Level 2 - Internal dependency for company - Do some work and pass data to level 1
46+
// Level 3 - Framework on top of which created Internal dependency.
47+
// Level 4 - Dependency of the framework. Objects from here still should be available.
48+
// Level 5 - Dependency of the framework provide some usefull primitive which would be used very often on the level of whole system. Hmmm.
49+
//
50+
// Example 2 (reason why I could increase to 5)
51+
//
52+
// Level 0 - Large system. Some code should use properties of the object on level 4 here.
53+
// Level 1 - Subsystem - pass data from level 4
54+
// Level 2 - Internal dependency for company - Wrap access to internal library and perform business logic. Do some work and pass data to level 1
55+
// Level 3 - Internal library which wrap access to API.
56+
// Level 4 - Http library.
57+
// Level 5 - Promise Polyfill.
58+
//
59+
// All these examples are highly speculative and I specifically try to create such deep level.
60+
// If you develop on windows with such deep level you already close to your limit, your maximum is probably 10.
61+
// </remarks>
62+
NestedModulesLimit = 4;
3863
}
3964

4065
/// <summary>
@@ -43,7 +68,9 @@ public AnalysisLimits() {
4368
/// <returns>An <see cref="AnalysisLimits"/> object representing medium level Initellisense settings.</returns>
4469
public static AnalysisLimits MakeMediumAnalysisLimits() {
4570
return new AnalysisLimits() {
46-
NestedModulesLimit = AnalysisConstants.MaxAnalysisDepthFast
71+
72+
// Maximum practical limit for the dependencies analysis oriented on producing faster results.
73+
NestedModulesLimit = 2
4774
};
4875
}
4976

Nodejs/Product/Analysis/AnalysisConstants.cs

Lines changed: 0 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -22,40 +22,5 @@
2222
namespace Microsoft.NodejsTools {
2323
internal sealed class AnalysisConstants {
2424
internal const string NodeModulesFolder = "node_modules";
25-
26-
/// <summary>
27-
/// Maximum practical limit for the dependencies analysis.
28-
/// </summary>
29-
/// <remarks>
30-
/// There no practical reasons to go deeper in dependencies analysis.
31-
/// Number 4 is very practical. Here the examples which hightlight idea.
32-
///
33-
/// Example 1
34-
///
35-
/// Level 0 - Large system. Some code should use properties of the object on level 4 here.
36-
/// Level 1 - Subsystem - pass data from level 4
37-
/// Level 2 - Internal dependency for company - Do some work and pass data to level 1
38-
/// Level 3 - Framework on top of which created Internal dependency.
39-
/// Level 4 - Dependency of the framework. Objects from here still should be available.
40-
/// Level 5 - Dependency of the framework provide some usefull primitive which would be used very often on the level of whole system. Hmmm.
41-
///
42-
/// Example 2 (reason why I could increase to 5)
43-
///
44-
/// Level 0 - Large system. Some code should use properties of the object on level 4 here.
45-
/// Level 1 - Subsystem - pass data from level 4
46-
/// Level 2 - Internal dependency for company - Wrap access to internal library and perform business logic. Do some work and pass data to level 1
47-
/// Level 3 - Internal library which wrap access to API.
48-
/// Level 4 - Http library.
49-
/// Level 5 - Promise Polyfill.
50-
///
51-
/// All these examples are highly speculative and I specifically try to create such deep level.
52-
/// If you develop on windows with such deep level you already close to your limit, your maximum is probably 10.
53-
/// </remarks>
54-
internal const int MaxAnalysisDepthQualty = 4;
55-
56-
/// <summary>
57-
/// Maximum practical limit for the dependencies analysis oriented on producing faster results.
58-
/// </summary>
59-
internal const int MaxAnalysisDepthFast = 2;
6025
}
6126
}

Nodejs/Product/Nodejs/Project/NodejsProjectNode.cs

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -491,11 +491,17 @@ internal bool IncludeNodejsFile(NodejsFileNode fileNode) {
491491
return false;
492492
}
493493

494-
int maxModulesDepth = AnalysisConstants.MaxAnalysisDepthQualty;
495-
if (this._analyzer.AnalysisLevel == Options.AnalysisLevel.Medium) {
496-
maxModulesDepth = AnalysisConstants.MaxAnalysisDepthFast;
494+
foreach (var path in _analysisIgnoredDirs) {
495+
if (url.IndexOf(path, 0, StringComparison.OrdinalIgnoreCase) != -1) {
496+
return false;
497+
}
498+
}
499+
if (new FileInfo(fileNode.Url).Length > _maxFileSize) {
500+
// skip obviously generated files...
501+
return false;
497502
}
498503

504+
int maxModulesDepth = this._analyzer.Project.Limits.NestedModulesLimit;
499505
var relativeFile = CommonUtils.GetRelativeFilePath(this.FullPathToChildren, fileNode.Url);
500506
int nestedModulesCount = 0;
501507
int startIndex = 0;
@@ -506,19 +512,10 @@ internal bool IncludeNodejsFile(NodejsFileNode fileNode) {
506512
return false;
507513
}
508514

509-
startIndex = index + 1;
515+
startIndex = index + AnalysisConstants.NodeModulesFolder.Length;
510516
index = relativeFile.IndexOf(AnalysisConstants.NodeModulesFolder, startIndex, StringComparison.OrdinalIgnoreCase);
511517
}
512518

513-
foreach (var path in _analysisIgnoredDirs) {
514-
if (url.IndexOf(path, 0, StringComparison.OrdinalIgnoreCase) != -1) {
515-
return false;
516-
}
517-
}
518-
if (new FileInfo(fileNode.Url).Length > _maxFileSize) {
519-
// skip obviously generated files...
520-
return false;
521-
}
522519
return true;
523520
}
524521

Nodejs/Tests/AnalysisTests/AnalysisFile.cs

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -132,20 +132,9 @@ public static JsAnalyzer Analyze(string directory, AnalysisLimits limits = null,
132132
foreach (var file in Directory.GetFiles(directory, "*", SearchOption.AllDirectories)) {
133133
if (String.Equals(Path.GetExtension(file), ".js", StringComparison.OrdinalIgnoreCase)) {
134134
var relativeFile = file.Substring(directory.Length);
135-
int nestedModulesCount = 0;
136-
int startIndex = 0;
137-
int index = relativeFile.IndexOf("node_modules", startIndex, StringComparison.OrdinalIgnoreCase);
138-
while (index != -1) {
139-
nestedModulesCount++;
140-
if (nestedModulesCount > limits.NestedModulesLimit) {
141-
continue;
142-
}
143-
144-
startIndex = index + 1;
145-
index = relativeFile.IndexOf("node_modules", startIndex, StringComparison.OrdinalIgnoreCase);
135+
if (!CheckExceedsNestedModulesLimit(relativeFile, limits.NestedModulesLimit)) {
136+
files.Add(new AnalysisFile(file, File.ReadAllText(file)));
146137
}
147-
148-
files.Add(new AnalysisFile(file, File.ReadAllText(file)));
149138
} else if (String.Equals(Path.GetFileName(file), "package.json", StringComparison.OrdinalIgnoreCase)) {
150139
JavaScriptSerializer serializer = new JavaScriptSerializer();
151140
Dictionary<string, object> json;
@@ -164,5 +153,29 @@ public static JsAnalyzer Analyze(string directory, AnalysisLimits limits = null,
164153

165154
return Analyze(limits, parseCallback, files.ToArray());
166155
}
156+
157+
/// <summary>
158+
/// Check that relative file path does not exceed required
159+
/// </summary>
160+
/// <param name="relativeFile">Relative file path to check.</param>
161+
/// <param name="maxDepth">Max depth of node dependencies.</param>
162+
/// <returns>True if file contains in the dependency which is too deep down dependency tree; false otherwise.</returns>
163+
private static bool CheckExceedsNestedModulesLimit(string relativeFile, int maxDepth)
164+
{
165+
int nestedModulesCount = 0;
166+
int startIndex = 0;
167+
int index = relativeFile.IndexOf("node_modules", startIndex, StringComparison.OrdinalIgnoreCase);
168+
while (index != -1) {
169+
nestedModulesCount++;
170+
if (nestedModulesCount > maxDepth) {
171+
return true;
172+
}
173+
174+
startIndex = index + "node_modules".Length;
175+
index = relativeFile.IndexOf("node_modules", startIndex, StringComparison.OrdinalIgnoreCase);
176+
}
177+
178+
return false;
179+
}
167180
}
168181
}

0 commit comments

Comments
 (0)