Skip to content

Conversation

@SamuelPalaj
Copy link
Contributor

@SamuelPalaj SamuelPalaj commented Dec 8, 2025

Description

  • changed bulkReindex endpoint to accept IndexParams as RequestBody instead of request param

Implements NAE-2265

How Has Been This Tested?

manually

Test Configuration

Test Configuration

Name Tested on
OS windows 10
Runtime java 21
Dependency Manager Maven 3.9.9
Framework version Spring Boot 3.4.4
Run parameters
Other configuration

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes have been checked, personally or remotely, with @...
  • I have commented my code, particularly in hard-to-understand areas
  • I have resolved all conflicts with the target branch of the PR
  • I have updated and synced my code with the target branch
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes:
    • Lint test
    • Unit tests
    • Integration tests
  • I have checked my contribution with code analysis tools:
  • I have made corresponding changes to the documentation:
    • Developer documentation
    • User Guides
    • Migration Guides

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced error handling in the bulk reindex endpoint to gracefully capture and report errors instead of potentially causing unhandled exceptions.

✏️ Tip: You can customize this high-level summary in your review settings.

- changed bulkReindex endpoint to accept IndexParams as RequestBody instead of request param
@coderabbitai
Copy link

coderabbitai bot commented Dec 8, 2025

Walkthrough

The bulkReindex endpoint in ElasticController was modified to explicitly bind its IndexParams parameter from the request body using the @RequestBody annotation, and error handling was added via a catch block to handle and report exceptions during reindexing operations.

Changes

Cohort / File(s) Summary
Request binding and error handling
application-engine/src/main/java/com/netgrif/application/engine/elastic/web/ElasticController.java
Added @RequestBody annotation to bulkReindex method parameter; added exception catch block for error reporting in bulkReindex method

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Single file modification with a straightforward annotation addition and basic exception handling wrapper
  • No complex logic or multi-file interactions to evaluate

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly references the issue ticket (NAE-2265) and indicates the specific change: enabling force reindex for workers, which aligns with the endpoint modification.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 46e49ec and 976b0d9.

📒 Files selected for processing (1)
  • application-engine/src/main/java/com/netgrif/application/engine/elastic/web/ElasticController.java (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java:145-149
Timestamp: 2025-11-07T13:11:20.622Z
Learning: In the netgrif/application-engine project, failed Elasticsearch bulk operations in ElasticQueueManager should not be retried or re-queued within the flush() method. The system relies on scheduled reindexing and manual reindex endpoints as fallback mechanisms for failed operations, making in-method retries unnecessary and potentially problematic (could cause deadlocks, capacity issues, or infinite loops with bad data).
📚 Learning: 2025-11-07T13:11:20.622Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java:145-149
Timestamp: 2025-11-07T13:11:20.622Z
Learning: In the netgrif/application-engine project, failed Elasticsearch bulk operations in ElasticQueueManager should not be retried or re-queued within the flush() method. The system relies on scheduled reindexing and manual reindex endpoints as fallback mechanisms for failed operations, making in-method retries unnecessary and potentially problematic (could cause deadlocks, capacity issues, or infinite loops with bad data).

Applied to files:

  • application-engine/src/main/java/com/netgrif/application/engine/elastic/web/ElasticController.java
⏰ 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). (6)
  • GitHub Check: task-list-completed
  • GitHub Check: task-list-completed
  • GitHub Check: task-list-completed
  • GitHub Check: task-list-completed
  • GitHub Check: task-list-completed
  • GitHub Check: Build

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.

@Retoocs Retoocs self-requested a review December 8, 2025 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants