Map messes up stored records ! Salesforce Bug or developer's bug?

Question

I will try to keep this short... We are experiencing an issue which looks like it's caused by a bug in the internal implementation of Map in Salesforce. In more details

We have a visualforce which allows the user to select users and Team roles and add them to the Opportunity Team members. A Map mOpportunityTeamMembers is used to store the existing and the new members added.

In the Visualforce a pageblock table is used to list the Opportunity Team Members. The pageblock uses a List which is set via the controller as mOpportunityTeamMembers.values()

The problem is that EVERY time a new member is added in the Map, the Map messes up the existing values ! So lets assume we initially have User K with role Team Member and user M with role Responsible after pressing the button to add one more user (BEFORE THE METHOD IS CALLED) we end up with K as Responsible and M as Team Member In fact the records are messed up even before entering the method which is adding the new member !!

and the funniest thing is the "workaround" ! Sort the Map.values before returning in the getter method !! (see solution part below)

Do you see something that I am missing ?? any ideas ???

Debug Log

USER_DEBUG|[18]|DEBUG|mOpportunityTeamMembers:{005D0000001moFQIAY >=OpportunityTeamMember:{CurrencyIsoCode=EUR, Id=00qL00000002TPTIA2, >UserId=005D0000001moFQIAY, TeamMemberRole=Team Member}, 005D0000001nDJdIAM >=OpportunityTeamMember:{CurrencyIsoCode=EUR, Id=00qL00000002RH4IAM, >UserId=005D0000001nDJdIAM, TeamMemberRole=Responsible}} SYSTEM_METHOD_EXIT|[18]|System.debug(ANY) METHOD_ENTRY|[19]|01pD0000000WGQv|OpportunityTeamMembersController.__sfdc_mOpportunityTeamMembers() |METHOD_EXIT|[19]|01pD0000000WGQv|OpportunityTeamMembersController.__sfdc_mOpportunityTeamMembers() |SYSTEM_METHOD_ENTRY|[19]|MAP.values() |SYSTEM_METHOD_EXIT|[19]|MAP.values() |CODE_UNIT_FINISHED|members |CODE_UNIT_FINISHED|OpportunityTeamMembersController get(members) |CODE_UNIT_STARTED|[EXTERNAL]|01pD0000000WGQv|OpportunityTeamMembersController get(newMember) |SYSTEM_MODE_ENTER|true |CODE_UNIT_STARTED|[EXTERNAL]|01pD0000000WGQv|newMember |CODE_UNIT_FINISHED|newMember |CODE_UNIT_FINISHED|OpportunityTeamMembersController get(newMember) |CODE_UNIT_STARTED|[EXTERNAL]|01pD0000000WGQv|OpportunityTeamMembersController invoke(addMember) |METHOD_ENTRY|[127]|01pD0000000WGQv|OpportunityTeamMembersController.__sfdc_mOpportunityTeamMembers() |METHOD_EXIT|[127]|01pD0000000WGQv|OpportunityTeamMembersController.__sfdc_mOpportunityTeamMembers() |SYSTEM_METHOD_ENTRY|[127]|String.valueOf(Object) |SYSTEM_METHOD_EXIT|[127]|String.valueOf(Object)1 |SYSTEM_METHOD_ENTRY|[127]|System.debug(ANY)

AND HERE !!! THE MAP MEMBER ROLES (!!!) are reordered...

Please note this is the first line of the method

|USER_DEBUG|[127]|DEBUG|addMember_start: OPP TEAM MEMBERS{005D0000001moFQIAY=OpportunityTeamMember:{CurrencyIsoCode=EUR, Id=00qL00000002TPTIA2, UserId=005D0000001moFQIAY, TeamMemberRole=Responsible}, 005D0000001nDJdIAM=OpportunityTeamMember:{CurrencyIsoCode=EUR, Id=00qL00000002RH4IAM, UserId=005D0000001nDJdIAM, TeamMemberRole=Team Member}}

Please find below the code for the visualforce page and the related part of the controller.

Visualforce Page

