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

json_ParseString from Webhelpers(4.1.3) adds "/r" to "/n" terminated values #350

Open
joekane101 opened this issue Jun 19, 2018 · 7 comments

Comments

@joekane101
Copy link

I have an excel client using vba-web which interacts with a server, passing data back and forth in json. The excel client passes up certain values in json which contain newlines (/n, chr(10), LF), which come from excel multivalue(multiline) cells. The server stores them ok, and passes them back later when requested.

However, when decoding the received json using the webhelpers module from VBA-WEB (and I realize this is also applicable to VBA-JSON), a "/r" is appended to all "/n" due to the case statement decoding. This breaks things since the client (and excel ) will misintepret the result, because the /n is used in excel for multi-value cells.

The offending line in json_ParseString is here:
Case "n" json_BufferAppend json_buffer, vbCrLf, json_BufferPosition, json_BufferLength json_Index = json_Index + 1

Here the \n case is translated to vbCrLf, instead of just vbLF.

This is different than the /r case, which only translates to vbCr
Case "r" json_BufferAppend json_buffer, vbCr, json_BufferPosition, json_BufferLength json_Index = json_Index + 1

For a simple patch workaround, I changed the case n to:
Case "n" json_BufferAppend json_buffer, vbLf, json_BufferPosition, json_BufferLength json_Index = json_Index + 1

I realize that line terminator issues between windows/linux can be complex, this change still seems correct. If there were actually a /r/n sequence in the decode, the case statement would find each and individually replace them. But I suppose there are lazy instances where this is not noticed/critical, or perhaps an expected assumption at the windows termination side.

@zgrose
Copy link

zgrose commented Jun 19, 2018

Was my suggestion in #270 as well.

@joekane101
Copy link
Author

zgrose,

Wow, sorry. No idea how I missed your previous issue. I'm a bit confused by the progression then, since the other proposed change didn't make it.

I'm not sure I understand why the more complex lookahead code is required (to check next character) in the accepted solution, rather than just let the second character get decoded discretely as either case n or case r.

@Sophist-UK
Copy link
Contributor

See #272 for a potential fix. Try applying this manually and see if it works.

@joekane101
Copy link
Author

joekane101 commented Jul 18, 2018

I am questioning the need for the more complicated look ahead statements in that fix, as opposed to a single line change which fixes it.
`
Case "n"

(delete)   json_BufferAppend json_buffer, vbCrLf, json_BufferPosition, json_BufferLength
(add)       json_BufferAppend json_buffer, vbLf, json_BufferPosition, json_BufferLength
             json_Index = json_Index + 1

`

This addresses only one character at a time. If there is a following /r, it will be handled in the next character pass through the existing case "r" statement.

@joekane101
Copy link
Author

pseudo patch didn't quite translate in the code brackets.

@Sophist-UK
Copy link
Contributor

No idea - #272 was a long time ago.

@timhall
Copy link
Member

timhall commented Jul 18, 2018

Sorry, never got around to merging either of the patches, I'll take another look shortly and see if we can't get this sorted

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

No branches or pull requests

4 participants