diff --git a/changelog/unreleased/PR#4877-docbasedversionconstraints-reject-native-version.yml b/changelog/unreleased/PR#4877-docbasedversionconstraints-reject-native-version.yml
new file mode 100644
index 00000000000..7f2936c39f3
--- /dev/null
+++ b/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
\ No newline at end of file
diff --git a/solr/core/src/java/org/apache/solr/update/processor/DocBasedVersionConstraintsProcessor.java b/solr/core/src/java/org/apache/solr/update/processor/DocBasedVersionConstraintsProcessor.java
index cbec1472649..c63afcd9530 100644
--- a/solr/core/src/java/org/apache/solr/update/processor/DocBasedVersionConstraintsProcessor.java
+++ b/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 _version_. 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.
+ *
+ *
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) {
diff --git a/solr/core/src/java/org/apache/solr/update/processor/DocBasedVersionConstraintsProcessorFactory.java b/solr/core/src/java/org/apache/solr/update/processor/DocBasedVersionConstraintsProcessorFactory.java
index 90307f9dc38..f70e05cf6c2 100644
--- a/solr/core/src/java/org/apache/solr/update/processor/DocBasedVersionConstraintsProcessorFactory.java
+++ b/solr/core/src/java/org/apache/solr/update/processor/DocBasedVersionConstraintsProcessorFactory.java
@@ -76,10 +76,20 @@
* false, but if set to true allows any documents written *before* this
* feature is enabled and which are missing the versionField to be overwritten.
*
tombstoneConfig - 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 deleteVersionParam
+ * , 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.
*
*
+ * This processor is incompatible with Solr's native optimistic concurrency. It uses the
+ * _version_ 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
+ * _version_ 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 versionField 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());
+ }
}
/**
diff --git a/solr/core/src/test/org/apache/solr/update/processor/TestDocBasedVersionConstraints.java b/solr/core/src/test/org/apache/solr/update/processor/TestDocBasedVersionConstraints.java
index 5b69ae0d37c..101cdc6ab20 100644
--- a/solr/core/src/test/org/apache/solr/update/processor/TestDocBasedVersionConstraints.java
+++ b/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