Re: [COMMITTERS] pgsql: Mark JSON error detail messages for translation. - Mailing list pgsql-hackers

On Tue, Jun 12, 2012 at 2:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <rhaas@postgresql.org> writes:
>> Mark JSON error detail messages for translation.
>> Per gripe from Tom Lane.
>
> OK, that was one generically bad thing about json.c's error messages.
> The other thing that was bothering me was the style of the errdetail
> strings, eg
>
>                ereport(ERROR,
>                        (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
>                         errmsg("invalid input syntax for type json"),
>                         errdetail("line %d: Invalid escape \"\\%s\".",
>                                   lex->line_number, extract_mb_char(s))));
>
> I'm not thrilled with the "line %d:" prefixes.  That seems to me to
> violate the letter and spirit of the guideline about making errdetail
> messages be complete sentences.  Furthermore, the line number is not
> the most important part of the detail message, if indeed it has any
> usefulness at all which is debatable.  (It's certainly not going to
> be tremendously useful when the error report doesn't show any of the
> specific input string being complained of.)

I am amenable to rephrasing the errdetail to put the line number
elsewhere in the string, but I am strongly opposed to getting rid of
it.  JSON blobs can easily run to dozens or hundreds of lines, and you
will want to know the line number.  Knowing the contents of the line
will in many cases be LESS useful, because the line might contain,
say, a single closing bracket.  That's not gonna help much if it
happens many times in the input value, which is not unlikely.

> Also, a minority of the errors report the whole input string:
>
>        ereport(ERROR,
>                (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
>                 errmsg("invalid input syntax for type json: \"%s\"",
>                        lex->input),
>                 errdetail("The input string ended unexpectedly.")));
>
> And then there are some that provide the whole input string AND a line
> number, just in case you thought there was any intentional design
> decision there.

I did try.

> A minimum change IMO would be to recast the detail messages as complete
> sentences, say "The escape sequence \"\\%s\" in line %d is invalid."
> But I'd like a better-thought-out solution to identifying the location
> of the error.
>
> I notice that there's an unfinished attempt to maintain a line_start
> pointer; if that were carried through, we could imagine printing the
> current line up to the point of an error, which might provide a
> reasonable balance between verbosity and insufficient context.
> As an example, instead of
>
> regression=# select '{"x":1,
> z  "y":"txt1"}'::json;
> ERROR:  invalid input syntax for type json
> LINE 1: select '{"x":1,
>               ^
> DETAIL:  line 2: Token "z" is invalid.
>
> maybe we could print
>
> regression=# select '{"x":1,
> z  "y":"txt1"}'::json;
> ERROR:  invalid input syntax for type json
> LINE 1: select '{"x":1,
>               ^
> DETAIL:  Token "z" is invalid in line 2: "z".
>
> or perhaps better let it run to the end of the line:
>
> regression=# select '{"x":1,
> z  "y":"txt1"}'::json;
> ERROR:  invalid input syntax for type json
> LINE 1: select '{"x":1,
>               ^
> DETAIL:  Token "z" is invalid in line 2: "z  "y":"txt1"}".
>
> Thoughts?

I'm not sure I find that an improvement, but I'm open to what other
people think.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company