Re: jsonpath
От | Alexander Korotkov |
---|---|
Тема | Re: jsonpath |
Дата | |
Msg-id | CAPpHfduxZqqnA1CU232+tbB6OrCdM8eHJUTGQO6G0BZ7tnXxxQ@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: jsonpath (Tom Lane <tgl@sss.pgh.pa.us>) |
Ответы |
Re: jsonpath
(Alexander Korotkov <a.korotkov@postgrespro.ru>)
|
Список | pgsql-hackers |
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. ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Peter GeogheganДата:
Сообщение: Re: Thoughts on nbtree with logical/varwidth table identifiers, v12on-disk representation