Re: jsonpath - Mailing list pgsql-hackers

On Wed, Apr 24, 2019 at 9:03 PM Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Mon, Apr 22, 2019 at 1:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> > >> On Wed, Apr 17, 2019 at 8:43 PM Tom Lane <tgl@sss.pgh.pa.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.

I'm going to commit these adjustments if no objections.

My question regarding jsonpath_yyerror() vs. bison errors is still
relevant.  Should we bother making bison-based errdetail() a complete
sentence starting from uppercase character?  If not, should we make
other yyerror() calls look the same?  Or should we rather move bison
error from errdetail() to errmsg()?

I would probably leave bison-based messages "as is", but make messages
in other invocations of yyerror() starting from lowercase characters
for the uniformity.  But I'd like to hear other opinions.

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