fix: address v0.1 acceptance review (1 BLOCKER + 6 IMPORTANT/NIT findings)

BLOCKER: wire Hibernate multi-tenancy - application.yaml: set hibernate.tenant_identifier_resolver and hibernate.multiTenancy=DISCRIMINATOR so HibernateTenantResolver is actually installed into the SessionFactory - AuditedJpaEntity.tenantId: add @org.hibernate.annotations.TenantId so every PBC entity inherits the discriminator - AuditedJpaEntityListener.onCreate: throw if a caller pre-set tenantId to a different value than the current TenantContext, instead of silently overwriting (defense against cross-tenant write bugs) IMPORTANT: dependency hygiene - pbc-identity no longer depends on platform-bootstrap (wrong direction; bootstrap assembles PBCs at the top of the stack) - root build.gradle.kts: tighten the architectural-rule enforcement to also reject :pbc:* -> platform-bootstrap; switch plug-in detection from a fragile pathname heuristic to an explicit extra["vibeerp.module-kind"] = "plugin" marker; reference plug-in declares the marker IMPORTANT: api.v1 surface additions (all non-breaking) - Repository: documented closed exception set; new PersistenceExceptions.kt declares OptimisticLockConflictException, UniqueConstraintViolationException, EntityValidationException, and EntityNotFoundException so plug-ins never see Hibernate types - TaskContext: now exposes tenantId(), principal(), locale(), correlationId() so workflow handlers (which run outside an HTTP request) can pass tenant-aware calls back into api.v1 - EventBus: subscribe() now returns a Subscription with close() so long-lived subscribers can deregister explicitly; added a subscribe(topic: String, ...) overload for cross-classloader event routing where Class<E> equality is unreliable - IdentityApi.findUserById: tightened from Id<*> to PrincipalId so the type system rejects "wrong-id-kind" mistakes at the cross-PBC boundary NITs: - HealthController.kt -> MetaController.kt (file name now matches the class name); added TODO(v0.2) for reading implementationVersion from the Spring Boot BuildProperties bean

