Fix for binary data representation when sending data through HTTP & MQTT #736
Conversation
|
Gentle Reminder! |
lib/bindings/HTTPBinding.js
Outdated
| @@ -127,7 +127,9 @@ function parseData(req, res, next) { | |||
| next(error); | |||
| } else { | |||
| req.jsonPayload = data; | |||
|
|
|||
| if (req.body !== undefined || req.body === Object || Buffer.isBuffer(req.body)) { | |||
There was a problem hiding this comment.
Could you elaborate why is required this condition? (specially why using OR operator)
There was a problem hiding this comment.
Hi @mapedraza , we require the condition because test case was failing in commands-Polling-test.js for undefind data.
There was a problem hiding this comment.
Tests are failling even after restoring this line. Maybe you should restore this line too:
iotagent-json/lib/bindings/HTTPBinding.js
Line 293 in bf45b6b
Additionaly, could you explain why is failing and why is required? I do not understand why the 3 conditions are nested with OR operator.
Thanks
There was a problem hiding this comment.
Hi @mapedraza, I have run the test files on my local system and all the test case were running properly and passing. Here you can find in the screenshot.
Please guide me how can I fix it here?
There was a problem hiding this comment.
Additionaly, could you explain why is failing and why is required? I do not understand why the 3 conditions are nested with OR operator.
I have resolved it by latest changes , as per the requirements
Co-authored-by: mapedraza <40356341+mapedraza@users.noreply.github.com>
|
Tests are failing. Maybe related with changes in bf45b6b? |
Hi @fgalan, @mapedraza, I have run the test files on my local system and all the test case were running properly and passing. Here you can find in the screenshot. Please guide me how can I fix it here. |
Results in GitHub are now the same as yours. There is some test related with MQTT that sometimes "glitches" (due to some unknown timing issue). We have re-run in GitHub and now it is ok. |
|
Hi @fgalan , please let me know if any other change is required or PR can be merged. |
Thanks for your contribution. We would provide a review with feedback and comments as soon as possible. |
|
@AlvaroVega It seems good for me. It does not break the tests. Can you have a quick look? |
|
Before merging this PR, a twin PR should be created under https://github.com/telefonicaid/iotagent-ul/ fixing the same issue |
Co-authored-by: Fermín Galán Márquez <fgalan@users.noreply.github.com>
Hi, same issue telefonicaid/iotagent-ul#601 was created and PR telefonicaid/iotagent-ul#603 was raised on iotagent-ul. Do I need to raise PR on iotagent-ul? Please confirm. |
@Keshav-NEC, you are right, no need to rise a new PR it for IoTA-UL (as you noted it was not necessary in previous PRs). |
|
Before merging this PR to master, we would like to do some tests in advance, so this is going to be merged into a prelanding branch before merging to master. @Keshav-NEC Thanks for your contribution. |
mapedraza
merged commit a79ee65
into
telefonicaid:prelanding/fix-binary-data
|
Continuation PR #760 |
Fix for issue #690