issue arangodb#925: allow attribute names starting with underscores

bdacode · Aug 7, 2014 · 3b9c4d1 · 3b9c4d1
1 parent 1b96f52
commit 3b9c4d1
Show file tree

Hide file tree

Showing 8 changed files with 230 additions and 158 deletions.
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,6 +1,36 @@
 v2.2.2-rc1 (XXXX-XX-XX)
 -------------------
 
+* allow storing non-reserved attribute names starting with an underscore
+
+  Previous versions of ArangoDB parsed away all attribute names that started with an 
+  underscore (e.g. `_test', '_foo', `_bar`) on all levels of a document (root level
+  and sub-attribute levels). While this behavior was documented, it was unintuitive and
+  prevented storing documents inside other documents, e.g.:
+
+      {
+        "_key" : "foo",
+        "_type" : "mydoc",
+        "references" : [
+          {
+            "_key" : "something",
+            "_rev" : "...",
+            "value" : 1
+          },
+          { 
+            "_key" : "something else",
+            "_rev" : "...",
+            "value" : 2
+          }
+        ]
+      }
+
+  In the above example, previous versions of ArangoDB removed all attributes and
+  sub-attributes that started with underscores, meaning the embedded documents would lose
+  some of their attributes. 2.2.2 should preserve such attributes, and will also allow
+  storing user-defined attribute names on the top-level even if they start with underscores
+  (such as `_type` in the above example).
+
 * fix conversion of JavaScript String, Number and Boolean objects to JSON.
 
   Objects created in JavaScript using `new Number(...)`, `new String(...)`, or

diff --git a/Documentation/Books/Users/Arangoimp/README.mdpp b/Documentation/Books/Users/Arangoimp/README.mdpp
@@ -220,14 +220,9 @@ ArangoDB:
 - *_rev*: this attribute contains the revision number of a document. However, the
   revision numbers are managed by ArangoDB and cannot be specified on import. Thus 
   any value in this attribute is ignored on import.
-- all other attributes starting with an underscore are discarded on import without
-  any warnings.
 
 If you import values into *_key*, you should make sure they are valid and unique.
 
 When importing data into an edge collection, you should make sure that all import
 documents can *_from* and *_to* and that their values point to existing documents.
 
-Finally you should make sure that all other attributes in the import file do not
-start with an underscore - otherwise they might be discarded.
-
diff --git a/Documentation/Books/Users/NamingConventions/AttributeNames.mdpp b/Documentation/Books/Users/NamingConventions/AttributeNames.mdpp
@@ -3,15 +3,15 @@
 Users can pick attribute names for document attributes as desired, provided the
 following attribute naming constraints are not violated:
 - Attribute names starting with an underscore are considered to be system
-  attributes for ArangoDB's internal use. Such attribute names are already used
+  attributes for ArangoDB's internal use. Such attribute names are used
   by ArangoDB for special purposes, e.g. *_id* is used to contain a document's
   handle, *_key* is used to contain a document's user-defined key, and *_rev* is
   used to contain the document's revision number. In edge collections, the
   *_from* and *_to* attributes are used to reference other documents.
 
   More system attributes may be added in the future without further notice so
-  end users should not use attribute names starting with an underscore for their
-  own attributes.
+  end users should try to avoid using their own attribute names starting with 
+  underscores. 
 
 * Attribute names should not start with the at-mark (*@*). The at-mark
   at the start of attribute names is reserved in ArangoDB for future use cases.
@@ -34,16 +34,15 @@ following attribute naming constraints are not violated:
   length is variable and depends on the number and data types of attributes
   used.
 * Attribute names are case-sensitive.
-* Attributes with empty names (the empty string) and attributes with names that
-  start with an underscore and don't have a special meaning (system attributes)
-  are removed from the document when saving it. 
+* Attributes with empty names (the empty string) are removed from the document 
+  when saving it. 
 
   When the document is later requested, it will be returned without these 
   attributes. For example, if this document is saved
 
-      { "a" : 1, "" : 2, "_test" : 3, "b": 4 }
+      { "a" : 1, "" : 2, "b": 3 }
 
   and later requested, it will be returned like this:
 
-      { "a" : 1, "b": 4 }
+      { "a" : 1, "b": 3 }
 
diff --git a/UnitTests/HttpInterface/api-attributes-spec.rb b/UnitTests/HttpInterface/api-attributes-spec.rb
@@ -89,7 +89,8 @@
       doc.parsed_response['_rev'].should_not eq('99')
       doc.parsed_response.should_not have_key('_from')
       doc.parsed_response.should_not have_key('_to')
-      doc.parsed_response.should_not have_key('_test')
+      doc.parsed_response.should have_key('_test')
+      doc.parsed_response['_test'].should eq('c')
       doc.parsed_response.should have_key('meow')
       doc.parsed_response['meow'].should eq('d')
       doc.parsed_response['foo'].should eq('002')
@@ -101,7 +102,7 @@
 
     it "creates a document with nested attribute names" do
       cmd = api + "?collection=" + @cn
-      body = "{ \"a\" : \"1\", \"b\" : { \"b\" : \"2\" , \"a\" : \"3\", \"\": \"4\", \"_from\": \"5\", \"c\" : 6 } }"
+      body = "{ \"a\" : \"1\", \"b\" : { \"b\" : \"2\" , \"a\" : \"3\", \"\": \"4\", \"_key\": \"moetoer\", \"_from\": \"5\", \"_lol\" : false, \"c\" : 6 } }"
       doc = ArangoDB.log_post("#{prefix}-create-duplicate-names", cmd, :body => body)
 
       doc.code.should eq(201)
@@ -118,11 +119,13 @@
       doc.parsed_response.should have_key('b')
 
       doc.parsed_response['b'].should_not have_key('')
-      doc.parsed_response['b'].should_not have_key('_from')
+      doc.parsed_response['b'].should have_key('_from')
+      doc.parsed_response['b'].should have_key('_key')
+      doc.parsed_response['b'].should have_key('_lol')
       doc.parsed_response['b'].should have_key('b')
       doc.parsed_response['b'].should have_key('a')
       doc.parsed_response['b'].should have_key('c')
-      doc.parsed_response['b'].should eq({ "b" => "2", "a" => "3", "c" => 6 })
+      doc.parsed_response['b'].should eq({ "b" => "2", "a" => "3", "_key" => "moetoer", "_from" => "5", "_lol" => false, "c" => 6 })
     end
 
 ################################################################################

diff --git a/arangod/V8Server/v8-vocbase.cpp b/arangod/V8Server/v8-vocbase.cpp
@@ -933,7 +933,7 @@ int ProcessBitarrayIndexFields (v8::Handle<v8::Object> const obj,
     // "fields" is a list of fields
     v8::Handle<v8::Array> fieldList = v8::Handle<v8::Array>::Cast(obj->Get(TRI_V8_SYMBOL("fields")));
 
-    const uint32_t n = fieldList->Length();
+    uint32_t const n = fieldList->Length();
 
     for (uint32_t i = 0; i < n; ++i) {
       if (! fieldList->Get(i)->IsArray()) {
@@ -948,7 +948,7 @@ int ProcessBitarrayIndexFields (v8::Handle<v8::Object> const obj,
         break;
       }
 
-      const string f = TRI_ObjectToString(fieldPair->Get(0));
+      string const f = TRI_ObjectToString(fieldPair->Get(0));
 
       if (f.empty() || (create && f[0] == '_')) {
         // accessing internal attributes is disallowed
@@ -9879,7 +9879,17 @@ static v8::Handle<v8::Value> MapGetNamedShapedJson (v8::Local<v8::String> name,
   v8::String::Utf8Value const str(name);
   string const key(*str, (size_t) str.length());
 
-  if (key.empty() || key[0] == '_' || strchr(key.c_str(), '.') != 0) {
+  if (key.empty()) {
+    return scope.Close(v8::Handle<v8::Value>());
+  }
+
+  if (key[0] == '_' && 
+    (key == "_key" || key == "_rev" || key == "_id" || key == "_from" || key == "_to")) {
+    // strip reserved attributes
+    return scope.Close(v8::Handle<v8::Value>());
+  }
+
+  if (strchr(key.c_str(), '.') != nullptr) {
     return scope.Close(v8::Handle<v8::Value>());
   }
 
@@ -9930,7 +9940,7 @@ static v8::Handle<v8::Array> KeysOfShapedJson (const v8::AccessorInfo& info) {
   // get shaped json
   void* marker = TRI_UnwrapClass<void*>(self, WRP_SHAPED_JSON_TYPE);
 
-  if (marker == 0) {
+  if (marker == nullptr) {
     return scope.Close(v8::Array::New());
   }
 
@@ -9976,7 +9986,7 @@ static v8::Handle<v8::Array> KeysOfShapedJson (const v8::AccessorInfo& info) {
     }
   }
 
-  TRI_v8_global_t* v8g = (TRI_v8_global_t*) v8::Isolate::GetCurrent()->GetData();
+  TRI_v8_global_t* v8g = static_cast<TRI_v8_global_t*>(v8::Isolate::GetCurrent()->GetData());
   result->Set(count++, v8g->_IdKey);
   result->Set(count++, v8g->_RevKey);
   result->Set(count++, v8g->_KeyKey);
@@ -10014,7 +10024,7 @@ static v8::Handle<v8::Integer> PropertyQueryShapedJson (v8::Local<v8::String> na
   }
 
   if (key[0] == '_') {
-    if (key == "_id" || key == TRI_VOC_ATTRIBUTE_REV || key == TRI_VOC_ATTRIBUTE_KEY) {
+    if (key == "_key" || key == "_rev" || key == "_id" || key == "_from" || key == "_to") {
       return scope.Close(v8::Handle<v8::Integer>(v8::Integer::New(v8::ReadOnly)));
     }
   }
@@ -10045,7 +10055,7 @@ static v8::Handle<v8::Integer> PropertyQueryShapedJson (v8::Local<v8::String> na
   TRI_shape_access_t const* acc = TRI_FindAccessorVocShaper(shaper, sid, pid);
 
   // key not found
-  if (acc == 0 || acc->_resultSid == TRI_SHAPE_ILLEGAL) {
+  if (acc == nullptr || acc->_resultSid == TRI_SHAPE_ILLEGAL) {
     return scope.Close(v8::Handle<v8::Integer>());
   }
 
@@ -10149,7 +10159,7 @@ TRI_index_t* TRI_LookupIndexByHandle (CollectionNameResolver const* resolver,
 
   // extract the index identifier from an object
   else if (val->IsObject()) {
-    TRI_v8_global_t* v8g = (TRI_v8_global_t*) v8::Isolate::GetCurrent()->GetData();
+    TRI_v8_global_t* v8g = static_cast<TRI_v8_global_t*>(v8::Isolate::GetCurrent()->GetData());
 
     v8::Handle<v8::Object> obj = val->ToObject();
     v8::Handle<v8::Value> iidVal = obj->Get(v8g->IdKey);

diff --git a/js/common/tests/shell-attributes.js b/js/common/tests/shell-attributes.js
@@ -151,7 +151,7 @@ function AttributesSuite () {
 ////////////////////////////////////////////////////////////////////////////////
 
     testReservedAttributes : function () {
-      var doc = { "_id" : "foo", "_rev": "99", "_key" : "meow", "_from" : "33", "_to": "99", "_test" : false };
+      var doc = { "_id" : "foo", "_rev": "99", "_key" : "meow", "_from" : "33", "_to": "99", "_test" : false, "_boom" : "bang" };
 
       var d1 = c.save(doc);
       var d2 = c.document(d1._id);
@@ -161,17 +161,33 @@ function AttributesSuite () {
       assertEqual(cn + "/meow", d1._id);
       assertEqual(cn + "/meow", d2._id);
       assertEqual(d1._rev, d2._rev);
+      assertFalse(d2._test);
+      assertEqual("bang", d2._boom);
+      assertFalse(d2.hasOwnProperty("_from"));
+      assertFalse(d2.hasOwnProperty("_to"));
 
       // user specified _rev value must have been ignored
       assertTrue(d1._rev !== "99");
-
-      // test attributes
-      var i;
-      for (i in d2) {
-        if (d2.hasOwnProperty(i)) {
-          assertTrue(i !== "_from" && i !== "_to" && i !== "_test");
-        }
-      }
+    },
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief reserved attribute names
+////////////////////////////////////////////////////////////////////////////////
+
+    testEmbeddedReservedAttributes : function () {
+      var doc = { "_id" : "foo", "_rev": "99", "_key" : "meow", "_from" : "33", "_to": "99", "_test" : false };
+
+      c.save({ _key: "mydoc", _embedded: doc });
+      var d = c.document("mydoc");
+
+      assertEqual(cn + "/mydoc", d._id);
+      assertEqual("mydoc", d._key);
+      assertEqual("foo", d._embedded._id);
+      assertEqual("99", d._embedded._rev);
+      assertEqual("meow", d._embedded._key);
+      assertEqual("33", d._embedded._from);
+      assertEqual("99", d._embedded._to);
+      assertFalse(d._embedded._test);
     },
 
 ////////////////////////////////////////////////////////////////////////////////