search

michaelwittig / node-q

Q interfacing with Node.js
MIT License
52 stars 14 forks source link

Replace null char with empty string in symbols #44

Closed wwarby closed 5 years ago

wwarby commented 5 years ago

A very simple fix for issue 43 in which strings containing the null character \u0000 can cause a corrupt message to be sent to the q process. This fix silently removes the null character from symbols, which I have confirmed with a consultant from Kx is actually an illegal character in a symbol.

The Java implementation of the serialiser apparently truncates at the first occurence of the null character which in my opinion is less desirable, since the expected result in most real world scenarios is that the string continues to look the same as it did before, hence replace.

This is my first ever pull request so not sure if I've done it correctly, but by all means change as you see fit or reject if you don't think it's the responsibility of this library to guard against this scenario (or you think the change should be applied at the c.js level).

michaelwittig commented 5 years ago

fixes #43

wwarby commented 5 years ago

Ah my bad I ran "npm run test" and it didn't seem to show any problems but it also didn't show the output of the tests so clearly I'm not doing that quite right. How do you run the unit tests locally? I'll fix the problem anyway now that I know what it is, but I'd like to be able to run the tests to prove it works.

michaelwittig commented 5 years ago

yes, npm test should run the tests locally (npm run test as well). If the commands exists with 0 the tests are fine (echo $?)

wwarby commented 5 years ago

Great, thanks - it's resulting in 0 now, so hopefully this build will pass

michaelwittig commented 5 years ago

released as node-q@2.4.3

wwarby commented 5 years ago

Thanks so much for doing this so quickly Michael! I'll integrate the new version immediately into my codebase.