Delete method non working on custom class that implements IEntity
Closed this issue · 4 comments
Hello, I have defined this baseclass for my objects. I copied it from the Entity class because extending it would mess the collection name mapping mechanism.
[DataContract]
[Serializable]
[BsonIgnoreExtraElements(Inherited = true)]
public abstract class BaseObject : IEntity<string>
{
/// <summary>
/// Gets or sets the id for this object (the primary record for an entity).
/// </summary>
/// <value>The id for this object (the primary record for an entity).</value>
[DataMember]
[BsonRepresentation(BsonType.ObjectId)]
public virtual string Id { get; set; }
/// <summary>
/// This method could be invoked to set up somestuff
/// </summary>
public virtual void Init() { }
public virtual DateTime CreationDate
{
get {
if (!string.IsNullOrWhiteSpace(Id))
return ObjectId.Parse(Id).CreationTime;
return DateTime.MinValue;
}
}
}
}
The problem is that when I call the repository.Delete method nothing happens. I tracked down the code and got to the definition of the method. The problem is in that If statement that only limit the execution to the entity class items while it should be more about who implements the IEntity interface I think.
/// <summary>
/// Deletes an entity from the repository by its id.
/// </summary>
/// <param name="id">The entity's id.</param>
public virtual void Delete(TKey id)
{
if (typeof(T).IsSubclassOf(typeof(Entity)))
{
this.collection.Remove(Query.EQ("_id", new ObjectId(id as string)));
}
else
{
this.collection.Remove(Query.EQ("_id", BsonValue.Create(id)));
}
}
A working solution would be to change it to
public virtual void Delete(TKey id)
{
if (typeof(T).GetInterfaces().Contains(typeof(IEntity<string>)))
{
this.collection.Remove(Query.EQ("_id", new ObjectId(id as string)));
}
else
{
this.collection.Remove(Query.EQ("_id", BsonValue.Create(id)));
}
}
Best regards
Marco
Hi Marco,
Okay, this one's a bit tough to explain but I'll try. I must admit you had me convinced at first this was a bug, but, now, I do not agree (totally) but I will admit it's a bit sketchy...
I'll try to work this out with an example:
Suppose you implemented a class as IEntity<int> instead; this way it's pretty clear that when you implement IEntity (instead of derive from Entity) you're pretty much responsible yourself for generating (unique) Id's upon inserting items. So you set MyObject.Id = 12345 on creation (or just before insertion) of the object and can then call MyRepo.Delete(MyObject.Id);. Everything's fine here.
Now you implement IEntity<string>. Because the Id is a string Mongo will generate an Id for you (specifically: an ObjectId) as you indicated with the BsonRepresentation attribute. However, upon deletion a string is passed in and Mongo goes off searching for a string which it will never find; it needs to look for an ObjectId. Your proposed fix fixes the specific case where the Id happens to also be an ObjectId. But it will fail if I created an entity implementing IEntity<string> on which I'd like to assign, say, a GUID (Guid.NewGuid().ToString("N")) for the Id property instead of an ObjectId. The IEntity interface implicitly means you have to handle assigning Id's yourself and you loose the ObjectId (MongoRepository tries to abstract as much Mongo-specific stuff away anyway to be as loosely-coupled as possible; the attributes I never recommend although they can come in handy admittedly).
I guess the best solution for you would be to drop the BsonRepresentation attribute and handle assigning Id's, possibly in the constructor or in your derived repository's Add(...) method (using ObjectId.GenerateNewId().ToString()), to make sure the value isn't persisted in MongoDB as an ObjectId but as actual string. This way, the Delete() method will also be able to find the matching object by it's (persisted-as-string) Id.
I admit it's a bit hairy but I can't break backwards compatibility (or at least: strive to do so as little as possible). I could, alternatively, check in the else clause of the Delete() method if the string 'looks like an ObjectId' (using ObjectId.TryParse()) and pass the string as ObjectId to Mongo or else just as a normal string. But this kind of "autodetection" usually causes more harm than it does good and has the potential to also create edge-cases/bugs that are hard to find/reproduce since you expect a string to be used as id which is then 'automagically' converted to an ObjectId without you knowing (essentially creating a similar issue as you have now).
Your proposed .GetInterfaces().Contains(...) solution is also, potentially, a pretty 'expensive' one, depending on the number of interfaces an object implements. I'm pretty sure this could be implemented like, for example, something along the lines of "if (T is IEntity)" or "typeof(IEntity).IsAssignableFrom(typeof(T))" (but then their 'generic variations') but I don't have an development environment, nor the time, at hand currently to work out what it would look like exactly. But we can't go on IEntity<string> as I explained earlier since the string that Id is may or may not be an ObjectId. I'm afraid we'd have to use some ugly reflection or other way to find out if the attribute is set on the Id property of T which, again, will probably be a costly operation and I'm not sure if we'd ever be able to get it rock solid. I'm just posting this as a hint for people wanting to think along with me/us.
I am, as always, open for suggestions and discussion. Let me know what you think! Meanwhile, I'll try to see if I can come up with a clean, satisfactory, solution that works in all cases. Maybe it turns out to be stupendously simple (I've been there before) and I (or someone) comes up with a beautifully elegant solution...
At least I hope I explained the issue and my problem(s) in (trying to) solving this.
Hi, thank you for the response.
I kind of understood what you've explained, even if this is my first mongodb project, so I'm new to this world.
As I don't have a solution I will try to explain you why I've implemented the IEntity instead of subclass the Entity class: I wanted some custom fields that comes in handy like the CreationDate property (obtained from the handy objectId) and an Init method that I call from my ultra generic webapi controller.
Obviously I wouldn't pollute the Entity class or ask you to (even if perhaps those fields could be useful to everybody I guess and shouldn't generate so much overhead), so at first I subclassed the entity with a custom class called BaseObject : Entity where I added my fields. Therefore all my business objects inherited from BaseObject.
Everything in the project started to fail at runtime, and after a bit of debugging I found out that MongoRepository was writing this objects into the BaseObject collection instead of the proper collection that matched the class name: I had a BaseObject collection with subjects, orders, addresses, etc. all mixed.
So I reverted it and done a "intelligent lending" (aka copypaste) of your Entity class, that led me to discover this issue.. and here I am :)
Regarding the GetInterfaces().Contains vs IsAssignableFrom I thought of that issue too but I read somewhere on stackoverflow that the IsAssignableFrom isn't so much reliable (but I mildly assimilated the info without a deeper understanding).
So, getting to the point, I can:
- Leave this as is and implement my own modification of your library every release (umh)
- Lure you into adding the CreationDate property and the Init() method to the Entity class (ummmmmh)
- Revert the BaseObject to be a subclass of entity and somehow fix the collection name resolver bug (if still there... it's been a while). Perhaps decorating the class with the attribute of the collection name should do the trick but I'm lazy :)
thanks for your time
Marco
I don't think you are doing this in a right way.
If you look inside MongoRepositoryTests folder you can see there is an implementation of Custom id property.
[CollectionName("MyTestCollection")]
public class CustomIDEntityCustomCollection : CustomIDEntity { }
You can have any structure for your entities (inheritance etc.) as far as you follow the directions. The scenario you'are describing are almost all covered in Tests.
Yes, as I said I will try decorating the class with the collectionname attribute and that should do the trick.
I would have expected, as long as I was only subclassing the Entity class and not making a custom Id entity, that the collection autonaming mechanism would work without it, but it's no big deal using it.
Regarding the oringial issue, perhaps the "autodetection" mechanism could be implemented in the else clause of the Delete method just to throw an Excepion explaining the issue so the developer is aware that something is wrong, because right now it just silently fail.
Best regards,
Marco