Fixed some issues with the schema allowing invalid attributes

StevenHarlow · Oct 1, 2014 · 8f396a8 · 8f396a8
1 parent 92d5a9f
commit 8f396a8
Show file tree

Hide file tree

Showing 7 changed files with 94 additions and 59 deletions.
diff --git a/app/models/paper.rb b/app/models/paper.rb
@@ -60,6 +60,7 @@ def metadata(include_cited_paper=false)
 
   def assign_metadata(metadata)
     return false unless JSON::Validator.validate(JSON_SCHEMA, metadata)
+
     metadata = metadata.dup
 
     # This is order dependent
@@ -74,6 +75,8 @@ def assign_metadata(metadata)
 
     self.uri           = metadata.delete('uri')
     self.extra         = metadata
+
+    true
   end
 
   def assign_bibliographic_metadata(metadata)
@@ -94,7 +97,7 @@ def assign_bibliographic_metadata(metadata)
   end
 
   def update_metadata(metadata, updating_user)
-    assign_metadata(metadata)
+    return false unless assign_metadata(metadata)
     saved = self.save
     AuditLogEntry.create(paper:self, user:updating_user) if saved
     saved

diff --git a/lib/json_attributes.rb b/lib/json_attributes.rb
@@ -22,11 +22,6 @@
 
 module JsonAttributes
 
-  # def extended(klass)
-  #   puts "--- extended"
-  #   klass.cattr_accessor :json_attribute_fields # unless defined?(:json_attribute_fields)
-  # end
-
   def json_attribute(*names)
     if ! defined? json_attribute_fields
       cattr_accessor(:json_attribute_fields) { [] }

diff --git a/schemas/base.json b/schemas/base.json
@@ -3,65 +3,72 @@
     "id": "http://api.richcitations.org/schema",
     "$schema": "http://json-schema.org/draft-04/schema#",
     "type": "object",
-    "required": ["uri"],
-    "properties": {
-        "uri": {
-            "type": "string"
-        },
-        "more_stuff": {
-            "type": "string"
+    "defenitions": {
+      "bibliographic": {
+          "type": "object",
+          "properties": {
+          },
+          "additionalProperties": true
         },
-        "references": {
+      "citation_group": {
+        "type": "object",
+        "properties": {
+          "id": {"type": "string"},
+          "text": {"type": "string"},
+          "section": {"type": "string"},
+          "references": {
             "type": "array",
             "items": {
-                "type": "object",
-                "required": ["id", "uri", "number"],
-                "properties": {
-                    "id": {
-                        "type": "string"
-                    },
-                    "uri": {
-                        "type": "string"
-                    },
-                    "bibliographic": {
-                        "type": "object",
-                        "additionalProperties": true
-                        },
-                    "number": {
-                        "type": "integer"
-                    },
-                    "literal": {
-                      "type": "string"
-                    },
-                    "citation_groups": {
-                        "type": "array",
-                        "items": {
-                            "type": "string"
-                        }
-                    }
-                }
+              "type": "string"
             }
+          }
         },
-        "citation_groups": {
+        "required": [ "id", "text", "references" ],
+        "additionalProperties": false
+      },
+      "reference": {
+        "type": "object",
+        "required": ["id", "uri", "number"],
+        "properties": {
+          "id": {
+            "type": "string"
+          },
+          "uri": {
+            "type": "string"
+          },
+          "bibliographic": { "$ref": "#/defenitions/bibliographic" },
+          "number": {
+            "type": "integer"
+          },
+          "literal": {
+            "type": "string"
+          },
+          "citation_groups": {
             "type": "array",
             "items": {
-                "type": "object",
-                "properties": {
-                    "id": {"type": "string"},
-                    "text": {"type": "string"},
-                    "section": {"type": "string"},
-                    "references": {
-                        "type": "array",
-                        "items": {
-                            "type": "string"
-                        }
-                    }
-                }
+              "type": "string"
             }
+          }
         },
-        "bibliographic": {
-            "type": "object",
-            "additionalProperties": true
+        "additionalProperties": false
+      }
+    },
+    "required": ["uri", "bibliographic"],
+    "properties": {
+        "uri": {
+            "type": "string"
+        },
+        "bibliographic": { "$ref": "#/defenitions/bibliographic" },
+        "references": {
+            "type": "array",
+            "items": { "$ref": "#/defenitions/reference" }
+        },
+        "citation_groups": {
+              "type": "array",
+              "items": { "$ref": "#/defenitions/citation_group" }
+        },
+        "more_stuff": {
+          "type": "string"
         }
     },
     "additionalProperties": false

diff --git a/test/controllers/v0/papers_controller_test.rb b/test/controllers/v0/papers_controller_test.rb
@@ -197,6 +197,30 @@ def setup
       assert_response :unprocessable_entity
     end
 
+    test "It should fail for missing reference metadata" do
+      data = metadata(paper_uri)
+      data['references'].first.delete('uri')
+      post :create, data.to_json
+
+      assert_response :unprocessable_entity
+    end
+
+    test "It should fail for extra metadata" do
+      data = metadata(paper_uri)
+      data['foo'] = 'bar'
+      post :create, data.to_json
+
+      assert_response :unprocessable_entity
+    end
+
+    test "It should fail for extra reference metadata" do
+      data = metadata(paper_uri)
+      data['references'].first['foo'] = 'bar'
+      post :create, data.to_json
+
+      assert_response :unprocessable_entity
+    end
+
   end
 
 end
diff --git a/test/models/paper_assign_metadata_test.rb b/test/models/paper_assign_metadata_test.rb
@@ -54,9 +54,10 @@ class PaperAssignMetadataTest < ActiveSupport::TestCase
   test "it should create References" do
     p = Paper.new
     p.assign_metadata('uri' => 'http://example.com/a',
+                      'bibliographic' => {},
                       'references' => [
-                          { 'id' => 'ref.1', 'uri' => 'http://example.com/c1', 'bibliographic' => {'title'=>'1'}, number: 1 },
-                          { 'id' => 'ref.2', 'uri' => 'http://example.com/c2', 'bibliographic' => {'title'=>'1'}, number: 2 },
+                          { 'id' => 'ref.1', 'uri' => 'http://example.com/c1', 'bibliographic' => {'title'=>'1'}, 'number' => 1 },
+                          { 'id' => 'ref.2', 'uri' => 'http://example.com/c2', 'bibliographic' => {'title'=>'1'}, 'number' => 2 },
                       ] )
 
     assert_equal p.references.size, 2
@@ -91,6 +92,7 @@ class PaperAssignMetadataTest < ActiveSupport::TestCase
     p = Paper.new
     p.assign_metadata(
             'uri' => 'http://example.com/a',
+            'bibliographic' => {},
             'references' => [
                 { 'id' => 'ref.1', 'uri' => 'http://example.com/c1', 'bibliographic' => {'title'=>'1'} , 'number' => 1},
                 { 'id' => 'ref.2', 'uri' => 'http://example.com/c2', 'bibliographic' => {'title'=>'1'} , 'number' => 2},

diff --git a/test/models/paper_test.rb b/test/models/paper_test.rb
@@ -179,4 +179,9 @@ class PaperTest < ActiveSupport::TestCase
     assert_equal(2, p.citation_groups[1].position)
     assert_equal([g2, g1], p.citation_groups)
   end
+
+  test 'The schema should be valid' do
+    assert_equal [], JSON::Validator.fully_validate_schema(Paper::JSON_SCHEMA)
+  end
+
 end
diff --git a/test/models/reference_test.rb b/test/models/reference_test.rb
@@ -61,7 +61,6 @@ class ReferenceTest < ActiveSupport::TestCase
     c = Paper.new(uri: 'http://example.org/c')
     r =     Reference.new(extra: 'foo', citing_paper: a, cited_paper: b, number:1, uri:'uri://1', ref_id:'ref.1')
     r.save
-    puts r.errors.full_messages
     assert r.save
     assert_not Reference.new(extra: 'baz', citing_paper: a, cited_paper: c, number:3, uri:'uri://2', ref_id:'ref.1').save
   end