ENT-1712 Warning on cross-reference between JPA entities from different mapped schemas (#3282)

At startup node warns about any MappedSchema containing a JPA entity referencing to another JPA entity from a different MappedSchema. Cross reference between JPA entities across different MappedSchema objects can cause operational issues while evolving one MappedSchema object or migrating it's data. Doc API: Persistence documentation no longer suggests mapping between different schemas.

core/src/main/kotlin/net/corda/core/schemas/PersistentTypes.kt

@@ -91,3 +91,43 @@ data class PersistentStateRef(
  * Marker interface to denote a persistable Corda state entity that will always have a transaction id and index
  */
 interface StatePersistable : Serializable
+
+object MappedSchemaValidator {
+    fun fieldsFromOtherMappedSchema(schema: MappedSchema) : List<SchemaCrossReferenceReport> =
+            schema.mappedTypes.map { entity ->
+                entity.declaredFields.filter { field ->
+                    field.type.enclosingClass != null
+                            && MappedSchema::class.java.isAssignableFrom(field.type.enclosingClass)
+                            && hasJpaAnnotation(field.declaredAnnotations)
+                            && field.type.enclosingClass != schema.javaClass
+                }.map { field -> SchemaCrossReferenceReport(schema.javaClass.name, entity.simpleName, field.type.enclosingClass.name, field.name, field.type.simpleName)}
+            }.flatMap { it.toSet() }
+
+    fun methodsFromOtherMappedSchema(schema: MappedSchema) : List<SchemaCrossReferenceReport> =
+            schema.mappedTypes.map { entity ->
+                entity.declaredMethods.filter { method ->
+                    method.returnType.enclosingClass != null
+                            && MappedSchema::class.java.isAssignableFrom(method.returnType.enclosingClass)
+                            && method.returnType.enclosingClass != schema.javaClass
+                            && hasJpaAnnotation(method.declaredAnnotations)
+                }.map { method -> SchemaCrossReferenceReport(schema.javaClass.name, entity.simpleName, method.returnType.enclosingClass.name, method.name, method.returnType.simpleName)}
+            }.flatMap { it.toSet() }
+
+    fun crossReferencesToOtherMappedSchema(schema: MappedSchema) : List<SchemaCrossReferenceReport> =
+         fieldsFromOtherMappedSchema(schema) + methodsFromOtherMappedSchema(schema)
+
+    /** Returns true if [javax.persistence] annotation expect [javax.persistence.Transient] is found. */
+    private inline fun hasJpaAnnotation(annotations: Array<Annotation>) =
+            annotations.any { annotation -> annotation.toString().startsWith("@javax.persistence.") && annotation !is javax.persistence.Transient }
+
+    class SchemaCrossReferenceReport(private val schema: String, private val entity: String, private val referencedSchema: String,
+                                     private val fieldOrMethod: String, private val fieldOrMethodType: String) {
+
+        override fun toString() = "Cross-reference between MappedSchemas '$schema' and '$referencedSchema'. " +
+                "MappedSchema '${schema.substringAfterLast(".")}' entity '$entity' field '$fieldOrMethod' is of type '$fieldOrMethodType' " +
+                "defined in another MappedSchema '${referencedSchema.substringAfterLast(".")}'."
+
+        fun toWarning() = toString() + " This may cause issues when evolving MappedSchema or migrating its data, " +
+                "ensure JPA entities are defined within the same enclosing MappedSchema."
+    }
+}

core/src/test/java/net/corda/core/schemas/BadSchemaJavaV1.java

@@ -0,0 +1,37 @@
+package net.corda.core.schemas;
+
+import javax.persistence.*;
+import java.util.Arrays;
+
+public class BadSchemaJavaV1 extends MappedSchema {
+
+    public BadSchemaJavaV1() {
+        super(TestJavaSchemaFamily.class, 1, Arrays.asList(State.class));
+    }
+
+    @Entity
+    public static class State extends PersistentState {
+        private String id;
+        private GoodSchemaJavaV1.State other;
+
+        @Column
+        public String getId() {
+            return id;
+        }
+
+        public void setId(String id) {
+            this.id = id;
+        }
+
+        @JoinColumns({@JoinColumn(name = "itid"), @JoinColumn(name = "outid")})
+        @OneToOne
+        @MapsId
+        public GoodSchemaJavaV1.State getOther() {
+            return other;
+        }
+
+        public void setOther(GoodSchemaJavaV1.State other) {
+            this.other = other;
+        }
+    }
+}

core/src/test/java/net/corda/core/schemas/BadSchemaNoGetterJavaV1.java

@@ -0,0 +1,29 @@
+package net.corda.core.schemas;
+
+import javax.persistence.*;
+import java.util.Arrays;
+
+public class BadSchemaNoGetterJavaV1 extends MappedSchema {
+
+    public BadSchemaNoGetterJavaV1() {
+        super(TestJavaSchemaFamily.class, 1, Arrays.asList(State.class));
+    }
+
+    @Entity
+    public static class State extends PersistentState {
+        @JoinColumns({@JoinColumn(name = "itid"), @JoinColumn(name = "outid")})
+        @OneToOne
+        @MapsId
+        public GoodSchemaJavaV1.State other;
+        private String id;
+
+        @Column
+        public String getId() {
+            return id;
+        }
+
+        public void setId(String id) {
+            this.id = id;
+        }
+    }
+}

core/src/test/java/net/corda/core/schemas/GoodSchemaJavaV1.java

@@ -0,0 +1,26 @@
+package net.corda.core.schemas;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import java.util.Arrays;
+
+public class GoodSchemaJavaV1 extends MappedSchema {
+
+    public GoodSchemaJavaV1() {
+        super(TestJavaSchemaFamily.class, 1, Arrays.asList(State.class));
+    }
+
+    @Entity
+    public static class State extends PersistentState {
+        private String id;
+
+        @Column
+        public String getId() {
+            return id;
+        }
+
+        public void setId(String id) {
+            this.id = id;
+        }
+    }
+}

core/src/test/java/net/corda/core/schemas/PoliteSchemaJavaV1.java

@@ -0,0 +1,37 @@
+package net.corda.core.schemas;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.Transient;
+import java.util.Arrays;
+
+public class PoliteSchemaJavaV1 extends MappedSchema {
+
+    public PoliteSchemaJavaV1() {
+        super(TestJavaSchemaFamily.class, 1, Arrays.asList(State.class));
+    }
+
+    @Entity
+    public static class State extends PersistentState {
+        private String id;
+        private GoodSchemaJavaV1.State other;
+
+        @Column
+        public String getId() {
+            return id;
+        }
+
+        public void setId(String id) {
+            this.id = id;
+        }
+
+        @Transient
+        public GoodSchemaJavaV1.State getOther() {
+            return other;
+        }
+
+        public void setOther(GoodSchemaJavaV1.State other) {
+            this.other = other;
+        }
+    }
+}

core/src/test/java/net/corda/core/schemas/TestJavaSchemaFamily.java

@@ -0,0 +1,4 @@
+package net.corda.core.schemas;
+
+public class TestJavaSchemaFamily {
+}

core/src/test/java/net/corda/core/schemas/TrickySchemaJavaV1.java

@@ -0,0 +1,36 @@
+package net.corda.core.schemas;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import java.util.Arrays;
+
+public class TrickySchemaJavaV1 extends MappedSchema {
+
+    public TrickySchemaJavaV1() {
+        super(TestJavaSchemaFamily.class, 1, Arrays.asList(TrickySchemaJavaV1.State.class));
+    }
+
+    @Entity
+    public static class State extends PersistentState {
+        private String id;
+        private GoodSchemaJavaV1.State other;
+
+        @Column
+        public String getId() {
+            return id;
+        }
+
+        public void setId(String id) {
+            this.id = id;
+        }
+
+        //the field is a cross-reference to other MappedSchema however the field is not persistent (no JPA annotation)
+        public GoodSchemaJavaV1.State getOther() {
+            return other;
+        }
+
+        public void setOther(GoodSchemaJavaV1.State other) {
+            this.other = other;
+        }
+    }
+}

core/src/test/kotlin/net/corda/core/schemas/MappedSchemasCrossReferenceDetectionTests.kt

@@ -0,0 +1,108 @@
+package net.corda.core.schemas
+
+import net.corda.core.schemas.MappedSchemaValidator.fieldsFromOtherMappedSchema
+import net.corda.core.schemas.MappedSchemaValidator.methodsFromOtherMappedSchema
+import net.corda.finance.schemas.CashSchema
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.Test
+import javax.persistence.*
+
+class MappedSchemasCrossReferenceDetectionTests {
+
+    object GoodSchema : MappedSchema(schemaFamily = CashSchema.javaClass, version = 1, mappedTypes = listOf(State::class.java)) {
+        @Entity
+        class State(
+                @Column
+                var id: String
+        ) : PersistentState()
+    }
+
+    object BadSchema : MappedSchema(schemaFamily = CashSchema.javaClass, version = 1, mappedTypes = listOf(State::class.java)) {
+        @Entity
+        class State(
+                @Column
+                var id: String,
+
+                @JoinColumns(JoinColumn(name = "itid"), JoinColumn(name = "outid"))
+                @OneToOne
+                @MapsId
+                var other: GoodSchema.State
+        ) : PersistentState()
+    }
+
+    object TrickySchema : MappedSchema(schemaFamily = CashSchema.javaClass, version = 1, mappedTypes = listOf(State::class.java)) {
+        @Entity
+        class State(
+                @Column
+                var id: String,
+
+                //the field is a cross-reference to other MappedSchema however the field is not persistent (no JPA annotation)
+                var other: GoodSchema.State
+        ) : PersistentState()
+    }
+
+    object PoliteSchema : MappedSchema(schemaFamily = CashSchema.javaClass, version = 1, mappedTypes = listOf(State::class.java)) {
+        @Entity
+        class State(
+                @Column
+                var id: String,
+
+                @Transient
+                var other: GoodSchema.State
+        ) : PersistentState()
+    }
+
+    @Test
+    fun `no cross reference to other schema`() {
+        assertThat(fieldsFromOtherMappedSchema(GoodSchema)).isEmpty()
+        assertThat(methodsFromOtherMappedSchema(GoodSchema)).isEmpty()
+    }
+
+    @Test
+    fun `cross reference to other schema is detected`() {
+        assertThat(fieldsFromOtherMappedSchema(BadSchema)).isNotEmpty
+        assertThat(methodsFromOtherMappedSchema(BadSchema)).isEmpty()
+    }
+
+    @Test
+    fun `cross reference via non JPA field is allowed`() {
+        assertThat(fieldsFromOtherMappedSchema(TrickySchema)).isEmpty()
+        assertThat(methodsFromOtherMappedSchema(TrickySchema)).isEmpty()
+    }
+
+    @Test
+    fun `cross reference via transient field is allowed`() {
+        assertThat(fieldsFromOtherMappedSchema(PoliteSchema)).isEmpty()
+        assertThat(methodsFromOtherMappedSchema(PoliteSchema)).isEmpty()
+    }
+
+    @Test
+    fun `no cross reference to other schema java`() {
+        assertThat(fieldsFromOtherMappedSchema(GoodSchemaJavaV1())).isEmpty()
+        assertThat(methodsFromOtherMappedSchema(GoodSchemaJavaV1())).isEmpty()
+    }
+
+    @Test
+    fun `cross reference to other schema is detected java`() {
+        assertThat(fieldsFromOtherMappedSchema(BadSchemaJavaV1())).isEmpty()
+        assertThat(methodsFromOtherMappedSchema(BadSchemaJavaV1())).isNotEmpty
+    }
+
+    @Test
+    fun `cross reference to other schema via field is detected java`() {
+        assertThat(fieldsFromOtherMappedSchema(BadSchemaNoGetterJavaV1())).isNotEmpty
+        assertThat(methodsFromOtherMappedSchema(BadSchemaNoGetterJavaV1())).isEmpty()
+    }
+
+    @Test
+    fun `cross reference via non JPA field is allowed java`() {
+        assertThat(fieldsFromOtherMappedSchema(TrickySchemaJavaV1())).isEmpty()
+        assertThat(methodsFromOtherMappedSchema(TrickySchemaJavaV1())).isEmpty()
+    }
+
+    @Test
+    fun `cross reference via transient field is allowed java`() {
+        assertThat(fieldsFromOtherMappedSchema(PoliteSchemaJavaV1())).isEmpty()
+        assertThat(methodsFromOtherMappedSchema(PoliteSchemaJavaV1())).isEmpty()
+    }
+}

docs/source/api-persistence.rst

@@ -57,11 +57,15 @@ integration points and not necessarily with every upgrade to the contract code.
 ``MappedSchema`` offered by a ``QueryableState``, automatically upgrade to a later version of a schema or even
 provide a ``MappedSchema`` not originally offered by the ``QueryableState``.
 
-It is expected that multiple different contract state implementations might provide mappings to some common schema.
+It is expected that multiple different contract state implementations might provide mappings within a single schema.
-For example an Interest Rate Swap contract and an Equity OTC Option contract might both provide a mapping to a common
+For example an Interest Rate Swap contract and an Equity OTC Option contract might both provide a mapping to
-Derivative schema. The schemas should typically not be part of the contract itself and should exist independently of it
+a Derivative contract within the same schema. The schemas should typically not be part of the contract itself and should exist independently
 to encourage re-use of a common set within a particular business area or Cordapp.
 
+.. note:: It's advisable to avoid cross-references between different schemas as this may cause issues when evolving ``MappedSchema``
+   or migrating its data. At startup, nodes log such violations as warnings stating that there's a cross-reference between ``MappedSchema``'s.
+   The detailed messages incorporate information about what schemas, entities and fields are involved.
+
 ``MappedSchema`` offer a family name that is disambiguated using Java package style name-spacing derived from the
 class name of a *schema family* class that is constant across versions, allowing the ``SchemaService`` to select a
 preferred version of a schema.

docs/source/changelog.rst

@@ -132,6 +132,10 @@ Unreleased
 
 * Table name with a typo changed from ``NODE_ATTCHMENTS_CONTRACTS`` to ``NODE_ATTACHMENTS_CONTRACTS``.
 
+* Node logs a warning for any ``MappedSchema`` containing a JPA entity referencing another JPA entity from a different ``MappedSchema`.
+  The log entry starts with `Cross-reference between MappedSchemas.`.
+  API: Persistence documentation no longer suggests mapping between different schemas.
+
 .. _changelog_v3.1:
 
 Version 3.1

node/src/main/kotlin/net/corda/node/internal/AbstractNode.kt

@@ -232,6 +232,11 @@ abstract class AbstractNode(val configuration: NodeConfiguration,
         initCertificate()
         initialiseJVMAgents()
         val schemaService = NodeSchemaService(cordappLoader.cordappSchemas, configuration.notary != null)
+        schemaService.mappedSchemasWarnings().forEach {
+            val warning = it.toWarning()
+            log.warn(warning)
+            Node.printWarning(warning)
+        }
         val (identity, identityKeyPair) = obtainIdentity(notaryConfig = null)
 
         // Wrapped in an atomic reference just to allow setting it before the closure below gets invoked.

node/src/main/kotlin/net/corda/node/services/schema/NodeSchemaService.kt

@@ -7,6 +7,8 @@ import net.corda.core.schemas.CommonSchemaV1
 import net.corda.core.schemas.MappedSchema
 import net.corda.core.schemas.PersistentState
 import net.corda.core.schemas.QueryableState
+import net.corda.core.schemas.*
+import net.corda.core.schemas.MappedSchemaValidator.crossReferencesToOtherMappedSchema
 import net.corda.core.serialization.SingletonSerializeAsToken
 import net.corda.node.internal.schemas.NodeInfoSchemaV1
 import net.corda.node.services.api.SchemaService
@@ -91,4 +93,9 @@ class NodeSchemaService(extraSchemas: Set<MappedSchema> = emptySet(), includeNot
             return VaultSchemaV1.VaultFungibleStates(state.owner, state.amount.quantity, state.amount.token.issuer.party, state.amount.token.issuer.reference, state.participants)
         return (state as QueryableState).generateMappedObject(schema)
     }
+
+    /** Returns list of [MappedSchemaValidator.SchemaCrossReferenceReport] violations. */
+    fun mappedSchemasWarnings(): List<MappedSchemaValidator.SchemaCrossReferenceReport> =
+            schemaOptions.keys.map { schema -> crossReferencesToOtherMappedSchema(schema) }.flatMap { it.toList() }
+
 }