From c97ee0e11b4bfb4c55a79a8db25df5fb60df5917 Mon Sep 17 00:00:00 2001 From: Blake Regalia Date: Wed, 22 Jul 2026 17:18:53 -0700 Subject: [PATCH 1/2] fix(diff): make POST /diffs functional with repo-addressed diff IRIs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Diff creation was broken end-to-end by several compounding defects: - the route never set diffId, so every reference to the diff resolved to the never-URN placeholder - the request body was filtered against the lock prefix ("morl") instead of the diff prefix - the base conditions required a commit in the request path (there is none on the diffs container), failing every request with 404 - genDiffUpdate ignored its diffTriples and conditions parameters entirely: user-supplied metadata was silently dropped, no conditions (permit, ref validation, uniqueness) were enforced at update time, and ?__mms_policy stayed unbound so the transaction lacked mms:appliedPolicy — which made the validation construct unmatchable and every request fail even after a successful write - cleanup deleted mt: while the transaction was created as mt:diff, leaking a transaction record per request Diffs are now minted at {repo}/diffs/{id} (Slug header or generated), consistent with every other resource; the commit-pair scheme's sha256 is retained as the mms:id and etag. The internal commit pipeline continues to mint its own hash-derived IRIs. Responds 201 with a Location header. Adds end-to-end tests: diff between a lock and a branch (including ins graph content), extra user metadata (exercising serializePairs), generated ids, and transaction cleanup. Co-Authored-By: Claude Fable 5 --- .../org/openmbee/flexo/mms/routes/Diffs.kt | 8 +- .../org/openmbee/flexo/mms/routes/Model.kt | 33 ++-- .../flexo/mms/routes/ldp/DiffCreate.kt | 46 +++-- .../org/openmbee/flexo/mms/DiffCreateTest.kt | 161 ++++++++++++++++++ 4 files changed, 209 insertions(+), 39 deletions(-) create mode 100644 src/test/kotlin/org/openmbee/flexo/mms/DiffCreateTest.kt diff --git a/src/main/kotlin/org/openmbee/flexo/mms/routes/Diffs.kt b/src/main/kotlin/org/openmbee/flexo/mms/routes/Diffs.kt index 91348108..4f30577d 100644 --- a/src/main/kotlin/org/openmbee/flexo/mms/routes/Diffs.kt +++ b/src/main/kotlin/org/openmbee/flexo/mms/routes/Diffs.kt @@ -20,8 +20,14 @@ fun Route.crudDiffs() { } // create a new diff - post { + post { slug -> + // set diff id on context so the diff is minted at {repo}/diffs/{slug} + diffId = slug + createDiff() } + + // method not allowed + otherwiseNotAllowed("diffs") } } \ No newline at end of file diff --git a/src/main/kotlin/org/openmbee/flexo/mms/routes/Model.kt b/src/main/kotlin/org/openmbee/flexo/mms/routes/Model.kt index 7d03768c..ce50eb4c 100644 --- a/src/main/kotlin/org/openmbee/flexo/mms/routes/Model.kt +++ b/src/main/kotlin/org/openmbee/flexo/mms/routes/Model.kt @@ -290,7 +290,7 @@ fun AnyLayer1Context.genCommitUpdate(delete: String="", insert: String="", where /** * Used by ModelLoad to get difference between current staging graph and newly loaded graph in the form of delete/insert graphs */ -fun AnyLayer1Context.genDiffUpdate(diffTriples: String="", conditions: ConditionsGroup?=null, rawWhere: String?=null): String { +fun AnyLayer1Context.genDiffUpdate(diffTriples: String="", conditions: ConditionsGroup?=null, rawWhere: String?=null, explicitDiffIri: String?=null): String { return buildSparqlUpdate { insert { subtxn("diff", mapOf( @@ -304,40 +304,49 @@ fun AnyLayer1Context.genDiffUpdate(diffTriples: String="", conditions: Condition raw(""" graph ?insGraph { - ?ins_s ?ins_p ?ins_o . + ?ins_s ?ins_p ?ins_o . } - + graph ?delGraph { ?del_s ?del_p ?del_o . } - + graph mor-graph:Metadata { ?diff a mms:Diff ; mms:id ?diffId ; + mms:etag ?diffId ; mms:createdBy mu: ; mms:srcCommit ?srcCommit ; mms:dstCommit ?dstCommit ; mms:insGraph ?insGraph ; mms:delGraph ?delGraph . + + $diffTriples } """) } where { raw(""" + ${conditions?.requiredPatterns()?.joinToString("\n") ?: ""} + ${rawWhere?: ""} - + bind( sha256( concat(str(?dstCommit), "\n", str(?srcCommit)) ) as ?diffId ) - - bind( - iri( - concat(str(?dstCommit), "/diffs/", ?diffId) - ) as ?diff - ) - + + ${if(explicitDiffIri != null) """ + bind(<$explicitDiffIri> as ?diff) + """ else """ + bind( + iri( + concat(str(?dstCommit), "/diffs/", ?diffId) + ) as ?diff + ) + """} + bind( iri( concat(str(mor-graph:), "Diff.Ins.", ?diffId) diff --git a/src/main/kotlin/org/openmbee/flexo/mms/routes/ldp/DiffCreate.kt b/src/main/kotlin/org/openmbee/flexo/mms/routes/ldp/DiffCreate.kt index 2f3d6fd7..50946fed 100644 --- a/src/main/kotlin/org/openmbee/flexo/mms/routes/ldp/DiffCreate.kt +++ b/src/main/kotlin/org/openmbee/flexo/mms/routes/ldp/DiffCreate.kt @@ -5,12 +5,15 @@ import io.ktor.server.response.* import org.openmbee.flexo.mms.* import org.openmbee.flexo.mms.server.LdpDcLayer1Context import org.openmbee.flexo.mms.server.LdpPostResponse +import org.openmbee.flexo.mms.routes.sparql.deleteTransaction import org.openmbee.flexo.mms.routes.sparql.genDiffUpdate import java.security.MessageDigest -// default starting conditions for any calls to create a lock -private val DEFAULT_CONDITIONS = COMMIT_CRUD_CONDITIONS.append { +// default starting conditions for any calls to create a diff. the repo must exist; the src and +// dst refs are validated by appendSrcRef()/appendDstRef() (a diff request has no commit in its +// path, so commit-level conditions do not apply here) +private val DEFAULT_CONDITIONS = REPO_CRUD_CONDITIONS.append { // require that the user has the ability to create diffs on a repo-level scope permit(Permission.CREATE_DIFF, Scope.REPO) @@ -46,7 +49,7 @@ suspend fun LdpDcLayer1Context.createDiff() { var createDiffUserDataTriples = "" // process RDF body from user about this new diff - filterIncomingStatements("morl") { + filterIncomingStatements("mord") { // relative to this diff node diffNode().apply { // assert cardinality for src and dst refs @@ -73,26 +76,26 @@ suspend fun LdpDcLayer1Context.createDiff() { // extend the default conditions with requirements for user-specified src and dst refs val localConditions = DEFAULT_CONDITIONS.appendSrcRef().appendDstRef() - // prep SPARQL UPDATE string + // prep SPARQL UPDATE string; the diff is minted at the repo-addressed IRI from the request val updateString = genDiffUpdate(createDiffUserDataTriples, localConditions, """ graph mor-graph:Metadata { # select the commit pointed to by the source ref ?srcRef mms:commit ?srcCommit . - + # locate its corresponding snapshot and model graph ?srcCommit ^mms:commit/mms:snapshot ?srcSnapshot . - ?srcSnapshot a mms:Model ; + ?srcSnapshot a mms:Model ; mms:graph ?srcGraph . - - # select the commit pointed to by the destination ref + + # select the commit pointed to by the destination ref ?dstRef mms:commit ?dstCommit . - + # locate its corresponding snapshot and model graph ?dstCommit ^mms:commit/mms:snapshot ?dstSnapshot . - ?dstSnapshot a mms:Model ; + ?dstSnapshot a mms:Model ; mms:graph ?dstGraph . } - """) + """, explicitDiffIri=prefixes["mord"]!!) // execute update executeSparqlUpdate(updateString) { @@ -141,21 +144,12 @@ suspend fun LdpDcLayer1Context.createDiff() { // check that the user-supplied HTTP preconditions were met handleEtagAndPreconditions(constructModel, prefixes["mord"]) + // provide location of new resource + call.response.header(HttpHeaders.Location, prefixes["mord"]!!) + // respond - call.respondText(constructResponseText, RdfContentTypes.Turtle) - - // delete transaction - run { - // submit update - val dropResponseText = executeSparqlUpdate(""" - delete where { - graph m-graph:Transactions { - mt: ?p ?o . - } - } - """) + call.respondText(constructResponseText, RdfContentTypes.Turtle, HttpStatusCode.Created) - // log response - log.info(dropResponseText) - } + // delete transaction (including the mt:diff subtransaction created by the diff update) + deleteTransaction() } diff --git a/src/test/kotlin/org/openmbee/flexo/mms/DiffCreateTest.kt b/src/test/kotlin/org/openmbee/flexo/mms/DiffCreateTest.kt new file mode 100644 index 00000000..06f5e947 --- /dev/null +++ b/src/test/kotlin/org/openmbee/flexo/mms/DiffCreateTest.kt @@ -0,0 +1,161 @@ +package org.openmbee.flexo.mms + +import io.kotest.assertions.ktor.client.shouldHaveStatus +import io.kotest.assertions.withClue +import io.kotest.matchers.shouldBe +import io.kotest.matchers.string.shouldStartWith +import io.ktor.client.request.* +import io.ktor.client.statement.* +import io.ktor.http.* +import io.ktor.server.testing.ApplicationTestBuilder +import org.apache.jena.rdf.model.ModelFactory +import org.apache.jena.sparql.exec.http.QueryExecutionHTTP +import org.apache.jena.vocabulary.DCTerms +import org.apache.jena.vocabulary.RDF +import org.openmbee.flexo.mms.util.* + +/** + * Tests diff creation via POST /orgs/{org}/repos/{repo}/diffs with repo-addressed diff IRIs. + */ +class DiffCreateTest : RefAny() { + + private val lockId = "before-change" + private val lockPath = "$demoRepoPath/locks/$lockId" + private val diffsPath = "$demoRepoPath/diffs" + + private fun askBackend(query: String): Boolean { + return QueryExecutionHTTP.service(backend.getQueryUrl()).query(query).build().use { + it.execAsk() + } + } + + private fun diffBody(extra: String = ""): String { + return withAllTestPrefixes(""" + <> + mms:srcRef <${localIri(lockPath)}> ; + mms:dstRef <${localIri(masterBranchPath)}> ; + . + $extra + """.trimIndent()) + } + + /** + * Commits a change on master, locks the pre-change commit, commits another change, so the + * lock and master differ by exactly one inserted triple. + */ + private suspend fun ApplicationTestBuilder.setupDivergentRefs() { + commitModel(masterBranchPath, withAllTestPrefixes(""" + insert data { + . + } + """.trimIndent())) + + createLock(demoRepoPath, masterBranchPath, lockId) + + commitModel(masterBranchPath, withAllTestPrefixes(""" + insert data { + . + } + """.trimIndent())) + } + + init { + "create diff between lock and branch" { + testApplication { + setupDivergentRefs() + + httpPost(diffsPath) { + header(HttpHeaders.SLUG, "my-diff") + setTurtleBody(diffBody()) + }.apply { + this shouldHaveStatus HttpStatusCode.Created + this.headers[HttpHeaders.Location] shouldBe localIri("$diffsPath/my-diff") + + // the response carries the diff's metadata at its repo-addressed IRI + this includesTriples { + subject(localIri("$diffsPath/my-diff")) { + includes( + RDF.type exactly MMS.Diff, + ) + } + } + } + + // the ins graph of the diff contains exactly the added triple + withClue("diff ins graph should contain the triple added after the lock") { + askBackend(""" + PREFIX mms: + ASK { + GRAPH <${localIri("$demoRepoPath/graphs/Metadata")}> { + <${localIri("$diffsPath/my-diff")}> mms:insGraph ?insGraph . + } + GRAPH ?insGraph { + . + } + } + """.trimIndent()) shouldBe true + } + } + } + + "create diff with extra user metadata" { + testApplication { + setupDivergentRefs() + + httpPost(diffsPath) { + header(HttpHeaders.SLUG, "titled-diff") + setTurtleBody(diffBody(""" + <> dct:title "A titled diff"@en . + """.trimIndent())) + }.apply { + this shouldHaveStatus HttpStatusCode.Created + + this includesTriples { + subject(localIri("$diffsPath/titled-diff")) { + includes( + RDF.type exactly MMS.Diff, + DCTerms.title exactly "A titled diff".en, + ) + } + } + } + } + } + + "create diff without slug generates an id" { + testApplication { + setupDivergentRefs() + + httpPost(diffsPath) { + setTurtleBody(diffBody()) + }.apply { + this shouldHaveStatus HttpStatusCode.Created + this.headers[HttpHeaders.Location]!! shouldStartWith localIri("$diffsPath/") + } + } + } + + "create diff leaves no dangling transaction" { + testApplication { + setupDivergentRefs() + + httpPost(diffsPath) { + header(HttpHeaders.SLUG, "txn-check-diff") + setTurtleBody(diffBody()) + }.apply { + this shouldHaveStatus HttpStatusCode.Created + } + + withClue("transactions graph should be empty after diff creation") { + askBackend(""" + ASK { + GRAPH <$ROOT_CONTEXT/graphs/Transactions> { + ?txn ?p ?o . + } + } + """.trimIndent()) shouldBe false + } + } + } + } +} From e393c26227d65008e9435d8b77215ec57299594d Mon Sep 17 00:00:00 2001 From: Blake Regalia Date: Wed, 22 Jul 2026 17:20:17 -0700 Subject: [PATCH 2/2] fix(commit): clean up diff metadata and policy on no-op updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit genDiffUpdate unconditionally inserts the mms:Diff metadata and an AutoDiffOwner policy before the delta is known (its WHERE ends in a guaranteed-solution union), but the empty-delta path of diffAndFinalizeCommit returns before the commit is created — so every no-op update (e.g. an idempotent client re-submitting the same change) permanently accreted a dangling mms:Diff referencing a commit that does not exist, plus an orphaned policy. Delete the diff metadata and the transaction's created policy in the empty-delta path before responding. Adds a test asserting that after a no-op update, no mms:Diff references a nonexistent commit. Co-Authored-By: Claude Fable 5 --- .../org/openmbee/flexo/mms/routes/Model.kt | 34 ++++++++++++++++++ .../org/openmbee/flexo/mms/DiffCreateTest.kt | 36 +++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/src/main/kotlin/org/openmbee/flexo/mms/routes/Model.kt b/src/main/kotlin/org/openmbee/flexo/mms/routes/Model.kt index ce50eb4c..de63c9b3 100644 --- a/src/main/kotlin/org/openmbee/flexo/mms/routes/Model.kt +++ b/src/main/kotlin/org/openmbee/flexo/mms/routes/Model.kt @@ -476,6 +476,40 @@ suspend fun AnyLayer1Context.diffAndFinalizeCommit(dstGraphIri: String, srcGraph val deleteModel = parseModelStripPrefixes(RdfContentTypes.Turtle, deleteDataResponseText) // empty delta (no changes) if (insertModel.isEmpty && deleteModel.isEmpty) { + // remove the diff metadata and auto-created policy inserted by the diff update above; + // the commit this diff describes will never be created, so leaving them would accrete a + // dangling mms:Diff and an orphaned policy on every no-op update + executeSparqlUpdate(""" + delete { + graph mor-graph:Metadata { + ?diff ?diff_p ?diff_o . + } + graph m-graph:AccessControl.Policies { + ?createdPolicy ?createdPolicy_p ?createdPolicy_o . + } + graph m-graph:Transactions { + mt: mms:createdPolicy ?createdPolicy . + } + } + where { + graph mor-graph:Metadata { + ?diff a mms:Diff ; + mms:dstCommit morc: ; + ?diff_p ?diff_o . + } + optional { + graph m-graph:Transactions { + mt: mms:createdPolicy ?createdPolicy . + } + graph m-graph:AccessControl.Policies { + ?createdPolicy ?createdPolicy_p ?createdPolicy_o . + } + } + } + """) { + prefixes(prefixes) + } + // locate branch node val branchNode = diffConstructModel.createResource(prefixes["morb"]) // get its etag value diff --git a/src/test/kotlin/org/openmbee/flexo/mms/DiffCreateTest.kt b/src/test/kotlin/org/openmbee/flexo/mms/DiffCreateTest.kt index 06f5e947..6f9b08a6 100644 --- a/src/test/kotlin/org/openmbee/flexo/mms/DiffCreateTest.kt +++ b/src/test/kotlin/org/openmbee/flexo/mms/DiffCreateTest.kt @@ -135,6 +135,42 @@ class DiffCreateTest : RefAny() { } } + "no-op update leaves no dangling diff metadata" { + testApplication { + val body = withAllTestPrefixes(""" + insert data { + . + } + """.trimIndent()) + + // first update commits the change + commitModel(masterBranchPath, body) + + // identical update is a no-op: responds without creating a commit + httpPost("$masterBranchPath/update") { + setSparqlUpdateBody(body) + }.apply { + this shouldHaveStatus HttpStatusCode.OK + } + + // every mms:Diff must reference a commit that exists + withClue("no diff should reference a commit that was never created") { + askBackend(""" + PREFIX mms: + ASK { + GRAPH <${localIri("$demoRepoPath/graphs/Metadata")}> { + ?diff a mms:Diff ; + mms:dstCommit ?dstCommit . + FILTER NOT EXISTS { + ?dstCommit a mms:Commit . + } + } + } + """.trimIndent()) shouldBe false + } + } + } + "create diff leaves no dangling transaction" { testApplication { setupDivergentRefs()