HTS API: Give views a typed lifecycle in HTS - #697
Conversation
BDP-108403 landed the entity_type substrate and table-scoped reads. This adds everything a view needs to be created, read, listed and dropped through the House Tables Service, and closes the gap the predecessor left open: type was selected by method on the read paths, but key mutations were still neutral. View reads mirror the table set against a new VIEW_ROW_PREDICATE, a plain equality with no legacy-null case to absorb. GET/PUT/DELETE /hts/views and the two view query routes complete the surface. Rename is deliberately absent: M1 does not support it. Deletes and rename become type-scoped. A single conditional statement carries the type predicate, so a wrong-type mutation affects zero rows and maps to 404 with the row retained, rather than a read-then-delete that leaves a TOCTOU window on an irreversible operation. The inherited key-addressed deletes are sealed, so the only way to remove a row is a method that names the type. No-arg deleteAll() stays, since it addresses no key and cannot confuse one type for another. Writes now stamp the discriminator at ingress, before validation, so an un-upgraded tables service that sends nothing still produces a correctly typed row. The wire field stays nullable for exactly that reason. A PUT whose body contradicts its route is rejected, and a PUT over a row of the other type is a conflict rather than a silent conversion. A corrupt discriminator fails closed: 500 on the neutral read, 404 on typed mutations, never "the name is free". GET /hts/entities answers what occupies a key, of any type, for collision detection that must see both.
The first pass over-documented. Javadoc that restated a signature, the same rationale repeated at three layers, and narrative that belongs in the pull request are all gone. What remains is the reasoning a reader cannot recover from the code: why TABLE_ROW_PREDICATE keeps its IS NULL arm, why the inherited key-addressed deletes are sealed while no-arg deleteAll is not, why renameTableId binds the discriminator rather than inlining it, and why the null-to-TABLE resolution belongs to the converter alone. Where a table method has no view counterpart, that is now stated. Rename and the soft-delete flag are table-only by design, and a reader who finds them unpaired should not have to guess whether that is deliberate.
The transport model is called UserTable because it predates views, but a parameter holding a view row should not repeat that. View-scoped methods now name their argument userView. The type is shared; the name is not. EntityType.fromName trims before parsing. MySQL's PAD SPACE collation already treats a trailing space as insignificant, so the predicates match 'TABLE ' while valueOf rejected it, and the same row was reachable by a bulk delete yet unreadable. Accents are still rejected: an accented spelling is more plausibly corruption than a value meaning TABLE, and guessing at it would undo the point of failing closed. Accent insensitivity is what remains of the collation gap, and it is documented where the predicates are defined. Comments no longer date themselves. Milestone labels, ticket numbers and anything phrased relative to this change are gone, because a reader in a year has none of that context.
The view handler methods took userViewKey. The key type is shared and the method name already says which entity it addresses, so the qualifier said nothing the signature did not. Matches the neutral and table methods alongside them, which have always called it key.
renameEntity took an EntityType argument that only ever held TABLE, which is the shape this work exists to remove: type belongs in the method you call, not in what you pass it. It is now renameTable, and the constant lives in the service, next to the repository call that needs it. The repository keeps its bound parameter. An inline enum literal in a bulk JPQL statement does not reliably route through the attribute converter, so that one stays as it is. The neutral rename is still sealed. Jobs and toggles have no discriminator and inherit it legitimately; for a row that might be a view it is exactly what must not be reachable.
renameTable was the only table operation named for its type, sitting beside getEntity, putEntity and deleteEntity which are equally table-only. It goes back to renameEntity, and the interface now states the convention once: entity means table, view operations say view, and getNeutralEntity is the one that spans both. The dropped EntityType argument stays dropped. With it gone the method is the inherited signature again, so the sealed override has nothing left to seal: table scoping comes from the repository predicate, not from what the handler is called.
trim() was too broad. PAD SPACE collations ignore trailing ordinary spaces; trim() also strips leading whitespace and anything at or below U+0020, so " TABLE" and "TABLE\t" began hydrating cleanly while matching no predicate. That is a row which reads as a healthy table but cannot be deleted or renamed, and says nothing about why. Only trailing U+0020 is ignored now, so Java and the predicate agree in both directions: a value either resolves and matches, or fails both. Leading and non-space whitespace are corrupt again, which is what they were before the trim.
The soft-delete mapper explained that views are hard deleted and so never reach that store. The repository and service already say so where the decision is actually made; repeating it at the mapper only spread the same fact thinner.
The PAD SPACE reasoning sat in the javadoc, where a caller reads it, and not next to the loop it justifies. Anyone tempted to swap the loop for trim() is looking at the loop, so the warning belongs there. It also explains on sight why the scan compares against ' ' rather than Character.isWhitespace, and why it only walks backward. The javadoc keeps what a caller needs: accents are corrupt, and null is rejected rather than guessed at.
The repository interface carried sixty-six lines of comment, including thirteen on a one-line predicate constant, five of which reassured the reader that trailing spaces are not a problem. Reassurance about a non-problem is not documentation. What survives is what stops someone breaking things: that the null arm makes legacy tables visible and must not become a plain equality, that accents diverge under the column collation, why the inherited deletes are sealed and the no-arg one is not, why the rename binds its discriminator instead of inlining it, and why the bulk statements flush and clear.
fromName ignored trailing spaces, on the assumption the column sits on a PAD SPACE collation where 'TABLE ' and 'TABLE' are one value. A MySQL 8 deployment puts it on utf8mb4_0900_ai_ci, which is NO PAD, and there the assumption inverts: Java resolves 'TABLE ' while the predicate does not match it. Such a row answers 200 on the neutral read, 404 on every typed one, and since no neutral delete exists it cannot be removed over HTTP at all. Parsing is exact again, so that row is corrupt on both paths and says so. Being wrong and loud beats being wrong and invisible, and this way the code encodes no belief about the collation either way. The predicate comment now states the assumption it makes rather than asserting a collation nobody has verified, and asks for confirmation.
CorruptEntityTypeException is only ever thrown from the attribute converter, which runs inside Hibernate's result-set materialization, so it reaches the advice already wrapped and the dedicated handler never fired. The status was right by accident, through the catch-all, but the message naming the column and the offending value never left the server. The advice now walks the cause chain on the data-access wrappers and answers with the converter's own message when it finds one, deferring to the generic handler when it does not, so unrelated failures are unchanged. The direct handler stays for a throw that never passes through JPA. Values are quoted in that message. An empty column read as [] looks like a formatting fault, and [TABLE ] and [TABLE] are the same to the eye.
The corrupt-row advice narrated Hibernate's wrapping, which wrapper the translator picks when, and how the unrelated cases answer as they always did. A stack trace shows the first, and the second reads as a changelog entry for code that is gone. The javadoc on the direct handler also claimed the exception is wrapped before any advice is consulted. Spring does recurse into getCause(), so that is wrong: the dedicated handler never fires because the catch-all on Exception matches the wrapper at the top level and the recursion is never reached. Stated in a clause rather than a paragraph. Kept the warnings a future reader needs: that the direct handler is not dead code, and that the cause walk is bounded by depth and identity so a cyclic chain terminates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
renameTableId sets u.entityType from a fully-qualified HQL enum literal instead of a bound parameter, so the argument is gone and UserTablesServiceImpl no longer passes EntityType.TABLE at the call site. The argument was not load-bearing. Both inlined forms parse on Hibernate 5.6.14, and an enum literal in a query string renders through the attribute converter as the converted string rather than an ordinal, so a legacy NULL row is still stamped TABLE. The method is table-only by name and by predicate, so no caller could pass anything else. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
renameTableId stamped u.entityType from a fully-qualified HQL enum literal while TABLE_ROW_PREDICATE, concatenated into the WHERE clause of that same statement, hard-codes 'TABLE' as a plain string. One statement spelled one value two ways. It now uses the string in both places. The qualified name looked type-safe and was not: it is characters inside a query string, exactly like 'TABLE', and the compiler checks neither. It only added a failure mode, since renaming or moving EntityType would break the query at context startup while 'TABLE' keeps working. The javadoc line explaining the qualification went with it. The neighbouring predicate hard-codes the same string without one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The strings TABLE and VIEW appeared as bare literals in the two row predicates and again in the rename query's SET clause. Declare them as interface constants and build the three sites from them, so each value has a single definition. These constants are hard-coded strings rather than derived from the EntityType enum, because EntityType.TABLE.name() is a method call and not a constant expression, so it cannot appear in an annotation value; renaming an enum constant would still silently diverge from these strings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The rename query spliced 'TABLE' inline, so the formatter split that one fragment across three lines while every neighbouring fragment stayed on one. Declare STAMP_TABLE_TYPE beside the row predicates, which already splice the same value, and the annotation reads one fragment per line again. The assembled query string is unchanged, trailing space included. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
STAMP_TABLE_TYPE sat in the shared constants block, but only renameTableId uses it. Move the declaration above that method so the value reads next to the query that folds it in. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mkuchenbecker
left a comment
mkuchenbecker
left a comment
There was a problem hiding this comment.
Request changes. This PR has 35 blocking findings involving persistence correctness, MySQL behavior, lifecycle contracts, API behavior, test evidence, documentation accuracy, and changed-line code quality. Each finding includes the smallest required fix and must be resolved before merge.
The review covered the HTS typed lifecycle introduced by this PR. It did not review the later tables-service view implementation, the Iceberg view commit protocol, deployment execution, rollback execution, or unrelated security and performance concerns.
Reviewed using the code-review-skills criteria.
Blocking comments without changed-line anchors
The following comments apply to the PR description, generated output, or supporting behavior that cannot be anchored to a changed diff line.
4
[testing-review][blocking] JdbcProviderConfiguration.java:40 Every changed repository, service, and controller integration test runs against H2 in MySQL mode, while the correctness of the shared type predicates, affected-row counts, key conflicts, and converter wrapping depends on real MySQL behavior and the deployed collation. A green H2 suite can therefore pin behavior that is false in production. The database-owned claims need focused MySQL evidence. The smallest fix is to cite the deployed MySQL major version and effective key and discriminator collations, then add a narrow MySQL repository or composition fixture for canonical and legacy type predicates, one collation-sensitive value, one typed delete, renameTableId, neutral-read converter wrapping, and one occupied-key rename. This fix is required before merge.
5
[architecture-lifecycle][blocking] UserTablesServiceImpl.java:160-171 The shared table and view write path maps every DataIntegrityViolationException to concurrent modification. The request model accepts identifiers longer than the schema's 128-character columns, so an invalid record can reach MySQL and return HTTP 409 as if another writer won a race; other integrity constraints are misclassified the same way. Duplicate-key conflict, invalid input, and dependency failure must remain distinguishable. The smallest fix is to enforce the schema length bounds at ingress, translate the exact duplicate-key case to conflict, and let remaining integrity failures map to their actual validation or server-failure category with focused tests. This fix is required before merge.
8
[writing-review][blocking] PR description, Testing, generated-client sentence The compatibility claim says the client adds six operations with no structural change to the existing six and leaves existing callers unaffected. The base generated API has ten existing operations, and the generated page models have the observable changes described in comment 34. The compatibility statement must distinguish additive ABI from observable generated-model behavior. The smallest fix is to correct the existing-operation count and qualify the PR description to disclose the page-model ordering, hash, serialization-order, and toString() changes. This fix is required before merge.
26
[writing-review][blocking] PR description, Deployment tests The deployment-test section cites only #2 while relying on that unidentified change for 23 MySQL tests, compose and CI wiring, the MySQL version, and migration-path evidence. A reviewer or release operator cannot locate or freeze that evidence from this PR. The smallest fix is to replace #2 with the absolute PR URL, repository, title, and tested revision. This fix is required before merge.
27
[writing-review][blocking] PR description, Testing, test-inventory sentence The testing inventory says 23 pre-existing tests changed and eight assertions changed, but the frozen diff affects 19 pre-existing test methods and removes nine assertion lines. Those replacement assertions are generally stronger, but the counts still misstate the review surface. The smallest fix is to correct the two counts and describe the categories of intentional test changes instead of presenting the current inventory. This fix is required before merge.
34
[api-client][blocking] PageUserTable.java:37-48 Regeneration preserves public members but changes property order, Objects.hash(...) input order, JSON text order, and toString() order in PageUserTable and PageJob. Existing consumers that use mutable generated pages as hash keys or compare serialized or logged text can observe the upgrade. The smallest fix is to stabilize schema property ordering before generation or add a semantic generated-client comparison that permits new operations while rejecting behavioral changes in existing models. This fix is required before merge.
| .findById( | ||
| UserTableRowPrimaryKey.builder().databaseId(databaseId).tableId(tableId).build()) | ||
| .orElseThrow(() -> new NoSuchUserTableException(databaseId, tableId)); | ||
| UserTableRowPrimaryKey key = |
There was a problem hiding this comment.
1
[persistence-rollout][blocking] UserTablesServiceImpl.java:218-235 The soft-delete path reads a table without a lock, archives that snapshot, and then runs a bulk delete with no version predicate. A concurrent writer can commit newer metadata after the read, after which this transaction can archive the old metadata, delete the newer row, and return success. The delete must remove exactly the committed version that was archived so restore remains lossless. The smallest fix is to lock the typed source row before copying it, or constrain the delete by the exact physical key, captured numeric version, and existing legacy-NULL-or-TABLE predicate, require exactly one affected row, roll back the archive on mismatch, and add a deterministic MySQL interleaving test. This fix is required before merge.
There was a problem hiding this comment.
Pre-existing, and this PR narrows rather than widens it.
On the base branch deleteUserTable already reads with an unlocked findById, archives that snapshot, and then deletes by key with no version predicate. The interleaving you describe is reachable on the base commit with the same result.
What changed here is the delete call only: deleteById(key) became a type-scoped conditional delete carrying TABLE_ROW_PREDICATE. That adds a discriminator constraint to the statement; it removes nothing. The archive read and the absent version predicate are both untouched.
Version-constraining the soft-delete archive is a real improvement and I agree with the reasoning, but it is a change to pre-existing lifecycle behaviour that this PR does not otherwise touch, and it needs its own tests and its own rollback story. Tracking separately rather than expanding this diff.
There was a problem hiding this comment.
Its not pre-existing for views, and if its an issue with tables please file a Jira.
There was a problem hiding this comment.
yes, this is definitely an issue with tables, but it's currently out of scope for this pr so I've created BDP-109006 and added a TODO comment. Just want to point it out though that this path is a table-only path and does not exist for views
| @Param("tableIdPattern") String tableIdPattern, | ||
| Pageable pageable); | ||
|
|
||
| /** Bulk statements bypass the persistence context, hence the flush and clear. */ |
There was a problem hiding this comment.
2
[persistence-rollout][blocking] UserTableHtsJdbcRepository.java:230-249 The new point mutations use case-folded key equality, but the composite primary key inherits an unverified database collation. Under a case-sensitive production collation, sales.orders and SALES.ORDERS can coexist physically while these deletes, reads, and renames treat them as one logical key, allowing a finder to return multiple rows or one mutation to affect both. Physical uniqueness must use the same equality relation as every logical key operation. The smallest fix is to capture the deployed SHOW CREATE TABLE, consolidate any case-equivalent duplicates, enforce either case-insensitive uniqueness or a canonical stored-key representation, and add mixed-case first-create and affected-row MySQL checks that require every key mutation to affect at most one row. This fix is required before merge.
There was a problem hiding this comment.
Pre-existing. lower()-wrapped key equality is the established convention in this repository — it appears 8 times in this file on the base commit, including in COMMON_FILTER_CLAUSES and the point finders. The new typed methods follow it rather than introduce it.
The underlying collation question is real and I have flagged it separately: the production SHOW CREATE TABLE for user_table_row is unverified, and ddl/0000__baseline.sql specifies no collation. That is called out in the PR description.
Worth adding, since it strengthens your point: I measured that the lower() wrapping also defeats the primary key. At 10k rows, lower(database_id)=lower(?) AND lower(table_id)=lower(?) plans as type: ALL, possible_keys: NULL, while plain equality is type: const, key: PRIMARY, rows: 1. On a _ci collation the wrapping is semantically redundant and costs the index.
That is a pre-existing defect affecting every key lookup in the service, not something the typed methods introduce, and it wants its own investigation with the owners of the current index tuning work.
|
|
||
| String VIEW = "VIEW"; | ||
|
|
||
| /** |
There was a problem hiding this comment.
3
[persistence-rollout][writing-review][pedantic-linter][PL-COMMENT-001][blocking] UserTableHtsJdbcRepository.java:53-67 The SQL predicates classify values under the column collation while EntityType.fromName accepts only exact case variants. An accent-insensitive collation can make TÁBLE match TABLE, and a PAD SPACE collation can make TABLE match TABLE, even though hydration rejects both; bulk delete and rename can then remove or normalize values that reads treat as corruption. The repository comment, PR description, and affected test comments also state mutually incompatible universal outcomes for those rows. SQL selection, mutation, conversion, and documentation must share one persisted vocabulary. The smallest fix is to verify and pin a case-insensitive, accent-sensitive, padding-sensitive database comparison and integrity constraint, or use an equivalent exact predicate strategy, then update the PR description and tests to describe only the verified behavior while preserving legacy SQL NULL as TABLE. This fix is required before merge.
There was a problem hiding this comment.
The divergence is real, it is measured rather than theoretical, and the documentation inconsistency you name has been fixed. What remains is genuinely blocked on information I do not have.
Measured behaviour. Against a deployed MySQL 8.4 whose entity_type is utf8mb4_0900_ai_ci (accent-insensitive, NO PAD), with rows planted by direct SQL:
| stored | predicate | typed GET | neutral GET |
|---|---|---|---|
'TABLE' |
match | 200 | 200 |
'TÁBLE' |
match | 500 | 500 |
'TABLE ' |
miss | 404 | 500 |
'' |
miss | 404 | 500 |
So both halves of your claim hold: an accent-insensitive collation selects 'TÁBLE' into a statement that hydration then rejects, and a mutation over that row succeeds without hydrating. A rename of a 'TÁBLE' row returns 204 and rewrites the discriminator to a clean 'TABLE' — verified at the byte level, 54C381424C45 before, 5441424C45 after. The row is silently repaired.
Documentation. You are right that the description asserted a universal outcome it could not support. It said corrupt discriminators get "404 on typed mutations", which is false for a predicate-matching value — measured 204 on both DELETE /hts/tables and PATCH .../rename. That sentence also contradicted the Known limitation section two headings below. It has been deleted rather than reworded, because a narrower true version would still have implied the design guarantees something about corrupt-row mutations, and it does not.
The repository comment states the assumption as an assumption and asks for confirmation, which I believe is accurate as written. If you read it as claiming more than that, tell me which clause and I will tighten it.
What I cannot close. Pinning "case-insensitive, accent-sensitive, padding-sensitive" requires knowing what production actually has. ddl/0000__baseline.sql specifies no collation, so a fresh deployment inherits the server default, and the deployment I measured is not evidence about a table that may have been created years ago. I have asked for the production SHOW CREATE TABLE on this PR; until someone with access provides it, changing the comparison would be guessing in the other direction.
That question is load-bearing for more than a comment: it decides whether EntityType.fromName should trim trailing spaces. It originally did, on a PAD SPACE assumption; that was reverted when the deployment measured NO PAD, because under NO PAD the trim makes Java and SQL disagree about the same row. Under PAD SPACE the trim is correct and removing it breaks working rows. Neither choice is safe under both, which is exactly your point about one persisted vocabulary — and why I would rather document the split than pin the wrong half of it.
There was a problem hiding this comment.
Are views actually compatible with the casing issue? We need to normalize the representation the view definition is my expectation.
There was a problem hiding this comment.
the discriminator is already normalized on write, convertToDatabaseColumn only ever writes EntityType.name() so stored values are always TABLE or VIEW uppercase. you'd need a direct db write to get anything else. Also hts doesn't store the view definition at all, just metadataLocation, that text lives in iceberg metadata which is BDP-108407 scope. So views aren't any more exposed to this than tables are.
| @ApiResponses( | ||
| value = { | ||
| @ApiResponse(responseCode = "200", description = "Entity GET: OK"), | ||
| @ApiResponse(responseCode = "400", description = "Entity GET: BAD_REQUEST"), |
There was a problem hiding this comment.
6
[writing-review][blocking] UserHouseTablesController.java:297 The generated OpenAPI contract omits implemented responses: the neutral read omits its corruption 500, both view queries omit validator-driven 400 responses, and current head can return collation-dependent corruption 500 responses from the view point and query routes. Generated-spec consumers therefore see a narrower contract than the service implements. The annotations should enumerate the expected current response categories so clients and operators can handle them deliberately. The smallest fix is to add the missing 400 and 500 @ApiResponse entries to these four GET operations, then revise any collation-dependent entries if comment 3 changes the executable behavior. This fix is required before merge.
There was a problem hiding this comment.
Done — the four GET operations now declare the responses they can actually produce.
The neutral read and the view point read gain a 500, and both view query routes gain a 400 and a 500.
I checked each against real behaviour rather than adding them uniformly. The 500 is reachable wherever a corrupt discriminator can be hydrated: unconditionally on the neutral read, which carries no type predicate and so materializes any row at the key, and on the typed view routes for a value the collation matches but EntityType.fromName rejects — an accent-insensitive collation makes a stored 'VÍEW' behave that way. The 400 on the query routes is the validator rejecting a malformed request before the query runs.
Descriptions follow the existing convention in this file, so "User View GET: BAD_REQUEST" and "Entity GET: INTERNAL_SERVER_ERROR" sit alongside the codes that were already declared.
| .orElse(UserTableRow.builder().build()); | ||
| assertThat(result.getMetadataLocation()).isEqualTo(newTableMetadata); | ||
|
|
||
| // The bound type is written, not merely assumed: the column itself holds TABLE. |
There was a problem hiding this comment.
7
[writing-review][blocking] HtsRepositoryTest.java:338-343 This comment says a bound type is written, and the PR description says renameTableId takes a bound EntityType supplied by the service. Final head has no type parameter and stamps the query-string constant STAMP_TABLE_TYPE. The prose should describe the actual hard-coded storage contract so reviewers do not infer converter-backed parameter binding or miss enum-string drift. The smallest fix is to replace this comment and the PR description's Repository surface statement with the final string-constant behavior. This fix is required before merge.
There was a problem hiding this comment.
Correct on both counts, and both are fixed.
The comment now reads "The type is written, not merely assumed: the column itself holds TABLE." The method takes no type parameter; the query stamps the string constant STAMP_TABLE_TYPE.
The PR description's Repository surface paragraph carried the same stale claim and has been rewritten:
renameTableIdstamps the discriminator from a string constant in the query itself, so no caller supplies it. That constant is not derived fromEntityType:EntityType.TABLE.name()is a method call and therefore illegal in an annotation value, so renaming an enum constant would not update the query.
For the record on how it got stale: the parameter did exist for most of this PR's life, and the justification given for it — that an inlined literal would not route through the attribute converter on this Hibernate version — turned out to be false when tested. Both a plain string literal and a fully-qualified enum literal render entity_type='TABLE' on 5.6.14, and an enum literal set to VIEW emits 'VIEW' rather than the ordinal, so the converter is consulted. The parameter was removed and the prose was not updated with it. Thanks for catching it.
The enum-string drift you point at is real and is now stated in the description rather than left implicit: nothing links the query's 'TABLE' to EntityType.TABLE at compile time. HtsControllerTest.testRenameTableStampsCanonicalTableOnLegacyRow is what would catch a divergence, since it reads the raw column back.
| SoftDeletedUserTableRow.builder() | ||
| .tableId(TestHouseTableModelConstants.TEST_TABLE_ID) | ||
| .databaseId(TestHouseTableModelConstants.TEST_DB_ID) | ||
| .deletedAtMs(1751907524L) |
There was a problem hiding this comment.
30
[pedantic-linter][PL-MAGIC-001][blocking] UserTablesMapperTest.java:192-197 The restore fixture uses two raw epoch-like literals without names, units, or an expressed retention relationship. The smallest fix is to introduce named timestamp constants with units or derive the purge time from the deletion time through a named duration. This fix is required before merge.
There was a problem hiding this comment.
Done. The two literals are now named constants, with the retention relationship expressed rather than implied:
private static final long TEST_DELETED_AT_MS = 1751907524L;
private static final long TEST_PURGE_RETENTION_MS = 1000000L;and the fixture derives purgeAfterMs as TEST_DELETED_AT_MS + TEST_PURGE_RETENTION_MS. Previously both were independent literals, so the fact that one was meant to be later than the other by a fixed interval was only visible by subtracting them.
| return EntityType.fromName(columnValue); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new IllegalArgumentException( | ||
| throw new CorruptEntityTypeException( |
There was a problem hiding this comment.
31
[architecture-lifecycle][pedantic-linter][PL-FAIL-001][blocking] EntityTypeConverter.java:28-40 The converter raises unchecked corruption during JPA hydration, and the service does not translate the resulting persistence failure before it crosses the housetables boundary. Non-HTTP callers therefore receive ORM vocabulary instead of a housetables-owned corruption outcome. The smallest fix is to catch the exact persistence wrapper at the repository or service edge, preserve its cause, and translate it to a checked module-owned failure before any protocol adapter handles it. This fix is required before merge.
There was a problem hiding this comment.
Same reasoning as my reply on comment 32, plus one point specific to the converter.
The unchecked throw is not really a choice the converter gets to make. AttributeConverter.convertToEntityAttribute is a JPA SPI method whose signature declares no checked exception, so a checked module-owned failure cannot be thrown from here — it would have to be wrapped in an unchecked one to escape, which is what already happens.
Where it could be translated is the layer above, and that is the substance of the finding. Two candidates, both awkward for the same reason: hydration is lazy. A findAll returning an Iterable may not materialize a corrupt row until the caller iterates it, which can be outside whatever try block the service wrapped the repository call in. So catching "the exact persistence wrapper at the repository or service edge" is not simply a try/catch at three call sites — it needs either eager materialization or a decorator that wraps the returned iterable, and getting that subtly wrong turns a loud 500 into a silent partial result.
On the non-HTTP caller: there is not one today. Every path into these reads is a controller, and the advice translates before anything leaves the process. I take the point that the service contract should not depend on that remaining true, and if a non-HTTP adapter is added, the translation should land with it rather than be retrofitted afterwards.
For this PR I would rather leave the vocabulary where it is than add a lazily-correct translation layer that no current caller exercises.
There was a problem hiding this comment.
The unchecked throw is not really a choice the converter gets to make. AttributeConverter.convertToEntityAttribute is a JPA SPI method whose signature declares no checked exception, so a checked module-owned failure cannot be thrown from here — it would have to be wrapped in an unchecked one to escape, which is what already happens.
The one case where I think unchecked exceptions are fine. :)
There was a problem hiding this comment.
Agreed, thanks. Leaving it unchecked.
| return buildResponseEntity(corruptEntityTypeBody(corruptEntityTypeException)); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
32
[pedantic-linter][PL-FAIL-003][blocking] OpenHouseExceptionHandler.java:408-424 Common HTTP advice now imports and interprets JpaSystemException and InvalidDataAccessApiUsageException, which proves those persistence wrappers crossed their immediate adapter. The HTTP boundary should receive a module-owned failure rather than inspect ORM causes. The smallest fix is to move exact-wrapper translation into the housetables persistence boundary and keep this advice focused on the translated outcome. This fix is required before merge.
There was a problem hiding this comment.
Pushing back, though the architectural observation is fair and I want to separate the two halves of it.
The wrapper was already crossing this boundary. This advice declares @ExceptionHandler(Exception.class) on handleGenericException, and Exception is assignable from JpaSystemException. So persistence wrappers were already arriving here and being handled here, on the base commit and for every service using this advice. That is not incidental to this PR — it is the mechanism that hid the defect. Spring's ExceptionHandlerMethodResolver does recurse into getCause(), but only when the top-level type matches nothing; the catch-all always matched first, so the recursion was never reached and @ExceptionHandler(CorruptEntityTypeException.class) never fired.
What changed is that the advice now names two wrapper types instead of swallowing them anonymously as Exception. That makes an existing dependency visible; I do not think it creates one. services:common also already has spring-boot-starter-data-jpa as an api dependency on the base, so these types were on the compile classpath of every dependent module before this PR.
The risk worth checking is regression for other services, and it is covered. services/tables shares this advice, so the question is whether a JpaSystemException from tables now gets a worse answer because our handler intercepts it first. It does not: when no CorruptEntityTypeException is found in the chain we delegate to handleGenericException, and OpenHouseExceptionHandlerTest.testUnrelatedDataAccessExceptionKeepsGenericBody asserts the message and cause are identical to what the generic path produces. The walk is bounded by depth and by identity, so a cyclic chain terminates rather than spinning.
On the fix. There is no service-specific @ControllerAdvice anywhere in this repository — this is the only one. So "keep this advice focused on the translated outcome" means introducing the first per-service advice and then reasoning about ordering between two advices, or catching at the repository edge where hydration is lazy and an Iterable may not throw until the caller iterates, possibly outside any try block.
Both are defensible designs. Neither is a small change, both refactor code that currently works and is covered, and the benefit is structural rather than behavioural. I would rather do that deliberately than fold it into a views PR — and if the answer is a housetables-owned advice, that is a change worth making for all of housetables' persistence failures, not only this one exception type.
There was a problem hiding this comment.
The HTTP boundary should receive a module-owned failure rather than inspect ORM causes.
I agree with this. Openhouse should not return HTTP errors because that's a layering inversion. The litmus test is "could this module be run as a CLI".
CLI -> Module
HTTP -> Module
Where the module speaks HTTP primitives you are building a microservice and not a module.
Each module boundary should speak its own dialect with explicit duplication and translation across boundaries.
There was a problem hiding this comment.
No argument with the litmus test. The problem is there's no per-module error vocabulary anywhere in the repo: every service throws and one shared @ControllerAdvice in services:common maps to HTTP. Adding one for views alone gives housetables two error models.
Same underlying question as 14 and 17, so let's fold it into that conversation.
| Page<UserTableDto> getAllUserTables(UserTable userTable, int page, int size, String sortBy); | ||
|
|
||
| /** Given a databaseId and tableId, delete the user table entry from the House Table. */ | ||
| /** |
There was a problem hiding this comment.
33
[architecture-lifecycle][blocking] UserTablesService.java:52-58 The new view-query service methods expose the transport-owned UserTable, so wire nullability, ignored fields, and future transport changes become part of the service contract. The service should own a narrow view-query vocabulary. The smallest fix is to replace the transport parameter with an owned UserViewQuery and map into it at the handler boundary described in comment 15. This fix is required before merge.
There was a problem hiding this comment.
Grouping the replies to comments 14, 15, 17 and 33 here, since they make one argument: the service and handler layers should own a narrower vocabulary instead of passing the transport UserTable inward and signalling absence and validation failure through unchecked exceptions.
I agree with the principle. I am declining it for this PR, because the view methods follow the pattern the base branch already established rather than introducing it.
On the base commit, UserTablesService already accepts the transport type in four methods:
List<UserTableDto> getAllUserTables(UserTable userTable);
Page<UserTableDto> getAllUserTables(UserTable userTable, int page, int size, String sortBy);
Pair<UserTableDto, Boolean> putUserTable(UserTable userTable);
Page<UserTableDto> getAllSoftDeletedUserTables(UserTable userTable, int page, int size, String sortBy);and getUserTable(String, String) already returns a mandatory UserTableDto, signalling absence with NoSuchUserTableException rather than an empty Optional. The new view methods mirror those signatures exactly.
Worth noting the interface's own javadoc names your concern:
Avoid using
UserTabledirectly for decoupling between service and transport layer.
So the codebase already holds this position and has deliberately deviated from it for the filter-shaped methods. That makes consistency with the existing shape the more defensible choice here, and the deviation itself a decision worth revisiting on its own terms rather than by silently splitting the surface.
The practical problem with fixing it views-only: it leaves views speaking an owned vocabulary while tables speaks the transport's, in one interface, with no principle distinguishing them. That is worse than either applied uniformly. Fixing both means changing every pre-existing table query path, which is a service-interface refactor with its own testing and review surface and no view-specific content.
On comment 17 specifically: stampEntityType throwing RequestValidationFailureException matches how this controller already reports request validation failures, and the advice already maps it to 400. Introducing a typed validation result for this one helper would make it the only path in the controller shaped that way.
I would support all four as a single follow-up that changes the whole interface at once. I do not think any of them should gate a change that adds no new pattern.
There was a problem hiding this comment.
The practical problem with fixing it views-only: it leaves views speaking an owned vocabulary while tables speaks the transport's, in one interface, with no principle distinguishing them.
The principle is that we can't move them all at once, and we are not perpetuating the existing pattern.
There was a problem hiding this comment.
fair enough, did it. added a UserViewQuery with just the six fields the view query path actually reads (databaseId, tableId, tableVersion, metadataLocation, storageType, creationTime) and map into it at the handler boundary. table query paths are untouched.
it's ~117 loc for what's essentially a type narrowing, which is worth flagging, but the service signature is honest now so I think it's worth it.
| return buildResponseEntity(corruptEntityTypeBody(corruptEntityTypeException)); | ||
| } | ||
|
|
||
| private ErrorResponseBody corruptEntityTypeBody( |
There was a problem hiding this comment.
35
[architecture-lifecycle][blocking] OpenHouseExceptionHandler.java:427-435 The corruption response returns the raw stored discriminator message and an abbreviated converter stack trace to the client. That exposes persistence detail as part of a common public error contract even though callers only need to know that HTS could not read the occupied key. The smallest fix is to log the column, value, cause, and stack trace internally while returning a stable generic 500 message and correlation context to the client. This fix is required before merge.
There was a problem hiding this comment.
Declining. The stack trace and cause are the pre-existing shape of ErrorResponseBody, not something this change introduces, and the "client" here is an internal service with no auth boundary.
On the response shape: handleGenericException on the base commit already builds every unhandled 500 as
.message(exception.toString())
.stacktrace(getAbbreviatedStackTrace(exception))
.cause(getExceptionCause(exception))so every 500 this service returns has carried an abbreviated stack trace and cause for as long as the advice has existed. corruptEntityTypeBody populates the same three fields the same way. Returning a generic message with correlation context for this one outcome would make it the only 500 in the service shaped differently, which is a change to the common error contract rather than a fix confined to this PR.
More specifically: corrupt rows were already taking that path. That is how the dead handler was found — CorruptEntityTypeException is thrown inside Hibernate result-set materialization, so it arrived wrapped, the catch-all matched the wrapper before Spring recursed into the cause, and the dedicated handler never fired. The status was 500 by accident. Those responses already carried a stack trace and a cause; what they lacked was a message naming the column and the value.
So the only genuinely new content in the body is the stored discriminator: TABLE, VIEW, or a corrupt spelling of one. That is not user data, a secret, or a key — it is the value that makes the failure actionable, and an operator seeing ['TÁBLE'] or [''] can resolve the row immediately, where a correlation id requires log access and a round trip.
On the boundary: House Tables is an internal service with no authentication — no security dependency, no filter chain, no interceptor. Its only caller is the OpenHouse tables service. There is no untrusted consumer of this error body to harden against, which is the assumption the finding rests on.
If the common error contract should stop returning stack traces, I agree that is worth doing — but it should be done for all 500s at once, deliberately, and it is a change to services:common behaviour affecting every service, not something to introduce for one exception type in a views PR.
There was a problem hiding this comment.
- Error handling should be built tracing each error to ensure they are returning the proper error type all the way up to the HTTP layer.
- There are existing issues with the error handling that should not be replicated into views.
- If you need to make changes to ensure views have proper error handling you should do so.
There was a problem hiding this comment.
The existing issues you're pointing at are the pattern itself, so "don't replicate them into views" and "don't refactor the base here" pull against each other. Every service throws and one shared @ControllerAdvice in services:common maps to HTTP; there's no per-module error vocabulary to put views into.
I'd rather fix it for all of housetables in one change than give views a private error path. Same underlying question as 14 and 17, so let's fold it into that conversation.
The comment described a bound type parameter that the method no longer takes; the query stamps a string constant. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The generated spec promised these four GET operations a narrower contract than they implement, so a client reading it had no reason to handle either failure. The neutral entity read carries no type predicate, so it hydrates whatever occupies the key, including a row whose entity_type is outside the vocabulary; the converter raises CorruptEntityTypeException and the handler answers 500. The view point read and both view query routes carry a predicate, but an accent- or space-insensitive collation lets a stored 'VÍEW' or 'VIEW ' satisfy it in SQL and still fail hydration, so they reach the same 500. Both view query routes run the entities validator, which rejects an unsupported filter, a malformed id, or bad paging with 400. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Replies to the individually-anchored findings are in their own threads. These six have no changed-line anchor, so they have nowhere to go but here. 4 — H2 test databaseTwo parts. More usefully, this finding is already addressed. #698 stacks on this branch and adds 23 tests that drive a deployed service over HTTP against MySQL 8.4, plus the compose recipe and CI wiring to run them. Your concern is not hypothetical and I want to confirm it directly: that suite found two real defects that the 279-test H2 suite passed.
Both are fixed here. The deployment suite now asserts the diagnostic message on both the predicate-matched and predicate-missed routes, so the first cannot regress silently. The specific MySQL evidence you ask for — typed predicates for canonical and legacy rows, a collation-sensitive value, a typed delete, 5 — integrity-violation classificationPre-existing. On the base commit The over-broad classification is real, and your reasoning about oversized identifiers reaching MySQL is sound. It is inherited by the view path rather than created by it, and separating duplicate-key from invalid-input from dependency failure changes behaviour for existing table writes, so it wants its own change with its own tests. 8 — generated-client compatibility claimCorrect, and fixed. The description said "6 new operations and no structural change to the existing 6". The base controller exposes 10 operations, not 6. It now states the correct count and discloses that regeneration reorders page-model properties, which changes 26 — unresolvable
|
The raw column read returned a bare String, so every call site had to remember that a SQL NULL meant a legacy row. It now returns an Optional and each assertion states presence or absence outright. The seed helper took a nullable EntityType as a mode selector between a raw legacy insert and a typed save. It is now two helpers, seedLegacyRow and seedTypedRow, each with required arguments, so a call site names the fixture state it wants. Three tests walked fixed spellings in a loop under a single assertion. They are parameterized now, so each case is named, discovered, and reported on its own. Two point reads were parameterized over a nullable enum only to pick a row shape. They are ordinary tests again, sharing one helper for the request they both make. A restore fixture spelled its deletion and purge times as bare epoch literals. The purge time is now derived from the deletion time through a named retention, and a converter test that read as a pass-through now names the contract it asserts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The cause-chain walk returned null for a chain carrying no corruption, and its only caller tested that sentinel. It returns an Optional now, and the caller maps the found cause to the diagnostic response and the empty case to the generic handler. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…-mysql Brings the upstream linkedin#697 review-feedback commits under the MySQL E2E. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kd6AVye3gAGs6LBpYgdb6B
|
A human review by yourself and myself may be required to go point by point by the two of us to ensure the PR is moving in the right direction. The rationelle used by the agent responses is suspect, in-particular referencing pre-existing issues given this is a new code path for views. I have not gone point by point yet. The bot has a maybe 60% hit rate in my experience with its feedback. This is a new codepath and api and we should evaluate its building for the future of views. I had it be pedantic because it was ignoring error handling wiht [follow up] vs blocking pri. The bot is a WIP and can be improved, but for now I'm using it as a filter given the size of the diffs to review. |
ruolin59
changed the title
Agreed, I would much prefer human review, would be happy to get together with you and go over the items specifically. I tend to find that overly aggressive AI reviewers tend to find many "issues" by looking only at surface level rather than deeply inspecting the codebase |
| return buildResponseEntity(corruptEntityTypeBody(corruptEntityTypeException)); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The HTTP boundary should receive a module-owned failure rather than inspect ORM causes.
I agree with this. Openhouse should not return HTTP errors because that's a layering inversion. The litmus test is "could this module be run as a CLI".
CLI -> Module
HTTP -> Module
Where the module speaks HTTP primitives you are building a microservice and not a module.
Each module boundary should speak its own dialect with explicit duplication and translation across boundaries.
| * Server-side corruption rather than a bad request, so the advice maps it to a server error; it | ||
| * extends {@link IllegalArgumentException} so existing callers keep catching it. | ||
| */ | ||
| public class CorruptEntityTypeException extends IllegalArgumentException { |
There was a problem hiding this comment.
Use checked exceptions, Errors are part of contracts. If its not in the signature of the method I am not satisfied.
| public static final String HTS_LIST_TABLES_REQUEST = "hts_list_tables_request"; | ||
| public static final String HTS_LIST_TABLES_TIME = "hts_list_tables_time"; | ||
| public static final String HTS_SEARCH_TABLES_TIME = "hts_search_tables_time"; | ||
| public static final String HTS_LIST_VIEWS_REQUEST = "hts_list_views_request"; |
There was a problem hiding this comment.
I agree with this one. I think we should seriously consider what information belongs in HTS vs Table service. Duplicate it into HTS for the HTS-specific usage so they are decoupled and independent.
| return buildResponseEntity(corruptEntityTypeBody(corruptEntityTypeException)); | ||
| } | ||
|
|
||
| private ErrorResponseBody corruptEntityTypeBody( |
There was a problem hiding this comment.
- Error handling should be built tracing each error to ensure they are returning the proper error type all the way up to the HTTP layer.
- There are existing issues with the error handling that should not be replicated into views.
- If you need to make changes to ensure views have proper error handling you should do so.
| } | ||
|
|
||
| @Override | ||
| public ApiResponse<GetAllEntityResponseBody<UserTable>> getViewEntities(UserTable userView) { |
There was a problem hiding this comment.
Why is this returning Table? The comment is basically "define UserView" object and return that.
| * first code to see an absent entity. The wire field stays nullable for rolling compatibility: a | ||
| * payload may agree with its route or stay silent, never override it. | ||
| */ | ||
| private static UserTable stampEntityType(UserTable userTable, EntityType entityType) { |
There was a problem hiding this comment.
Do not use unchecked exceptions RequestValidationFailureException.
| return EntityType.fromName(columnValue); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new IllegalArgumentException( | ||
| throw new CorruptEntityTypeException( |
There was a problem hiding this comment.
The unchecked throw is not really a choice the converter gets to make. AttributeConverter.convertToEntityAttribute is a JPA SPI method whose signature declares no checked exception, so a checked module-owned failure cannot be thrown from here — it would have to be wrapped in an unchecked one to escape, which is what already happens.
The one case where I think unchecked exceptions are fine. :)
mkuchenbecker
left a comment
mkuchenbecker
left a comment
There was a problem hiding this comment.
@ruolin59 I have done a pass on the open comments in code. Please review.
| */ | ||
| UserTableDto getUserTable(String databaseId, String tableId); | ||
|
|
||
| /** |
There was a problem hiding this comment.
Use checked exceptions.
| Page<UserTableDto> getAllUserTables(UserTable userTable, int page, int size, String sortBy); | ||
|
|
||
| /** Given a databaseId and tableId, delete the user table entry from the House Table. */ | ||
| /** |
There was a problem hiding this comment.
The practical problem with fixing it views-only: it leaves views speaking an owned vocabulary while tables speaks the transport's, in one interface, with no principle distinguishing them.
The principle is that we can't move them all at once, and we are not perpetuating the existing pattern.
|
|
||
| String VIEW = "VIEW"; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Are views actually compatible with the casing issue? We need to normalize the representation the view definition is my expectation.
| .findById( | ||
| UserTableRowPrimaryKey.builder().databaseId(databaseId).tableId(tableId).build()) | ||
| .orElseThrow(() -> new NoSuchUserTableException(databaseId, tableId)); | ||
| UserTableRowPrimaryKey key = |
There was a problem hiding this comment.
Its not pre-existing for views, and if its an issue with tables please file a Jira.
The eight view metric names sat in the shared MetricsConstant, but only housetables reads them: the user tables service emits them and one test asserts them. Renaming one meant publishing a module that nothing else in the chain cares about. They now live in HouseTablesMetricsConstant beside the code that emits them. The names themselves are unchanged, so nothing reading them downstream moves. The table constants stay where they are; this moves what is already view-only, not everything. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CorruptEntityTypeException extended IllegalArgumentException, which made a server-state failure catch-compatible with client-input failures. It extends RuntimeException now. Unchecked is still required: the attribute converter that raises it implements a JPA SPI method declaring no checked exception. The handler javadoc justified the explicit advice by that ancestry. The advice is still needed for its own reason, which the javadoc now states: the catch-all would answer a generic body in place of the diagnostic naming the column and the value. Two converter tests asserted IllegalArgumentException on the read path, which only held through the ancestry. They assert the corruption type directly, and one drops an instanceof check the assertion now makes for it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The soft-delete path reads the row, archives that snapshot and then deletes without a version predicate, so a concurrent write can leave the archive holding a version other than the one removed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The view query path took the transport UserTable all the way into the service. That model carries nine fields and also serves PUT, paging and soft-delete responses, so the signature promised far more than the six fields the path actually reads. Give the path its own UserViewQuery holding databaseId, tableId, tableVersion, metadataLocation, storageType and creationTime, and convert to it at the handler boundary once the incoming UserTable has been validated. The conversion sits with the other transport-to-internal hops in UserTablesMapper, and is a field-for-field copy: the three list/pattern/search branches dispatch on which fields are null, so defaulting anything here would silently reroute a query. entityType does not come along. The query path never read it, and the type is already fixed by the route and by the view predicate in the SQL. The table query keeps its own predicates and its own UserTable, so database enumeration is untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
2 participants
What this does
This adds everything a view needs to be created, read, listed and dropped through HTS, and closes the gap it left open: type was selected by method on the read paths, but key mutations were still neutral.
Base is
views-entity-type-discriminatorwhich added theentity_typecolumn to handle the new "VIEW" type. HTS must ship before any tables-service code that calls the new paths.~600 lines of production code across 12 files; the rest of the diff is tests.
Endpoints
All five view routes are new. Table routes are pre-existing, but PUT gains ingress stamping and DELETE and rename become table-scoped.
GET /hts/tablesGET /hts/viewsGET /hts/tables/queryGET /hts/views/queryGET /v1/hts/tables/queryGET /v1/hts/views/queryPUT /hts/tablesPUT /hts/viewsDELETE /hts/tablesDELETE /hts/viewsDELETE /v1/hts/tables?isSoftDelete=PATCH /hts/tables/renamePlus
GET /hts/entities, a neutral point read answering what occupies a key, of either type, for collision detection that must see both.Repository surface
15 unchanged, 2 modified, 1 removed, 12 added — 29 methods.
Type is selected by the method or route you call; no public caller supplies a type selector.
renameTableIdstamps the discriminator from a string constant in the query itself, so no caller supplies it. That constant is not derived fromEntityType:EntityType.TABLE.name()is a method call and therefore illegal in an annotation value, so renaming an enum constant would not update the query.Design notes
Key mutations are type-scoped. One conditional statement carries the type predicate, so a wrong-type delete or rename affects zero rows and returns 404 with the row intact — rather than read-then-check-then-delete, a TOCTOU window on an irreversible operation.
Inherited key-addressed deletes are sealed.
deleteById,delete(T),deleteAllByIdanddeleteAll(Iterable)throw, so the only way to remove a row is a method that names the type. No-argdeleteAll()stays: it addresses no key, so it cannot confuse one type for another.Writes stamp the discriminator at ingress, before validation, so an un-upgraded tables service that sends nothing still produces a correctly typed row — which is why the wire field stays nullable. A body contradicting its route is 400; a write over a row of the other type is 409 rather than a silent conversion, and that check runs before version mapping because a type collision is not a stale write.
TABLE_ROW_PREDICATEkeeps itsIS NULLarm. There is no backfill, so legacy rows remain SQL NULL and must keep reading as tables.Known limitation
Typed JPQL predicates inherit the column collation. The deployment measured below uses
utf8mb4_0900_ai_ci, which is accent-insensitive, so a stored'TÁBLE'is matched by the predicate, reaches the converter and fails every read. Not reproducible in H2, which is case-sensitive inMODE=MySQL.Documented rather than fixed: binary-exact comparison is not expressible in JPQL, and
COLLATEwould mean native SQL across twelve queries composed from shared constants.ENTITY_TYPE_REGEXrejects such values at every endpoint, so only a direct DB write creates one, and no collation makesVIEWmatchTABLE— cross-type deletion stays impossible.Trailing spaces do diverge. An earlier version of this description claimed they did not; that was wrong, and
a8aa8fd0reverts the code that assumed it. Under a NO PAD collationupper(entity_type) = 'TABLE'does not match'TABLE ', so the typed routes answer 404 while the neutral read hydrates the row and 500s. Strict parsing makes that visible on every route but does not make the row recoverable, because deletes are predicate-gated in exactly the same way — the row is then unreachable by any HTTP route and removable only by direct SQL. Under a PAD SPACE collation the same row is matched, converted and entirely healthy.The production collation is unverified.
ddl/0000__baseline.sqlspecifies none, so the container inherits the MySQL server default and every measurement here is against that. If production differs, the behaviour above differs with it. ASHOW CREATE TABLEfrom production would settle it.Unfiltered queries are unbounded, and this PR does not change that.
COMMON_FILTER_CLAUSESis entirely(:param IS NULL OR ...), soGET /hts/tables/queryandGET /hts/views/querywith no filter return every row of that type, unpaginated. That is pre-existing behaviour on tables; the view route inherits it by symmetry rather than introducing it./v1/hts/{tables,views}/queryis the paged path. Worth addressing, but not here, since any fix that is not views-only is a breaking change to a live endpoint.Deploy and rollback
Additive to the generated client, so existing callers are unaffected.
Rolling back removes the views feature, so it is a data migration: view rows become unreachable and should be deleted, before reverting the column. Dropping
entity_typewhile they remain would make them indistinguishable from tables. Table reads are safe either way, since BDP-108403 already filters them.Testing
279 tests in
services/housetables, 15 inservices/common, 0 failures.Pre-existing tests were modified in three categories: assertions that changed because the behaviour changed, call sites updated for changed signatures, and seeding updated because a legacy NULL discriminator can no longer be written through JPA. Nine assertion lines were removed and replaced with stronger or type-scoped equivalents; no assertion was weakened. Eight tests are labelled as preserved-behaviour regression tests — they pass on the base commit too, and exist to pin behaviour this PR must not change.
:client:htsregenerates with 6 new operations alongside the 10 existing ones. The addition is ABI-compatible, but regeneration also reorders properties in the shared page models, which changesObjects.hash(...)input order, JSON field order andtoString()output. Generated sources are not committed here, so that is a property of the generator over the current schema rather than a diff in this PR — but a consumer that hashes a mutable page or compares serialized text can observe it.Deployment tests
Everything above runs through MockMvc against H2. #698 (
linkedin/openhouse, "BDP-108627: Cover House Tables against a deployed MySQL") stacks on this branch and adds 23 tests that drive a deployed service over HTTP against MySQL, together with the compose recipe and CI wiring needed to run them. They pass: 23 passed, 0 skipped.The database is built by the
ddl/*.sqlfiles themselves, which the service never executes: against an empty schema0000__baseline.sqlcreates the four tables and0001__add_entity_type_to_user_table_row.sqlthen addsentity_type, itsALGORITHM=INSTANTaccepted on MySQL 8.4.11 and the column landing last as that file's comment predicts. Spring'sschema.sqlruns afterwards asCREATE TABLE IF NOT EXISTSand no-ops over them, so the service starts on exactly the schema the DDL produced rather than the bootstrap one, and the tests cover the migration path as well as the endpoints. It also means the column's collation is whatever the server defaults to, since the DDL names none — the gap described under Known limitation.Out of scope
ViewCatalog, the view commit protocol,
OpenHouseInternalViewOperations, occupancy wiring intoTablesServiceImpl, the Iceberg 1.2/1.5 bean boundary, and orphan-directory reclamation — all BDP-108407.