Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

SERVER-7511: fix update with positional mod or index offset

  • Loading branch information...
commit 38f42b2a42011c23385ba899ab84f78bfa9dc1af 1 parent c0ce210
Eliot Horowitz erh authored
113 jstests/update_arraymatch8.js
View
@@ -0,0 +1,113 @@
+// Checking for positional array updates with either .$ or .0 at the end
+// SERVER-7511
+
+// array.$.name
+t = db.jstests_update_arraymatch8;
+t.drop();
+t.ensureIndex( {'array.name': 1} );
+t.insert( {'array': [{'name': 'old'}]} );
+assert( t.findOne({'array.name': 'old'}) );
+t.update( {'array.name': 'old'}, {$set: {'array.$.name': 'new'}} );
+assert( t.findOne({'array.name': 'new'}) );
+assert( !t.findOne({'array.name': 'old'}) );
+
+// array.$ (failed in 2.2.2)
+t = db.jstests_update_arraymatch8;
+t.drop();
+t.ensureIndex( {'array.name': 1} );
+t.insert( {'array': [{'name': 'old'}]} );
+assert( t.findOne({'array.name': 'old'}) );
+t.update( {'array.name': 'old'}, {$set: {'array.$': {'name':'new'}}} );
+assert( t.findOne({'array.name': 'new'}) );
+assert( !t.findOne({'array.name': 'old'}) );
+
+// array.0.name
+t = db.jstests_update_arraymatch8;
+t.drop();
+t.ensureIndex( {'array.name': 1} );
+t.insert( {'array': [{'name': 'old'}]} );
+assert( t.findOne({'array.name': 'old'}) );
+t.update( {'array.name': 'old'}, {$set: {'array.0.name': 'new'}} );
+assert( t.findOne({'array.name': 'new'}) );
+assert( !t.findOne({'array.name': 'old'}) );
+
+// array.0 (failed in 2.2.2)
+t = db.jstests_update_arraymatch8;
+t.drop();
+t.ensureIndex( {'array.name': 1} );
+t.insert( {'array': [{'name': 'old'}]} );
+assert( t.findOne({'array.name': 'old'}) );
+t.update( {'array.name': 'old'}, {$set: {'array.0': {'name':'new'}}} );
+assert( t.findOne({'array.name': 'new'}) );
+assert( !t.findOne({'array.name': 'old'}) );
+
+// // array.12.name
+t = db.jstests_update_arraymatch8;
+t.drop();
+arr = new Array();
+for (var i=0; i<20; i++) {
+ arr.push({'name': 'old'});
+}
+t.ensureIndex( {'array.name': 1} );
+t.insert( {_id:0, 'array': arr} );
+assert( t.findOne({'array.name': 'old'}) );
+t.update( {_id:0}, {$set: {'array.12.name': 'new'}} );
+// note: both documents now have to be in the array
+assert( t.findOne({'array.name': 'new'}) );
+assert( t.findOne({'array.name': 'old'}) );
+
+// array.12 (failed in 2.2.2)
+t = db.jstests_update_arraymatch8;
+t.drop();
+arr = new Array();
+for (var i=0; i<20; i++) {
+ arr.push({'name': 'old'});
+}
+t.ensureIndex( {'array.name': 1} );
+t.insert( {_id:0, 'array': arr} );
+assert( t.findOne({'array.name': 'old'}) );
+t.update( {_id:0}, {$set: {'array.12': {'name':'new'}}} );
+// note: both documents now have to be in the array
+assert( t.findOne({'array.name': 'new'}) );
+assert( t.findOne({'array.name': 'old'}) );
+
+// array.$.123a.name
+t = db.jstests_update_arraymatch8;
+t.drop();
+t.ensureIndex( {'array.123a.name': 1} );
+t.insert( {'array': [{'123a':{'name': 'old'}}]} );
+assert( t.findOne({'array.123a.name': 'old'}) );
+t.update( {'array.123a.name': 'old'}, {$set: {'array.$.123a.name': 'new'}} );
+assert( t.findOne({'array.123a.name': 'new'}) );
+assert( !t.findOne({'array.123a.name': 'old'}) );
+
+// array.$.123a
+t = db.jstests_update_arraymatch8;
+t.drop();
+t.ensureIndex( {'array.name': 1} );
+t.insert( {'array': [{'123a':{'name': 'old'}}]} );
+assert( t.findOne({'array.123a.name': 'old'}) );
+t.update( {'array.123a.name': 'old'}, {$set: {'array.$.123a': {'name': 'new'}}} );
+assert( t.findOne({'array.123a.name': 'new'}) );
+assert( !t.findOne({'array.123a.name': 'old'}) );
+
+// array.0.123a.name
+t = db.jstests_update_arraymatch8;
+t.drop();
+t.ensureIndex( {'array.123a.name': 1} );
+t.insert( {'array': [{'123a':{'name': 'old'}}]} );
+assert( t.findOne({'array.123a.name': 'old'}) );
+t.update( {'array.123a.name': 'old'}, {$set: {'array.0.123a.name': 'new'}} );
+assert( t.findOne({'array.123a.name': 'new'}) );
+assert( !t.findOne({'array.123a.name': 'old'}) );
+
+// array.0.123a
+t = db.jstests_update_arraymatch8;
+t.drop();
+t.ensureIndex( {'array.name': 1} );
+t.insert( {'array': [{'123a':{'name': 'old'}}]} );
+assert( t.findOne({'array.123a.name': 'old'}) );
+t.update( {'array.123a.name': 'old'}, {$set: {'array.0.123a': {'name': 'new'}}} );
+assert( t.findOne({'array.123a.name': 'new'}) );
+assert( !t.findOne({'array.123a.name': 'old'}) );
+
50 src/mongo/db/ops/update_internal.cpp
View
@@ -1113,4 +1113,54 @@ namespace mongo {
updateIsIndexed( i->second, idxKeys , backgroundKeys );
}
+ bool getCanonicalIndexField( const StringData& fullName, string* out ) {
+ // check if fieldName contains ".$" or ".###" substrings (#=digit) and skip them
+ if ( fullName.find( '.' ) == string::npos )
+ return false;
+
+ bool modified = false;
+
+ StringBuilder buf;
+ for ( size_t i=0; i<fullName.size(); i++ ) {
+
+ char c = fullName[i];
+
+ if ( c != '.' ) {
+ buf << c;
+ continue;
+ }
+
+ // check for ".$", skip if present
+ if ( fullName[i+1] == '$' ) {
+ i++;
+ modified = true;
+ continue;
+ }
+
+ // check for ".###" for any number of digits (no letters)
+ if ( isdigit( fullName[i+1] ) ) {
+ size_t j = i;
+ // skip digits
+ while ( j+1 < fullName.size() && isdigit( fullName[j+1] ) )
+ j++;
+
+ if ( j+1 == fullName.size() || fullName[j+1] == '.' ) {
+ // only digits found, skip forward
+ i = j;
+ modified = true;
+ continue;
+ }
+ }
+
+ buf << c;
+ }
+
+ if ( !modified )
+ return false;
+
+ *out = buf.str();
+ return true;
+ }
+
+
} // namespace mongo
59 src/mongo/db/ops/update_internal.h
View
@@ -30,6 +30,12 @@ namespace mongo {
class ModState;
class ModSetState;
+ /**
+ * a.$ -> a
+ * @return true if out is set and we made a change
+ */
+ bool getCanonicalIndexField( const StringData& fullName, string* out );
+
/* Used for modifiers such as $inc, $set, $push, ...
* stores the info about a single operation
* once created should never be modified
@@ -143,6 +149,7 @@ namespace mongo {
// check if there is an index key equal to mod
if ( idxKeys.count(fullName) )
return true;
+
// check if there is an index key that is a child of mod
set< string >::const_iterator j = idxKeys.upper_bound( fullName );
if ( j != idxKeys.end() && j->find( fullName ) == 0 && (*j)[fullName.size()] == '.' )
@@ -151,51 +158,21 @@ namespace mongo {
return false;
}
+ /**
+ * checks if mod is in the index by inspecting fieldName, and removing
+ * .$ or .### substrings (#=digit) with any number of digits.
+ *
+ * @return true iff the mod is indexed
+ */
bool isIndexed( const set<string>& idxKeys ) const {
- string fullName = fieldName;
- if ( isIndexed( fullName , idxKeys ) )
+ // first, check if full name is in idxKeys
+ if ( isIndexed( fieldName , idxKeys ) )
return true;
- if ( strstr( fieldName , "." ) ) {
- // check for a.0.1
- StringBuilder buf;
- for ( size_t i=0; i<fullName.size(); i++ ) {
- char c = fullName[i];
-
- if ( c == '$' &&
- i > 0 && fullName[i-1] == '.' &&
- i+1<fullName.size() &&
- fullName[i+1] == '.' ) {
- i++;
- continue;
- }
-
- buf << c;
-
- if ( c != '.' )
- continue;
-
- if ( ! isdigit( fullName[i+1] ) )
- continue;
-
- bool possible = true;
- size_t j=i+2;
- for ( ; j<fullName.size(); j++ ) {
- char d = fullName[j];
- if ( d == '.' )
- break;
- if ( isdigit( d ) )
- continue;
- possible = false;
- break;
- }
-
- if ( possible )
- i = j;
- }
- string x = buf.str();
- if ( isIndexed( x , idxKeys ) )
+ string x;
+ if ( getCanonicalIndexField( fieldName, &x ) ) {
+ if ( isIndexed( x, idxKeys ) )
return true;
}
32 src/mongo/dbtests/updatetests.cpp
View
@@ -1295,6 +1295,36 @@ namespace UpdateTests {
};
+ class IndexFieldNameTest {
+ public:
+ void run() {
+ string x;
+
+ ASSERT_FALSE( getCanonicalIndexField( "a", &x ) );
+ ASSERT_FALSE( getCanonicalIndexField( "aaa", &x ) );
+ ASSERT_FALSE( getCanonicalIndexField( "a.b", &x ) );
+
+ ASSERT_TRUE( getCanonicalIndexField( "a.$", &x ) );
+ ASSERT_EQUALS( x, "a" );
+ ASSERT_TRUE( getCanonicalIndexField( "a.0", &x ) );
+ ASSERT_EQUALS( x, "a" );
+ ASSERT_TRUE( getCanonicalIndexField( "a.123", &x ) );
+ ASSERT_EQUALS( x, "a" );
+
+ ASSERT_TRUE( getCanonicalIndexField( "a.$.b", &x ) );
+ ASSERT_EQUALS( x, "a.b" );
+ ASSERT_TRUE( getCanonicalIndexField( "a.0.b", &x ) );
+ ASSERT_EQUALS( x, "a.b" );
+ ASSERT_TRUE( getCanonicalIndexField( "a.123.b", &x ) );
+ ASSERT_EQUALS( x, "a.b" );
+
+ ASSERT_FALSE( getCanonicalIndexField( "a.123a", &x ) );
+ ASSERT_FALSE( getCanonicalIndexField( "a.a123", &x ) );
+ ASSERT_FALSE( getCanonicalIndexField( "a.123a.b", &x ) );
+ ASSERT_FALSE( getCanonicalIndexField( "a.a123.b", &x ) );
+ }
+ };
+
class All : public Suite {
public:
All() : Suite( "update" ) {
@@ -1401,6 +1431,8 @@ namespace UpdateTests {
add< basic::bit1 >();
add< basic::unset >();
add< basic::setswitchint >();
+
+ add< IndexFieldNameTest >();
}
} myall;
Please sign in to comment.
Something went wrong with that request. Please try again.