fix: address v0.1 acceptance review (1 BLOCKER + 6 IMPORTANT/NIT findings)
BLOCKER: wire Hibernate multi-tenancy - application.yaml: set hibernate.tenant_identifier_resolver and hibernate.multiTenancy=DISCRIMINATOR so HibernateTenantResolver is actually installed into the SessionFactory - AuditedJpaEntity.tenantId: add @org.hibernate.annotations.TenantId so every PBC entity inherits the discriminator - AuditedJpaEntityListener.onCreate: throw if a caller pre-set tenantId to a different value than the current TenantContext, instead of silently overwriting (defense against cross-tenant write bugs) IMPORTANT: dependency hygiene - pbc-identity no longer depends on platform-bootstrap (wrong direction; bootstrap assembles PBCs at the top of the stack) - root build.gradle.kts: tighten the architectural-rule enforcement to also reject :pbc:* -> platform-bootstrap; switch plug-in detection from a fragile pathname heuristic to an explicit extra["vibeerp.module-kind"] = "plugin" marker; reference plug-in declares the marker IMPORTANT: api.v1 surface additions (all non-breaking) - Repository: documented closed exception set; new PersistenceExceptions.kt declares OptimisticLockConflictException, UniqueConstraintViolationException, EntityValidationException, and EntityNotFoundException so plug-ins never see Hibernate types - TaskContext: now exposes tenantId(), principal(), locale(), correlationId() so workflow handlers (which run outside an HTTP request) can pass tenant-aware calls back into api.v1 - EventBus: subscribe() now returns a Subscription with close() so long-lived subscribers can deregister explicitly; added a subscribe(topic: String, ...) overload for cross-classloader event routing where Class<E> equality is unreliable - IdentityApi.findUserById: tightened from Id<*> to PrincipalId so the type system rejects "wrong-id-kind" mistakes at the cross-PBC boundary NITs: - HealthController.kt -> MetaController.kt (file name now matches the class name); added TODO(v0.2) for reading implementationVersion from the Spring Boot BuildProperties bean
vibe_erp
1 parent d8aa39eb
Showing 12 changed files with 275 additions and 25 deletions
api/api-v1/src/main/kotlin/org/vibeerp/api/v1/event/EventBus.kt
api/api-v1/src/main/kotlin/org/vibeerp/api/v1/ext/identity/IdentityApi.kt
api/api-v1/src/main/kotlin/org/vibeerp/api/v1/persistence/PersistenceExceptions.kt
api/api-v1/src/main/kotlin/org/vibeerp/api/v1/persistence/Repository.kt
api/api-v1/src/main/kotlin/org/vibeerp/api/v1/workflow/TaskHandler.kt
build.gradle.kts
distribution/src/main/resources/application.yaml
pbc/pbc-identity/build.gradle.kts
pbc/pbc-identity/src/main/kotlin/org/vibeerp/pbc/identity/ext/IdentityApiAdapter.kt
platform/platform-bootstrap/src/main/kotlin/org/vibeerp/platform/bootstrap/web/HealthController.kt → platform/platform-bootstrap/src/main/kotlin/org/vibeerp/platform/bootstrap/web/MetaController.kt
platform/platform-persistence/src/main/kotlin/org/vibeerp/platform/persistence/audit/AuditedJpaEntity.kt
reference-customer/plugin-printing-shop/build.gradle.kts
@@ -31,11 +31,46 @@ interface EventBus {
     fun publish(event: DomainEvent)
     /**
-     * Register a listener for events of [eventType] (and its subtypes).
+     * Register a typed listener for events of [eventType] (and its subtypes).
      *
-     * The listener stays registered until the plug-in is stopped, at which
-     * point the platform tears it down automatically — plug-ins do not
-     * need (and should not have) a manual `unsubscribe` method.
+     * Plug-ins can ignore the returned [Subscription] — when the plug-in is
+     * stopped, the platform tears down all of its listeners automatically.
+     * Core PBCs and tooling that legitimately need to drop a subscription
+     * mid-life (feature toggles, completed sagas, per-tenant subscriptions)
+     * call `subscription.close()` to deregister explicitly.
      */
-    fun <E : DomainEvent> subscribe(eventType: Class<E>, listener: EventListener<E>)
+    fun <E : DomainEvent> subscribe(
+        eventType: Class<E>,
+        listener: EventListener<E>,
+    ): Subscription
+
+    /**
+     * Register a listener keyed on the event's [DomainEvent.aggregateType]
+     * string instead of its concrete Java class.
+     *
+     * Why this overload exists: cross-classloader event listening cannot rely
+     * on `Class<E>` equality, because two plug-ins compiling against the
+     * same `OrderCreated` event class will see two different `Class` objects
+     * loaded by two different classloaders. The string topic is the only
+     * stable identifier across the boundary.
+     *
+     * @param topic the [DomainEvent.aggregateType] (or topic-style) string
+     *   to match. Convention: `<plugin-or-pbc>.<aggregate>` (e.g.
+     *   `orders_sales.Order`, `printingshop.Plate`).
+     */
+    fun subscribe(
+        topic: String,
+        listener: EventListener<DomainEvent>,
+    ): Subscription
+
+    /**
+     * Handle returned by `subscribe`, used to deregister explicitly.
+     *
+     * Implements [AutoCloseable] so `use { ... }` patterns work in tests
+     * and short-lived listeners.
+     */
+    interface Subscription : AutoCloseable {
+        /** Deregister this listener. Idempotent: calling twice is a no-op. */
+        override fun close()
+    }
 }
@@ -2,6 +2,7 @@ package org.vibeerp.api.v1.ext.identity
 import org.vibeerp.api.v1.core.Id
 import org.vibeerp.api.v1.core.TenantId
+import org.vibeerp.api.v1.security.PrincipalId
 /**
  * Cross-PBC facade for the identity bounded context.
@@ -32,11 +33,12 @@ interface IdentityApi {
     fun findUserByUsername(username: String): UserRef?
     /**
-     * Look up a user by id. The id is `Id<*>` rather than `Id<UserRef>` so
-     * callers do not have to know the exact entity type — they pass the id
-     * they have, and the implementation does the matching internally.
+     * Look up a user by their principal id. The id is typed as
+     * [PrincipalId] (which is `Id<Principal>`) so the compiler refuses to
+     * accept, e.g., an `Id<Order>` here. Callers that have a generic id
+     * from an audit row construct a `PrincipalId(rawUuid)` deliberately.
      */
