Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nullify batchRequestId immediatelly #92

Merged
merged 2 commits into from
Apr 11, 2021

Conversation

Kukunin
Copy link
Contributor

@Kukunin Kukunin commented Apr 6, 2021

Sometimes the library threw unhandledException
that parseParams is undefined in parseResponse.js:312

It happens because a single requestId is assigned to two different
requests, and after the first one is done, the info for the second request
is deleted.

A single requestId is assigned because of the batch implementation.
After executeBatch function, the global _batchRequestId is not nulled,
so any further request will get the same requestId, until executeBatch
promise is not finished.

I didn't find to write a test for this in a meaningful manner of time. Nor I found an existing test that is sensitive to this moment.

Sometimes the library threw unhandledException
that parseParams is undefined in parseResponse.js:312

It happens because a single requestId is assigned to two different
requests, and after the first one is done, the info for second request
is deleted.

A single requestId is assigned because of the batch implementation.
After executeBatch function, the global _batchRequestId is not nulled,
so any further request will get the same requestId, until executeBatch
promise is not finished.
@AleksandrRogov
Copy link
Owner

@Kukunin thank you for the PR, I will need to run some tests and/or write new ones before I can publish this. But I can see that the issue you are describing can happen. The only thing I want to check if this fix does not break anything else.

Also the same fix needs to be done for the callbacks version.

@Kukunin
Copy link
Contributor Author

Kukunin commented Apr 7, 2021

Sounds great.

I'm not sure if it's related, but after the deployment of the fixed version, another problem started occurring:

TypeError: Cannot read property 'length' of undefined

at _convertToBatch (/usr/src/app/node_modules/dynamics-web-api/lib/requests/sendRequest.js:117:40)
    at sendRequest (/usr/src/app/node_modules/dynamics-web-api/lib/requests/sendRequest.js:240:21)
    at /usr/src/app/node_modules/dynamics-web-api/lib/requests/sendRequest.js:429:4
    at _checkCollectionName (/usr/src/app/node_modules/dynamics-web-api/lib/requests/sendRequest.js:394:3)
    at Object.makeRequest (/usr/src/app/node_modules/dynamics-web-api/lib/requests/sendRequest.js:424:3)
    at /usr/src/app/node_modules/dynamics-web-api/lib/dynamics-web-api.js:184:12
    at new Promise (<anonymous>)
    at _makeRequest (/usr/src/app/node_modules/dynamics-web-api/lib/dynamics-web-api.js:183:10)
    at DynamicsWebApi.executeBatch (/usr/src/app/node_modules/dynamics-web-api/lib/dynamics-web-api.js:1478:10)
    at ...app_code

for some reason, requestCollection in for (var i = 0; i < requestCollection.length; i++) { line is undefined.

Before I dive deep into the bug, just wanted to share this with you. maybe you have any clue

Thank you

@Kukunin
Copy link
Contributor Author

Kukunin commented Apr 7, 2021

it seems like this exception happens if executeBatch is called right after startBatch, so there are no batch operations

UPD: I can confirm, it happens on

   const api = new DynamicsWebApi(dynamicsOpts)
   api.startBatch()
   api.executeBatch()

I've fixed it in my app's code to not start batch, but I believe it'd be good if a library can handle this case too

at _convertToBatch dynamics-web-api/lib/requests/sendRequest.js:117:40

The problem happens because we nullify _batchRequestId too early,
before the batch request is made. With _batchRequestId it generates a
new requestId, other than all operations were saved to, thus it crashes
@Kukunin
Copy link
Contributor Author

Kukunin commented Apr 7, 2021

@AleksandrRogov it turned out, that with the first fix all batch requests were broken. I described the reason in the second commit. But the previous problem remain the same - it still crashes on empty batches

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 95.597% when pulling f832891 on Kukunin:fix-crash into 8e94116 on AleksandrRogov:master.

@AleksandrRogov AleksandrRogov merged commit 7583f8b into AleksandrRogov:master Apr 11, 2021
@AleksandrRogov
Copy link
Owner

Thank you @Kukunin for looking further into the issue.

I am not sure what should I do with the 2nd issue, because I am not completely sure why would anyone send an empty batch request. I would still return an error but a more meaningful one, saying that the batch is empty. Developers need to check their request collections or any arrays that they use to build a batch first, before starting executing a batch operation.

Let me know if you have another case that will show that the error should not be thrown and that I just need to add a code that would simply skip the operation.

AleksandrRogov added a commit that referenced this pull request Apr 11, 2021
… before batch is completed; added a meaningful error message when the batch payload is empty #92
@Kukunin
Copy link
Contributor Author

Kukunin commented Apr 12, 2021

Thank you for packaging and merging my PR, appreciate it. The solution for empty batches makes total sense for me.
I'm going to switch back to the upstream version, will report if a problem occurred

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.

3 participants