jooby icon indicating copy to clipboard operation
jooby copied to clipboard

Empty object list in query param

Open csisy opened this issue 2 years ago • 2 comments

We have an object, for example:

class Foo {
  public Integer a;
  public Integer b;

  public Foo(Integer a, Integer b) {
    checkNotNull(a);
    checkNotNull(b);

    this.a = a;
    this.b = b;
  }
}

We need to send a (possibly empty) list of this object from the frontend in query parameter. Important to note that the constructor of Foo requires that both parameters are present and this is intended and required. So if a Foo is coming in a query parameter, it must be fully defined. The corresponding controller looks like this:

class FooController {
  public void something(@QueryParam List<Foo> foo) {
  }
}

According to the documentation the following syntax is supported: foo[0][a]=10&foo[0][b]=20. And it's indeed working as expected. However, if the query string does not contain any foo then the generated controller code tries to convert to a Foo from the query map directly.

  • foo[0][a]=10&foo[0][b]=20&foo[1][a]=30&foo[1][b]=40 -> [ Foo{ a=10; b=20; }, Foo{ a=30; b=40; } ]
  • <empty> -> exception
  • something=else -> exception

Proposal

A method in the generated controller class looks something like this:

private static List $lookupFoo(Context var0) {
  return var0.query("foo").isMissing() ? (List)var0.query().toList(Foo.class) : (List)var0.query("foo").toList(Foo.class);
}

According to the documentation, this check was necessary to support the Tabular data uses the bracket array notation. So if the full query string contains only Foo elements (?[0]a=10&[0]b=20) then it can parse it as a List<Foo>. However, this breaks if there are any other parameters in the query string (?token=blah&[0]a=10&[0]b=20).

In my opinion, the @QueryParam suggests that we'd like to read only a part of the query string with the specified name. And this is the behaviour for simpler types, like an integer or a list of integers. Example generated code snippets:

(Integer)var1x.query("id").to(Integer.class)
// or
List var10001 = (List)var1x.query("ids").toList(Integer.class);

Remove the ternary from the generated code when the @QueryParam is used. This does not break the code present in the documentation because the ctx.query() is used directly by the user code and not by a generated code.

Workaround

In the meantime, this behaviour can be forced by using the Context directly instead of the @QueryParam. So the controller method sketched above would be transformed into this one:

class FooController {
  public void something(Context context) {
    final var fooList = context.query("foo").toList(Foo.class);
  }
}

Edit: Fixed ContextParam typo

csisy avatar Feb 09 '22 11:02 csisy

Yes sadly I don't think this can be fixed to till 3.0

See #2325

And you can see the fix here where basically it was decided to do a precedence based on whether or not the parameter is there.

https://github.com/jooby-project/jooby/commit/67ff12c24b38a14ce7f2580bdc8f7937dbe85b7e

Basically it is trying to take the whole query query() for bean mapping if the parameter is missing.

I proposed some extensions to the annotations to be explicit about whether it is a bean or value.

I believe @jknack does not want to change the behavior now as it would break peoples code (see his comment in #2325 on putting it in 3.0).

As a side note the Value Converter API probably needs some rework as well as I think there are many scenarios where it is desirable to have access to the whole request instead of just a hash or single value.

The other issue is coercing empty parameters and null parameters. This is relevant to your bug.

For example given the URI query of:

(I'm reusing your Foo class which notable has Integer which can be null)

?foo[0][a]=&foo[0][b]

What do you think should happen in the above?

Notice the last parameter has no = after it.

Should foo.a be set to null? Should foo.a fail with a MismatchTypeException (I believe that is what it will do) or should it fail with a MissingValueException?

How about foo.b. Is it a MissingValueException or is the value null? I can't recall what jooby does in this case but it probably should set foo.b to null.

I'm not sure if there is even a 80-20 solution to the above. That is why I think 3.0 needs more options for explicit mapping.

agentgt avatar May 13 '22 14:05 agentgt

Also to add to your work arounds and I believe this is what we do in some places:

record FooList(List<Foo> foo) {}
class FooController {
  public void something(@QueryParam FooList ignore_foo) {
  }
}

The bean converter will always get used because we don't pass the parameter ignore_foo.

However we have a custom BeanConverter so I'm not sure if the above works with the default ReflectiveBeanConverter.

agentgt avatar May 13 '22 14:05 agentgt