Conversation
ruolin59
changed the title
ruolin59
force-pushed
the
rufan/hts-deployment-tests
branch
from
August 28, 2026 00:43
003d58b to
94bb668
Compare
ruolin59
added a commit
that referenced
this pull request
Sep 10, 2026
…TS (alternative to #697) (#703) ## What this is An **alternative implementation of BDP-108627**, the same ticket as **#697**. It is meant to be compared against that PR, not stacked on it. Both branch from `rufan/views-entity-type-discriminator`. The premise: keep #697's HTTP contract and its SQL layer as they are, and rebuild the part between them — the CRUD wiring and error handling. One deliberate exception, called out below: views are paginated-only, so the non-paginated `GET /hts/views/query` that #697 mirrored from tables is dropped. | | #697 | this PR | |---|---|---| | HTTP API | — | **identical except views are paginated-only** — the one non-paginated view route is removed by design (see below); every other route, verb, param, body and status is unchanged | | SQL / JPQL | — | **identical** apart from removing the two now-dead non-paginated view finders; every constant, table finder and paginated view finder is verbatim | | Controller ↔ repository wiring | as landed | rebuilt | ## What is deliberately unchanged **The HTTP surface.** `GET /hts/entities` and every pre-existing table route keep their paths, verbs, parameters, defaults, bodies and status codes. The view routes match #697's with one exception: there is no non-paginated `GET /hts/views/query` (see the next section). **The SQL layer.** Every JPQL constant (`COMMON_FILTER_CLAUSES`, `PATTERN_KEY_CLAUSES`, `TABLE_ROW_PREDICATE`, `VIEW_ROW_PREDICATE`, `STAMP_TABLE_TYPE`), every query and `countQuery`, the typed conditional deletes returning an affected-row count, and the type-scoped rename are carried over verbatim. `TABLE_ROW_PREDICATE` keeps its `IS NULL` arm, so legacy rows still read as tables. ## JDBC repository surface (`UserTableHtsJdbcRepository`) An accounting of the MySQL-backed repository methods, grouped by the entity type each targets. "New" and "reshaped" are relative to the base branch `rufan/views-entity-type-discriminator`, which already carried the table-typed **reads** (the frozen tables path) but only **neutral writes**. **View-scoped — every method is new in this PR** (the repository had no view methods before): | Method | Returns | Purpose | |---|---|---| | `findViewByDatabaseIdIgnoreCaseAndTableIdIgnoreCase(db, table)` | `Optional<UserTableRow>` | point read under `VIEW_ROW_PREDICATE` | | `findAllViewsByFilters(db, table, version, metadataLocation, storageType, creationTime, Pageable)` | `Page<UserTableRow>` | paginated multi-attribute filter | | `findAllViewsByDatabaseIdAndTableIdLikeAllIgnoreCase(db, pattern, Pageable)` | `Page<UserTableRow>` | paginated name-pattern list | | `deleteViewByDatabaseIdIgnoreCaseAndTableIdIgnoreCase(db, table)` | `int` | conditional delete under `VIEW_ROW_PREDICATE`, affected-row count | | `deleteViewById(key)` | `int` | default delegating to the delete above | Plus the constants `VIEW` and `VIEW_ROW_PREDICATE`. There is deliberately **no** non-paginated view finder, no `findViewById` default, and no view rename: views are paginated-only and are not renameable. **Table-scoped:** - `findTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase → Optional`, `findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase` (`Iterable` and `Page`), `findAllTablesByFilters` (`Iterable` and `Page`) — *inherited from base*, unchanged. - `deleteTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase → int` and `deleteTableById(key) → int` — **new**: replace the base's single neutral `void delete…`, now scoped by `TABLE_ROW_PREDICATE` and returning an affected-row count so the service maps missing and wrong-type alike to 404. - `renameTableId(…) → int` — **reshaped**: the base's neutral `void renameTableId` gains `TABLE_ROW_PREDICATE` scoping and `STAMP_TABLE_TYPE` (rewriting a legacy null or non-canonical spelling as the row moves), and returns a row count. Table-only because views are not renameable. **Both (type-neutral):** - `findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase → Optional` and `existsByDatabaseIdIgnoreCaseAndTableIdIgnoreCase → boolean` — *inherited*; the writers read through these so they can see a row of any type and detect a collision at a shared key, which is why they stay unfiltered. - `findAllByDatabaseIdAndTableIdLikeAllIgnoreCase` (`Iterable` and `Page`), `findAllByFilters` (`Iterable` and `Page`), `findAllDistinctDatabaseIds()` and its paginated form — *inherited*. - `findById` / `existsById` defaults — *inherited*. - `deleteById`, `delete`, `deleteAllById`, `deleteAll(Iterable)`, `deleteAll()` — **sealed in this PR** to throw `UnsupportedOperationException`, forcing callers onto the typed deletes; the base's neutral `deleteBy…` derived delete is removed. ## What is rebuilt, and why **A view write cannot persist a table.** `putUserTable` and `putUserView` each supply their own `EntityType` to one private persistence primitive, so the named method — not the payload — is the authority. In #697, `putView` delegates to `putUserTable` and relies on the controller having stamped the type; a direct Java caller could persist the wrong one. **"Not found" cannot be manufactured from a failure.** The new point reads return `Optional`, and only the handler turns empty into a 404. A dependency or hydration failure travels as an exception and surfaces as a 500. This matters most for the neutral read, where the ticket calls out "the name is free" as the dangerous default. **The service owns its view-query vocabulary.** View queries cross the boundary as an owned `UserViewQuery`, built once in the handler after validation, so the transport `UserTable`'s nullability and its inert `entityType` stop at the handler. The invalid "table pattern without a database" state cannot be constructed. **Corruption is translated at the repository boundary, structurally.** An `@Around` aspect (`UserTableRepositoryTranslationAspect`) wraps every `UserTableHtsJdbcRepository` method: when a corrupt discriminator surfaces during hydration it is rethrown as an unwrapped `CorruptEntityTypeException`, and every other `DataAccessException` is rethrown exactly as it arrived, so non-corrupt failures behave as before. Because it is an aspect over the whole repository rather than a wrap at each call site, a read added later is covered without anyone remembering to wrap it. The interception point is load-bearing and can fail silently — Spring Data's own proxy holds `PersistenceExceptionTranslationInterceptor`, and the aspect must land in the outer proxy to see the already-translated `DataAccessException` — so a structural test pins it (`RepositoryTranslationAspectTest`) and a Spring Data upgrade that relocated the interceptor fails loudly rather than downgrading every corrupt read to a generic 500. Because corruption is unwrapped before it leaves the repository, the shared advice gains **one handler and no ORM vocabulary**. #697 instead teaches `services/common` to import and unwrap `JpaSystemException` and `InvalidDataAccessApiUsageException`. `CorruptEntityTypeException` does not extend `IllegalArgumentException`, so stored-data corruption stays distinguishable from client-input error. **Views are paginated-only.** #697 gave views a non-paginated `GET /hts/views/query` mirroring `GET /hts/tables/query`. This PR drops it: views can be listed only through the paginated `GET /v1/hts/views/query`. The non-paginated table query is legacy — it still backs the tables service's `/v0` and `/v1` list-all, and its empty-filter form returns database names rather than tables — and a trace confirmed neither the tables service nor the internal catalog references any view query path. Mirroring that shape would inherit tech debt into a greenfield surface with no caller, so on this new path we took the more principled option, which also removes the unbounded-query concern for views entirely. Tables are untouched: `GET /hts/tables/query` stays, because it has live callers. **Smaller ones.** PUT ingress validation is an injectable, unit-testable component that throws `RequestValidationFailureException` directly, rather than a private static helper in the controller. No-arg `deleteAll()` is sealed alongside the four key-addressed deletes, with teardown moved to a test fixture. View metrics are owned by housetables; the pre-existing `HTS_`/`REPO_` constants stay in `services/common` because `services/tables` consumes 19 of them. ## Footprint on shared code ``` CorruptEntityTypeException.java +19 (new) OpenHouseExceptionHandler.java +22 (one @hidden handler, 0 deletions) ``` `ServiceAuditAspect` and every other file in `services/common` are byte-identical to base. ORM imports added to `services/common`: **0** (#697 adds 3). The translation mechanism itself — a small `CorruptEntityTypeTranslation` utility and the aspect that applies it — lives in `services/housetables`, the module that already owns JPA. ## Deliberate asymmetries Two remain, both in signatures rather than behaviour: `getUserTable` signals absence by exception while the new point reads return `Optional`, and table queries take the transport `UserTable` while view queries take an owned type. Both were measured rather than assumed. Unifying the absence convention costs 16 pre-existing test edits and risks a silent wire change — the current 404 carries `cause: null` because it wraps a message-less `NoSuchElementException`, and no test pins that field. Unifying the query vocabulary means rewriting 11 private query helpers and 3 branch predicates that encode the frozen empty-filter behaviour. Both are worth doing on their own terms, with their own review; neither has view-specific content. ## Not adopted - **A generic corruption 500 body.** Would make it the only differently-shaped 500 in the service. - **Removing the `.stacktrace(...)` sites from the shared handler.** These do not leak: `ServiceAuditAspect` already strips the stack trace from the client body and keeps it only in the internal audit event, so deleting the sites would degrade the audit trail without changing any client response. The populate-then-strip indirection is real, but it is a `services/common` contract shared by tables, jobs and housetables — a whole-platform refactor, not a views concern. The views-specific error-handling improvement Mike's review asked for was instead made through the corruption path (`CorruptEntityTypeException` + the repository aspect), which gives a corrupt read a proper diagnostic 500 rather than raw ORM vocabulary. - **The soft-delete version TOCTOU.** Pre-existing; needs a version-guarded delete and its own interleaving test, so tracked separately rather than folded into a views PR. The production collation, an open question across both PR stacks, is now confirmed `utf8mb4_0900_ai_ci` — accent-insensitive and NO PAD. This needs no code change: it means a corrupt `'TÁBLE'` matches the type predicate and fails hydration with a diagnostic 500, and a corrupt `'TABLE '` is invisible to the typed routes. Both require a direct database write, since the API writes only canonical enum names, so neither is a reachable state. ## Testing | Module | Tests | Failures | |---|---|---| | `services:housetables` | 404 | 0 | | `services:common` | 12 | 0 | | `services:tables` | 526 | 0 | | `services:jobs` | 52 | 0 | `tables` (526) and `jobs` (52) match their base counts after this branch was rebased onto the current base. `:client:hts` regenerates cleanly with **five** new operations — `getEntity`, `getUserView`, `getPaginatedUserViews`, `putUserView`, `deleteView` (no non-paginated `getUserViews`). The generated OpenAPI response sets match #697's for every route that still exists, including a documented `500` on the three GET routes that can reach a corrupt row (`/hts/entities`, `/hts/views`, `/v1/hts/views/query`). The change was also driven through #698's deployment e2e suite against a real MySQL 8.4 container: **23 passed, 0 skipped**. That suite was written against #697's API; the only edit needed was to route its view-listing through the paginated endpoint, since the non-paginated one is intentionally gone — every other assertion passed unchanged, which is independent evidence the rest of the contract is indistinguishable to a client. The confirmed production collation (`utf8mb4_0900_ai_ci`) is exercised by that run. Pre-existing tests were changed only where the frozen contract or an adopted decision forced it, and each was strengthened rather than weakened; the three teardown edits come from sealing `deleteAll()`. No assertion was deleted or relaxed. ## A note on a pre-existing flake `UserTablesServiceTest.testUserTablePurge` failed twice under concurrent multi-module load and passed 6/6 in isolation. `SoftDeletedUserTablesMapper` derives `deletedAtMs` from `Instant.now()`, and that field is part of the soft-deleted composite key; the test soft-deletes the same table twice and expects two rows, so two calls in the same millisecond upsert into one. The mapper, the key and that test are untouched here, and #697 carries identical code. ## Review process Design-first: an implementation plan reviewed independently over two rounds, tests written before the production logic and reviewed against it, then an implementation review, followed by several rounds of simplification that removed roughly 1,400 lines of machinery in favour of the smallest thing that holds the contract. Load-bearing mechanics were verified empirically rather than assumed — most importantly the aspect's interception point, proven against Spring 5.3.25 by dumping the nested proxy chains: the aspect sits in a separate outer proxy from Spring Data's exception-translation interceptor, so no `@Order` value could move it, and a negative control (aspect disabled) confirmed the corrupt-row tests genuinely depend on it. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The suite in services/housetables drives the slice through MockMvc on H2. That leaves untested the parts which only exist in a deployment: the JSON a caller actually receives, the exception advice that has to survive persistence wrapping, and the discriminator column as MySQL stores it. These tests drive the running service over HTTP. Where the REST API cannot express the state under test they plant rows with SQL, since no endpoint writes a legacy NULL discriminator or a corrupt one. Those tests skip when no database is reachable rather than fail, so the suite still runs against a deployment whose database is not exposed. CI moves from the oh-only recipe to a MySQL-backed variant of it. That costs one container per run and changes what the existing integration test runs against. oh-only itself is untouched, so the IN_MEMORY path keeps its coverage. The recipe mounts services/housetables/ddl as the database's init directory, so every run also checks that the recorded DDL is a working migration path, which nothing else does today. The fixed sleep before the tests becomes a readiness poll, because MySQL starts more slowly and less predictably than H2. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use the v1 view listing, verify recorded MySQL DDL and mixed-case identifiers, wait for initialized MySQL before HTS startup, and require database coverage in CI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ruolin59
marked this pull request as ready for review
September 21, 2026 18:57
Reuse the MySQL Compose recipe with a disposable project, dynamic loopback ports and temporary Python environment. Build the services, wait for readiness, run the existing suites and clean up. Document the entrypoint in README. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ruolin59
force-pushed
the
rufan/hts-deployment-tests
branch
from
September 21, 2026 19:24
6668e0b to
8b3e2f5
Compare
List the two HTTP suites and distinguish them from client SDK, Spark integration and repository-wide tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
18 tasks
Collaborator
Author
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
Add deployment-level House Tables E2E coverage against MySQL initialized from the current recorded DDL. This PR now targets
main; it no longer depends on the closed #697 branch. Production service code and DDL are unchanged from upstream.Changes
Client-facing API Changes
Internal API Changes
Bug Fixes
New Features
Performance Improvements
Code Style
Refactoring
Documentation
Tests
Add an
oh-only-mysqlrecipe that appliesservices/housetables/ddl/*.sqlin filename order on a fresh volume. HTS waits for an authenticated TCP schema query to succeed, avoiding the reproduced first-start race with MySQL initialization.Add 25 HTTP/SQL E2E cases covering typed table/view lifecycles, conflicts, legacy NULL discriminators, restore stamping, corrupt-value diagnostics, and mixed-case identifiers. View listings use the current
/v1/hts/views/queryAPI, not the removed legacy route.Check the updated live-table column order, identifier/metadata widths, nullable version/discriminator, recorded table collations, and
idx_user_table_upper_db_tablefunctional index. This detects stale volumes or accidental use of the service's different bootstrap schema.Run both the existing tables integration suite and the new HTS suite against MySQL in CI. Readiness polling has bounded HTTP requests, validates expected statuses, and prints diagnostics on timeout.
--require-databasemakes unavailable database coverage fail instead of skip.Document local setup, strict versus optional database mode, and the fresh-volume requirement.
Add
python3 scripts/python/run_local_e2e.pyas the README-documented, one-command local entrypoint. It reuses the Compose recipe, builds both services, installs test dependencies in a temporary virtual environment, waits for readiness, and runs both suites against a fresh isolated database. Random loopback ports avoid conflicts with existing stacks; its own containers and volumes are removed afterward. The existing tables test accepts an optionalOPENHOUSE_TABLES_HOST, preserving localhost:8000 by default.Testing Done
Built the current upstream services with Java 17:
Started the recipe as Compose project
oh698, with a local-only override exposing MySQL on127.0.0.1:13306and giving OPA a unique container name. The existing MySQL container on port 3306 was left untouched. Recreated the deployment with a fresh anonymous MySQL volume to verify the DDL sequence and health-gated startup, then ran:Both suites passed on the warm deployment and again after cold initialization on MySQL 8.4.11. With the database port deliberately unreachable, strict mode exited nonzero before running HTTP cases; optional mode reported 21 passed, 4 skipped.
Also ran the new entrypoint end to end:
This separate deployment used dynamically assigned ports while the existing stacks remained running. Verified scoped container cleanup after both a failed startup-readiness attempt and the successful run.
./gradlew spotlessCheck -x CopyGitHooksTaskpassed.CopyGitHooksTaskwas excluded because it assumes.gitis a directory and fails in a Git worktree; the commit/push formatting check was run directly with that task excluded. Compose configuration validation andgit diff --checkalso passed.Additional Information
CI now exercises the MySQL recipe instead of
oh-only; the H2 recipe remains unchanged but is not exercised by this CI job. Corrupt-discriminator expectations are pinned to the recordedutf8mb4_0900_ai_ciNO PAD collation, including the explicit 404-vs-500 typed/neutral disagreement for malformed values.The recorded live and soft-deleted table schemas are production-verified upstream.
job_rowandtable_toggle_ruleremain bootstrap-derived approximations, as documented in the baseline. Docker initialization runs only on an empty data volume; it does not migrate an existing volume.The branch is rebased onto
mainwith only the E2E changes. Index metadata is checked, but query-plan/index-selection assertions are intentionally out of scope.