MX-230: Improve the CodeRabbit yml file for Spring boot#114
MX-230: Improve the CodeRabbit yml file for Spring boot#114ansh-varshney wants to merge 2 commits intoopenMF:developfrom
Conversation
- Switch profile from 'chill' to 'assertive' to align with request_changes_workflow enforcement - Ban @Autowired on fields explicitly; require constructor injection - Add explicit forbidden import list for domain layer (Spring, JPA, Hibernate, Jersey, Jackson, Swagger namespaces) - Define standard SS-XXXX error response schema in API layer rules - Add dedicated sections for handler (CQRS), Liquibase migrations, and split pom.xml / README.md into separate path rules - Consolidate cross-cutting rules (logging ban, secrets, magic numbers) into global section to eliminate duplication - Enforce @transactional(readOnly=true) on read-only service methods - Add finishing_touches: true for auto Javadoc/test suggestions
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSwitched Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yaml:
- Line 21: The config sets finishing_touches as a boolean but the schema
requires an object; replace the boolean value for the finishing_touches key with
an object containing the nested properties (docstrings, unit_tests, simplify,
custom) and populate each with the appropriate type/values (e.g., booleans or
sub-objects per your schema) so the config validates—look for the
finishing_touches key and ensure it becomes an object with the docstrings,
unit_tests, simplify, and custom fields instead of true/false.
- Around line 186-220: The domain classes AppSelfServiceUserClientMapping,
AppSelfServiceUser, SelfServiceRegistration, and SelfBeneficiariesTPT currently
contain prohibited JPA annotations/imports; remove all jakarta.persistence (and
javax.persistence) imports and annotations from these domain classes, preserve
only pure domain logic (constructors/factories, invariants, value-object
immutability, equals/hashCode based on identity) and move all persistence
mapping into new or existing infrastructure-layer JPA entity or mapper classes
(e.g., create corresponding JPA entity DTOs and mapping methods or use a
dedicated infrastructure mapper to translate between the domain objects and
persistence entities), and ensure repository types in the domain remain
interfaces only (implementations live in infrastructure).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ddd67ed-a485-486f-839b-6f14e274db1c
📒 Files selected for processing (1)
.coderabbit.yaml
.coderabbit.yaml
Outdated
| # ── DOMAIN LAYER — ZERO FRAMEWORK TOLERANCE ─────────────────────────────── | ||
| - path: 'src/main/java/**/domain/**/*.java' | ||
| instructions: | | ||
| ## Domain Layer | ||
| - Contains entities, value objects, and domain events only. | ||
| - No dependencies on Spring, frameworks, or infrastructure. | ||
| - Entities should encapsulate behavior and invariants. | ||
| - Use immutable patterns for Value Objects (explicit constructors, final fields). Lombok `@Value` may be used as it's compile-time only. | ||
| - Domain events should be POJOs with clear purpose. | ||
| - Domain services should express business logic that doesn’t belong to entities. | ||
|
|
||
| - path: 'src/main/java/**/infrastructure/**/*.java' | ||
| ## Domain Layer — Framework Independence is MANDATORY | ||
|
|
||
| ### Forbidden Imports — Flag Every Occurrence | ||
| The following import namespaces must NEVER appear in the domain layer. | ||
| Flag immediately if any of the following prefixes are found in an import statement: | ||
|
|
||
| - `org.springframework.` — No Spring annotations, beans, context, or utilities. | ||
| - `jakarta.persistence.` — No JPA annotations (`@Entity`, `@Table`, `@Column`, `@Id`, `@ManyToOne`, etc.). | ||
| - `javax.persistence.` — Legacy JPA namespace, also banned. | ||
| - `org.hibernate.` — No Hibernate-specific annotations or types. | ||
| - `jakarta.ws.rs.` — No JAX-RS (Jersey) web annotations. | ||
| - `javax.ws.rs.` — Legacy JAX-RS, also banned. | ||
| - `jakarta.servlet.` — No Servlet API. | ||
| - `com.fasterxml.jackson.` — No Jackson annotations on domain objects. | ||
| - `io.swagger.` — No Swagger/OpenAPI annotations on domain objects. | ||
|
|
||
| ### What Belongs Here | ||
| - **Entities**: model business concepts with behaviour, not just data. | ||
| Entities must protect their own invariants in constructors or factory methods. | ||
| - **Value Objects**: immutable, all fields `final`, no setters. | ||
| Lombok `@Value` is acceptable (compile-time only, no runtime Spring dependency). | ||
| - **Domain Events**: plain POJOs named after what happened (e.g., `LoanRepaymentMadeEvent`). | ||
| - **Repository interfaces**: interfaces only — zero implementation code. | ||
| Implementations live in the infrastructure or starter layer. | ||
| - **Domain Services**: stateless classes for business logic that spans multiple entities. | ||
|
|
||
| ### Invariants & Behaviour | ||
| - Avoid anemic domain models (entities with only getters/setters and no behaviour). | ||
| Flag any entity class where all methods are trivial accessors with no business logic. | ||
| - Do not expose setters for fields that must not change after construction — use private setters or none. | ||
| - `equals` and `hashCode` on entities must be based solely on the persistent identity (primary key). | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if existing domain layer classes use JPA annotations
rg -l 'jakarta\.persistence\.' --glob '**/domain/**/*.java' || echo "No JPA imports found in domain layer"Repository: openMF/selfservice-plugin
Length of output: 455
Domain layer contains prohibited JPA annotations that violate framework independence rules.
The .coderabbit.yaml configuration explicitly forbids jakarta.persistence.* imports in domain classes. The following files violate this rule:
src/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUserClientMapping.javasrc/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUser.javasrc/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRegistration.javasrc/main/java/org/apache/fineract/selfservice/account/domain/SelfBeneficiariesTPT.java
Remove JPA annotations from these domain classes and relocate persistence mapping logic to the infrastructure layer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.coderabbit.yaml around lines 186 - 220, The domain classes
AppSelfServiceUserClientMapping, AppSelfServiceUser, SelfServiceRegistration,
and SelfBeneficiariesTPT currently contain prohibited JPA annotations/imports;
remove all jakarta.persistence (and javax.persistence) imports and annotations
from these domain classes, preserve only pure domain logic
(constructors/factories, invariants, value-object immutability, equals/hashCode
based on identity) and move all persistence mapping into new or existing
infrastructure-layer JPA entity or mapper classes (e.g., create corresponding
JPA entity DTOs and mapping methods or use a dedicated infrastructure mapper to
translate between the domain objects and persistence entities), and ensure
repository types in the domain remain interfaces only (implementations live in
infrastructure).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.coderabbit.yaml (2)
380-381: Broken link detection is a manual review instruction.CodeRabbit does not automatically verify URL availability or file path existence. This instruction will require manual reviewer verification or integration of a separate link-checking tool in the CI pipeline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.coderabbit.yaml around lines 380 - 381, The rule "Must not contain broken links" in .coderabbit.yaml is currently a manual-only instruction and cannot be enforced automatically; update the configuration or workflow to either (A) mark this rule explicitly as "manual review required" in the rule metadata and clarify the reviewer action, or (B) integrate an automated link-checking step into CI (e.g., add a job that runs a tool like markdown-link-check, linkinator, or a custom script) that validates HTTP 4xx/5xx responses and repo file path existence, then reference that job in the policy so the rule is enforced automatically during PRs.
89-94: Dependency injection rules will flag existing codebase patterns.The rule on line 93 correctly states that
@Autowiredon constructors is redundant since Spring 4.3. However, existing code (e.g.,SelfSavingsDataValidator) uses this pattern and will be flagged. Additionally,SelfServiceSecurityConfigurationhas 14+ field-level@Autowiredannotations that violate the ban on line 90.Consider whether a migration plan or tracking issue should accompany this enforcement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.coderabbit.yaml around lines 89 - 94, The rule flags field-level and constructor-level `@Autowired/`@Inject usages; update offending classes (e.g., SelfServiceSecurityConfiguration and SelfSavingsDataValidator) to use constructor injection: remove `@Autowired/`@Inject from fields and constructors, make injected dependencies final, and either add an explicit constructor or annotate the class with Lombok's `@RequiredArgsConstructor` to generate it; ensure all former field injections become constructor parameters and delete redundant `@Autowired` on constructors so the code conforms to the STRICT DI policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.coderabbit.yaml:
- Around line 380-381: The rule "Must not contain broken links" in
.coderabbit.yaml is currently a manual-only instruction and cannot be enforced
automatically; update the configuration or workflow to either (A) mark this rule
explicitly as "manual review required" in the rule metadata and clarify the
reviewer action, or (B) integrate an automated link-checking step into CI (e.g.,
add a job that runs a tool like markdown-link-check, linkinator, or a custom
script) that validates HTTP 4xx/5xx responses and repo file path existence, then
reference that job in the policy so the rule is enforced automatically during
PRs.
- Around line 89-94: The rule flags field-level and constructor-level
`@Autowired/`@Inject usages; update offending classes (e.g.,
SelfServiceSecurityConfiguration and SelfSavingsDataValidator) to use
constructor injection: remove `@Autowired/`@Inject from fields and constructors,
make injected dependencies final, and either add an explicit constructor or
annotate the class with Lombok's `@RequiredArgsConstructor` to generate it; ensure
all former field injections become constructor parameters and delete redundant
`@Autowired` on constructors so the code conforms to the STRICT DI policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c51be6b0-d271-43bd-8a1b-8f7570493795
📒 Files selected for processing (1)
.coderabbit.yaml
ansh-varshney
force-pushed
the
MX230-improve-coderabbit-config-rules
branch
from
3fcf4f0 to
ef84bcd
Compare
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
I am setting this "Request changes" until we fix the tech debt. Please feel free to add your comments!!
2 participants
Fixes: MX-230
Summary by CodeRabbit