-    fun findUserById(id: Id<*>): UserRef?
+    fun findUserById(id: PrincipalId): UserRef?
 }
 /**
+package org.vibeerp.api.v1.persistence
+
+/**
+ * The closed set of exceptions that [Repository] is allowed to raise.
+ *
+ * Why a small custom hierarchy in api.v1 instead of letting JPA exceptions
+ * leak through:
+ *
+ *   • Plug-ins live in their own classloader and cannot import JPA or
+ *     Hibernate types — those packages are not on the parent classloader's
+ *     exported list. A `jakarta.persistence.OptimisticLockException` thrown
+ *     across the plug-in/host boundary would surface as a wrapped runtime
+ *     error the plug-in cannot meaningfully `catch`.
+ *
+ *   • Plug-in authors need a stable, semver-governed contract for "what
+ *     can go wrong when I save this thing?". JPA exception names and
+ *     hierarchies have shifted across Hibernate majors more than once.
+ *
+ *   • Optimistic-locking conflicts and unique-constraint violations are
+ *     **expected** failure modes (the kind of thing the [Result] doctrine
+ *     applies to). They aren't bugs; they're business outcomes that callers
+ *     legitimately want to handle.
+ *
+ * The platform translates underlying JPA / Hibernate exceptions to these
+ * types at the persistence boundary. Anything else (the database is down,
+ * the connection pool is exhausted, …) bubbles up as the generic
+ * [PersistenceException] and is treated as infrastructure failure.
+ *
+ * **Adding a new subtype** is a non-breaking change within api.v1.x as long
+ * as the new type extends [PersistenceException] (so existing `catch`
+ * blocks still match). **Removing or renaming** a subtype is a major bump.
+ */
+sealed class PersistenceException(
+    message: String,
+    cause: Throwable? = null,
+) : RuntimeException(message, cause)
+
+/**
+ * The save was rejected because another transaction concurrently updated
+ * the same row. The caller's in-memory copy is stale; reload, re-apply the
+ * intent, and try again — or surface a "someone else changed this" UX.
+ *
+ * Backed by Hibernate's `OptimisticLockException` / `StaleObjectStateException`.
+ */
+class OptimisticLockConflictException(
+    val entityName: String,
+    val entityId: String,
+    cause: Throwable? = null,
+) : PersistenceException(
+    "Optimistic lock conflict on $entityName id=$entityId — the row was modified by another transaction",
+    cause,
+)
+
+/**
+ * A unique constraint was violated by the save. [constraint] is the index
+ * or constraint name as reported by the database (e.g.
+ * `identity__user_tenant_username_uk`). The platform surfaces the raw name
+ * because plug-ins legitimately want to map specific constraints to
+ * specific user-facing messages.
+ */
+class UniqueConstraintViolationException(
+    val entityName: String,
+    val constraint: String,
+    cause: Throwable? = null,
+) : PersistenceException(
+    "Unique constraint '$constraint' violated on $entityName",
+    cause,
+)
+
+/**
+ * The save failed validation against entity invariants enforced by the
+ * platform (custom-field constraints, jakarta.validation annotations, etc.)
+ * [errors] is a per-field map of message keys (NOT translated strings) so
+ * the caller can render them in any locale.
+ */
+class EntityValidationException(
+    val entityName: String,
+    val errors: Map<String, String>,
+) : PersistenceException(
+    "Validation failed on $entityName: ${errors.entries.joinToString(", ") { "${it.key}=${it.value}" }}",
+)
+
+/**
+ * The targeted entity does not exist for the current tenant. Distinct from
+ * `findById` returning `null`: this is for operations that semantically
+ * require the row to exist (update, delete, increment, …).
+ */
+class EntityNotFoundException(
+    val entityName: String,
+    val entityId: String,
+) : PersistenceException(
+    "$entityName with id=$entityId not found in current tenant",
+)
@@ -43,12 +43,26 @@ interface Repository&lt;T : Entity&gt; {
      * Insert or update. Returning the saved entity (rather than `Unit`) gives
      * the platform a place to populate generated columns (audit timestamps,
      * generated ids) before the plug-in sees them again.
+     *
+     * The closed set of expected failure modes is declared in
+     * [PersistenceException]. The platform translates underlying JPA /
+     * Hibernate exceptions to those types at the persistence boundary so
+     * plug-ins never see Hibernate types crossing their classloader.
+     *
+     * @throws OptimisticLockConflictException when another transaction
+     *   concurrently updated the same row.
+     * @throws UniqueConstraintViolationException when a unique index would
+     *   be violated.
+     * @throws EntityValidationException when entity invariants fail.
      */
     fun save(entity: T): T
     /**
      * Delete by id. Returns `true` if a row was deleted, `false` if no
      * matching row existed for the current tenant.
+     *
+     * Does NOT throw [EntityNotFoundException] — use `findById` first if
+     * you need "must exist" semantics.
      */
     fun delete(id: Id<T>): Boolean
 }
 package org.vibeerp.api.v1.workflow
