Project tracker example / Invalid collection

Project tracker example / Invalid collection

Old forum URL: forums.lhotka.net/forums/t/8510.aspx


rxelizondo posted on Wednesday, February 10, 2010

Hello,

If you look at the ResourceAssignments class under the ProjectTracker example there is a function called AssingTo that goes something like this:

public void AssignTo(Guid projectId)
{
    if (!(Contains(projectId)))
    {
        ResourceAssignment project = ResourceAssignment.NewResourceAssignment(projectId);
        this.Add(project);
    }
    else
    {
        throw new InvalidOperationException("Resource already assigned to project");
    }
}

Key point here is that the application raises an error if the project ID is already included on the collection.

For the most part, this is perfectly fine but the problem is that a user is not forced to use the AssignTo function to add a child to the collection, children can also be added in other ways, for example, here are some other ways:

resource.Assignments[ ]
resource.Assignments.AddNew
resource.Assignments.AddRange

So I think Project Tracker should stop rising an error from within the AssignTo function. I think the error should be thrown in some other central function that is able to handle the user adding child object in all the different ways, else we would end up with an invalid collection.

I have some ideas on how to go about fixing this but before I go there, as always, there is the possibility that I am missing something here so I appreciate if someone could set me straight.

Thanks.

ajj3085 replied on Wednesday, February 10, 2010

Is there a public constructor though for the ResourceAssignment class?  If not, none of those ways should work, because you can't create an instance of ResourceAssignment anyway.

rxelizondo replied on Wednesday, February 10, 2010

Andy,

I wish things were that simple! Smile I had looked into that and unfortunately the issue is a little more complex, the problem is the implementation of the InsertItem override:

---------------------------------------------------------------------------

protected override void InsertItem(int index, C item)
{
    // set parent reference
    item.SetParent(this);
    // set child edit level
    Core.UndoableBase.ResetChildEditLevel(item, this.EditLevel, false);
    // when an object is inserted we assume it is
    // a new object and so the edit level when it was
    // added must be set
    item.EditLevelAdded = _editLevel;
    InsertIndexItem(item);
    base.InsertItem(index, item);
    InsertIntoMap(item, index);
}

---------------------------------------------------------------------------

Without digging too deep, it looks like the code happily accepts the new object. So even without a public constructor you can assign object to the collection like this:

var xyz = someOtherResource.Assignments[0];
resource.Assignments.Add(xyz);

Or let’s go even crazier, what about something like this:

resource.Assignments.Add(resource.Assignments[0]);

You may argue that someone doing that is just being plain stupid and they deserve to suffer the consequences but I would argue that the CSLA should protect the user from such acts.

ajj3085 replied on Wednesday, February 10, 2010

Well, I'm not sure Csla supports neither of those scenarios.

Personally I use business rules to invalidate the child objects which refer to the same item.  But ProjectTracker is just a simple example to go along with the book.. not necessarly an illustration of how you should do things.  Wink

rxelizondo replied on Wednesday, February 10, 2010

I think it would be best if we removed the exception from the AssignTo method.

If anything, removing the exception from there will stop giving new developers a false sense of security since many of the may not be aware of the other backdoors.

As of this time, I think that in cases like this, the simplest solution to the problem would be to create a broken rule that prevents duplicate project ids.

Also, I really think that the CSLA needs to raise and exception if a child object that already has a parent, tries to be inserted somewhere else where its parent property needs to be set again. So basically something like this:
 
protected override void InsertItem(int index, C item)
{
    if (this.Parent != null)
        throw new YouCantDoThisException();

    // set parent reference
    item.SetParent(this);

    …………
}

If it’s illegal for a CSLA object to have multiple parents then why not make it official.

Rocky, if you are reading this what is your opinion? I realize that there are a lot of other more important issues going on right now and something like this is very low priority but I would still like to hear your thoughts if you don’t mind.

Thanks.

Copyright (c) Marimer LLC