Progress emitter throws error if jobId has comma in it

[abdatta]

I'm getting an error when I set a jobId that has a comma in it.

SyntaxError: Unexpected token h in JSON at position 2
    at JSON.parse (<anonymous>)
    at Redis.messageHandler (/usr/lib/node_modules/@abdatta/bulls-eye/node_modules/bull/lib/queue.js:404:50)
    at Redis.emit (events.js:315:20)
    at DataHandler.handleSubscriberReply (/usr/lib/node_modules/@abdatta/bulls-eye/node_modules/ioredis/built/DataHandler.js:80:32)
    at DataHandler.returnReply (/usr/lib/node_modules/@abdatta/bulls-eye/node_modules/ioredis/built/DataHandler.js:47:18)
    at JavascriptRedisParser.returnReply (/usr/lib/node_modules/@abdatta/bulls-eye/node_modules/ioredis/built/DataHandler.js:21:22)
    at JavascriptRedisParser.execute (/usr/lib/node_modules/@abdatta/bulls-eye/node_modules/redis-parser/lib/parser.js:544:14)
    at Socket.<anonymous> (/usr/lib/node_modules/@abdatta/bulls-eye/node_modules/ioredis/built/DataHandler.js:25:20)
    at Socket.emit (events.js:315:20)
    at addChunk (_stream_readable.js:295:12)

On following the error message, I end up on this line

bull/lib/queue.js

Line 404 in 4177e57

this.emit('global:progress', jobId, JSON.parse(progress));

I see the code tried to extract the jobId using the location of the first comma, but since the jobId itself has a comma in it, it fails the extract the full jobId, and the remaining text is not parseable by JSON.parse.

I need to fix this urgently since a code in production is breaking. I would be grateful if somebody can guide be on how to best fix this.

[manast]

I was not aware of this limitation myself, but after examining the code it seems like it is going to be impossible to fix other than renaming the ids so that they do not include commas...

[manast]

I will see if there is any possibility improving the lua code that actually pubs the progress event.

[abdatta]

@manast As a quickfix, can we replace indexOf with lastIndexOf at this line

bull/lib/queue.js

Line 401 in 4177e57

const commaPos = message.indexOf(',');

I found this line here,

bull/lib/scripts.js

Line 100 in 4177e57

.updateProgress(keys, [progressJson, job.id + ',' + progressJson])

and it seems we only pass job.id + ',' + progress, so since progress can never contain commas, I think it can be guaranteed that the last comma will always be the one separating the progress.

Can you pls confirm if my understanding is correct? I'll raise a PR for the same then.

[manast]

so since progress can never contain commas, I think it can be guaranteed that the last comma will always be the one separating the progress.

but progress can be a json object so it can indeed include commas...

[abdatta]

Which is the scenario when progress can be a json object? Can you please help me find it?

[manast]

you can send a complete object as progress

[abdatta]

Oh okay, I didn't know that.

Then can we escape commas by passing the entire thing as a stringified json?

bull/lib/scripts.js

Line 100 in 4177e57

.updateProgress(keys, [progressJson, job.id + ',' + progressJson])

For example,

.updateProgress(keys, [progressJson, JSON.stringify({jobId: job.id,  progress})]) 

[manast]

Let me take a deeper look at this later today and i will give you an answer

[abdatta]

Thank you for looking into this 😀

[manast]

Take a look at the PR, I wrote it so that we are still backwards compatible with the old way of sending the data: #2004