Merge pull request #543 from rom-rb/schema_raise_multiple_names

Make Schema#[] raise when attribute name is not unique

[changelog]

version: unreleased
changed: "`Schema#[]` and `Relation#[]` now raise an error if a given attribute is not unique (issue #529 fixed via #543) (@waiting-for-dev)"

Author: solnic

lib/rom/schema.rb

@@ -221,17 +221,16 @@ def to_h
     # @raise KeyError
     #
     # @api public
-    def [](key, src = name.to_sym)
-      attr =
-        if count_index[key].equal?(1)
-          name_index[key]
-        else
-          source_index[src][key]
-        end
-
-      raise(KeyError, "#{key.inspect} attribute doesn't exist in #{src} schema") unless attr
-
-      attr
+    def [](key, src = nil)
+      if count_index[key].zero?
+        raise(KeyError, "#{key.inspect} attribute doesn't exist in #{src} schema")
+      elsif count_index[key] > 1 && src.nil?
+        raise(KeyError, "#{key.inspect} attribute is not unique") if count_index[key] > 1
+      elsif src
+        source_index[src][key]
+      else
+        name_index[key]
+      end
     end
 
     # Project a schema to include only specified attributes
@@ -460,7 +459,9 @@ def set!(key, value)
 
     # @api private
     def count_index
-      map(&:name).map { |name| [name, count { |attr| attr.name == name }] }.to_h
+      reduce(Hash.new(0)) do |index, attr|
+        index.merge(attr.name => index[attr.name] + 1)
+      end
     end
 
     # @api private

spec/unit/rom/schema/accessing_attributes_spec.rb

@@ -36,10 +36,6 @@
       define_schema(:tasks, id: :Integer, title: :String)
     end
 
-    it "returns an attribute identified by its canonical name" do
-      expect(schema[:id]).to eql(define_attribute(:Integer, {name: :id}, source: :users))
-    end
-
     it "returns an attribute identified by its canonical name when its unique" do
       expect(schema[:title]).to eql(define_attribute(:String, {name: :title}, source: :tasks))
     end
@@ -52,5 +48,9 @@
       expect { schema[:not_here] }.to raise_error(KeyError, /not_here/)
       expect { schema[:not_here, :tasks] }.to raise_error(KeyError, /not_here/)
     end
+
+    it "raises KeyError when attribute name is not unique" do
+      expect { schema[:id] }.to raise_error(KeyError, /id/)
+    end
   end
 end