revise latest package improvements (#93)

* Introduce resolve_symlinks utility; normalize JAVA_HOME and use effective_java_home for cache keys; replace repeated symlink resolution with helper in java_find; remove redundant version-sorting

* improve symlink path resolution robustness, and add rJava version caching tests.

* fix tests

* simplify effective Java home determination logic.

Author: e-kotov

R/internal_utilities.R

@@ -267,6 +267,34 @@ is_rjavaenv_cache_path <- function(path) {
   return(startsWith(path_norm, cache_norm))
 }
 
+#' Resolve symlinks recursively on Unix systems
+#'
+#' @param path Character. Path to resolve.
+#' @param max_depth Integer. Maximum symlink depth to follow (default 10).
+#' @return Character. Resolved path (or original if not a symlink or on Windows).
+#' @keywords internal
+#' @noRd
+resolve_symlinks <- function(path, max_depth = 10L) {
+  if (.Platform$OS.type != "unix" || !nzchar(path)) {
+    return(path)
+  }
+
+  real_path <- path
+  for (i in seq_len(max_depth)) {
+    link <- Sys.readlink(real_path)
+    if (is.na(link) || !nzchar(link)) {
+      break
+    }
+    # Handle relative symlinks
+    if (!startsWith(link, "/")) {
+      link <- file.path(dirname(real_path), link)
+    }
+    real_path <- link
+  }
+
+  return(real_path)
+}
+
 #' Internal readline wrapper for testability
 #'
 #' Wraps base::readline() to enable mocking in tests without polluting public API.

R/java_env.R

@@ -187,10 +187,8 @@ java_env_set_rprofile <- function(
   project_path <- ifelse(is.null(project_path), getwd(), project_path)
   rprofile_path <- file.path(project_path, ".Rprofile")
 
-  # Normalize the path for Windows
-  if (.Platform$OS.type == "windows") {
-    java_home <- gsub("\\\\", "/", java_home)
-  }
+  # Normalize the path for consistency (especially on Windows)
+  java_home <- normalizePath(java_home, winslash = "/", mustWork = FALSE)
 
   lines_to_add <- c(
     "# rJavaEnv begin: Manage JAVA_HOME",
@@ -273,10 +271,11 @@ java_check_version_rjava <- function(
   }
 
   # Get check result (either cached or fresh)
-  cache_key <- Sys.getenv("JAVA_HOME")
+  # Use the effective java_home as cache key (what we're actually checking)
+  effective_java_home <- java_home
 
   if (.use_cache) {
-    data <- ._java_version_check_rjava_impl(java_home, cache_key)
+    data <- ._java_version_check_rjava_impl(java_home, effective_java_home)
   } else {
     # Bypass cache - call the implementation directly
     data <- ._java_version_check_rjava_impl_original(java_home)
@@ -400,10 +399,15 @@ java_check_version_cmd <- function(
   .use_cache = FALSE
 ) {
   # Get data (either cached or fresh)
-  cache_key <- Sys.getenv("JAVA_HOME")
+  # Use the effective java_home as cache key
+  effective_java_home <- if (is.null(java_home)) {
+    Sys.getenv("JAVA_HOME")
+  } else {
+    java_home
+  }
 
   if (.use_cache) {
-    data <- ._java_version_check_impl(java_home, cache_key)
+    data <- ._java_version_check_impl(java_home, effective_java_home)
   } else {
     # Bypass cache - call the implementation directly
     data <- ._java_version_check_impl_original(java_home)

R/java_find.R

@@ -31,20 +31,7 @@
   # 2. PATH Lookup (Resolving Symlinks)
   java_bin <- Sys.which("java")
   if (nzchar(java_bin)) {
-    real_path <- java_bin
-    # Recursive symlink resolution for Linux (handles /etc/alternatives)
-    if (.Platform$OS.type == "unix") {
-      for (i in 1:10) {
-        link <- Sys.readlink(real_path)
-        if (!nzchar(link)) {
-          break
-        }
-        if (!startsWith(link, "/")) {
-          link <- file.path(dirname(real_path), link)
-        }
-        real_path <- link
-      }
-    }
+    real_path <- resolve_symlinks(java_bin)
     # If path ends in /bin/java, the grandparent dir is JAVA_HOME
     if (grepl("[/\\\\]bin[/\\\\]java(\\.exe)?$", real_path)) {
       candidates <- c(candidates, dirname(dirname(real_path)))
@@ -197,13 +184,6 @@
   # Set is_default to FALSE - it will be calculated by the caller
   result_df$is_default <- FALSE
 
-  # 7. Sort by version (descending only, since is_default is all FALSE)
-  sort_order <- order(
-    -as.numeric(result_df$version),
-    decreasing = TRUE
-  )
-  result_df <- result_df[sort_order, ]
-
   rownames(result_df) <- NULL
   return(result_df)
 }
@@ -321,20 +301,7 @@ java_find_system <- function(quiet = TRUE, .use_cache = FALSE) {
     if (is.null(default_java)) {
       java_bin <- Sys.which("java")
       if (nzchar(java_bin)) {
-        real_path <- java_bin
-        # Recursive symlink resolution
-        if (.Platform$OS.type == "unix") {
-          for (i in 1:10) {
-            link <- Sys.readlink(real_path)
-            if (!nzchar(link)) {
-              break
-            }
-            if (!startsWith(link, "/")) {
-              link <- file.path(dirname(real_path), link)
-            }
-            real_path <- link
-          }
-        }
+        real_path <- resolve_symlinks(java_bin)
         # Extract JAVA_HOME from /bin/java path
         if (grepl("[/\\\\]bin[/\\\\]java(\\.exe)?$", real_path)) {
           default_java <- normalizePath(

tests/testthat/test-internal_utilities_symlinks.R

@@ -0,0 +1,103 @@
+test_that("resolve_symlinks returns path as-is on non-Unix or empty path", {
+  # We can't easily mock Windows on Linux/Mac for the OS check itself if it uses .Platform$OS.type
+  # But we can test empty path
+  expect_equal(rJavaEnv:::resolve_symlinks(""), "")
+})
+
+test_that("resolve_symlinks returns original path if it is not a symlink", {
+  skip_on_os("windows")
+
+  tmp <- withr::local_tempdir()
+  target <- file.path(tmp, "target")
+  file.create(target)
+
+  expect_equal(rJavaEnv:::resolve_symlinks(target), target)
+})
+
+test_that("resolve_symlinks resolves simple symlink", {
+  skip_on_os("windows")
+
+  tmp <- withr::local_tempdir()
+  target <- file.path(tmp, "target")
+  file.create(target)
+  link <- file.path(tmp, "link")
+  file.symlink(target, link)
+
+  expect_equal(rJavaEnv:::resolve_symlinks(link), target)
+})
+
+test_that("resolve_symlinks resolves relative symlink", {
+  skip_on_os("windows")
+
+  tmp <- withr::local_tempdir()
+  # Use a subdirectory to ensure we are testing relative path resolution
+  dir.create(file.path(tmp, "subdir"))
+
+  # Create the target file
+  target <- file.path(tmp, "subdir", "target")
+  file.create(target)
+
+  # create a symlink "link1" in subdir that points to "target" (relative)
+  withr::with_dir(file.path(tmp, "subdir"), {
+    file.symlink("target", "link1")
+  })
+
+  link1_abs <- file.path(tmp, "subdir", "link1")
+
+  # verify it is a symlink
+  expect_true(nzchar(Sys.readlink(link1_abs)))
+
+  # resolve it
+  expect_equal(rJavaEnv:::resolve_symlinks(link1_abs), target)
+})
+
+test_that("resolve_symlinks handles nested symlinks", {
+  skip_on_os("windows")
+
+  tmp <- withr::local_tempdir()
+  target <- file.path(tmp, "target")
+  file.create(target)
+
+  link1 <- file.path(tmp, "link1")
+  link2 <- file.path(tmp, "link2")
+  link3 <- file.path(tmp, "link3")
+
+  file.symlink(target, link1)
+  file.symlink(link1, link2)
+  file.symlink(link2, link3)
+
+  expect_equal(rJavaEnv:::resolve_symlinks(link3), target)
+})
+
+test_that("resolve_symlinks respects max_depth", {
+  skip_on_os("windows")
+
+  tmp <- withr::local_tempdir()
+  link1 <- file.path(tmp, "link1")
+  link2 <- file.path(tmp, "link2")
+
+  # Circular reference
+  # link1 -> link2
+  # link2 -> link1
+
+  file.symlink(link2, link1)
+  file.symlink(link1, link2)
+
+  # With default depth (10), it should stop eventually and return one of the links
+  # It shouldn't hang or crash stack.
+
+  res <- rJavaEnv:::resolve_symlinks(link1, max_depth = 5)
+  expect_true(res %in% c(link1, link2))
+})
+
+test_that("resolve_symlinks handles broken links", {
+  skip_on_os("windows")
+
+  tmp <- withr::local_tempdir()
+  target <- file.path(tmp, "missing_target")
+  link <- file.path(tmp, "link")
+  file.symlink(target, link)
+
+  res <- rJavaEnv:::resolve_symlinks(link)
+  expect_equal(res, target)
+})