<apex:page standardController="Opportunity" extensions="OpportunityTeamMembersController" >
<apex:form >
    <apex:pageBlock title="">

        <apex:pageBlockButtons location="bottom" >
              <apex:commandButton value="{!$Label.Save}" action="{!saveChanges}" disabled="{!NOT(isUserInTeam)}" /> 
              <apex:commandButton value="{!$Label.Cancel}" action="{!cancelChanges}"/> 
        </apex:pageBlockButtons>

        <apex:pageBlockSection id="newMemberSection" title="{!$Label.New_Team_member}" columns="5" rendered="{!isUserInTeam}">
                <apex:inputField value="{!newMember.UserId}" required="false" />                
                <apex:inputField value="{!newMember.TeamMemberRole}" />             
                <apex:commandButton value="{!$Label.Add_member}" action="{!addMember}" rerender="membersSection,newMemberSection" />                
        </apex:pageBlockSection>

        <apex:pageBlockSection id="membersSection" title="{!$Label.Team_members}" columns="1">
            <apex:pageBlockTable value="{!members}" var="member">
                <apex:column headerValue="{!$Label.Name}">
                    <apex:outputField value="{!member.User.Name}" />
                </apex:column> 

                <apex:column headerValue="{!$Label.Role}"  rendered="{!isUserInTeam}">
                    <apex:inputField value="{!member.TeamMemberRole}" />
                </apex:column>

                 <apex:column headerValue="{!$Label.Role}" rendered="{!NOT(isUserInTeam)}" >
                    <apex:outputField value="{!member.TeamMemberRole}" />
                </apex:column> 

                <apex:column headerValue="{!$Label.Remove}" style="width:15em" rendered="{!isUserInTeam}">
                    <apex:commandLink action="{!deleteMember}" >
                        <apex:param name="selectedMemberId" value="{!member.UserId}" assignTo="{!selectedMemberId}"/>
                        <apex:outputLabel value="{!$Label.Delete}"/>
                    </apex:commandLink>
                </apex:column>
            </apex:pageBlockTable>
        </apex:pageBlockSection>
    </apex:pageBlock>

</apex:form>

Controller

public List<OpportunityTeamMember> members {
    get {
        system.debug('Calling getMembers Method !');
        system.debug('mOpportunityTeamMembers:'+mOpportunityTeamMembers);
        system.debug('mOpportunityTeamMembers.values():'+mOpportunityTeamMembers.values());
        return mOpportunityTeamMembers.values();
    }
    set;
}


/**
 * @brief Map of User ID to OpportunityTeamMember record.
 */
public Map<ID, OpportunityTeamMember> mOpportunityTeamMembers ;//{get; set;}    

/**
 * @brief Reference to OpportunityTeamMember record. Used to add new member to Opportunity team.
 */
public OpportunityTeamMember newMember {get; set;}

/**
 * @brief Id of member selected to delete.
 */
public ID selectedMemberId {get; set;}

/**
 * @brief Flag to store information is user in Opportunity Team Members. It is use to check is user has access to edit page or not. 
 */
public Boolean isUserInTeam {get; set;}

/**
 * @brief Reference to Opportunity controller.
 */
private Opportunity opp;

/**
 * @brief List of ids of OpportunityTeamMember records which will be deleted.
 */
private List<ID> membersToDeleteIDs;

private Boolean isRoleFree;


/**
 * @brief   Class constructor.
 */
public OpportunityTeamMembersController (ApexPages.StandardController controller) {

    system.debug('OpportunityTeamMembersController');   



    this.opp = (Opportunity) controller.getRecord();
    this.mOpportunityTeamMembers = OpportunityTeamMemberServices.getOpportuniyTeamMembersMap('Id, UserId, User.Name, TeamMemberRole', opp.Id);
    this.newMember = new OpportunityTeamMember();  
    this.membersToDeleteIDs = new List<Id>();        
    this.isUserInTeam = isUserAvailableToManageTeam(UserInfo.getUserId());
    system.debug('OpportunityTeamMembersController_end: OPP TEAM MEMBERS'+this.mOpportunityTeamMembers);        
    system.debug('OpportunityTeamMembersController_end: MEMBERS'+this.members);
    system.debug('OpportunityTeamMembersController_end: NEWMEMBERS'+this.newMember);        

}



/**
 */
public void addMember () {
    system.debug('addMember_start: OPP TEAM MEMBERS'+this.mOpportunityTeamMembers);     
    system.debug('addMember_start: MEMBERS'+this.members);
    system.debug('addMember_start: NEWMEMBERS'+this.newMember);          

    isRoleFree = this.isRoleAvailable(newMember.TeamMemberRole);


    if (newMember.UserId != null) {
        if (this.isRoleFree == false) {
            this.newMember.UserId.addError('Role is already used');                 
        }
        else if (!this.mOpportunityTeamMembers.containsKey(newMember.UserId)) {
            this.newMember.OpportunityId = this.opp.Id;
            this.newMember.User = UserServices.getUser('ID, Name', newMember.UserId);                

            this.mOpportunityTeamMembers.put(this.newMember.UserId, this.newMember);               


            this.newMember = new OpportunityTeamMember();             

        }               
        else {
            this.newMember.UserId.addError(System.Label.Error_user_in_team);
        }
    } else {
        this.newMember.UserId.addError(System.Label.Error_field_is_required);
    }    
    system.debug('addMember_end: OPP TEAM MEMBERS'+this.mOpportunityTeamMembers);       
    system.debug('addMember_end: MEMBERS'+this.members);
    system.debug('addMember_end: NEWMEMBERS'+this.newMember);        

}

SOLUTION

    public List<OpportunityTeamMember> members {
    get {
        system.debug('Calling getMembers Method !');
        system.debug('mOpportunityTeamMembers:'+mOpportunityTeamMembers);
        system.debug('mOpportunityTeamMembers.values():'+mOpportunityTeamMembers.values());
        List<OpportunityTeamMember> members_sorted = mOpportunityTeamMembers.values();
        members_sorted.sort();
        return members_sorted;
    }
    set;

Answer 1

The values() list is not guaranteed to be in order by default, because the hashing algorithm may arbitrarily reorder the elements in the list. I am not sure if I would classify this as a bug, but I do know I would never depend on the values() list being in any particular order; Visualforce has a hard time dealing with maps in general.

What I would do is instead create a wrapper class that keeps the list in a predefined order. I created this technique after I ran across a bug where Visualforce would die unexpectedly when a key was missing from a map, but I'm sure it has other uses as well. Here's the design pattern:

public class Controller {
    Map<Id, SObject> objects;
    public ObjectWrapper[] objectList { get; set; }

    public class ObjectWrapper {
        Controller controller;
        Id key;

        public ObjectWrapper(Controller controller, Id key) {
            this.controller = controller;
            this.key = key;
        }

        public SObject record { 
            get { return controller.get(value); } 
            set { controller.put(key, value); }
        }
    }

    public Controller() {
        objects = new Map<Id, SObject>();
        objectList = new ObjectWrapper[0];
        for(SObject record: [...]) {
            objects.put(record.Id, record);
            objectList.add(new ObjectWrapper(this, record.Id);
        }
    }
}

The basic premise is that the objectList provides a stable ordering of records which guarantees that the data won't be obliterated during deserialization. You can add values to the objectList, and you can even specify the same key twice to cause the value to appear more than once (but then you risk the object being clobbered randomly).

Best of all, you could implement Comparable to allow automatic sorting of the objectList while leaving the map alone. Since the map is naturally unordered, storing the order as a separate list will set things straight. Best of all, this pattern specifically avoids the "missing map key" error.

Update

As an example version of faulty logic, imagine we have the following code:

Opportunity[] records = somemap.values().deepClone(true, true, true);
String[] fields = new String[] { 'Name', 'CloseDate', 'Id' };

for(Opportunity record: records)
    record.closedate = System.Today()+(1000*Math.random()).intValue();

for(Integer fdx = 0; fdx < fields.size(); fdx++)
    for(Integer idx = 0; idx < records.size(); idx++)
        somemap.values()[idx].put(fdx, records[idx].get(fields[fdx]));

This code would be perfectly sane if we replaced somemap.values() with someList, instead. That's because Map.values() returns an unordered list, which means that the results are subject to re-order themselves arbitrarily, probably due to internal hashing. Therefore, since somemap.values() is not guaranteed to return the same order, the fields could easily become scrambled; you may as well be calling: somemap.values()[(somemap.size()*Math.random()).intValue()].put(fdx, records[idx].get(fields[fdx]));

Hopefully this illustrates the problem better. It's nothing to do with serializing or deserializing, it has to do with the values() list not necessarily retaining its order, since it is, by definition, an unordered list.