Re: Bug in error reporting for multi-line JSON

On Tue, Jan 26, 2021 at 2:07 PM Hamid Akhtar <hamid(dot)akhtar(at)gmail(dot)com> wrote:

>
>
> On Mon, Jan 25, 2021 at 6:08 PM Simon Riggs <simon(dot)riggs(at)enterprisedb(dot)com>
> wrote:
>
>> On Thu, Jan 21, 2021 at 6:08 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> >
>> > Simon Riggs <simon(dot)riggs(at)enterprisedb(dot)com> writes:
>> > > JSON parsing reports the line number and relevant context info
>> > > incorrectly when the JSON contains newlines. Current code mostly just
>> > > says "LINE 1" and is misleading for error correction. There were no
>> > > tests for this previously.
>> >
>> > Couple thoughts:
>> >
>> > * I think you are wrong to have removed the line number bump that
>> > happened when report_json_context advances context_start over a
>> > newline. The case is likely harder to get to now, but it can still
>> > happen can't it? If it can't, we should remove that whole stanza.
>>
>> OK, I'm playing around with this to see what is needed.
>>
>> > * I'd suggest naming the new JsonLexContext field "pos_last_newline";
>> > "linefeed" is not usually the word we use for this concept. (Although
>> > actually, it might work better if you make that point to the char
>> > *after* the newline, in which case "last_linestart" might be the
>> > right name.)
>>
>> Yes, OK
>>
>> > * I'm not enthused about back-patching. This behavior seems like an
>> > improvement, but that doesn't mean people will appreciate changing it
>> > in stable branches.
>>
>> OK, as you wish.
>>
>> Thanks for the review, will post again soon with an updated patch.
>>
>
> I agree with Tom's feedback.. Whether you change pos_last_linefeed to
> point to the character after the linefeed or not, we can still simplify the
> for loop within the "report_json_context" function to:
>
> =================
> context_start = lex->input + lex->pos_last_linefeed;
> context_start += (*context_start == '\n'); /* Let's move beyond the
> linefeed */
> context_end = lex->token_terminator;
> line_start = context_start;
> while (context_end - context_start >= 50 && context_start < context_end)
> {
> /* Advance to next multibyte character */
> if (IS_HIGHBIT_SET(*context_start))
> context_start += pg_mblen(context_start);
> else
> context_start++;
> }
> =================
>
> IMHO, this should work as pos_last_linefeed points to the position of the
> last linefeed before the error occurred, hence we can safely skip it and
> move the start_context forward.
>
>
This thread has been inactive for more than a month now.

So, I have reworked Simon's patch and incorporated Tom's feedback. The
changes include:
- Changing the variable name from "pos_last_linefeed" to "last_linestart"
as it now points to the character after the newline character,
- The "for" loop in report_json_context function has been significantly
simplified and uses a while loop.

The attached patch is created against the current master branch.

>
>>
>> --
>> Simon Riggs http://www.EnterpriseDB.com/
>>
>>
>>
>
> --
> Highgo Software (Canada/China/Pakistan)
> URL : www.highgo.ca
> ADDR: 10318 WHALLEY BLVD, Surrey, BC
> CELL：+923335449950 EMAIL: mailto:hamid(dot)akhtar(at)highgo(dot)ca
> SKYPE: engineeredvirus
>