ITs and archetype updates for more audits#17
Merged
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (31)
📝 WalkthroughWalkthroughThis PR adds nine new database audits (catalog, JPA, and runtime families) with corresponding assertion classes, exclusion configuration, and suite wiring. It extends FixRenderer with remediation guidance for the new finding types, adds archetype integration test templates with entity version fields, updates documentation, and bumps a dependency version. ChangesNew Audit Assertions and Integration
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant DatabaseAuditSuite
participant AuditAssertion
participant AuditScope
participant Audit
DatabaseAuditSuite->>AuditAssertion: construct with audit + platform
AuditScope->>AuditAssertion: assertClean(scope)
AuditAssertion->>AuditScope: excludes()
AuditAssertion->>Audit: audit(schema/params, excludedSet)
Audit-->>AuditAssertion: violations
AuditAssertion->>AuditAssertion: failOnViolations(message)
Possibly related PRs
Suggested labels: enhancement, documentation Suggested reviewers: database-audits maintainers familiar with audit assertion internals and archetype templates 🐰 A rabbit hopped through catalogs deep, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
PR Summary by QodoWire nine new audits into integration suite, archetype ITs, and fix rendering
AI Description
Diagram
High-Level Assessment
Files changed (30)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
35 rules 1. Test method name lacks underscores
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+8
to
+11
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.beans.factory.annotation.Value; | ||
|
|
There was a problem hiding this comment.
1. Spring imports in archetype its 📘 Rule violation § Compliance
New archetype-generated integration test templates import Spring types (e.g., @Autowired, @Value), which violates the rule restricting archetype-generated code imports to only JUnit, java.*/javax.*, and database-audits namespaces. This increases third-party coupling in generated projects and breaks the stated dependency constraints.
Agent Prompt
## Issue description
Archetype-generated test templates import Spring classes (`org.springframework.*`), but the compliance rule allows only JUnit + database-audits (plus `java.*`/`javax.*`) in generated classes.
## Issue Context
This affects newly-added archetype test templates and makes generated projects depend on disallowed third-party packages.
## Fix Focus Areas
- archetype/src/main/resources/archetype-resources/src/test/java/catalog/DuplicateForeignKeyAuditIT.java[8-11]
- archetype/src/main/resources/archetype-resources/src/test/java/runtime/OffsetPaginationAuditIT.java[9-11]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| #if($disabledTests == 'true') | ||
| @Disabled("Generated as disabled; remove @Disabled to enable") | ||
| #end | ||
| void testNoDuplicateForeignKeyConstraints() { |
There was a problem hiding this comment.
2. Test method name lacks underscores 📘 Rule violation ▣ Testability
New test methods (e.g., testNoDuplicateForeignKeyConstraints) do not match the required naming pattern with exactly two underscores separating state and outcome segments. This violates the enforced descriptive test method naming convention.
Agent Prompt
## Issue description
New test method names do not follow the required pattern `^test[A-Z][A-Za-z0-9]*_[A-Za-z0-9]+_[A-Za-z0-9]+$`.
## Issue Context
The archetype adds multiple new `*AuditIT` templates whose test methods are named without the required state/outcome underscore segments.
## Fix Focus Areas
- archetype/src/main/resources/archetype-resources/src/test/java/catalog/DuplicateForeignKeyAuditIT.java[42-44]
- archetype/src/main/resources/archetype-resources/src/test/java/runtime/OffsetPaginationAuditIT.java[40-42]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+383
to
+396
| /** | ||
| * Sets the minimum capture count (inclusive) for a SELECT shape to be | ||
| * reported by the repeated-statement (N+1) audit; must be at least 2. | ||
| * Defaults to {@value #DEFAULT_REPEATED_STATEMENT_THRESHOLD}, a | ||
| * generous regression tripwire rather than a precise count. | ||
| * | ||
| * @param threshold | ||
| * the minimum capture count. | ||
| * @return this builder. | ||
| */ | ||
| public Builder repeatedStatementThreshold(final int threshold) { | ||
| this.repeatedStatementThreshold = threshold; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
4. Unvalidated repeated threshold 🐞 Bug ☼ Reliability
DatabaseAuditExcludes.Builder.repeatedStatementThreshold() and RepeatedStatementAuditAssertion document that the threshold “must be at least 2” but do not enforce it. This allows invalid configuration (0/1) that violates the API contract and can make the audit behavior nonsensical/noisy.
Agent Prompt
### Issue description
The public API states `repeatedStatementThreshold` must be at least 2, but both the excludes builder and the assertion entry points accept any integer. This creates a contract mismatch and allows misconfiguration.
### Issue Context
- The Javadoc for both the excludes builder and the assertion methods explicitly says the threshold must be >= 2.
- The current implementation simply assigns/forwards the value.
### Fix Focus Areas
- integration/src/main/java/io/github/databaseaudits/spring/boot/assertion/DatabaseAuditExcludes.java[383-396]
- integration/src/main/java/io/github/databaseaudits/spring/boot/assertion/RepeatedStatementAuditAssertion.java[36-65]
### Acceptance criteria
- Reject thresholds < 2 with a clear `IllegalArgumentException` (or clamp to 2, but then update docs accordingly).
- Add a small unit test (new test class is fine) proving the validation behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@integration/src/main/java/io/github/databaseaudits/spring/boot/assertion/DatabaseAuditExcludes.java`:
- Around line 383-396: The repeatedStatementThreshold setter in Builder
currently accepts any int even though its Javadoc says the minimum is 2; update
this method to validate the input and fail fast for values below 2 with a clear
error message. Keep the change localized to repeatedStatementThreshold in
DatabaseAuditExcludes.Builder, and preserve the existing assignment/return flow
for valid values.
In
`@integration/src/main/java/io/github/databaseaudits/spring/boot/report/FixRenderer.java`:
- Around line 100-101: The PRECISE fix for DuplicateForeignKeyFinding in
FixRenderer currently always uses dropDuplicateConstraintsDdl(), which emits
DROP CONSTRAINT even for MariaDB. Update FixRenderer and
dropDuplicateConstraintsDdl() to accept platform context and branch by database
family so MySQL 8.0.19+ keeps DROP CONSTRAINT while MariaDB emits DROP FOREIGN
KEY. Use the existing platform value passed into the renderer and the
DuplicateForeignKeyFinding case to route to the correct DDL.
🪄 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 Plus
Run ID: 0606402a-7207-47fc-a35f-b9e23842fdb3
📒 Files selected for processing (30)
archetype/src/main/resources/META-INF/archetype-post-generate.groovyarchetype/src/main/resources/archetype-resources/src/test/java/app/Child.javaarchetype/src/main/resources/archetype-resources/src/test/java/app/Parent.javaarchetype/src/main/resources/archetype-resources/src/test/java/catalog/DuplicateForeignKeyAuditIT.javaarchetype/src/main/resources/archetype-resources/src/test/java/catalog/PrimaryKeyTypeAuditIT.javaarchetype/src/main/resources/archetype-resources/src/test/java/catalog/UniqueIndexNotNullAuditIT.javaarchetype/src/main/resources/archetype-resources/src/test/java/jpa/EagerCollectionFetchAuditIT.javaarchetype/src/main/resources/archetype-resources/src/test/java/jpa/MissingVersionAttributeAuditIT.javaarchetype/src/main/resources/archetype-resources/src/test/java/jpa/UnmappedDatabaseObjectAuditIT.javaarchetype/src/main/resources/archetype-resources/src/test/java/runtime/OffsetPaginationAuditIT.javaarchetype/src/main/resources/archetype-resources/src/test/java/runtime/RepeatedStatementAuditIT.javaarchetype/src/main/resources/archetype-resources/src/test/java/runtime/UnusedIndexAuditIT.javaarchetype/src/main/resources/archetype-resources/src/test/resources/db/changelog/db.changelog-master.xmlintegration/pom.xmlintegration/src/main/java/io/github/databaseaudits/spring/boot/DatabaseAuditSuite.javaintegration/src/main/java/io/github/databaseaudits/spring/boot/assertion/DatabaseAuditExcludes.javaintegration/src/main/java/io/github/databaseaudits/spring/boot/assertion/DuplicateForeignKeyAuditAssertion.javaintegration/src/main/java/io/github/databaseaudits/spring/boot/assertion/EagerCollectionFetchAuditAssertion.javaintegration/src/main/java/io/github/databaseaudits/spring/boot/assertion/MissingVersionAttributeAuditAssertion.javaintegration/src/main/java/io/github/databaseaudits/spring/boot/assertion/OffsetPaginationAuditAssertion.javaintegration/src/main/java/io/github/databaseaudits/spring/boot/assertion/PrimaryKeyTypeAuditAssertion.javaintegration/src/main/java/io/github/databaseaudits/spring/boot/assertion/RepeatedStatementAuditAssertion.javaintegration/src/main/java/io/github/databaseaudits/spring/boot/assertion/UniqueIndexNotNullAuditAssertion.javaintegration/src/main/java/io/github/databaseaudits/spring/boot/assertion/UnmappedDatabaseObjectAuditAssertion.javaintegration/src/main/java/io/github/databaseaudits/spring/boot/assertion/UnusedIndexAuditAssertion.javaintegration/src/main/java/io/github/databaseaudits/spring/boot/report/FixRenderer.javaintegration/src/site/asciidoc/audits.adocintegration/src/site/asciidoc/exclusions.adocintegration/src/site/asciidoc/usage.adocintegration/src/test/java/io/github/databaseaudits/spring/boot/report/FixRendererTest.java
jeffjensen
force-pushed
the
feat/more-audits
branch
from
a48aa98 to
554a0ee
Compare
…g types Finding is sealed, so FixRenderer's two exhaustive switches (fixFor, liquibaseChangeSet) failed to compile the moment core's SNAPSHOT carried DuplicateForeignKeyFinding, NarrowPrimaryKeyFinding, UniqueIndexNullableColumnFinding, OffsetPaginationFinding, RepeatedStatementFinding, MissingVersionAttributeFinding, EagerCollectionFetchFinding, UnmappedTableFinding/UnmappedColumnFinding, and UnusedIndexFinding - the intended signal to add a case for each. Only DuplicateForeignKeyFinding is PRECISE (drop every duplicate constraint but the first, via a real dropForeignKeyConstraint changeset); NarrowPrimaryKeyFinding and UnusedIndexFinding are TEMPLATE (concrete DDL but a human must complete a TODO first - widen referencing FKs, or confirm against production pg_stat_user_indexes); the rest are ADVISORY (a mapping or code change, no DDL to emit). Bundled with this since one audit alone can't compile against the sealed type - the whole roster of new findings had to land together.
Adds a *AuditAssertion for each new core audit (DuplicateForeignKey, PrimaryKeyType, UniqueIndexNotNull in the catalog family; EagerCollectionFetch, MissingVersionAttribute, UnmappedDatabaseObject in the JPA family; OffsetPagination, RepeatedStatement, UnusedIndex in the runtime family - the last one PostgreSQL-only alongside Join/OrderBy/WhereClause) and wires each into DatabaseAuditSuite's roster in family order, plus a matching DatabaseAuditExcludes field/builder method for each (RepeatedStatementAudit also gets a repeatedStatementThreshold, defaulting to a generous 50 as a regression tripwire rather than a precise N+1 count). DatabaseAuditSuite now also constructs a ForeignKeyCatalog collaborator alongside the existing IndexCatalog, needed by DuplicateForeignKeyAudit and UnusedIndexAudit. The roster's List.of(...) needed an explicit <AuditAssertion> type witness - without it javac's common-supertype inference across nine assertion types picked the package-private AbstractAuditAssertion instead of the public AuditAssertion interface, which fails to compile from a different package. The roster-guard test (testSuiteAll_WiresAnAssertionForEveryCoreAudit) now passes again: every core *Audit has a wired *AuditAssertion.
One <Audit>AuditIT template per new audit, under its family directory (catalog/jpa/runtime), following the existing pattern: extends AbstractDatabaseAuditIT (or the user's parentClass), @Autowired the assertion bean, and demonstrate the exclusion constant. UnusedIndexAuditIT is plan-based (PostgreSQL-only), so it joins Join/OrderBy/WhereClause in archetype-post-generate.groovy's deletion list for non-PostgreSQL engines. Fixing the demo schema to pass all three new catalog/JPA audits cleanly surfaced two real gaps: * Liquibase's own bookkeeping tables (databasechangelog, databasechangeloglock) are unmapped by design, and databasechangeloglock's PK is a narrow INT - both genuine findings PrimaryKeyTypeAuditIT and UnmappedDatabaseObjectAuditIT now exclude, the same way PrimaryKeyPresenceAuditAssertion already special-cases them for the presence audit. * The demo's Parent/Child entities had no @Version attribute. Rather than excluding them, added a genuine version column (Liquibase + entity + getter) to both, since a versioned entity is worth demonstrating rather than suppressing - Spring Data's save() handles it transparently, so RepositoryWorkloadIT needed no change. Full reactor `clean install` (which drives archetype:integration-test against Testcontainers PostgreSQL/MySQL) is green: 20/20 in the full and targeted-datasource projects, 16/16 in full-mysql.
Adds each new audit's row to audits.adoc's per-family tables (DuplicateForeignKey/PrimaryKeyType/UniqueIndexNotNull in catalog; EagerCollectionFetch/MissingVersionAttribute/UnmappedDatabaseObject in JPA, renaming the "JPA audit" heading to plural; OffsetPagination/ RepeatedStatement/UnusedIndex in runtime, splitting the "token-scan" heading to plural and fixing usage.adoc's stale plan-based/token-scan audit lists) and each new DatabaseAuditExcludes builder method to exclusions.adoc's per-family reference tables, noting that PrimaryKeyTypeAuditAssertion and UnmappedDatabaseObjectAuditAssertion - unlike PrimaryKeyPresenceAuditAssertion - do not auto-exclude the Liquibase bookkeeping tables.
jeffjensen
force-pushed
the
feat/more-audits
branch
from
554a0ee to
e5b3b92
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation