HTS API alternative implementation: Give views a typed lifecycle in HTS (alternative to #697) by ruolin59 · Pull Request #703 · linkedin/openhouse

ruolin59 · 2026-08-31T16:39:32Z

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.

@hidden

Adds everything a view needs to be created, read, listed and dropped through the House Tables Service, plus a neutral entity read for name occupancy. This is an alternative implementation of the same ticket as linkedin#697. The HTTP contract and the SQL layer are deliberately identical to that PR; what differs is the wiring between them, which is rebuilt along the lines of the review feedback on linkedin#697. Frozen from linkedin#697: - All five view routes, the neutral read, and the table routes, with identical params, bodies and status codes. - Every JPQL constant and query, the typed conditional deletes that return an affected-row count, and the type-scoped rename. Rebuilt between those two ends: - putUserTable and putUserView each supply their own EntityType to one private persistence primitive, so neither named method can persist the other's type regardless of what the transport object carries. - The new point reads return Optional; only the handler turns absence into 404, so a dependency or hydration failure can never read as "the name is free". - View queries cross the service boundary as an owned UserViewQuery / PagedUserViewQuery rather than a transport UserTable, and the invalid "pattern without database" state is unconstructible. - Corrupt-discriminator hydration is translated to a module-owned, non-IllegalArgumentException failure at a housetables persistence adapter that consumes every result to exhaustion, so a corrupt row fails the whole call instead of yielding a partial list or page. - services:common keeps no ORM knowledge: a controller-scoped advice in housetables declares three @hidden mappings and everything else falls through to the shared advice, which is refactored behaviour-neutrally onto an extracted ErrorResponseBodyFactory. - PUT ingress validation returns an explicit outcome rather than throwing from a helper. - View metrics are owned by housetables. Existing table paths are reused as-is and not refactored; the resulting asymmetries between the old table paths and the new view paths are deliberate and documented. Tests: housetables 463, common 20, tables 496, jobs 52 - all passing. :client:hts regenerates cleanly with the six new operations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

@throws

Comments only; no code, behaviour, assertion or name changed. Removes javadoc that restated the name it sat on, and collapses the repeated @throws boilerplate on UserTableReadRepository and UserTablesService into a single statement of the failure contract at the type level. Load-bearing rationale is kept but shortened: the legacy-NULL arm of TABLE_ROW_PREDICATE, the collation assumption, why views never route through soft delete, why the type collision precedes version mapping, why the named service method owns the EntityType, why the read adapter consumes results to exhaustion, why deleteAll() is left unsealed, and the deliberately pinned stack-trace quirk. Also corrects one stale test javadoc that claimed to pin the collision ordering after that assertion moved to its own test. 459 comment lines removed; comments fall from 17% to 11% of the change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

@hidden

Replaces the scoped-advice design with the standard pattern: catch at the persistence boundary and rethrow an owned type, applied uniformly to table and view reads rather than views only. All 11 table read call sites now go through the translating repository alongside the view reads. It catches DataAccessException, and rethrows the unwrapped CorruptEntityTypeException when the cause chain holds one; any other failure is rethrown unchanged, so non-corrupt infrastructure failures behave exactly as before. Because corruption is unwrapped before it leaves the repository, the shared advice needs one @hidden handler and no ORM vocabulary. That removes the machinery the previous design needed to keep JPA types out of services:common: - UserHouseTablesExceptionHandler (controller-scoped advice) - ErrorResponseBodyFactory and the delegation in the shared advice - the ServiceAuditAspect pointcut widening - four housetables exception types and the cause finder - the mutation-failure wrapper on view writes services:common is now +22 lines in OpenHouseExceptionHandler and one new exception class; ServiceAuditAspect is byte-identical to base. CorruptEntityTypeException does not extend IllegalArgumentException, so stored-data corruption stays distinguishable from client-input error. View mutations now surface raw DataAccessException as a generic 500, matching table mutations rather than differing from them. Table behaviour is unchanged: no pre-existing test was modified, and tables (496) and jobs (52) match base counts. The one intended difference is that a corrupt discriminator on a table read now reports the offending column and value instead of the ORM toString(). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

The four key-addressed generic deletes were sealed because each can remove a row of the wrong type. No-arg deleteAll() was left open only because test teardown called it, which is test convenience shaping a production contract. Seals it the same way and moves whole-store cleanup into a test-only UserTableStoreCleaner: a bulk JPQL delete, chosen over a second CrudRepository because @EnableJpaRepositories lives in main-scope configuration. It removes rows regardless of discriminator, so a planted corrupt row cannot survive teardown, matching what deleteAll() did. Only three pre-existing teardowns are affected. The job tests autowire a different repository, and the two iceberg implementations implement HtsRepository directly rather than the JDBC sub-interface, so neither is touched. No pre-existing assertion changed; services/common is untouched. Tables 496 and jobs 52 match base counts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Three read-then-write windows survive this change because they are pre-existing and sit in paths it does not otherwise touch. Marking them so they are not mistaken for hazards the typed conditional statements already closed. Soft delete archives an unlocked snapshot and then deletes without a version predicate, so a writer committing in between makes the archive stale and loses that commit; the conditional delete closes only the type and existence window. Rename has the same missing version guard. Restore's occupancy check is a read-then-write whose real backstop is the DataIntegrityViolationException catch below it. Comments only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

EntityTypeIngressResult existed so normalization could report failure explicitly rather than throw. Its only caller immediately unwrapped it and threw RequestValidationFailureException, so the indirection moved the throw two frames and bought nothing; the unreachable orElse fallback in the controller was the tell. The validator now throws that exception directly and returns the normalized entity, the result type is deleted, and the controller calls it inline rather than through a private unwrapping helper. Behaviour is unchanged: same two validation cases, same messages, same 400 via the existing advice. The validator stays an injectable component so the rule is still unit-tested directly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

The IS NULL arm of TABLE_ROW_PREDICATE exists to carry rows that predate the discriminator. Nothing should be adding new ones, but the converter happily wrote null, so that depended on every caller remembering to stamp a type rather than on anything enforcing it. convertToDatabaseColumn now rejects null, making the guarantee structural at the last point before the column. It throws IllegalArgumentException rather than CorruptEntityTypeException, and the two stay disjoint: an unstamped write is a caller bug, corrupt stored data is server state. Read-side parsing is unchanged. EntityType.fromName(null) still returns null and the wire field stays nullable, because the type is decided by the route and stamped before mapping. Test fixtures that persisted a legacy row through JPA now insert it by raw SQL. That is far wider than the four seedLegacyRow helpers: the shared TestTuple builder omits entityType, so every row derived from it was a null write. Raw seeding leaves the stored bytes identical, so no assertion changed; only the three lines in EntityTypeConverterTest that asserted the old pass-through behaviour were replaced, by parameterized equivalents plus a test pinning the rejection. Not HTTP-observable: a PUT that omits entityType over a legacy NULL occupant still returns 200 and migrates the column. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

The adapter's javadoc said consuming every result completely was "the point", because a corrupt row found after the boundary would surface as a partial success. That overstates it. The frozen finders return Optional, Iterable and Page, all of which Hibernate hydrates during the query, so corruption already throws inside the translation; none returns a Stream. Materializing inside the boundary is defensive against a lazily-returning finder being added later, not a guard against something reachable today. The three tests making that claim drove the point home by constructing a hand-rolled iterable that fails mid-iteration, which real Spring Data does not hand back here. They are renamed to say they pin a hypothetical, and the fixture says why it exists. The translation itself is unaffected, and is what the class is for. Comments and test names only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

The type bundled four values that its only consumer unpacked one frame later, and the guarantee claimed for it -- that an unpaged call cannot carry paging and a paged one cannot omit it -- already comes from having two overloads with different parameter lists. It also overshot the comment it was implementing. That review asked to parse the request once into a UserViewQuery "containing only the accepted database ID, optional table pattern, and paging data", twice calling it the smallest fix. One type, not two. List<UserTableDto> getAllUserViews(UserViewQuery query); Page<UserTableDto> getAllUserViews(UserViewQuery query, int page, int size, String sortBy); That is also the shape of the table pair it sits beside. UserViewQuery stays: its named factories are the only enumeration of the three reachable query states. The signature change alters no behaviour. Route defaults, the service's tableId sort default and the HTTP contract are untouched. The captor-based handler tests now capture the query and match the paging arguments, asserting the same facts; a deliberate page/size swap and an injected sort default were used to confirm they still fail. Also states that views cannot be soft-deleted by design, so the missing isSoftDelete parameter does not read as unimplemented. Two comments had the causality backwards, offering the soft-deleted store's lack of a discriminator as the reason for the decision. It is the consequence: the column is absent because views are not soft-deleted, and it would be added if that ever changed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

UserTableReadRepository and its implementation were 333 lines, of which 31 did the work: catching DataAccessException, recovering the converter's CorruptEntityTypeException from the wrapper, and rethrowing everything else untouched. The rest was thirteen wrappers re-exposing finders the frozen repository already had, plus an interface to declare them, plus 640 lines of tests largely pinning that structure. The seam was also wrong rather than merely large. findTableRow and findRowForWrite returned JPA entities, so the boundary did not contain what it claimed to, and findRowForWrite served the write path from a class named read repository. The translation is now a static utility, unchanged line for line including the depth bound and the identity cycle guard. The service calls the frozen repository directly and wraps each call, and DTO mapping returns to the shapes it had before this branch touched it, so the table reads end up closer to their original form. The eager materialize helpers are dropped rather than relocated. Every frozen finder returns Optional, Iterable or Page, all hydrated during the query, so corruption already surfaces inside the translation; they guarded a Stream-returning finder that does not exist. Behaviour is unmoved, checked by mutation rather than assertion: suppressing the unwrap fails 16 tests, and wrapping an unrelated DataAccessException instead of rethrowing it fails 4, including the byte-identical generic body and the neutral read refusing to report a failure as absence. Cycle and depth coverage is restored; it had been lost when the earlier cause finder was folded into the adapter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

@order

The read collapse left translation as a discipline requirement: 16 call sites each wrapped their repository read in translating(...), and a read added later without the wrap would return a generic 500 with no diagnostic and still compile. Sixteen wraps against eight reads is not obviously complete from inspection, and findViewRows hid four reads inside a helper the caller wrapped -- exactly the shape that hides a gap. An @around aspect on every UserTableHtsJdbcRepository method makes it structural: a corrupt discriminator surfaces with its column-and-value diagnostic whoever calls the repository, and a new read needs nothing at its call site. The interception point was verified empirically rather than reasoned about, because it can fail silently. Spring Data builds its own proxy holding PersistenceExceptionTranslationInterceptor and TransactionInterceptor; this advice lands in a second, outer proxy, so it sees the translated DataAccessException, not the converter's raw PersistenceException, and sits outside the repository transaction exactly where the call-site wraps did. That nesting is structural: no @order value can move the advice inside Spring Data's chain, so none is declared. A negative control -- disabling the aspect with the 16 wraps removed -- failed 12 tests, proving the coverage is real. RepositoryTranslationAspectTest pins both halves: that the advice runs outside PETI (so a Spring Data upgrade relocating it fails loudly rather than downgrading every corrupt read to a generic 500), and that a corrupt row carries the diagnostic with no call-site wrap anywhere. Advising writes is harmless: translating rethrows every non-corruption DataAccessException as the same instance, so the 409 conflict mapping is unchanged, verified across the cross-type PUT and occupied-rename tests. CorruptEntityTypeTranslation and its unit tests are unchanged. Verified on real MySQL 8.4: the deployment e2e suite passes 23/0, the corrupt-row split among them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

@hidden

Eight tests, all added by this branch, removed after confirming their coverage survives elsewhere -- verified by mutation, not by inspection: - UserHouseTablesControllerApiContractTest (3): reflected over the @ApiResponses annotations; UserHouseTablesOpenApiContractTest pins the same matrix from the document springdoc actually emits, which is strictly stronger. A 404->410 mutation failed both. - testLegacyRowPlantedThroughTheColumnStillReadsAsTable: a strict subset of the base testStoredNullHydratesAsTable, which now seeds through the same raw SQL. - testNeutralReadIsThePathThatSurfacesCorruption: a strict subset of testGetNeutralEntityAtCorruptKeyFailsLoudly. - testViewDeleteTranslationDoesNotTurnAbsenceIntoAFailure: absence-is- false is pinned by six survivors; its name referenced a wrapper deleted earlier. - the HtsControllerTest copy of testViewPatternQueryKeepsUnderscoreAsA- SqlWildcard: underscore-as-wildcard is a SQL property; making the wildcard literal failed the surviving service test on its own, so the HTTP copy caught nothing distinct. - testTheScopedAdviceContributesNoResponsesToAnyOperation: named a scoped advice deleted three commits ago, and cannot fail for the cause it claims. Removing all 19 @hidden from OpenHouseExceptionHandler and regenerating leaves the document byte-identical, because the handlers carry no @ResponseStatus for springdoc to attribute. It was also a strict subset of testGeneratedDocumentDeclaresExactlyTheFrozenResponse- Codes. The @hidden annotations are left in place: harmless, matching the base, and defensive if a handler ever gains a @ResponseStatus. Two rationale javadocs were moved verbatim onto the survivors so no explanation was lost. No assertion was added, changed or weakened -- the seven added lines are all comments. No base test or production code touched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

The deployed collation for user_table_row.entity_type is confirmed utf8mb4_0900_ai_ci -- accent-insensitive and NO PAD. This was the open question flagged across both PR stacks; the repository comment hedged it as unconfirmed and assumed the opposite (a collation folding neither accents nor trailing spaces). No code change. The behaviour is already correct for it: a stored 'TÁBLE' matches the type predicate but fails hydration as a diagnostic 500, and a 'TABLE ' does not match at all. Both require a direct database write, since the API writes only canonical enum names, so neither is a reachable state. It also settles the EntityType.fromName trailing-space question -- the no-trim behaviour is correct under NO PAD, which is why it was reverted earlier. Comment only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

GET /hts/views/query is dropped. Views can be listed only through the paginated /v1/hts/views/query. This is a deliberate divergence from linkedin#697, whose view query mirrored the table shape. The non-paginated table query /hts/tables/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. Views are new and have no caller -- a trace confirmed neither the tables service nor the internal catalog references any view query path -- so mirroring that shape would inherit tech debt into a greenfield surface. Paginated-only also removes the unbounded-query concern for views entirely. Removed: the getUserViews route and its endpoint constant, the non-paged handler and service methods, the two Iterable-returning view finders, and the two now-unused HTS_LIST_VIEWS_* metrics. The paginated path, the point reads, the neutral read, every table path including the non-paginated one, and all shared JPQL constants are untouched. Tests that held assertions with no paged equivalent were retargeted to the paged path rather than deleted, so no coverage was lost. The generated client now exposes five new operations rather than six; getUserViews is absent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

abhisheknath2011 · 2026-09-08T16:15:18Z

+ * silent, never override it.
+ */
+@Component
+public class EntityTypeIngressValidator {


Ingress also means API gateway layer. In which layer this validator is invoked?

this should be the ingress for HouseTables service, aka HTS

abhisheknath2011 · 2026-09-08T16:17:35Z


  @Autowired private UserTablesMapper userTablesMapper;

+  @Autowired private EntityTypeIngressValidator entityTypeIngressValidator;


As this used at controller layer, can just keep the validator name as just EntityTypeValidator?

yeah i also think it's a bit confusing which file's for which service, I'll look into a better name for this

Review feedback: "Ingress" reads as the API-gateway layer, and the name did not say which service the validator belongs to. It runs at the HTS controller layer, so name it for that service (matching the module's existing OpenHouseUserTableHtsApiValidator convention) and reword the class doc to say where it is invoked. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

abhisheknath2011 · 2026-09-08T18:23:02Z

+ * on a corrupt discriminator, and Spring's persistence exception translation buries that inside a
+ * {@link DataAccessException} whose message is the wrapper's rather than the diagnostic.
+ */
+public final class CorruptEntityTypeTranslation {


Seems like translation is happing as part of JPA layer exception. Can we make this code generic so that this can be applied to other exception as well if needed?

Kept it specific on purpose. There's exactly one exception that needs unwrapping today (CorruptEntityTypeException), and this util is actually what a much larger read-repository abstraction earlier in this PR got collapsed down to. The reusable part — the cause walk in findCorruptEntityTypeCause — is already a generic algorithm; only the target type is fixed.

What I'd rather not generalize yet is the policy in translating() (unwrap corruption, rethrow everything else as the same instance): a second exception would likely want a different policy — its own HTTP status, maybe no unwrap at all — which is better designed against a real second case than guessed now. If one shows up, generalizing the finder to findCause(Throwable, Class<T>) is a ~2-line change.

abhisheknath2011 · 2026-09-08T18:24:25Z

+public final class CorruptEntityTypeTranslation {
+
+  /** Bounds the cause walk, so a cyclic chain terminates instead of spinning. */
+  private static final int CAUSE_CHAIN_MAX_DEPTH = 20;


How did arrive at this max depth? Should we increase the depth so that it covers important stacktrace?

Good catch that this reads ambiguously. CAUSE_CHAIN_MAX_DEPTH bounds the cause chain (getCause() hops between nested exceptions), not stack-trace frames — so it has no effect on how much of the stack trace is captured (that's preserved via exception chaining regardless).

It isn't really load-bearing for termination either: the identity visited-set already guarantees we stop (finitely many distinct causes), so the cap is just a secondary bound on a pathologically long acyclic chain. The real corruption cause sits ~2-3 hops down (DataAccessException → optional JPA wrapper → CorruptEntityTypeException), so 20 is already generous — no need to increase. Reworded the comment in 25a5b31 to make the cause-chain-vs-stack-trace distinction explicit.

… trace Review question read CAUSE_CHAIN_MAX_DEPTH as limiting stack-trace capture. It bounds the getCause() chain (nested exceptions), which is unrelated to stack-trace frames; reword the comment to say so and to note the identity visited-set is what guarantees termination. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

abhisheknath2011 · 2026-09-08T21:31:02Z

+ */
+@Aspect
+@Component
+public class UserTableRepositoryTranslationAspect {


Why do we need aspect for translation? Trying to understand the context behind this.

This is addressing two #697 blocking comments that got deferred there: converter throws on corruption during hydration but nothing translates it before it leaves housetables, and common advice having to import JpaSystemException etc. The ask was to translate at the housetables persistence boundary. The aspect is that boundary. Shared advice (what #697 does) makes common import ORM types, and a per-call-site try/catch is fragile since hydration is lazy (a findAll might not throw til it's iterated, outside the try). The aspect wraps every repo method instead, so every read incl future ones gets translated at the edge and common stays ORM-free.

abhisheknath2011 · 2026-09-08T21:33:57Z

+ */
+@EqualsAndHashCode
+@ToString
+public final class UserViewQuery {


Why do we need a separate class for mapping/validating query params specifically for tableId and databaseId?

This is the fix Mike asked for on #697. He named UserViewQuery in these two comments. Point is to keep the transport UserTable (nullable everything + an inert entityType) out of the service's query contract; UserViewQuery only lets the 3 real states exist and rejects pattern-without-db at construction. #697 deferred it to stay consistent with the base table-query methods. Here views are a new path so we took the principled route and left tables alone.

abhisheknath2011 · 2026-09-08T21:44:05Z

+    }

+    // Overwritten before mapping, so no transport spelling reaches the enum boundary.
+    UserTable ownedEntity = userTable.toBuilder().entityType(entityType.name()).build();


Suggested change

UserTable ownedEntity = userTable.toBuilder().entityType(entityType.name()).build();

UserTable updatedEntity = userTable.toBuilder().entityType(entityType.name()).build();

Went with stampedEntity instead of updatedEntity: this method is an upsert so "updated" reads a bit ambiguous, and "stamped" matches the existing stamp-the-type language here (the ingress validator + STAMP_TABLE_TYPE). Renamed in 5eb646f.

abhisheknath2011 · 2026-09-08T21:58:16Z

+ * null write so that the legacy population the {@code IS NULL} predicate arm carries cannot grow.
+ */
+@Component
+public class UserTableRawSeeder {


Where this is used? Why JPA can't handle this usecase. Also why do we need this as well the new create will have either entity type as table or view.

This is only used for testing (the seedLegacyRow split Mike asked for on #697). It plants a legacy NULL entity_type row the app can't create anymore since null writes are rejected, so tests can verify those rows still read back as TABLE.

abhisheknath2011 · 2026-09-08T21:59:06Z

+  /** Removes every row regardless of discriminator, so a corrupt row cannot survive a teardown. */
+  @Transactional
+  public void clear() {
+    entityManager.createQuery("DELETE FROM UserTableRow").executeUpdate();


Why do we need this? There is no filter condition.

This is only used for testing. It clears the whole store between tests, which the repo no longer exposes now that deleteAll() is sealed.

Review suggested updatedEntity; stampedEntity reads clearer in an upsert method and matches the existing stamp-the-type vocabulary in this module (the ingress validator and STAMP_TABLE_TYPE). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

abhisheknath2011 · 2026-09-10T18:26:25Z

+      value = {
+        @ApiResponse(responseCode = "204", description = "User View DELETE: NO_CONTENT"),
+        @ApiResponse(responseCode = "400", description = "User View DELETE: BAD_REQUEST"),
+        @ApiResponse(responseCode = "404", description = "User View DELETE: TBL_DB_NOT_FOUND")


Do we need to have 500 for corrupt entity here?

no need, we aren't checking for the specific entitytype here, only doing a query of "delete ... where ... entity_type = VIEW", so corrupt entities are ignored and a 404 will return

abhisheknath2011 · 2026-09-10T18:34:50Z

+
+  @Around(
+      "execution(* com.linkedin.openhouse.housetables.repository.impl.jdbc.UserTableHtsJdbcRepository.*(..))")
+  public Object translateCorruption(ProceedingJoinPoint joinPoint) {


Did we verify if the aspect actually works?

This is critical because the whole corruption→500 contract hinges on the aspect and this can silently fail as well.

Yeah, RepositoryTranslationAspectTest covers exactly this. testAspectRunsOutsidePersistenceExceptionTranslationSoItSeesTheTranslatedFailure checks it two ways: it walks the real repo proxy chain and asserts the aspect sits in an outer proxy than PersistenceExceptionTranslationInterceptor, then plants a real UNKNOWN row via raw JDBC and asserts the real read comes back as an unwrapped CorruptEntityTypeException. testCorruptRowStillCarriesTheDiagnosticWithNoCallSiteWrapAnywhere does the same through the service with no call-site wrap, and testCorruptDiscriminatorResponseCarriesColumnDiagnostic in HtsControllerTest drives it over real HTTP (GET /hts/entities on a real corrupt row, 500 + diagnostic).

On the silent-fail worry: that structural assertion is the guard. If a Spring Data upgrade moved the translator so the aspect no longer sat outside it, that test fails loudly instead of every corrupt read quietly degrading to a generic 500.

abhisheknath2011

Thanks @ruolin59 for addressing the comments!

ruolin59 marked this pull request as draft August 31, 2026 23:46

ruolin59 force-pushed the rufan-linkedin-views-typed-lifecycle-hts branch 2 times, most recently from 5c0e542 to acdcb63 Compare September 1, 2026 00:13

ruolin59 changed the title ~~BDP-108627: Give views a typed lifecycle in HTS (alternative to #697)~~ HTS API alternative implementation: Give views a typed lifecycle in HTS (alternative to #697) Sep 1, 2026

ruolin59 force-pushed the rufan/views-entity-type-discriminator branch from 96d4e7c to 47ac84e Compare September 1, 2026 21:25

ruolin59 marked this pull request as ready for review September 1, 2026 22:39

ruolin59 force-pushed the rufan-linkedin-views-typed-lifecycle-hts branch from 6cd4a82 to 0063cbb Compare September 1, 2026 22:57

ruolin59 mentioned this pull request Sep 3, 2026

HTS API: Give views a typed lifecycle in HTS #697

Closed

ruolin59 and others added 14 commits September 3, 2026 09:54

ruolin59 force-pushed the rufan-linkedin-views-typed-lifecycle-hts branch from 0063cbb to d900980 Compare September 3, 2026 16:57

abhisheknath2011 reviewed Sep 8, 2026

View reviewed changes

aastha25 approved these changes Sep 10, 2026

View reviewed changes

abhisheknath2011 reviewed Sep 10, 2026

View reviewed changes

abhisheknath2011 approved these changes Sep 10, 2026

View reviewed changes

ruolin59 merged commit faaa56b into linkedin:rufan/views-entity-type-discriminator Sep 10, 2026


		@Autowired private UserTablesMapper userTablesMapper;

		@Autowired private EntityTypeIngressValidator entityTypeIngressValidator;

	UserTable ownedEntity = userTable.toBuilder().entityType(entityType.name()).build();
	UserTable updatedEntity = userTable.toBuilder().entityType(entityType.name()).build();

Conversation

ruolin59 commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this is

What is deliberately unchanged

JDBC repository surface (UserTableHtsJdbcRepository)

What is rebuilt, and why

Footprint on shared code

Deliberate asymmetries

Not adopted

Testing

A note on a pre-existing flake

Review process

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhisheknath2011 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ruolin59 commented Aug 31, 2026 •

edited

Loading

JDBC repository surface (`UserTableHtsJdbcRepository`)