src: simplify node modules traverse path

PR-URL: nodejs#53061 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
EliphazBouye · Jun 20, 2024 · df5b0a0 · df5b0a0
1 parent 129a199
commit df5b0a0
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 34 deletions.
diff --git a/src/node_modules.cc b/src/node_modules.cc
@@ -3,7 +3,6 @@
 #include "base_object-inl.h"
 #include "node_errors.h"
 #include "node_external_reference.h"
-#include "node_process-inl.h"
 #include "node_url.h"
 #include "permission/permission.h"
 #include "permission/permission_base.h"
@@ -33,14 +32,6 @@ using v8::String;
 using v8::Undefined;
 using v8::Value;
 
-#ifdef __POSIX__
-constexpr char kPathSeparator = '/';
-constexpr std::string_view kNodeModules = "/node_modules";
-#else
-constexpr char kPathSeparator = '\\';
-constexpr std::string_view kNodeModules = "\\node_modules";
-#endif
-
 void BindingData::MemoryInfo(MemoryTracker* tracker) const {
   // Do nothing
 }
@@ -287,18 +278,16 @@ void BindingData::ReadPackageJSON(const FunctionCallbackInfo<Value>& args) {
 }
 
 const BindingData::PackageConfig* BindingData::TraverseParent(
-    Realm* realm, std::string_view check_path) {
+    Realm* realm, const std::filesystem::path& check_path) {
+  std::filesystem::path current_path = check_path;
   auto env = realm->env();
-  auto root_separator_index = check_path.find_first_of(kPathSeparator);
-  size_t separator_index = 0;
   const bool is_permissions_enabled = env->permission()->enabled();
 
   do {
-    separator_index = check_path.find_last_of(kPathSeparator);
-    check_path = check_path.substr(0, separator_index);
+    current_path = current_path.parent_path();
 
     // We don't need to try "/"
-    if (check_path.empty()) {
+    if (current_path.parent_path() == current_path) {
       break;
     }
 
@@ -308,26 +297,22 @@ const BindingData::PackageConfig* BindingData::TraverseParent(
                  !env->permission()->is_granted(
                      env,
                      permission::PermissionScope::kFileSystemRead,
-                     std::string(check_path) + kPathSeparator))) {
+                     current_path.generic_string()))) {
       return nullptr;
     }
 
     // Check if the path ends with `/node_modules`
-    if (check_path.size() >= kNodeModules.size() &&
-        std::equal(check_path.end() - kNodeModules.size(),
-                   check_path.end(),
-                   kNodeModules.begin())) {
+    if (current_path.generic_string().ends_with("/node_modules")) {
       return nullptr;
     }
 
-    auto package_json = GetPackageJSON(
-        realm,
-        std::string(check_path) + kPathSeparator + "package.json",
-        nullptr);
+    auto package_json_path = current_path / "package.json";
+    auto package_json =
+        GetPackageJSON(realm, package_json_path.string(), nullptr);
     if (package_json != nullptr) {
       return package_json;
     }
-  } while (separator_index > root_separator_index);
+  } while (true);
 
   return nullptr;
 }
@@ -339,7 +324,8 @@ void BindingData::GetNearestParentPackageJSON(
 
   Realm* realm = Realm::GetCurrent(args);
   Utf8Value path_value(realm->isolate(), args[0]);
-  auto package_json = TraverseParent(realm, path_value.ToStringView());
+  auto package_json =
+      TraverseParent(realm, std::filesystem::path(path_value.ToString()));
 
   if (package_json != nullptr) {
     args.GetReturnValue().Set(package_json->Serialize(realm));
@@ -353,7 +339,8 @@ void BindingData::GetNearestParentPackageJSONType(
 
   Realm* realm = Realm::GetCurrent(args);
   Utf8Value path(realm->isolate(), args[0]);
-  auto package_json = TraverseParent(realm, path.ToStringView());
+  auto package_json =
+      TraverseParent(realm, std::filesystem::path(path.ToString()));
 
   if (package_json == nullptr) {
     return;
@@ -390,10 +377,7 @@ void BindingData::GetPackageScopeConfig(
   // TODO(@anonrig): Rewrite this function and avoid calling URL parser.
   while (true) {
     auto pathname = package_json_url->get_pathname();
-    if (pathname.size() >= node_modules_package_path.size() &&
-        pathname.compare(pathname.size() - node_modules_package_path.size(),
-                         node_modules_package_path.size(),
-                         node_modules_package_path) == 0) {
+    if (pathname.ends_with(node_modules_package_path)) {
       break;
     }
 

diff --git a/src/node_modules.h b/src/node_modules.h
@@ -1,7 +1,6 @@
 #ifndef SRC_NODE_MODULES_H_
 #define SRC_NODE_MODULES_H_
 
-#include "v8-function-callback.h"
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #include "node.h"
@@ -11,6 +10,7 @@
 #include "v8-fast-api-calls.h"
 #include "v8.h"
 
+#include <filesystem>
 #include <optional>
 #include <string>
 #include <string_view>
@@ -80,8 +80,8 @@ class BindingData : public SnapshotableObject {
       Realm* realm,
       std::string_view path,
       ErrorContext* error_context = nullptr);
-  static const PackageConfig* TraverseParent(Realm* realm,
-                                             std::string_view check_path);
+  static const PackageConfig* TraverseParent(
+      Realm* realm, const std::filesystem::path& check_path);
 };
 
 }  // namespace modules