keys with type "Set" returning an Array rather than set in 0.13.1 #614

Activity

1. cheald commented on Dec 4, 2015

   

   cheald

   on Dec 4, 2015

   Member

   More actions

   Ah, interesting. That looks like a regression! I'll see what I can find!
2. cheald commented on Dec 4, 2015

   

   cheald

   on Dec 4, 2015

   Member

   More actions

   describe "a key with a Set type" do
     DocWithSetKey = Doc do
       set_collection_name "docs_with_set_keys"
       key :foo, Set
     end
   
     before do
       DocWithSetKey.create foo: [1, 2, 3]
     end
   
     it "should persist as a Set" do
       expect(DocWithSetKey.first.foo).to be_a(Set)
     end
   end

   This spec passes for me. Can you help me figure out how to break it?
3. jamieorc commented on Dec 4, 2015

   

   jamieorc

   on Dec 4, 2015

   ContributorAuthor

   More actions

   Thanks, I'll have a look this afternoon.
4. jamieorc commented on Dec 7, 2015

   

   jamieorc

   on Dec 7, 2015

   ContributorAuthor

   More actions

   I've verified the problem again, so digging into it.

   p.components.first.ancestor_ids.class
    => Array 
   
5. jamieorc commented on Dec 7, 2015

   

   jamieorc

   on Dec 7, 2015

   ContributorAuthor

   More actions

   I've found what causes the problem, but not the regression in the code:

   When :typecast is set, Array is returned instead of Set. Here, :foo_ids is typecast to ObjectId:

   2.1.7 :021 > class Foo; include MongoMapper::EmbeddedDocument; key :foo_ids, Set, typecast: "ObjectId"; key :bars, Set;end
    => #<MongoMapper::Plugins::Keys::Key:0x007ffbdb61a740 @type=Set, @name="bars", @options={}, @dynamic=false, @embeddable=false, @is_id=false, @typecast=nil, @accessors=[], @has_default=false, @ivar=:@bars> 
   2.1.7 :022 > f = Foo.new
    => #<Foo _id: BSON::ObjectId('5665d7335fd76c19f6000005'), bars: #<Set: {}>, foo_ids: []> 
   2.1.7 :023 > class Foo; include MongoMapper::EmbeddedDocument; key :foo_ids, Set; key :bars, Set;end
   

   Here :foo_ids is set to String:

   2.1.7 :026 >   class Foo; include MongoMapper::EmbeddedDocument; key :foo_ids, Set, typecast: "String"; key :bars, Set;end
    => #<MongoMapper::Plugins::Keys::Key:0x007ffbdb8d45a8 @type=Set, @name="bars", @options={}, @dynamic=false, @embeddable=false, @is_id=false, @typecast=nil, @accessors=[], @has_default=false, @ivar=:@bars> 
   2.1.7 :027 > f = Foo.new
    => #<Foo _id: BSON::ObjectId('5665d7fa5fd76c19f6000007'), bars: #<Set: {}>, foo_ids: []> 
   

   With no :typecast, a Set is returned, as expected:

    => #<MongoMapper::Plugins::Keys::Key:0x007ffbddad20d8 @type=Set, @name="bars", @options={}, @dynamic=false, @embeddable=false, @is_id=false, @typecast=nil, @accessors=[], @has_default=false, @ivar=:@bars> 
   2.1.7 :024 > f = Foo.new
    => #<Foo _id: BSON::ObjectId('5665d7645fd76c19f6000006'), bars: #<Set: {}>, foo_ids: #<Set: {}>> 
   
6. jamieorc commented on Dec 7, 2015

   

   jamieorc

   on Dec 7, 2015

   ContributorAuthor

   More actions

   I've found the regression and will work up a pull request. It's around using map instead of map! when typecasting:

   • map(currently used) always returns an Array
   • map! (formerly used) operates on the existing Array or Set, so preserves the original list type
7. jamieorc commented on Dec 7, 2015

   

   jamieorc

   on Dec 7, 2015

   ContributorAuthor

   More actions

   If you want to see a spec that fails, add this into functional/keys_spec.rb:

     context "when persisting a typecast Set" do
       TypecastedKeyModel = Doc do
         key :people, Set, :typecast => "Person"
       end
   
       Person = Struct.new(:name) do
         def self.to_mongo(value)
           value.name
         end
   
         def self.from_mongo(value)
           Person.new value
         end
       end
   
       it "should not mutate the model's state" do
         person = Person.new "Bob"
         doc = TypecastedKeyModel.new(:people => Set.new([person]))
   
         doc.save
   
         doc.people.should == Set.new([person])
       end
     end
   
8. jamieorc commented on Dec 7, 2015

   

   jamieorc

   on Dec 7, 2015

   ContributorAuthor

   More actions

   Question for you, Chris: is there any reason not to use map! to modify what is returned by from_mongo? I'd rather not make several lists to get to the final one unless necessary.
9. cheald commented on Dec 7, 2015

   

   cheald

   on Dec 7, 2015

   Member

   More actions

   I can't think of a reason off the top of my head, but generally I make those kinds of changes when there may be potential side-effects of mutating a provided list rather than returning a new copy.

   Where's the offending line? I might be able to provide better opinions if I can look at the blame history.
10. jamieorc commented on Dec 7, 2015

    

    jamieorc

    on Dec 7, 2015

    ContributorAuthor

    More actions

    Line 65 in key.rb:

    https://github.com/mongomapper/mongomapper/blob/master/lib/mongo_mapper/plugins/keys/key.rb#L65

    I've got a spec and the change done on a fork. I'll make a pull request if you don't see any reason not to.