From 05778ef59adc154df1a81bcfbd7b5913859f4b31 Mon Sep 17 00:00:00 2001 From: Filipe Silva Date: Wed, 24 Nov 2021 20:43:22 +0000 Subject: [PATCH] Fix for Bug#103324 (32770013), X DevAPI Collection.replaceOne() missing matching _id check. --- CHANGES | 2 + .../cj/LocalizedErrorMessages.properties | 2 + .../com/mysql/cj/xdevapi/CollectionImpl.java | 13 +++-- .../testsuite/x/devapi/CollectionAddTest.java | 2 +- .../x/devapi/CollectionModifyTest.java | 53 +++++++------------ 5 files changed, 33 insertions(+), 39 deletions(-) diff --git a/CHANGES b/CHANGES index d3bf57bc4..9a6eb54c0 100644 --- a/CHANGES +++ b/CHANGES @@ -3,6 +3,8 @@ Version 8.0.28 + - Fix for Bug#103324 (32770013), X DevAPI Collection.replaceOne() missing matching _id check. + - Fix for Bug#105197 (33461744), Statement.executeQuery() may return non-navigable ResultSet. - Fix for Bug#105323 (33507321), README.md contains broken links. diff --git a/src/main/resources/com/mysql/cj/LocalizedErrorMessages.properties b/src/main/resources/com/mysql/cj/LocalizedErrorMessages.properties index 1e612ae26..2b9704d90 100644 --- a/src/main/resources/com/mysql/cj/LocalizedErrorMessages.properties +++ b/src/main/resources/com/mysql/cj/LocalizedErrorMessages.properties @@ -103,6 +103,8 @@ Clob.11=Cannot truncate CLOB of length Clob.12=\ to length of Clob.13=. +Collection.DocIdMismatch=Replacement document has an _id that is different than the matched document. + ColumnDefinition.0={0} is not applicable to the {1} type of column ''{2}''. ColumnDefinition.1=Length must be specified before decimals for column ''{0}''. diff --git a/src/main/user-impl/java/com/mysql/cj/xdevapi/CollectionImpl.java b/src/main/user-impl/java/com/mysql/cj/xdevapi/CollectionImpl.java index 5883498d3..43eae5edb 100644 --- a/src/main/user-impl/java/com/mysql/cj/xdevapi/CollectionImpl.java +++ b/src/main/user-impl/java/com/mysql/cj/xdevapi/CollectionImpl.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2020, Oracle and/or its affiliates. + * Copyright (c) 2015, 2021, Oracle and/or its affiliates. * * This program is free software; you can redistribute it and/or modify it under * the terms of the GNU General Public License, version 2.0, as published by the @@ -188,6 +188,10 @@ public Result replaceOne(String id, DbDoc doc) { if (doc == null) { throw new XDevAPIError(Messages.getString("CreateTableStatement.0", new String[] { "doc" })); } + JsonValue docId = doc.get("_id"); + if (docId != null && (!JsonString.class.isInstance(docId) || !id.equals(((JsonString) docId).getString()))) { + throw new XDevAPIError(Messages.getString("Collection.DocIdMismatch")); + } return modify("_id = :id").set("$", doc).bind("id", id).execute(); } @@ -214,10 +218,11 @@ public Result addOrReplaceOne(String id, DbDoc doc) { if (doc == null) { throw new XDevAPIError(Messages.getString("CreateTableStatement.0", new String[] { "doc" })); } - if (doc.get("_id") == null) { + JsonValue docId = doc.get("_id"); + if (docId == null) { doc.add("_id", new JsonString().setValue(id)); - } else if (!id.equals(((JsonString) doc.get("_id")).getString())) { - throw new XDevAPIError("Document already has an _id that doesn't match to id parameter"); + } else if (!JsonString.class.isInstance(docId) || !id.equals(((JsonString) docId).getString())) { + throw new XDevAPIError(Messages.getString("Collection.DocIdMismatch")); } return add(doc).setUpsert(true).execute(); } diff --git a/src/test/java/testsuite/x/devapi/CollectionAddTest.java b/src/test/java/testsuite/x/devapi/CollectionAddTest.java index d15465f57..30b54c815 100644 --- a/src/test/java/testsuite/x/devapi/CollectionAddTest.java +++ b/src/test/java/testsuite/x/devapi/CollectionAddTest.java @@ -260,7 +260,7 @@ public void testAddOrReplaceOne() { assertTrue(this.collection.find("a = 4").execute().hasNext()); // a new document with _id field that doesn't match id parameter - assertThrows(XDevAPIError.class, "Document already has an _id that doesn't match to id parameter", new Callable() { + assertThrows(XDevAPIError.class, "Replacement document has an _id that is different than the matched document\\.", new Callable() { public Void call() throws Exception { CollectionAddTest.this.collection.addOrReplaceOne("id2", CollectionAddTest.this.collection.newDoc().add("_id", new JsonString().setValue("id111"))); diff --git a/src/test/java/testsuite/x/devapi/CollectionModifyTest.java b/src/test/java/testsuite/x/devapi/CollectionModifyTest.java index 6ca88c62d..935ae38ff 100644 --- a/src/test/java/testsuite/x/devapi/CollectionModifyTest.java +++ b/src/test/java/testsuite/x/devapi/CollectionModifyTest.java @@ -549,42 +549,23 @@ public void testReplaceOne() { DbDoc doc = this.collection.getOne("existingId"); assertNotNull(doc); - assertEquals(new Integer(2), ((JsonNumber) doc.get("a")).getInteger()); + assertEquals(2, ((JsonNumber) doc.get("a")).getInteger()); - res = this.collection.replaceOne("notExistingId", "{\"_id\":\"existingId\",\"a\":3}"); - assertEquals(0, res.getAffectedItemsCount()); - - res = this.collection.replaceOne("", "{\"_id\":\"existingId\",\"a\":3}"); - assertEquals(0, res.getAffectedItemsCount()); - - /* - * FR5.1 The id of the document must remain immutable: - * - * Use a collection with some documents - * Fetch a document - * Modify _id: _new_id_ and modify any other field of the document - * Call replaceOne() giving original ID and modified document: expect affected = 1 - * Fetch the document again, ensure other document modifications took place - * Ensure no document with _new_id_ was added to the collection - */ - this.collection.remove("1=1").execute(); - assertEquals(0, this.collection.count()); - this.collection.add("{\"_id\":\"id1\",\"a\":1}").execute(); - - doc = this.collection.getOne("id1"); - assertNotNull(doc); - ((JsonString) doc.get("_id")).setValue("id2"); - ((JsonNumber) doc.get("a")).setValue("2"); - res = this.collection.replaceOne("id1", doc); - assertEquals(1, res.getAffectedItemsCount()); - - doc = this.collection.getOne("id1"); - assertNotNull(doc); - assertEquals("id1", ((JsonString) doc.get("_id")).getString()); - assertEquals(new Integer(2), ((JsonNumber) doc.get("a")).getInteger()); + // Original behavior changed by Bug#32770013. + assertThrows(XDevAPIError.class, "Replacement document has an _id that is different than the matched document\\.", new Callable() { + public Void call() throws Exception { + CollectionModifyTest.this.collection.replaceOne("nonExistingId", "{\"_id\":\"existingId\",\"a\":3}"); + return null; + } + }); - doc = this.collection.getOne("id2"); - assertNull(doc); + // Original behavior changed by Bug#32770013. + assertThrows(XDevAPIError.class, "Replacement document has an _id that is different than the matched document\\.", new Callable() { + public Void call() throws Exception { + CollectionModifyTest.this.collection.replaceOne("", "{\"_id\":\"existingId\",\"a\":3}"); + return null; + } + }); /* * FR5.2 The id of the document must remain immutable: @@ -596,6 +577,10 @@ public void testReplaceOne() { * Fetch the document again, ensure other document modifications took place * Ensure the number of documents in the collection is unaltered */ + this.collection.remove("true").execute(); + assertEquals(0, this.collection.count()); + this.collection.add("{\"_id\":\"id1\",\"a\":1}").execute(); + doc = this.collection.getOne("id1"); assertNotNull(doc); doc.remove("_id");