|
| 1 | +# Database Layer Refactor Plan |
| 2 | + |
| 3 | +## Goals |
| 4 | + |
| 5 | +- Remove raw JDBC boilerplate (`PreparedStatement`, `ResultSet`, manual `Connection` try/finally) |
| 6 | +- Remove manual MongoDB `Document` construction/reads |
| 7 | +- Add lightweight query harness without pulling in a full ORM or Spring Boot auto-config |
| 8 | +- Keep the existing `PushStore` interface contract unchanged — this is an internal implementation refactor only |
| 9 | + |
| 10 | +--- |
| 11 | + |
| 12 | +## Current State |
| 13 | + |
| 14 | +| Class | Storage | Problem | |
| 15 | +| ------------------- | ------------------------ | ------------------------------------------------------------------------------------------------------- | |
| 16 | +| `JdbcPushStore` | H2 / SQLite / PostgreSQL | Raw JDBC: manual statement prep, positional `?` params, no transaction management, exception swallowing | |
| 17 | +| `MongoPushStore` | MongoDB | Manual `Document` key/value construction and extraction | |
| 18 | +| `PushRecordMapper` | — | Already close to a `RowMapper<T>`, just not implementing the interface | |
| 19 | +| `DataSourceFactory` | HikariCP | Already good — wraps `javax.sql.DataSource`, no changes needed | |
| 20 | + |
| 21 | +--- |
| 22 | + |
| 23 | +## Recommendation 1 — Add `spring-jdbc` + `spring-tx` to `jgit-proxy-core` |
| 24 | + |
| 25 | +These are standalone Spring modules with no Spring Boot, no Spring Data JPA, no auto-configuration. They give you: |
| 26 | + |
| 27 | +- `NamedParameterJdbcTemplate` — named `:param` SQL with `MapSqlParameterSource` |
| 28 | +- `JdbcTemplate` — positional `?` fallback when needed |
| 29 | +- `TransactionTemplate` — programmatic transactions, no AOP or proxy magic |
| 30 | +- `DataSourceTransactionManager` — wires `DataSource` to the transaction abstraction |
| 31 | +- `RowMapper<T>` — interface for result set → object mapping |
| 32 | + |
| 33 | +Add to `jgit-proxy-core/build.gradle`: |
| 34 | + |
| 35 | +```gradle |
| 36 | +api 'org.springframework:spring-jdbc:6.2.6' |
| 37 | +api 'org.springframework:spring-tx:6.2.6' |
| 38 | +``` |
| 39 | + |
| 40 | +Version-aligned with the existing `spring-webmvc:6.2.6` used in the dashboard — no version drift. |
| 41 | + |
| 42 | +--- |
| 43 | + |
| 44 | +## Recommendation 2 — Refactor `JdbcPushStore` to `NamedParameterJdbcTemplate` |
| 45 | + |
| 46 | +Replace manual `PreparedStatement` / `ResultSet` / `Connection` lifecycle with template-based calls. |
| 47 | + |
| 48 | +Named parameters (`:status`, `:pushId`) are preferable to positional `?` for column-heavy queries. The existing `find(PushQuery)` dynamic WHERE clause is a direct fit for `MapSqlParameterSource`. |
| 49 | + |
| 50 | +Key construction change: |
| 51 | + |
| 52 | +```java |
| 53 | +private final NamedParameterJdbcTemplate jdbc; |
| 54 | +private final TransactionTemplate tx; |
| 55 | + |
| 56 | +public JdbcPushStore(DataSource ds) { |
| 57 | + this.jdbc = new NamedParameterJdbcTemplate(ds); |
| 58 | + this.tx = new TransactionTemplate(new DataSourceTransactionManager(ds)); |
| 59 | +} |
| 60 | +``` |
| 61 | + |
| 62 | +`approve()`, `reject()`, and `cancel()` are the only methods that touch multiple tables (status update + attestation insert) and need `tx.execute(...)`. All read paths (`find`, `findById`) are plain queries with no transaction wrapper needed. |
| 63 | + |
| 64 | +--- |
| 65 | + |
| 66 | +## Recommendation 3 — Promote `PushRecordMapper` to `RowMapper<PushRecord>` |
| 67 | + |
| 68 | +The existing mapper class already does the right work. Implement the Spring interface so it composes cleanly with the template: |
| 69 | + |
| 70 | +```java |
| 71 | +public class PushRecordMapper implements RowMapper<PushRecord> { |
| 72 | + @Override |
| 73 | + public PushRecord mapRow(ResultSet rs, int rowNum) throws SQLException { ... } |
| 74 | +} |
| 75 | +``` |
| 76 | + |
| 77 | +Create equivalents for the child entities: |
| 78 | + |
| 79 | +- `PushStepMapper implements RowMapper<PushStep>` |
| 80 | +- `PushCommitMapper implements RowMapper<PushCommit>` |
| 81 | +- `AttestationMapper implements RowMapper<Attestation>` |
| 82 | + |
| 83 | +Each mapper class is trivially unit-testable with a mocked `ResultSet`. |
| 84 | + |
| 85 | +--- |
| 86 | + |
| 87 | +## Recommendation 4 — MongoDB: POJO Codec Registry (no `spring-data-mongodb`) |
| 88 | + |
| 89 | +`spring-data-mongodb` gives `MongoTemplate` but drags in `spring-data-commons` (~800KB) and its annotation-driven mapping layer. The MongoDB Java driver already ships a **POJO codec registry** that handles marshalling with zero additional dependencies. |
| 90 | + |
| 91 | +Register domain classes at startup: |
| 92 | + |
| 93 | +```java |
| 94 | +CodecRegistry pojoRegistry = fromRegistries( |
| 95 | + MongoClientSettings.getDefaultCodecRegistry(), |
| 96 | + fromProviders(PojoCodecProvider.builder() |
| 97 | + .register(PushRecord.class, PushCommit.class, PushStep.class, Attestation.class) |
| 98 | + .automatic(true) |
| 99 | + .build()) |
| 100 | +); |
| 101 | +``` |
| 102 | + |
| 103 | +Then `MongoPushStore` works with typed collections instead of raw `Document`: |
| 104 | + |
| 105 | +```java |
| 106 | +MongoCollection<PushRecord> collection = mongoClient |
| 107 | + .getDatabase("gitproxy") |
| 108 | + .withCodecRegistry(pojoRegistry) |
| 109 | + .getCollection("pushes", PushRecord.class); |
| 110 | + |
| 111 | +// clean reads/writes |
| 112 | +collection.find(eq("status", PushStatus.APPROVED)).into(new ArrayList<>()); |
| 113 | +collection.findOneAndUpdate(eq("id", id), |
| 114 | + Updates.combine(Updates.set("status", APPROVED), ...), |
| 115 | + new FindOneAndUpdateOptions().returnDocument(AFTER)); |
| 116 | +``` |
| 117 | + |
| 118 | +`Filters`, `Updates`, and `Sorts` from `com.mongodb.client.model` are the Mongo-native equivalent of `MapSqlParameterSource` — composable and readable without string `Document` keys everywhere. |
| 119 | + |
| 120 | +Requires the domain model classes (`PushRecord`, `PushStep`, etc.) to have default constructors and standard getters/setters (or public fields). If they currently use builder/record patterns, a small codec customisation or `@BsonProperty` annotations bridge the gap. |
| 121 | + |
| 122 | +--- |
| 123 | + |
| 124 | +## Recommendation 5 — Dashboard Wiring (no Boot, no auto-config) |
| 125 | + |
| 126 | +The dashboard already registers beans manually in `SpringWebConfig`. Expose the JDBC helpers as Spring-managed beans so controllers and any future services receive them by injection rather than constructing templates themselves: |
| 127 | + |
| 128 | +```java |
| 129 | +// in SpringWebConfig (or a new DataConfig @Configuration) |
| 130 | +@Bean |
| 131 | +public NamedParameterJdbcTemplate jdbcTemplate(DataSource dataSource) { |
| 132 | + return new NamedParameterJdbcTemplate(dataSource); |
| 133 | +} |
| 134 | + |
| 135 | +@Bean |
| 136 | +public DataSourceTransactionManager transactionManager(DataSource dataSource) { |
| 137 | + return new DataSourceTransactionManager(dataSource); |
| 138 | +} |
| 139 | +``` |
| 140 | + |
| 141 | +`DataSource` is already constructed by `DataSourceFactory` and passed in externally — this just promotes it to a Spring bean so the wiring stays explicit and testable without any `@Autowired` magic on construction. |
| 142 | + |
| 143 | +--- |
| 144 | + |
| 145 | +## What This Does Not Change |
| 146 | + |
| 147 | +- `PushStore` interface — unchanged, no API break |
| 148 | +- `DataSourceFactory` / HikariCP setup — unchanged |
| 149 | +- `schema.sql` — raw SQL stays as the schema of record, no JPA `@Entity` annotations, no Flyway (unless added separately) |
| 150 | +- `PushStoreFactory` convenience builders — unchanged |
| 151 | +- The MongoDB document structure — POJO codec maps to the same document shape as the current `Document`-based code |
| 152 | + |
| 153 | +--- |
| 154 | + |
| 155 | +## Summary |
| 156 | + |
| 157 | +| Concern | Approach | |
| 158 | +| -------------------------- | ----------------------------------------------------------- | |
| 159 | +| SQL connection boilerplate | `NamedParameterJdbcTemplate` | |
| 160 | +| Transaction management | `TransactionTemplate` + `DataSourceTransactionManager` | |
| 161 | +| Row mapping | `RowMapper<T>` per entity | |
| 162 | +| MongoDB document mapping | POJO codec registry (driver-native) | |
| 163 | +| Spring Boot / JPA | Not needed — `spring-jdbc` + `spring-tx` are self-contained | |
| 164 | +| ORM | None — schema.sql is the source of truth | |
| 165 | + |
| 166 | +New transitive deps (dashboard): `spring-jdbc` + `spring-tx` only. Both are already version-aligned with Spring MVC 6.2.6 in use today. |
0 commit comments