+import org.vibeerp.api.v1.core.TenantId
+import org.vibeerp.api.v1.security.Principal
+import java.util.Locale
+
 /**
  * Implementation contract for a single workflow task type.
  *
@@ -65,4 +69,37 @@ interface TaskContext {
      * Passing `null` removes the variable.
      */
     fun set(name: String, value: Any?)
+
+    /**
+     * Owning tenant of the workflow instance this task belongs to.
+     *
+     * Workflow tasks do NOT run inside an HTTP request, so the usual
+     * `RequestContext` injection is unavailable. The platform binds the
+     * task's tenant context for the duration of [TaskHandler.execute] and
+     * exposes it here so handlers can pass tenant-aware calls back into
+     * api.v1 services.
+     */
+    fun tenantId(): TenantId
+
+    /**
+     * The principal whose action triggered this task instance. For
+     * automatically scheduled tasks (timer-driven, signal-driven), this
+     * is a system principal — never null.
+     */
+    fun principal(): Principal
+
+    /**
+     * The locale to use for any user-facing strings produced by this task
+     * (notification titles, audit messages, etc.). Resolved from the
+     * triggering user, the workflow definition, or the tenant default,
+     * in that order.
+     */
+    fun locale(): Locale
+
+    /**
+     * Correlation id for this task execution. The same id appears in the
+     * structured logs and the audit log so an operator can trace a task
+     * end-to-end.
+     */
+    fun correlationId(): String
 }
@@ -11,19 +11,33 @@ allprojects {
     version = providers.gradleProperty("vibeerp.version").get()
 }
-// Enforce the architecture-level dependency rule (CLAUDE.md guardrail #9):
+// Enforce the architecture-level dependency rules (CLAUDE.md guardrail #9):
 //
-//   • PBCs may NEVER depend on other PBCs.
-//     Cross-PBC interaction goes through api.v1 service interfaces or the
-//     event bus.
+//   1. PBCs may NEVER depend on other PBCs.
+//      Cross-PBC interaction goes through api.v1.ext.<pbc> service
+//      interfaces or the event bus.
 //
-//   • Plug-ins (and the reference plug-in) may ONLY depend on :api:api-v1.
-//     They never see :platform:* or :pbc:* internals.
+//   2. PBCs may NEVER depend on `:platform:platform-bootstrap`.
+//      Bootstrap is the application entry point that ASSEMBLES PBCs;
+//      depending on it inverts the layering. PBCs depend on platform
+//      runtime modules (persistence, plugins, i18n, etc.) only.
 //
