Feature ETP-3003: Enhance call sync

[sebastianbarrozo]

No description provided.

[etendobot]

Suggestion

Consider either implementing the intended asynchronous process invocation logic (following existing Etendo async/background process patterns) or removing this empty class until it is needed, to avoid leaving unused stubs in the codebase.

+public class CallAsyncProcess {
+
+  // TODO: Implement async process invocation logic or remove this class if unused.
+

Rationale: Empty public classes without fields or methods can cause confusion about their purpose, complicate maintenance, and risk becoming dead code. Etendo async/background processing usually follows well-defined patterns (e.g., DAL-based background processes or the asyncprocess module), so helper classes should encapsulate concrete behavior aligned with those patterns.

[etendobot]

[RESOLVED] > [!WARNING]
⚠️ Blocking Issue

uniqueResult() will throw a NonUniqueResultException if more than one process shares the same procedure name. If that can happen due to bad data, it is safer to explicitly validate the result list size and raise a controlled OBException, similar to the previous implementation.

+    final Process process = (Process) processCriteria.uniqueResult();
+
+    if (processCriteria.list().size() > 1) {
+      throw new OBException("More than one process found with procedure name " + processName);
+    }

Rationale: The original code explicitly checked that there was exactly one matching process and threw an OBException otherwise. The new implementation will now surface a Hibernate NonUniqueResultException if there are duplicates, which is less controlled and may not be properly localized or handled. Restoring an explicit size check keeps behavior consistent and avoids unexpected runtime failures.

[etendobot]

Suggestion

Cache the DalConnectionProvider in a local variable and use equalsIgnoreCase for the RDBMS check. This avoids creating multiple provider instances and is more robust to case differences in configuration values.

+    final DalConnectionProvider cp = new DalConnectionProvider(false);
+    final boolean isPostgre = "POSTGRE".equalsIgnoreCase(cp.getRDBMS());

Rationale: Existing Etendo code (for example, SQLC-generated classes and CallStoredProcedure tests) typically caches the ConnectionProvider and performs case-insensitive RDBMS checks. Reusing that pattern avoids potential surprises if configuration values change case and prevents unnecessary object creation in a method that may be called frequently.

[etendobot]

Suggestion

Consider documenting the expected SQL shape for Oracle vs PostgreSQL (CALL vs SELECT ... FROM DUAL) and how it interacts with procedures/functions that return a value. This will help future maintainers understand why buildSqlQuery mixes CALL and SELECT syntaxes and when returnResults should be true.

+    if (isOracle && !returnResults) {

Rationale: The mix of different SQL forms (CALL vs SELECT name(...) AS RESULT FROM DUAL) is subtle and driven by DB-specific behavior. Explicit Javadoc or inline comments reduce the risk of regressions when adding new stored procedures or supporting additional databases.

[etendobot]

Suggestion

Wrap the SQLException in an OBException instead of IllegalStateException so that errors propagate through the standard Etendo exception handling and translation mechanism.

+    } catch (SQLException e) {
+      // Use OBException to integrate with Etendo error handling and translation
+      throw new OBException("Error executing raw process " + procedureName, e);

Rationale: Etendo guidelines recommend using OBException (or subclasses) for business/runtime exceptions that need to be surfaced to the application layer, rather than generic runtime exceptions. This also aligns better with existing DAL and process code which expects OBException for controlled failures.

[etendobot]

Suggestion

Use a multi-line if block instead of a single-line early return to keep style consistent with the rest of the class and typical Etendo formatting conventions.

+    if (parameters == null || parameters.isEmpty()) {
+      return;
+    }

Rationale: While functionally equivalent, multi-line blocks improve readability and reduce the chance of errors when extending the method (e.g., adding logging before the return). Most core Etendo code follows this style.

[etendobot]

Changes requested by agent. Please resolve blocking issues.

[etendobot]

Suggestion

Clarify in the Javadoc that this class is now just a backward-compatible façade over CallProcess#executeRaw, so developers know where the actual implementation lives and which entry point to prefer for new code.

+ * Facade for executing stored procedures via {@link CallProcess#executeRaw}, kept as a backward-compatible entry point.

Rationale: Etendo Wiki explicitly documents CallStoredProcedure and CallProcess as DAL utilities; when refactoring to a façade it helps maintainers to know which is canonical for new development and that behavior is now delegated.

[etendobot]

[RESOLVED] > [!WARNING]
⚠️ Blocking Issue

Before calling uniqueResult(), explicitly check that the criteria returns at most one Process, and if more than one is found, throw an OBException mirroring the original behavior. This prevents a Hibernate NonUniqueResultException and preserves the previous controlled error handling.

+    if (processCriteria.list().size() > 1) {+      throw new OBException("More than one process found with procedure name " + processName);+    }+    final Process process = (Process) processCriteria.uniqueResult();

Rationale: Per the earlier implementation, the logic guaranteed exactly one matching process and raised an OBException otherwise. Using uniqueResult() directly will throw a NonUniqueResultException on duplicates, which is less controlled and may not be localized or handled consistently by callers. Adding a size check maintains compatibility and avoids an unexpected runtime failure in case of data inconsistencies.

[etendobot]

[RESOLVED] > [!WARNING]
⚠️ Blocking Issue

Remove the explicit "CALL" keyword from the Oracle branch and rely on the plain function/procedure call syntax, as the final query already uses "SELECT ... FROM DUAL" for non-returning cases. Using "CALL" together with a SELECT/DUAL wrapper will generate invalid SQL.

+    if (isOracle && !returnResults) {+      sb.append(name);+    } else {+      sb.append("SELECT ").append(name);+    }

Rationale: The new buildSqlQuery method appends either "CALL" or "SELECT" and then, for Oracle and other DBs, may append "AS RESULT FROM DUAL". Combining "CALL" with a SELECT/DUAL suffix (e.g., "CALL MYPROC(...) AS RESULT FROM DUAL") is not valid Oracle syntax and will cause runtime SQL errors. Aligning with the previous CallStoredProcedure patterns where Oracle procedures are invoked without "CALL" avoids this issue.

[etendobot]

Suggestion

Avoid concatenating e.getMessage() into the exception text; just pass the original SQLException as the cause and keep a concise, stable outer message.

+    } catch (SQLException e) {+      throw new IllegalStateException("Error executing raw process " + procedureName, e);

Rationale: Including the inner exception message in the outer message duplicates information exposed by the cause, can lead to overly verbose logs, and makes messages harder to localize or change. Wrapping with a stable message and the original cause is the typical Etendo/Java pattern and keeps logs clean while preserving full stack details.

[etendobot]

Suggestion

Extract the hard‑coded thread pool size into a named constant. This aligns with Etendo patterns for tunable limits and makes future adjustments or externalization easier.

+  private static final int DEFAULT_THREAD_POOL_SIZE = 10;
+
+  private final ExecutorService executorService = Executors
+      .newFixedThreadPool(DEFAULT_THREAD_POOL_SIZE);

Rationale: Magic numbers in concurrency configuration make it harder to reason about resource usage and to tune behavior in different environments. Etendo code typically uses named constants for limits and thresholds.

[etendobot]

[RESOLVED] > [!WARNING]
⚠️ Blocking Issue

Clone the current OBContext before passing it to another thread instead of reusing the same instance reference.

+      final OBContext currentContext = OBContext.getOBContext().clone();

Rationale: OBContext is stored in a ThreadLocal and is not designed to be shared directly across threads. Passing the same instance reference to a different thread can lead to subtle concurrency issues if the context is mutated. Cloning the OBContext to hand off an independent copy is safer and better aligned with the documented ThreadLocal usage.

[etendobot]

[RESOLVED] > [!WARNING]
⚠️ Blocking Issue

Instead of setting the OBContext instance directly, reconstruct the context in the worker thread using the user, role, client, org and language IDs from the original context.

+    OBContext.setOBContext(context.getUser(), context.getRole(), context.getCurrentClient().getId(),
+        context.getCurrentOrganization().getId(), context.getLanguage().getLanguage());

Rationale: The OBContext API and documentation recommend creating the ThreadLocal context via the setOBContext(userId, roleId, clientId, orgId, lang) overload. This ensures correct internal initialization and avoids accidentally reusing internal references across threads.

[etendobot]

[RESOLVED] > [!WARNING]
⚠️ Blocking Issue

Honor the doCommit flag when handling the transaction in the async thread, similar to how DalBaseProcess and other Etendo process infrastructure respect this setting.

+      // D. Commit Transaction
+      // In async threads, we are responsible for the transaction boundary.
+      if (Boolean.TRUE.equals(doCommit)) {
+        OBDal.getInstance().commitAndClose();
+      } else {
+        OBDal.getInstance().getSession().clear();
+      }

Rationale: Ignoring the doCommit flag changes the expected transactional behavior of processes that might deliberately control commit/rollback themselves. Respecting this flag avoids surprising commits and keeps behavior aligned with synchronous CallProcess/DalBaseProcess semantics.

[etendobot]

Suggestion

Consider catching OBException separately or rethrowing it after updating the ProcessInstance, and use a proper logger instead of printing stack traces directly to stdout/stderr.

+    } catch (Exception e) {

Rationale: Etendo guidelines favor using OBException to propagate business errors and using the logging framework (LogManager/Logger) instead of printStackTrace() for error reporting. This improves observability and avoids leaking raw stack traces to console-only logs.

[etendobot]

Suggestion

Expand the one-line if into a multi-line block to align with common Etendo formatting and improve readability.

+          if (msg.length() > 2000) {
+            msg = msg.substring(0, 2000);
+          }

Rationale: Multi-line if blocks with braces are less error-prone when the condition body evolves and better match the predominant style in Etendo core and modules.

[etendobot]

Changes requested by agent. Please resolve blocking issues.

[etendobot]

[RESOLVED] > [!WARNING]
⚠️ Blocking Issue

Clone OBContext before handing it to another thread. Create a cloned context instance from OBContext.getOBContext() and pass that clone into the async runnable instead of the original ThreadLocal instance reference.

+      final OBContext currentContext = OBContext.getOBContext().clone();
+      executorService.submit(() -> runInBackground(pInstanceId, processId, currentContext, doCommit));

Rationale: OBContext is implemented on top of ThreadLocal for per-thread isolation. Sharing the same OBContext instance across threads can cause subtle concurrency and security issues if the context state is mutated. Etendo recommendations for background/async processing emphasize creating an independent context per thread.

[etendobot]

Suggestion

Remove the colon after the parameter name in the @param tags to align with standard Javadoc style used in Etendo (parameter name followed by a space and the description).

+   * @param name the stored procedure name.

Rationale: Etendo’s JavaDoc and general Java conventions expect @param <name> <description> without punctuation after the parameter name, improving consistency and tooling compatibility.

[etendobot]

Suggestion

Drop the colon after parameters in the @param tag for consistency with Etendo Javadoc style.

+   * @param parameters the parameters to pass to the stored procedure.

Rationale: Consistent Javadoc formatting (@param name description) makes documentation easier to read and aligns with the rest of the CallStoredProcedure class and Etendo guidelines.

[etendobot]

Suggestion

Remove the colon after types in the Javadoc @param tag.

+   * @param types the types of the parameters.

Rationale: Using the standard @param name description form keeps the Javadoc consistent with existing Etendo code and avoids small style discrepancies.

[etendobot]

Suggestion

Remove the colon after doFlush in this @param tag for consistency with Etendo’s standard Javadoc format.

+   * @param doFlush whether to flush the session before executing the stored procedure.

Rationale: Standardizing on @param name description improves readability and matches the pattern used in existing CallStoredProcedure Javadoc.

[etendobot]

Suggestion

Drop the colon after returnResults in the Javadoc tag to follow the common @param format.

+   * @param returnResults whether to return the results of the stored procedure.

Rationale: Aligns with the general Java and Etendo Javadoc style, keeping documentation consistent throughout the class.

[etendobot]

Suggestion

(Optional) Ensure the @return description explicitly mentions if null is ever returned; if not, current text is acceptable.

+   * @return the result of the stored procedure.

Rationale: Etendo’s Javadoc guidelines recommend documenting when a method can return null so callers can handle it appropriately. If this method never returns null, you can keep the current wording.

[etendobot]

Suggestion

Consider adding a brief comment explaining why the explicit size check is required before uniqueResult(), so future maintainers understand it is intentionally preserving the old OBException-based behavior instead of relying on Hibernate's NonUniqueResultException.

+    if (processCriteria.list().size() > 1) { // safeguard against NonUniqueResultException and preserve previous controlled behavior

Rationale: The current logic is functionally correct and fixes the previous blocking issue, but the rationale (compatibility with the legacy behavior and avoiding uncontrolled NonUniqueResultException) is non-obvious. A short comment prevents future refactors from inadvertently removing this guard and reintroducing runtime failures.

[etendobot]

Suggestion

Use Boolean.TRUE.equals(parameter) instead of BooleanUtils.toBoolean((Boolean) parameter) here to avoid the unnecessary cast and external dependency, while still mapping true/false to "Y"/"N" safely and clearly.

+      ps.setString(index, Boolean.TRUE.equals(parameter) ? "Y" : "N");

Rationale: The current code works but introduces an extra dependency and a cast that can be avoided. Using Boolean.TRUE.equals(parameter) keeps the semantics clear, avoids potential ClassCastException if this branch is ever reached with a non-Boolean type, and aligns with common patterns in the Etendo core when converting Booleans to Y/N flags.

[etendobot]

Suggestion

Capitalize 'etendo' to 'Etendo'.

 * @author Etendo

Rationale: The author name should be capitalized as 'Etendo' to follow branding and documentation standards.

[etendobot]

Suggestion

Change visibility from 'public' to 'private'.

  private static final String RESULT_SHOULD_BE_DELEGATED_FROM_CALL_PROCESS = "Result should be delegated from CallProcess";

Rationale: Constants used only within the test class should have 'private' visibility to adhere to encapsulation principles unless they are intended to be used by other classes.

[etendobot]

Suggestion

Remove the '@' symbol from the author name.

* @author sebastianbarrozo

Rationale: The @ symbol is typically not used before the username in @author Javadoc tags. Removing it aligns with standard Javadoc conventions.

[etendobot]

Suggestion

Consider using a more standard lazy initialization pattern with AtomicReference to ensure the constructor is only called when absolutely necessary.

CallProcess res = instance.get();
if (res == null) {
  res = new CallProcess();
  if (!instance.compareAndSet(null, res)) {
    res = instance.get();
  }
}
return res;

Rationale: The updateAndGet method with a lambda that performs 'new CallProcess()' may create an instance even if it's not used (if the reference is updated by another thread simultaneously), although it is technically safe. A standard double-checked locking or a more traditional Atomic compareAndSet is often preferred for singleton instantiation to be explicit about creation. However, the current implementation is acceptable for a non-blocking suggestion.

[etendobot]

Warning

⚠️ Blocking Issue

Replace the manual setter-based hydration logic with a single call to 'OBContext.setOBContext' using the IDs captured in 'contextValues'.

  private void hydrateContext(ContextValues contextValues) {
    OBContext.setOBContext(contextValues.userId, contextValues.roleId, contextValues.clientId,
        contextValues.organizationId, contextValues.languageId, contextValues.warehouseId);
  }

Rationale: Manually setting OBContext fields (User, Role, Client, Org) using setters is unsafe in Etendo. This approach bypasses critical internal initialization logic, such as calculating 'readableOrganizations', 'writableOrganizations', and 'entityAccessChecker', which are essential for DAL security and data filtering. The framework provides 'OBContext.setOBContext' overloads to safely initialize a thread-local context from IDs. This specific issue has been flagged in 14 previous reviews (UUIDs: 2721ca5e, c3952fd1, 77221531, 6efd61c9, 17fad40e, 46fd1d9b, 27892098, 66a5195b, cb735f12, cb3ee609, 797499cd, f48f6c0b, 09b05fa8, 3adfceef) and remains unfixed.

[etendobot]

Suggestion

The note is helpful; no change needed.

 * New code should prefer using {@link CallProcess} directly.

Rationale: This is a good documentation update to guide developers towards modern framework APIs (CallProcess) instead of legacy ones.

[etendobot]

Suggestion

Maintain as is.

 * @author etendo

Rationale: Adding the organization author is standard when modifying core files in the context of Etendo.

[etendobot]

Changes requested by agent. Please resolve blocking issues.

[etendobot]

Suggestion

Remove the leading '@' from the username in the @author tag.

 * @author sebastianbarrozo

Rationale: The @ symbol in '@sebastianbarrozo' is redundant when used with the @author JavaDoc tag and does not follow standard JavaDoc conventions.

[etendobot]

Suggestion

Consider using a more efficient singleton initialization pattern to avoid potential multiple instantiations of CallProcess during contention.

  public static CallProcess getInstance() {
    CallProcess res = instance.get();
    if (res == null) {
      instance.compareAndSet(null, new CallProcess());
      res = instance.get();
    }
    return res;
  }

Rationale: Using updateAndGet with a lambda can lead to unnecessary object instantiation if multiple threads call getInstance() simultaneously, as the lambda might be executed multiple times even if only one result is stored. A double-checked locking pattern or simple compareAndSet is more efficient for singletons.

[etendobot]

Suggestion

Change the visibility of 'TEST_PROCEDURE' from public to private if it is not intended for use in other classes.

  private static final String TEST_PROCEDURE = "test_procedure";

Rationale: The constant is only used within this test class. Reducing visibility to private follows the principle of least privilege and keeps the API clean.

[etendobot]

Suggestion

Capitalize 'Etendo' in the @author tag.

 * @author Etendo

Rationale: Standardizing the author tag name ensures consistency across the codebase.

[etendobot]

Suggestion

The addition of the recommendation to use CallProcess is correct as it aligns with modern Etendo development patterns.

 * New code should prefer using {@link CallProcess} directly.

Rationale: Providing guidance on modern alternatives (CallProcess) within legacy classes is a recommended practice in Etendo to phase out older patterns.

[etendobot]

Suggestion

Correctly added etendo as an author.

 * @author etendo

Rationale: Updating authorship to reflect Etendo's maintenance or contribution is standard for modules or core classes modified within the Etendo ecosystem.

[etendobot]

Changes requested by agent. Please resolve blocking issues.

[etendobot]

Suggestion

The Javadoc improvement is good practice.

 * Unit tests for the {@link CallStoredProcedure} class.

Rationale: The addition of descriptive Javadoc improves maintainability and explains the purpose of the test suite.

[etendobot]

Suggestion

Extracting strings to constants is a good refactoring.

  public static final String TEST_PROCEDURE = "test_procedure";

Rationale: Using constants instead of magic strings in tests improves readability and makes the test suite easier to maintain.

[etendobot]

Suggestion

Good documentation of the test lifecycle.

   * Sets up the test environment by mocking the {@link CallProcess} singleton.

Rationale: Documentation for setup and teardown methods helps other developers understand how the test lifecycle is managed.

[etendobot]

Warning

⚠️ Blocking Issue

Replace the manual setter-based hydration logic in 'hydrateContext' with a single call to 'OBContext.setOBContext(...)' using the IDs stored in 'contextValues'.

  private void hydrateContext(ContextValues contextValues) {
    OBContext.setOBContext(contextValues.userId, contextValues.roleId, contextValues.clientId,
        contextValues.organizationId, contextValues.languageId, contextValues.warehouseId);
  }

Rationale: Manually setting OBContext fields (User, Role, Client, Org) using setters in a new thread is unsafe. This approach bypasses critical internal initialization logic, such as calculating 'readableOrganizations', 'writableOrganizations', and 'entityAccessChecker', which are essential for DAL security and data filtering. The framework provides 'OBContext.setOBContext' overloads to safely initialize a thread-local context from IDs. This issue has been flagged in 15 previous reviews (UUIDs: 2721ca5e, c3952fd1, 77221531, 6efd61c9, 17fad40e, 46fd1d9b, 27892098, 66a5195b, cb735f12, cb3ee609, 797499cd, f48f6c0b, 09b05fa8, 3adfceef, 1b3a8be4) and remains unfixed.

[etendobot]

Suggestion

Change the logger to be 'private static final' and rename it to 'log' to follow standard patterns.

  private static final Logger log = LogManager.getLogger(CallAsyncProcess.class);

Rationale: Standard Etendo/Java practice uses 'private static final' for loggers, typically named 'log' or 'logger'. Making it 'public' and non-final is unnecessary and breaks encapsulation.

[etendobot]

Suggestion

The addition of the preference note is correct and helpful for developers.

* New code should prefer using {@link CallProcess} directly.

Rationale: Providing guidance on preferred classes (CallProcess over CallStoredProcedure) is a documented best practice for DAL-based operations in Etendo.

[etendobot]

Suggestion

The import is correct for the new thread-safe implementation.

import java.util.concurrent.atomic.AtomicReference;

Rationale: Added support for thread-safe singleton management using AtomicReference.

[etendobot]

Suggestion

Remove the '@' prefix from the author name in the Javadoc.

 * @author sebastianbarrozo

Rationale: Standard Javadoc @author tags usually do not include the '@' symbol before the username.

[etendobot]

Suggestion

Consider using a standard lazy-initialization pattern to minimize object creation during contention.

  public static CallProcess getInstance() {
    CallProcess res = instance.get();
    if (res == null) {
      res = new CallProcess();
      if (!instance.compareAndSet(null, res)) {
        res = instance.get();
      }
    }
    return res;
  }

Rationale: While updateAndGet is thread-safe, it might execute the 'new CallProcess()' multiple times in a high-concurrency race before discarding the extra instances. A double-checked locking or compareAndSet approach is more conventional for singletons to avoid unnecessary object allocation.

[etendobot]

Suggestion

Reformat Javadoc parameters for better consistency with the existing codebase.

   * @param processName the procedure name defined in AD_Process.
   * @param recordID the record ID associated with the execution (optional).
   * @param parameters a map of parameters to be injected into AD_PInstance_Para.
   * @param doCommit explicit commit flag (if supported by the SP).
   * @return the result ProcessInstance.

Rationale: Standard Etendo/Openbravo Javadoc typically keeps the description on the same line as the tag or follows a more compact indentation style.

[etendobot]

Changes requested by agent. Please resolve blocking issues.