Before Insert or Upsert list must not have two identical equal elements

Question

I am trying to populate a custom object(Account_Report__c) by a batch Apex that will fetch related records from two other objects (Account and Product_Metrics_vod__c).

Account and Product Metrics object are having master-detail relation. Product Metrics have Product Id + Account Id - which I am using in my code as unique key.

For each of Product + Account combination in Product Metrics, we should have one record in the resultant Account_report__c object. So, if this combination key is present in Target (Account_report__c )object we should update the record in target or else we need to insert the record.

But I am getting same recird multiple times in the arInslst List. and the code fails with the error - System.ListException: Before Insert or Upsert list must not have two identically equal elements Please assist.

Thanks, Below is my code

   global class batchAccountProductMetricReporting implements
     Database.Batchable<sObject> {
      global Database.QueryLocator start(Database.BatchableContext BC) {
       String query = 'SELECT Id,Name , Products_vod__c,   Account_vod__c,Segment_Loyalty_LBK_FI__c,Segment_Potential_LBK_FI__c 
                       FROM Product_Metrics_vod__c WHERE Segment_Loyalty_LBK_FI__c != null';
       // where Segment_Potential_LBK_FI__c ! = null or Segment_Loyalty_LBK_FI__c ! = null';
       return Database.getQueryLocator(query);
   }

   global void execute(Database.BatchableContext BC, List<Product_Metrics_vod__c> scope) {
       Map<id,Product_Metrics_vod__c> pmMap1 = new Map<id,Product_Metrics_vod__c>();
       Map<id,id> pmMap2 = new Map<id,id>();
       Map<string,id> pmMap3 = new Map<string,id>();

       List<Account> acclist;
       List<Account_Report__c> acReplst;

       List<Account_Report__c> arInslst = new List<Account_Report__c>();
       List<Account_Report__c> arUpdlst = new List<Account_Report__c>();

       Set<Id> pmId = new Set<Id>();
       String pmKey='';
       String pmKey2='';

        for(Product_Metrics_vod__c pm: scope)
        {
            system.debug ('1-------------------Scope '+ pm.Id);
            pmId.add(pm.Account_vod__c);
            pmKey= pm.Account_vod__c+'_'+pm.Products_vod__c;
            pmMap1.put(pm.Id,pm);
            pmMap3.put(pmKey,pm.Id);

        }
        Account_Report__c accMet= new Account_Report__c();
        List<Id> acMetNamelst= new List<Id>();
        acReplst = new List <Account_Report__c> ([SELECT Id,Name  FROM Account_Report__c where Name IN: pmMap3.keySet()]);

        Map<id,Account_Report__c> arMap = new Map<id,Account_Report__c>();
        Map<id,string> arMap2 = new Map<id,string>();

        for (Account_Report__c  ar : acReplst)
        {
            arMap.put(ar.Id,ar);
            arMap2.put(ar.Id,ar.Name);
            acMetNamelst.add(ar.Id);
        }
        acclist= new List <Account> ([SELECT Id,Name  FROM Account where id IN: pmId]);

        for(Product_Metrics_vod__c pm: scope)
        {
            pmKey2= pm.Account_vod__c+'_'+pm.Products_vod__c;
            system.debug ('4-------------------PmId inside for 1 ------------>'+ pm);
            for (Account acc : acclist)
            {
                  if (acc.Id==pmMap1.get(pm.Id).Account_vod__c) 
                  {
                      if (arMap2.get(acc.Id)== null){                        
                        if (!pmKey2.Equals(arMap2.get(acc.Id)))
                        {
                            accMet.Account_Name__c=pm.Account_vod__c;
                            accMet.Product_Name__c=pm.Products_vod__c;
                            accMet.Loyalty__c=Integer.ValueOf(pm.Segment_Loyalty_LBK_FI__c) ;
                            accMet.Name=pm.Account_vod__c+'_'+pm.Products_vod__c;
                            accMet.Potential__c=pm.Segment_Potential_LBK_FI__c ;
                             system.debug('20---------------------hi apurba 2----->'+accMet);
                            arInslst.add(accMet);
                        }
                        system.debug('21---------------------hi apurba 3 list ----->'+arInslst);
                    }
                    //update
                    else 
                    {
                        system.debug('10---------------------hi apurba 2----->');
                        accMet.Loyalty__c=Integer.ValueOf(pm.Segment_Loyalty_LBK_FI__c) ;
                        accMet.Potential__c=pm.Segment_Potential_LBK_FI__c ;

                        arUpdlst.add(accMet);
                    }
                                     }
                //}//3rd for close


            }

        }
        insert arInslst;
        update arUpdlst;
       }   

   global void finish(Database.BatchableContext BC) {
    //nothing to write here.
   } }

@dphil : no, My problem is with arInsList....the problm lies with list population only..as same Record from Product Metrics is getting added again and again...I understand that currently I am taking the records from one object only...but going fwd I also would need the 2nd object as well so I would need the nested for loop.

@Keith C - thanks for your suggestions. I am sure It would work.However, oing fwd I also would need the 2nd object as well so I would need the nested for loop. Basically I will be doing a join type of stuff b/w the two input objects and will store the result in the Target object based on some criteria.

Answer 1

The existing code isn't too easy to follow. And with all the nested loops it's easy to imagine that duplicate items are getting added to a list. Rather than eliminating duplicates by using maps, I suggest it would be better to stop the code duplicating the data in the first place.

It is worth naming variables after their content and limiting their scope i.e. declare them where you need them and don't re-use them. The sort of code I would write for what I think you are doing (you may be doing something other than this so just take it as an example) is this:

   global void execute(
           Database.BatchableContext bc,
           List<Product_Metrics_vod__c> scope
           ) {
       // Find all keys
       Set<String> keys = new Set<String();
       for(Product_Metrics_vod__c pm : scope) {
           keys.add(key(pm));
       }
       // Query the existing ARs
       Map<String, Account_Report__c> keyToAr = new Map<String, Account_Report__c>();
       for (Account_Report__c ar : [
               SELECT Id, Name
               FROM Account_Report__c
               where Name IN :keys
               ]) {
           keyToAr.put(ar.Name, ar);
       }
       // Will update if already exists or will insert
       for (Product_Metrics_vod__c pm : scope) {
            Account_Report__c ar = keyToAr.get(key(pm));
            if (ar == null) {
                ar = new Account_Report__c(Name = key(pm));
                keyToAr.put(key(pm), ar);
            }
            ar.Account_Name__c = pm.Account_vod__c;
            ar.Product_Name__c = pm.Products_vod__c;
            ar.Loyalty__c = Integer.ValueOf(pm.Segment_Loyalty_LBK_FI__c);
            ar.Potential__c = pm.Segment_Potential_LBK_FI__c;
       }
       // Guaranteed only one value per key in this map
       upsert keyToAr.value();
   }
   private String key(Product_Metrics_vod__c pm) {
       return pm.Account_vod__c + '_' + pm.Products_vod__c;
   }

The above avoids multiple levels of loop nesting by using maps: this allows a value to be grabbed in a single statement rather than a set of values having to be looped over. Less code and execution time grows linearly rather than exponentially.

Answer 2

This can occur, when the list being inserted/updated/upserted has similar record multiple times.

To resolve this, use map instead of lists to store records to be inserted/updated. This will help you to ensure that you have only one instance of object in your map at any given point of time.

Answer 3

I'm assuming you are talking about your arUpdlst. If I go and look at what you are adding to this list:

arUpdlst.add(accMet);

Ok, so what is accMet?

Account_Report__c accMet= new Account_Report__c();

This appears to be OUTSIDE of your loop. Ok... while it's a bad development practice, it doesn't mean anything right?

accMet.Loyalty__c=Integer.ValueOf(pm.Segment_Loyalty_LBK_FI__c) ;
accMet.Potential__c=pm.Segment_Potential_LBK_FI__c ;

Turns out that this is the only interaction you are having with your object. It's still the same object but you keep adding it to the list over and over again.

If you are trying to update something, you should be getting it from somewhere. It looks like your setting some values on it but that's about it. You should instead be fetching the instances of this which should be done inside the loop. (You will need to keep it bulkified though and so will need a query for ALL of them outside the loop and then use a Map to fetch them.)

You have a similar issue on your insert, but you need to be creating a new one each time instead of fetching one.