-// The build fails at configuration time if either rule is violated.
+//   3. Plug-ins (modules carrying the `vibeerp.module-kind=plugin` ext
+//      property) may ONLY depend on `:api:api-v1`. They never see
+//      `:platform:*` or `:pbc:*` internals.
+//
+// All three rules fail the build at configuration time.
+//
+// We use an explicit `extra["vibeerp.module-kind"]` marker rather than a
+// pathname heuristic so that adding a plug-in under a different directory
+// (e.g. `customer-plugins/foo/`) does not silently bypass the rule, and
+// so that `:platform:platform-plugins` (which contains the substring
+// "plugin-" in its path) is correctly classified as a runtime module, not
+// a plug-in.
 gradle.projectsEvaluated {
     rootProject.allprojects.forEach { p ->
         val ownPath: String = p.path
+        val moduleKind: String? = p.extra.properties["vibeerp.module-kind"] as String?
         val checkedConfigs = listOf("api", "implementation", "compileOnly", "compileClasspath")
         val projectDeps = p.configurations
@@ -32,10 +46,12 @@ gradle.projectsEvaluated {
                 cfg.dependencies.filterIsInstance<org.gradle.api.artifacts.ProjectDependency>()
             }
+        // Rule 1 & 2 — PBC dependency hygiene
         if (ownPath.startsWith(":pbc:")) {
             projectDeps.forEach { dep ->
                 @Suppress("DEPRECATION")
                 val depPath = dep.dependencyProject.path
+
                 if (depPath.startsWith(":pbc:") && depPath != ownPath) {
                     throw GradleException(
                         "Architectural violation in $ownPath: depends on $depPath. " +
@@ -43,12 +59,18 @@ gradle.projectsEvaluated {
                             "interfaces (org.vibeerp.api.v1.ext.<pbc>) or the event bus."
                     )
                 }
+                if (depPath == ":platform:platform-bootstrap") {
+                    throw GradleException(
+                        "Architectural violation in $ownPath: depends on $depPath. " +
+                            "PBCs MUST NOT depend on platform-bootstrap (the application " +
+                            "entry point). Depend on platform runtime modules only."
+                    )
+                }
             }
         }
-        val isPlugin = ownPath.startsWith(":reference-customer:") ||
-            ownPath.contains("plugin-")
-        if (isPlugin) {
+        // Rule 3 — Plug-in dependency hygiene (driven by an explicit marker)
+        if (moduleKind == "plugin") {
             projectDeps.forEach { dep ->
                 @Suppress("DEPRECATION")
                 val depPath = dep.dependencyProject.path
@@ -22,6 +22,18 @@ spring:
     hibernate:
       ddl-auto: validate
     open-in-view: false
+    # Multi-tenant wall #1 (the application-layer wall):
+    # Hibernate's DISCRIMINATOR strategy means every query and every save
+    # is automatically filtered/written with the tenant id from
+    # `HibernateTenantResolver`, which reads `TenantContext` (set per-request
+    # by `TenantResolutionFilter`). Wall #2 — Postgres Row-Level Security —
+    # is enforced by the `RlsTransactionHook` (planned: implementation-plan
+    # P1.1) once it lands. Both walls are required by CLAUDE.md guardrail #5;
+    # disabling either is a release-blocker.
+    properties:
+      hibernate:
+        tenant_identifier_resolver: org.vibeerp.platform.persistence.tenancy.HibernateTenantResolver
+        multiTenancy: DISCRIMINATOR
   liquibase:
     change-log: classpath:db/changelog/master.xml
@@ -26,13 +26,19 @@ allOpen {
     annotation("jakarta.persistence.Embeddable")
 }
-// CRITICAL: pbc-identity may depend on api-v1 and platform-* but NEVER on
-// another pbc-*. The root build.gradle.kts enforces this; if you add a
-// `:pbc:pbc-foo` dependency below, the build will refuse to load.
+// CRITICAL: pbc-identity may depend on api-v1 and platform-* runtime modules
+// (persistence, plugins, i18n, etc.) but NEVER on platform-bootstrap, and
+// NEVER on another pbc-*.
+//
+//   • No cross-PBC: enforced by the root build.gradle.kts.
+//   • No dependency on platform-bootstrap: bootstrap is the application
+//     entry point that ASSEMBLES PBCs at the top of the stack. Depending
+//     on it inverts the layering — every change to the HTTP filter or the
+//     properties bean would force a recompile of every PBC, and a future
+//     PBC graph would tangle into an unfixable cycle.
 dependencies {
     api(project(":api:api-v1"))
     implementation(project(":platform:platform-persistence"))
-    implementation(project(":platform:platform-bootstrap"))
     implementation(libs.kotlin.stdlib)
     implementation(libs.kotlin.reflect)
@@ -6,9 +6,9 @@ import org.vibeerp.api.v1.core.Id
 import org.vibeerp.api.v1.core.TenantId
 import org.vibeerp.api.v1.ext.identity.IdentityApi
 import org.vibeerp.api.v1.ext.identity.UserRef
+import org.vibeerp.api.v1.security.PrincipalId
 import org.vibeerp.pbc.identity.domain.User
 import org.vibeerp.pbc.identity.infrastructure.UserJpaRepository
-import java.util.UUID
 /**
  * Concrete [IdentityApi] implementation. The ONLY thing other PBCs and
@@ -36,7 +36,7 @@ class IdentityApiAdapter(
     override fun findUserByUsername(username: String): UserRef? =
         users.findByUsername(username)?.toRef()
-    override fun findUserById(id: Id<*>): UserRef? =
+    override fun findUserById(id: PrincipalId): UserRef? =
         users.findById(id.value).orElse(null)?.toRef()
     private fun User.toRef(): UserRef = UserRef(
@@ -20,6 +20,10 @@ class MetaController {
     fun info(): Map<String, Any> = mapOf(
         "name" to "vibe-erp",
         "apiVersion" to "v1",
+        // TODO(v0.2): read implementationVersion from the Spring Boot
+        // BuildProperties bean once `springBoot { buildInfo() }` is wired
+        // up in the distribution module. Hard-coding a fallback here is
+        // fine for v0.1 but will silently drift from `gradle.properties`.
         "implementationVersion" to (javaClass.`package`.implementationVersion ?: "0.1.0-SNAPSHOT"),
     )
 }
@@ -6,6 +6,7 @@ import jakarta.persistence.MappedSuperclass
 import jakarta.persistence.PrePersist
 import jakarta.persistence.PreUpdate
 import jakarta.persistence.Version
+import org.hibernate.annotations.TenantId
 import org.vibeerp.platform.persistence.tenancy.TenantContext
 import java.time.Instant
 import java.util.UUID
@@ -36,6 +37,15 @@ abstract class AuditedJpaEntity {
     @Column(name = "id", updatable = false, nullable = false, columnDefinition = "uuid")
     open var id: UUID = UUID.randomUUID()
+    /**
+     * Tenant discriminator. Hibernate's `@TenantId` makes this column the
+     * automatic filter for every query and save: the value is sourced from
+     * `HibernateTenantResolver`, which in turn reads `TenantContext`. PBC
+     * code MUST NOT write to this property directly — the JPA listener
+     * fills it in on `@PrePersist`. Attempting to set it to a value other
+     * than the current tenant before save throws (see [AuditedJpaEntityListener]).
+     */
+    @TenantId
     @Column(name = "tenant_id", updatable = false, nullable = false, length = 64)
     open var tenantId: String = ""
@@ -71,6 +81,17 @@ class AuditedJpaEntityListener {
             val now = Instant.now()
             val tenant = TenantContext.currentOrNull()
                 ?: error("Cannot persist ${entity::class.simpleName}: no tenant in context")
+
+            // Defense against the "buggy caller pre-set tenantId to the wrong
+            // value" failure mode flagged in the v0.1 acceptance review. Silent
+            // overwrite would mask a real bug; throwing here surfaces it.
+            if (entity.tenantId.isNotBlank() && entity.tenantId != tenant.value) {
+                error(
+                    "Cannot persist ${entity::class.simpleName}: entity tenantId='${entity.tenantId}' " +
+                        "but current TenantContext is '${tenant.value}'. Cross-tenant write attempt?"
+                )
+            }
+
             val principal = currentPrincipal()
             entity.tenantId = tenant.value
             entity.createdAt = now
@@ -4,6 +4,10 @@ plugins {
 description = "vibe_erp reference plug-in: a hello-world printing-shop customer expressed entirely through api.v1."
+// Marker that the root build.gradle.kts uses to apply the plug-in dependency
+// rule (no `:platform:*`, no `:pbc:*`, only `:api:api-v1`).
+extra["vibeerp.module-kind"] = "plugin"
+
 java {
     toolchain {
         languageVersion.set(JavaLanguageVersion.of(21))