feat(persistence): set up JPA,Flyway and integration test with Testcontainers#21
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. |
martian56
left a comment
There was a problem hiding this comment.
Round three, and the substance is there. I rebuilt and ran this branch:
- :apps:api:test now passes as a plain @SpringBootTest. The exclude hack is gone from the test, the autoconfigure excludes are restored exactly in application.yml, and the context loads. The boot blocker from the last two rounds is resolved, confirmed by actually running it, not just green CI.
- RepoEntity.visibility now has @Enumerated(EnumType.STRING), so it stores as text against the varchar column instead of an ordinal.
- ddl-auto: validate is back in the persistence test, which is the net that proves the entities and the migration agree.
- The enums are plain again, no Jackson annotations bleeding into persistence.
One honest caveat on my end: I could not execute the persistence Testcontainers test locally. My Docker Desktop is handing the Java docker client an empty HTTP 400 on the named pipe, so Testcontainers cannot initialize. That is my machine, not your code. The test is written correctly and runs on Linux CI, and the api context-load test plus the validate config give me enough confidence.
Approving. Two things to handle before you hit merge, both small:
- The gradlew exec bit, inline. Still unfixed from last round, still a one-command change.
- Coordinate the api application.yml with Nurlan's #23. His PR wires a real datasource into that exact file, which is the proper fix and supersedes the excludes you restored here. Whichever of you merges second rebases, and his datasource version should be the one that survives. Talk to him on ordering so neither of you clobbers the other.
Good work seeing this through.
| uses: gradle/actions/setup-gradle@v4 | ||
|
|
||
| - name: Grant execute permission for gradlew | ||
| run: chmod +x gradlew |
There was a problem hiding this comment.
Still here from last round. This patches the symptom; the cause is that backend/gradlew is committed as mode 100644 instead of 100755. Fix the file once with git update-index --chmod=+x backend/gradlew, commit it, and delete this step. Otherwise every fresh clone on Linux or Mac still hits permission denied: ./gradlew, and CI just hides it. One line, please do it before merge.
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)