Skip to content

Fix callbacks for RestAPI#3958

Merged
bbirman merged 5 commits intoforcedotcom:devfrom
bbirman:rest-callbacks
Dec 17, 2025
Merged

Fix callbacks for RestAPI#3958
bbirman merged 5 commits intoforcedotcom:devfrom
bbirman:rest-callbacks

Conversation

@bbirman
Copy link
Member

@bbirman bbirman commented Dec 2, 2025

Manually tested for now, I still want to see if we can add a unit/integration test for it

id dataForDelegate = [strongSelf prepareDataForDelegate:data request:request response:response];
if (successBlock) {
successBlock(dataForDelegate, response);
if (request.successBlock) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main change, use the callback on the individual request, otherwise the callback on one of the requests is used for all the requests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it a recent regression? Or was it always like that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it came in with #3862 in 13.1. That thread had a similar issue with replaying a request which I thought was fixed but the sample app doesn't exercise having different callbacks for each request so I think we missed this part of it

@codecov
Copy link

codecov bot commented Dec 2, 2025

Codecov Report

❌ Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.37%. Comparing base (972e43e) to head (9f1264e).
⚠️ Report is 8 commits behind head on dev.

Files with missing lines Patch % Lines
...Core/SalesforceSDKCore/Classes/RestAPI/SFRestAPI.m 82.35% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #3958      +/-   ##
==========================================
- Coverage   63.85%   61.37%   -2.49%     
==========================================
  Files         252      255       +3     
  Lines       22111    22722     +611     
==========================================
- Hits        14120    13945     -175     
- Misses       7991     8777     +786     
Components Coverage Δ
Analytics 70.78% <ø> (ø)
Common 70.23% <ø> (+0.46%) ⬆️
Core 51.34% <82.35%> (-3.40%) ⬇️
SmartStore 73.99% <ø> (-0.60%) ⬇️
MobileSync 87.41% <ø> (ø)
Files with missing lines Coverage Δ
...Core/SalesforceSDKCore/Classes/RestAPI/SFRestAPI.m 86.63% <82.35%> (-0.97%) ⬇️

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link

github-actions bot commented Dec 2, 2025

1 Warning
⚠️ Static Analysis found an issue with one or more files you modified. Please fix the issue(s).

Clang Static Analysis Issues

File Type Category Description Line Col
SFRestAPI Nullability Memory error nil assigned to a pointer which is expected to have non-null value 98 5

Generated by 🚫 Danger

[SFSDKCoreLogger e:[strongSelf class] format:@"Failed to refresh expired session. Error: %@", refreshError];

// Call the failure block for the triggering request first
if (request.failureBlock) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For line 478, I saw an issue where the same request is being removed multiple times along with the callback being executed more than once (if the active queue is at least two requests), I'm thinking it's because flushPendingRequestQueue is synchronized to self but the removeObject call isn't.

The part I don't know the history on is why the failure block for first request was triggered separately, it looks like the same values are passed to the failure block whether it's called here or through flushPendingRequestQueue

@bbirman
Copy link
Member Author

bbirman commented Dec 17, 2025

  • Unit tests are passing locally (core, common, analytics, SmartStore & MobileSync)
  • Added a test for scenario where refresh is successful and queue is replayed to make sure success block is only called once per request, also manually tested in RestAPIExplorer
  • Manually tested scenario where refresh fails for issue where failure block was being called multiple times

@bbirman bbirman merged commit bc63575 into forcedotcom:dev Dec 17, 2025
17 of 35 checks passed
@bbirman bbirman deleted the rest-callbacks branch December 17, 2025 22:11
ihershkovitzSF pushed a commit to ihershkovitzSF/SalesforceMobileSDK-iOS that referenced this pull request Jan 5, 2026
ihershkovitzSF pushed a commit to ihershkovitzSF/SalesforceMobileSDK-iOS that referenced this pull request Jan 5, 2026
…-crash

@W-20494096 - Cherry pick of pull request forcedotcom#3958 from bbirman/rest-callbacks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants