DocBasedVersionConstraints URP: reject _version_

changelog/unreleased/PR#4877-docbasedversionconstraints-reject-native-version.yml

@@ -0,0 +1,9 @@
+title: >
+  DocBasedVersionConstraintsProcessor now rejects a client-supplied _version_ with a 400 instead of silently ignoring it.
+  And it no longer validates the tombstone document shape when deleteVersionParam isn't configured
+type: changed
+authors:
+  - name: David Smiley
+links:
+  - name: PR#4877
+    url: https://github.com/apache/solr/pull/4877

solr/core/src/java/org/apache/solr/update/processor/DocBasedVersionConstraintsProcessor.java

@@ -414,6 +414,8 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
       return;
     }
 
+    rejectClientSuppliedVersion(cmd);
+
     final SolrInputDocument newDoc = cmd.getSolrInputDocument();
     Object[] newVersions = getUserVersionsFromDocument(newDoc);
     validateUserVersions(newVersions, versionFieldNames, "Doc does not have versionField: ");
@@ -440,6 +442,28 @@ public void processAdd(AddUpdateCommand cmd) throws IOException {
     }
   }
 
+  /**
+   * Rejects an update carrying Solr's native <code>_version_</code>. This processor overwrites that
+   * field to guard its own read-then-write, so a client precondition could not be honored. Consults
+   * the same sources, in the same order, as {@link DistributedUpdateProcessor} would.
+   *
+   * <p>NOTE: Perhaps we could do either-or, or combine both somehow.
+   */
+  private static void rejectClientSuppliedVersion(AddUpdateCommand cmd) {
+    if (cmd.getVersion() == 0
+        && cmd.getSolrInputDocument().getField(CommonParams.VERSION_FIELD) == null
+        && cmd.getReq().getParams().get(CommonParams.VERSION_FIELD) == null) {
+      return;
+    }
+    throw new SolrException(
+        BAD_REQUEST,
+        "Optimistic concurrency via "
+            + CommonParams.VERSION_FIELD
+            + " is not supported when DocBasedVersionConstraintsProcessorFactory is configured,"
+            + " since it uses that field itself. Express version constraints with the configured"
+            + " versionField(s) instead.");
+  }
+
   private static void logOverlyFailedRetries(int i, UpdateCommand cmd) {
     // Log a warning every 256 retries.... even a few retries should normally be very unusual.
     if ((i & 0xff) == 0xff) {

solr/core/src/java/org/apache/solr/update/processor/DocBasedVersionConstraintsProcessorFactory.java

@@ -76,10 +76,20 @@
  *       false</code>, but if set to <code>true</code> allows any documents written *before* this
  *       feature is enabled and which are missing the versionField to be overwritten.
  *   <li><code>tombstoneConfig</code> - a list of field names to values to add to the created
- *       tombstone document. In general is not a good idea to populate tombsone documents with
- *       anything other than the minimum required fields so that it doean't match queries
+ *       tombstone document. Only has any effect in combination with <code>deleteVersionParam
+ *       </code>, since tombstones are created solely by the Delete By Id handling that param
+ *       enables; without it this init param is inert. In general, it is not a good idea to populate
+ *       tombstone documents with anything beyond the minimum required fields, so that they don't
+ *       match queries.
  * </ul>
  *
+ * <p><b>This processor is incompatible with Solr's native optimistic concurrency.</b> It uses the
+ * <code>_version_</code> field for itself, setting it on every update so that its read of the
+ * existing document's version and the subsequent write are atomic, retrying on conflict. A <code>
+ * _version_</code> supplied by the client — on the document or as a request parameter — could
+ * therefore not be honored, so an Add carrying one is rejected with a 400. Express ordering through
+ * the per-document <code>versionField</code> values instead.
+ *
  * @since 4.6.0
  */
 public class DocBasedVersionConstraintsProcessorFactory extends UpdateRequestProcessorFactory
@@ -205,7 +215,11 @@ public void inform(SolrCore core) {
       }
     }
 
-    canCreateTombstoneDocument(core.getLatestSchema());
+    if (!deleteVersionParamNames.isEmpty()) {
+      // Tombstones are only produced by the Delete-By-Id handling that deleteVersionParam enables,
+      // so without it there is no tombstone shape to validate.
+      canCreateTombstoneDocument(core.getLatestSchema());
+    }
   }
 
   /**

solr/core/src/test/org/apache/solr/update/processor/TestDocBasedVersionConstraints.java

@@ -53,6 +53,43 @@ public void before() {
     assertU(commit());
   }
 
+  /**
+   * The processor uses {@code _version_} for its own optimistic concurrency, so a client-supplied
+   * one cannot be honored and must fail rather than be silently ignored.
+   */
+  public void testNativeVersionIsRejected() throws Exception {
+    try (ErrorLogMuter muter = ErrorLogMuter.regex("Optimistic concurrency via _version_")) {
+      // on the document
+      SolrException ex =
+          expectThrows(
+              SolrException.class,
+              () ->
+                  updateJ(
+                      jsonAdd(sdoc("id", "aaa", "my_version_l", "1001", "_version_", "-1")),
+                      params("update.chain", "external-version-constraint")));
+      assertEquals(400, ex.code());
+
+      // as a request param
+      ex =
+          expectThrows(
+              SolrException.class,
+              () ->
+                  updateJ(
+                      jsonAdd(sdoc("id", "aaa", "my_version_l", "1001")),
+                      params("update.chain", "external-version-constraint", "_version_", "-1")));
+      assertEquals(400, ex.code());
+
+      assertEquals(2, muter.getCount());
+    }
+
+    // sanity check: the same add without a version still works
+    updateJ(
+        jsonAdd(sdoc("id", "aaa", "my_version_l", "1001")),
+        params("update.chain", "external-version-constraint"));
+    assertU(commit());
+    assertJQ(req("q", "id:aaa"), "/response/numFound==1");
+  }
+
   public void testSimpleUpdates() throws Exception {
 
     // skip low version against committed data