Re: jsonpath

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.

@@ -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.
(I also noticed some messages that are using single-quotes around
interpolated strings, which is not the project standard either.)

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.)

* 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 :-(


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.

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.

            regards, tom lane