feat(persistence): set up JPA,Flyway and integration test with Testcontainers#21
feat(persistence): set up JPA,Flyway and integration test with Testcontainers#21Arzu-N wants to merge 42 commits into
Conversation
…credentials in persistence test
…r runtime and test
|
I have successfully resolved all the CI pipeline issues (including Checkstyle, Spotless formatting, and database integration tests). All checks are now passing successfully! Regarding the requirements in the issue:
The PR is now fully ready for your review and approval. Thank you! |
martian56
left a comment
There was a problem hiding this comment.
Good news first: the Testcontainers setup is the right approach and mostly textbook. DataJpaTest with replace NONE, dynamic properties from the container, Flyway running the real migration, validate mode catching drift. The migration applies cleanly and matches the entities, and you worked the CI failures down yourself. That part of #5 is in good shape.
The rest of the PR has problems, and one of them is serious.
The api can no longer start. I built this branch and ran the api without a database: it crashes on boot because Flyway cannot reach localhost:5432, connection refused, exit code 1. The excludes you removed were guarding exactly this, and the hardcoded datasource (a test database name, no credentials) is not something the api can ship. The api test only stays green because it excludes the DB autoconfigurations, which means it boots a different context than the real app and hides the regression. #5 is scoped to libs/persistence, so please revert all three apps/api changes (main class, yml, test). Wiring the api to a real datasource comes after #4 gives us a local Postgres to point at.
Also in the must-fix list:
- Three stray empty files got committed at the backend root:
Task,There, andinitializationError. They look like shell redirection accidents. Please remove them. gradle.propertiesnow hardcodes DOCKER_HOST to a unix socket. That is your machine's setup, not the project's. Details inline.- The persistence tests must not depend on
:apps:api. Details inline. - The root build script now hardcodes the Boot version in a second place. Details inline.
And one to discuss rather than just fix: Lombok arrived in this PR without being raised. A dependency that shapes how everyone writes Java should be a deliberate team decision, not a side effect of one PR. See the inline comment.
Smaller items are inline too. One last thing: the PR title says Flayway, worth fixing before it ends up in the history.
Once the api revert and the cleanup are in, the persistence module itself is close.
…neratedValue to RepoEntity
… for robust validation
|
Thanks for the feedback! I've addressed the build.gradle.kts configuration as requested and synchronized the RepoEntity mappings with the database schema. The tests are passing now. Ready for review. |
martian56
left a comment
There was a problem hiding this comment.
Solid round of fixes. The junk files are gone, DOCKER_HOST is out, the BOM import is back, the entities now carry the real columns with plain accessors, the migration got its index and ON DELETE CASCADE, and the test now flushes, clears, and re-reads, which makes it a real test. Thank you for taking the feedback seriously.
One big item from the last round is still open, and there are two new ones.
Still open: the api still cannot start. I built this branch and ran the api with no database: it fails with "Failed to configure a DataSource: 'url' attribute is not specified". The yml now has neither the datasource nor the excludes, and CleatApiTests still carries the exclude hack, so CI stays green while the real app is broken. The fix is what the last review asked for: restore the excludes block exactly as it is on main, and drop the exclude list from the test. Heads up that Nurlan's #23 wires the api and worker to the new compose stack and will own those yml files, so expect to coordinate the rebase with him.
New: RepoEntity.visibility is missing @Enumerated, details inline. Related: the schema validation disappeared from the test in this round, and it is the net that catches exactly this class of bug. Bring it back.
New: backend/gradlew lost its executable bit in this branch, and a chmod step got added to CI to compensate. Fix the cause, not the symptom, details inline.
Small things: the Jackson @jsonvalue annotations on the new enums put API serialization concerns into the persistence layer; the DTO layer coming from #7 is where that mapping belongs, so keep these enums plain. And the migration picked up some odd deep indentation from the IDE, worth a quick tidy.
The persistence core of this PR is in good shape now. One more focused pass and it lands.
|
Hi! I’ve finished all the requested fixes. |
What does this change?
This PR configures the core data access and persistence layer for the Cleat platform within the
:libs:persistencemodule. The following changes have been made:Related issue
Closes #5
Area
Screenshots
N/A (This is a backend foundation change with no UI impact).
Checklist
bun run typecheck && bun run buildinapps/web(for frontend changes)