Re: jsonpath

On Mon, Apr 22, 2019 at 1:39 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> >> On Wed, Apr 17, 2019 at 8:43 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>> Yeah, I'd noticed that one too :-(. I think the whole jsonpath patch
> >>> needs a sweep to bring its error messages into line with our style
> >>> guidelines, but no harm in starting with the obvious bugs.
>
> > I went trough the jsonpath errors and made some corrections. See the
> > attached patch.
>
> Please don't do this sort of change:
>
> - elog(ERROR, "unrecognized jsonpath item type: %d", item->type);
> + ereport(ERROR,
> + (errcode(ERRCODE_INTERNAL_ERROR),
> + errmsg("unrecognized jsonpath item type: %d", item->type)));
>
> elog() is the appropriate thing for shouldn't-happen internal errors like
> these. The only thing you've changed here, aside from making the source
> code longer, is to expose the error message for translation ... which is
> really just wasting translators' time. Only messages we actually think
> users might need to deal with should be exposed for translation.

Makes sense. Removed from the patch.

> @@ -623,7 +624,7 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
> ereport(ERROR,
> (errcode(ERRCODE_JSON_MEMBER_NOT_FOUND), \
> errmsg(ERRMSG_JSON_MEMBER_NOT_FOUND),
> - errdetail("JSON object does not contain key %s",
> + errdetail("JSON object does not contain key %s.",
> keybuf.data)));
> }
> }
>
> OK as far as it went, but you should also put double quotes around the %s.

This code actually passed key trough escape_json(), which adds double
quotes itself. However, we don't do such transformation in other
places. So, patch removes call of ecsape_json() while putting double
quotes to the error message.

> (I also noticed some messages that are using single-quotes around
> interpolated strings, which is not the project standard either.)

Single-quotes are replaced with double-quotes.

> Other specific things I wanted to see fixed:
>
> * jsonpath_scan.l has some messages like "bad ..." which is not project
> style; use "invalid" or "unrecognized". (There's probably no good
> reason not to use the same string "invalid input syntax for type jsonpath"
> that is used elsewhere.)

Fixed.

> * This in jsonpath_gram.y is quite unhelpful:
>
> yyerror(NULL, "unrecognized flag of LIKE_REGEX predicate");
>
> since it doesn't tell you what flag character it doesn't like
> (and the error positioning info isn't accurate enough to let the
> user figure that out). It really needs to be something more like
> "unrecognized flag character \"%c\" in LIKE_REGEX predicate".
> That probably means you can't use yyerror for this, but I don't
> think yyerror was providing any useful functionality anyway :-(

Fixed.

> More generally, I'm not very much on board with this coding technique:
>
> /* Standard error message for SQL/JSON errors */
> #define ERRMSG_JSON_ARRAY_NOT_FOUND "SQL/JSON array not found"
>
> ...
>
> RETURN_ERROR(ereport(ERROR,
> (errcode(ERRCODE_JSON_ARRAY_NOT_FOUND),
> errmsg(ERRMSG_JSON_ARRAY_NOT_FOUND),
> errdetail("Jsonpath wildcard array accessor "
>
> In the first place, I'm not certain that this will result in the error
> message being translatable --- do the gettext tools know how to expand
> macros?
>
> In the second place, the actual strings are just restatements of their
> ERRMSG macro names, which IMO is not conformant to our message style,
> but it's too hard to see that from source code like this. Also this
> style is pretty unworkable/unfriendly if the message needs to contain
> any %-markers, so I suspect that having a coding style like this may be
> discouraging you from providing values in places where it'd be helpful to
> do so. What I actually see happening as a consequence of this approach is
> that you're pushing the useful information off to an errdetail, which is
> not really helpful and it's not per project style either. The idea is to
> make the primary message as helpful as possible without being long, not
> to make it a simple restatement of the SQLSTATE that nobody can understand
> without also looking at the errdetail.
>
> In the third place, this makes it hard for people to grep for occurrences
> of an error string in our source code.
>
> And in the fourth place, we don't do this elsewhere; it does not help
> anybody for jsonpath to invent its own coding conventions that are unlike
> the rest of Postgres.
>
> So I think you should drop the ERRMSG_xxx macros, write out these error
> messages where they are used, and rethink your use of errmsg vs. errdetail.

OK, ERRMSG_* macros are removed.

> Along the same line of not making it unnecessarily hard for people to grep
> for error texts, it's best not to split texts across lines like this:
>
> RETURN_ERROR(ereport(ERROR,
> (errcode(ERRCODE_INVALID_JSON_SUBSCRIPT),
> errmsg(ERRMSG_INVALID_JSON_SUBSCRIPT),
> errdetail("Jsonpath array subscript is not a "
> "singleton numeric value."))));
>
> Somebody grepping for "not a singleton" would not get a hit on that, which
> could be quite misleading if they do get hits elsewhere. I think for the
> most part people have decided that it's better to have overly long source
> lines than to break up error message literals. It's especially pointless
> to break up source lines when the result still doesn't fit in 80 columns.

OK, now no line breaks in error messages.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company