Skip to content

fix: NPE in OPL calculation during branch creation#41331

Merged
subrata71 merged 2 commits intoreleasefrom
fix/npe-in-opl-on-create-ref
Oct 28, 2025
Merged

fix: NPE in OPL calculation during branch creation#41331
subrata71 merged 2 commits intoreleasefrom
fix/npe-in-opl-on-create-ref

Conversation

@subrata71
Copy link
Collaborator

@subrata71 subrata71 commented Oct 28, 2025

Description

Tip

Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).

Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.

Fixes #Issue Number
or
Fixes Issue URL

Warning

If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.

Automation

/ok-to-test tags="@tag.Sanity"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/18872268996
Commit: 89b384e
Cypress dashboard.
Tags: @tag.Sanity
Spec:


Tue, 28 Oct 2025 11:25:59 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • Bug Fixes
    • Improved stability of on-load scheduling by defensively handling missing executables: inconsistent entries are now logged, skipped or removed to preserve data integrity and avoid failures.

@subrata71 subrata71 self-assigned this Oct 28, 2025
@subrata71 subrata71 added the ok-to-test Required label for CI label Oct 28, 2025
@github-actions github-actions bot added the Bug Something isn't working label Oct 28, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

Walkthrough

Adds null-safety checks when resolving executables from a map during on-load scheduling transformation: missing executables are now logged (with diagnostic details) and skipped/removed to avoid null dereferences and inconsistent scheduling state.

Changes

Cohort / File(s) Summary
Null-safety in scheduling transform
app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java
When mapping executable names to executables and while transforming scheduling orders to DTOs, add null checks; log warnings with the missing name and contextual keys and skip/remove the absent entries to preserve data integrity.

Sequence Diagram(s)

sequenceDiagram
    participant Transformer as SchedulingTransformer
    participant MapStore as ExecutableMap
    participant Logger as Logger

    Transformer->>MapStore: lookup(executableName)
    alt executable found
        MapStore-->>Transformer: executable
        Transformer->>Transformer: add executable to flat list / DTO
    else executable missing
        MapStore-->>Transformer: null
        Transformer->>Logger: warn("missing executable", name, schedulingNames, mapKeys)
        Transformer->>Transformer: skip / remove name from set
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Check the warning message content for needed additional context (IDs, stack, or trace) if helpful.
  • Confirm that skipping/removing the missing entry is acceptable for all callers and won't hide underlying data consistency issues.
  • Ensure the logging level (WARN) and any metrics/alerts are appropriate for repeated occurrences.

Poem

A missing key, a cautious log,
We skip the gap and clear the fog.
No null to stumble, no runtime fray,
The scheduler smiles and moves on its way. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description is largely incomplete and does not meet the template requirements. While the Cypress test results and automation sections are properly filled in, the critical "Fixes #Issue Number" field remains as an unfilled placeholder with no issue reference provided. The description section itself contains only the template guidance text without substantive content—there is no actual description of the changes, motivation, context, dependencies, or relevant documentation links. Without the issue reference and description details, it's difficult to understand the full scope and justification for this change beyond what the code changes and PR title suggest.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "fix: NPE in OPL calculation during branch creation" directly aligns with the changes made in the code. The raw summary shows that null checks were added to OnLoadExecutablesUtilCEImpl.java to prevent NullPointerExceptions when map entries are missing, which matches the "NPE" problem referenced in the title. The title is concise, uses a clear conventional format, and specifically identifies the issue, the component involved (OPL calculation), and the context (branch creation), making it easy for teammates to understand the primary change.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/npe-in-opl-on-create-ref

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

sondermanish
sondermanish previously approved these changes Oct 28, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 15113b4 and 4d12146.

📒 Files selected for processing (1)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: perform-test / server-build / server-unit-tests
  • GitHub Check: perform-test / rts-build / build
  • GitHub Check: perform-test / client-build / client-build
  • GitHub Check: server-unit-tests / server-unit-tests
  • GitHub Check: server-spotless / spotless-check

name,
names,
executableMap.keySet());
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we continue, it can cause us to be inconsistent, users may lose their changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you please elaborate?

@sondermanish sondermanish self-requested a review October 28, 2025 08:23
@subrata71 subrata71 merged commit 7461f8a into release Oct 28, 2025
45 checks passed
@subrata71 subrata71 deleted the fix/npe-in-opl-on-create-ref branch October 28, 2025 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working ok-to-test Required label for CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants