forcedotcom / SalesforceMobileSDK-Android

Android SDK for Salesforce
Other
342 stars 388 forks source link

CollectionSyncUpTarget is relying on Id not ExternalId #2337

Closed aureosouza closed 1 week ago

aureosouza commented 2 years ago
  1. Version of Mobile SDK Used: 10.1.0
  2. Issue found in Native App or Hybrid App: Native
  3. OS Version: All
  4. Device: All
  5. Steps to reproduce:

Some steps in debugging process that may help with investigation:

5.1) SyncUp with target:

{
   "androidImpl":"com.salesforce.androidsdk.mobilesync.target.CollectionSyncUpTarget",
   "idFieldName":"Id",
   "modificationDateFieldName":"LastModifiedDate",
   "createFieldlist":[
      "Field01__c",
      "Field02__c",
      "Field03__c",
   ],
   "externalIdFieldName":"ExternalId__c",
   "maxBatchSize":200
}

5.2) In com.salesforce.androidsdk.mobilesync.target.BatchSyncUpTarget syncUpRecords has records:

[{
   "Field01__c":"Value01",
   "Field02__c":"Value02",
   "Field03__c":"Value03",
   "__locally_created__":false,
   "__locally_deleted__":false,
   "__local__":true,
   "__locally_updated__":true,
   "attributes":{
      "type":"Soup01__c"
   },
   "ExternalId__c":"ExternalIdValue123",
   "Id":"local_47494233522985",
   "_soupEntryId":411,
   "_soupLastModifiedDate":1660041375340
}]

And mergeMode is OVERWRITE

5.3) In com.salesforce.androidsdk.mobilesync.target.CompositeRequestHelper the method sendAsCollectionRequests has request:

{
   "method":"PATCH",
   "url":"\/services\/data\/v55.0\/composite\/sobjects",
   "body":{
      "allOrNone":false,
      "records":[
         {
            "attributes":{
               "type":"Soup01__c"
            },
            "Field01__c":"Value01",
            "Field02__c":"Value02",
            "Field03__c":"Value03",
            "Id":"local_47494233522985"
         }
      ]
   }
}

We believe here the api to be chosen should be the composite/sobjects/SobjectName/ExternalIdFieldName from here, and the ExternalId__c should be passed . Since we should rely only on externalId not the SF Id, since this record is created in mobile app.

Obs. 1: We noticed in some cases the correct api composite/sobjects/SobjectName/ExternalIdFieldName is called on creation of the record and syncUp, but when updating the value it syncs up with the api composite/sobjects which relies on Id not externalId and we are getting MALFORMED_ID response error from SF.

Obs. 2: We noticed as well that the referenceId is always passed from PR as this is a part of sObject Tree not sObject Collections which could be possible issue:

RecordRequest request = buildRequestForRecord(record, fieldlist);
if (request != null) {
   request.referenceId = id;
   recordRequests.add(request);
}
  1. Actual behavior: Using api sobjects which relies on Id not externalId
  2. Expected Behavior: Use api sobjects/SobjectName/ExternalIdFieldName that does not rely on Id
  3. Error Log:
[
   {
      "success":false,
      "errors":[
         {
            "statusCode":"MALFORMED_ID",
            "message":"Record ID: id value of incorrect type: local_47494233522985",
            "fields":[
               "Id"
            ]
         }
      ]
   }
]
gkotula commented 2 years ago

Hi @aureosouza, could you provide your entire test userstore.json and usersyncs.json instead of just the sync target? That would greatly help in debugging this, thanks!

aureosouza commented 2 years ago

Sending you the globalsyncs.json, let me know if this is enough:

{  "syncs":
[
  {
    "syncName": "syncDownSoup01",
    "syncType": "syncDown",
    "soupName": "Soup01__c",
    "target": {"type":"soql",
      "query":"SELECT ExternalId__c, Field01__c, Field02__c, Field03__c FROM Soup01__c WHERE Field01__c >= LAST_N_DAYS:30"},
      "options": {"mergeMode":"LEAVE_IF_CHANGED"}
    },
  {
    "syncName": "syncUpSoup01",
    "syncType": "syncUp",
    "soupName": "Soup01__c",
    "target": {"idFieldName": "Id",
               "externalIdFieldName": "ExternalId__c",
      "createFieldlist":[
        "ExternalId__c",
        "Field01__c",
        "Field02__c",
        "Field03__c"]},
    "options": {"fieldlist":["Id",
      "ExternalId__c",
      "Field01__c",
      "Field02__c",
      "Field03__c"],
      "mergeMode":"OVERWRITE"}
  }
]
}
aureosouza commented 2 years ago

We suspect it could have something to do with the decision making of which requestType and eventually the API to be choosen here in BatchSyncUpTarget:

image

Because it's seems only when isCreate is true we are able to set UPSERT (which eventually chooses API composite/sobjects/SobjectName/ExternalIdFieldName in getCollectionRequest in CompositeRequestHelper), if not it falls to UPDATE (which chooses previous API composite/sobjects). And we believe we should always choose UPSERT if we have externalId as the primary id, let us know if this helps with the investigation @gkotula.

wmathurin commented 2 years ago

The way it is currently implemented and documented is to do an upsert instead of a create for locally created records if there is an external id field name in the target definition that is populated on the record. When you update a record, the Id field should be populated already and therefore doing and update should work fine even if the external id field itself is updated. Why do you think it should be an upsert in that case?

aureosouza commented 2 years ago

@wmathurin the issue we are facing is that we did 2 sync up, one creating and another editing, but no sync down. So we do not have yet the Id from Sales Force, that's why we believe that we should still perform an UPSERT with the sobjects/SobjectName/ExternalIdFieldName API relying only on externalId. This was not an issue with the previous versions.

The reproduction steps and the error output returned by Sales Force API are all described above and it's critical scenario in our application with the new version 10.1.0, please let us know if we can provide any more info.

wmathurin commented 2 years ago

There should be no need to sync down. After a sync up, we update the id on the local record with the id returned by the server. See this code which runs for both BatchSyncUpTarget and the new CollectionSyncUpTarget.

To be sure I follow, you run the sync up with the config you pasted above twice.

The first time after the record was locally created and the second time after it was locally updated

Before 10.1.0 you were using BatchSyncUpTarget, and it was working correctly?

wmathurin commented 2 years ago

I have been testing locally and see the server id saved back on the record (in SmartStore) after the upsert as expected. Are you reading the record back from SmartStore between the first and second sync up?

aureosouza commented 2 years ago

Thanks for info, we confirmed SDK was saving the SF Id after the first syncUp. Issue seems to be that when upserting locally the Id was not being passed, only the externalId so SDK was updating the value of Id to null (which seems like wrong behaviour as well in upsert function in com.salesforce.androidsdk.smartstore.store.SmartStore). Then the second syncUp was failing. If we pass the Id we do not get the error anymore, but we still believe that if we have externalId, this should always be the primary key for both local and remote upserts and SDK should not depend on the Id.

wmathurin commented 1 week ago

Closing issue - working as expected/documented - it was more of an enhancement request "if we have externalId, this should always be the primary key for both local and remote upserts and SDK should not depend on the Id".