Re: More new SQL/JSON item methods

Thank you for the fix!

At Tue, 30 Jan 2024 13:46:17 +0530, Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote in
> On Mon, Jan 29, 2024 at 11:03 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> > Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > > Having said that, I'm a bit concerned about the case where an overly
> > > long string is given there. However, considering that we already have
> > > "invalid input syntax for type xxx: %x" messages (including for the
> > > numeric), this concern might be unnecessary.
> >
> > Yeah, we have not worried about that in the past.
> >
> > > Another concern is that the input value is already a numeric, not a
> > > string. This situation occurs when the input is NaN of +-Inf. Although
> > > numeric_out could be used, it might cause another error. Therefore,
> > > simply writing out as "argument NaN and Infinity.." would be better.
> >
> > Oh!  I'd assumed that we were discussing a string that we'd failed to
> > convert to numeric.  If the input is already numeric, then either
> > the error is unreachable or what we're really doing is rejecting
> > special values such as NaN on policy grounds.  I would ask first
> > if that policy is sane at all.  (I'd lean to "not" --- if we allow
> > it in the input JSON, why not in the output?)  If it is sane, the
> > error message needs to be far more specific.
> >
> >                         regards, tom lane
> >
>
> *Consistent error message related to type:*
...
> What if we use phrases like "for type double precision" at both places,
> like:
> numeric argument of jsonpath item method .%s() is out of range for type
> double precision
> string argument of jsonpath item method .%s() is not a valid representation
> for type double precision
>
> With this, the rest will be like:
> for type numeric
> for type bigint
> for type integer
>
> Suggestions?

FWIW, I prefer consistently using "for type hoge".

> ---
>
> *Showing input string in the error message:*
>
> argument "...input string here..." of jsonpath item method .%s() is out of
> range for type numeric
>
> If we add the input string in the error, then for some errors, it will be
> too big, for example:
>
> -ERROR:  numeric argument of jsonpath item method .double() is out of range
> for type double precision
> +ERROR:  argument
> "100000<many zeros follow>"
> of jsonpath item method .double() is out of range for type double precision

As Tom suggested, given that similar situations have already been
disregarded elsewhere, worrying about excessively long input strings
in this specific instance won't notably improve safety in total.

> Also, for non-string input, we need to convert numeric to string just for
> the error message, which seems overkill.

As I suggested and you seem to agree, using literally "Nan or
Infinity" would be sufficient.

I am more concerned about .bigint() and .integer(). We can have errors when the numeric input is out of range, but not NaN or Infinity. At those places, we need to convert numeric to string to put that value into the error.

Do you mean we should still put "Nan or Infinity" there?

This is the case:
 select jsonb_path_query('12345678901', '$.integer()');

> On another note, irrespective of these changes, is it good to show the
> given input in the error messages? Error messages are logged and may leak
> some details.
>
> I think the existing way seems ok.

In my opinion, it is quite common to include the error-causing value
in error messages. Also, we have already many functions that impliy
the possibility for revealing input values when converting text
representation into internal format, such as with int4in. However, I
don't stick to that way.

> ---
>
> *NaN and Infinity restrictions:*
>
> I am not sure why NaN and Infinity are not allowed in conversion to double
> precision (.double() method). I have used the same restriction for
> .decimal() and .number(). However, as you said, we should have error
> messages more specific. I tried that in the attached patch; please have
> your views. I have the following wordings for that error message:
> "NaN or Infinity is not allowed for jsonpath item method .%s()"
>
> Suggestions...

They seem good to *me*.

By the way, while playing with this feature, I noticed the following
error message:

> select jsonb_path_query('1.1' , '$.boolean()');
> ERROR:  numeric argument of jsonpath item method .boolean() is out of range for type boolean

The error message seems a bit off to me. For example, "argument '1.1'
is invalid for type [bB]oolean" seems more appropriate for this
specific issue. (I'm not ceratin about our policy on the spelling of
Boolean..)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center