On Wed, Apr 14, 2021 at 09:52:31PM +0200, Aleksandar Lazic wrote:
> > > + - string : This is the default search type and returns a String;
> > > + - boolean : If the JSON value is not a String or a Number
> > > + - number : When the JSON value is a Number then will the value be
> > > + converted to a String. If its known that the value is a
> > > + integer then add 'int' to the <output_type> which helps
> > > + haproxy to convert the value to a integer for further
> > > usage;
> >
> > I'd probably completely rephrase this as:
> >
> > The json_query converter supports the JSON types string, boolean and
> > number. Floating point numbers will be returned as a string. By specifying
> > the output_type 'int' the value will be converted to an Integer. If
> > conversion is not possible the json_query converter fails.
>
> Well I would like to here also some other opinions about the wording.
I, by far, prefer Tim's proposal here, as I do not even understand the
first one, sorry Aleks, please don't feel offended :-)
> > > + Example:
> > > + # get the value of the key 'iss' from a JWT Bearer token
> > > + http-request set-var(txn.token_payload)
> > > req.hdr(Authorization),word(2,.),ub64dec,json_query('$.iss')
> > > +
> > > + # get a integer value from the request body
> > > + # "{"integer":4}" => 5
> > > + http-request set-var(txn.pay_int)
> > > req.body,json_query('$.integer','int'),add(1)
> > > +
> > > + # get a key with '.' in the name
> > > + # {"my.key":"myvalue"} => myvalue
> > > + http-request set-var(txn.pay_mykey)
> > > req.body,json_query('$.my\\.key')
> > > +
> > > + # {"boolean-false":false} => 0
> > > + http-request set-var(txn.pay_boolean_false)
> > > req.body,json_query('$.boolean-false')
> >
> > These examples look good to me. I'd just move the JWT example to the
> > bottom, so that the simple examples come first.
>
> I prefer to keep it like this.
I would also have preferred to start with the simpler ones but that's
not important as long as there are both simple ones and advanced ones.
> > > + switch(tok) {
> > > + case MJSON_TOK_NUMBER:
> > > + if (args[1].type == ARGT_SINT) {
> > > + smp->data.u.sint = atoll(p);
> > > +
> > > + if (smp->data.u.sint < 0 && smp->data.u.sint <
> > > JSON_INT_MIN) {
> > > + /* JSON integer too big negativ value */
> >
> > This comment appears to be useless. It is implied by the 'if'. I also
> > believe that the 'sint < 0' check is not needed.
>
> Well I prefer to document in the comment what the if is doing.
OK but then please be careful about spelling, or it will force Ilya to
send yet another spell-checker patch.
> From my point of view is it necessary to check if the value is a negative
> value and only then should be checked if the max '-' range is reached.
But the first one is implied by the second. It looks like a logical
error when read like this, it makes one think the author had something
different in mind. It's like writing "if (a < 0 && a < -2)". It is
particularly confusing.
> Maybe there is a better solution, I'm open for suggestions.
>
> I can move the comment above the 'if'.
You have the choice as long as it's clear:
- above the if, you describe what you're testing and why
- inside the if, you describe the condition you've validated.
As it is now, it's best inside the if.
Thanks!
Willy
