quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.44k stars 2.58k forks source link

Nested form elements mapping #34800

Open kucharzyk opened 1 year ago

kucharzyk commented 1 year ago

Description

There is one missing feature in resteasy reactive - mapping nested form elements.

Suppose I have multipart form with the following fields:

user.name
user.roles[0].name
user.roles[0].enabled
user.roles[1].name
user.roles[1].enabled
user.address.city
user.address.street

I would like to map them automaticaly to one POJO

In resteasy classic something similar can be achieved using @Form annotation. https://docs.jboss.org/resteasy/docs/6.2.4.Final/userguide/html/ch12.html

In Spring such elements can be handled automatically even without annotating my POJO class.

But in resteasy reactive there is no easy way of handling such cases

Implementation ideas

It would be great if it could be handled automatically for all fields of class annotated as @BeanParam.

kucharzyk commented 1 year ago

cc: @FroMage

kucharzyk commented 1 year ago

Implementation idea:

@Target({ ElementType.PARAMETER, ElementType.METHOD, ElementType.FIELD })
@Retention(RetentionPolicy.RUNTIME)
@Documented
public @interface NestedFormParam {

}

@Provider
@Consumes(MediaType.MULTIPART_FORM_DATA)
public class NestedFormParamReader implements MessageBodyReader<Object> {

    @Override
    public boolean isReadable(Class<?> type, Type genericType, Annotation[] annotations, MediaType mediaType) {
        return Arrays.stream(annotations).anyMatch(NestedFormParamReader::isNestedFormParam);
    }

    private static boolean isNestedFormParam(Annotation it) {
        return it.annotationType().isAssignableFrom(NestedFormParam.class);
    }

    @Override
    public Object readFrom(Class<Object> type,
                           Type genericType,
                           Annotation[] annotations,
                           MediaType mediaType,
                           MultivaluedMap<String, String> httpHeaders,
                           InputStream entityStream) throws IOException, WebApplicationException {

        try {
            return type.newInstance(); // map values from form to this object
        } catch (InstantiationException | IllegalAccessException e) {
            throw new RuntimeException(e);
        }
    }
}
quarkus-bot[bot] commented 1 year ago

/cc @FroMage (resteasy-reactive), @Sgitario (resteasy-reactive), @geoand (resteasy-reactive), @stuartwdouglas (resteasy-reactive)

FroMage commented 1 year ago

Oh, you're thinking of a MessageBodyWriter, but that would mean the entire input is consumed by this. It's a bit like JSON or XML then, a new serialisation format. I don't think this is related to multipart at all, even a regular form could work, provided there's no binary field.

I wonder if there are JS clients that generate forms like this, for input. Otherwise, I'm not sure I see the point, especially when it comes to variable-sized collections. Static HTML can't handle that. What does your input side look like?

kucharzyk commented 1 year ago

My input side is just a regular HTML form with lots of fields. I am using htmx and forms are natural way to send data to backend. It could be regular form also - not necessary multipart.

If you need to edit complex objects you need to handle it in a more sophisticated way than using @BeanParam. In such scenario it really make sense to map all fields to one object instead of handling nested collections manually.

MessageBodyWriter was just a an idea how to implement it - probably there is a better way

kucharzyk commented 1 year ago

@FroMage What’s the best way of implementing it? You said that MessageBodyReader would consume entire input data. What could try instead?

kucharzyk commented 1 year ago

@geoand @FroMage What do you think about adding optional prefix to @BeanParam and allowing nesting them? This solution could be the easiest to implement.

kucharzyk commented 1 year ago

@gastaldi?

gastaldi commented 1 year ago

Why do you need it to be a Multipart request? In your example using a application/x-www-form-urlencoded should work with a @BeanParam, no?

kucharzyk commented 1 year ago

@gastaldi It doesn't need to be multipart request. It could be application/x-www-form-urlencoded

The case here is I would like to support nested properties.

For example I would like to add n invoices in my form and then submit form with following values

invoices[0].name=Item A
invoices[0].number=A/123
invoices[0].total=1000
invoices[1].name=Item B
invoices[1].number=B/123
invoices[1].total=2000
...

Currently the only way to handle such form is manual mapping from values map. I would like to handle mapping of such form to bean automatically.

FroMage commented 11 months ago

So, you generate forms with these names manually?

Like, <input name="invoices[0].name"/>?

Because, HTML supports sending multiple form elements of the same name, so we support deserialising invoices.name=one&invoices.name=two into a @FormParam("invoices.name") List<String> but that's not what you're after.

FroMage commented 11 months ago

We could add a @FormPrefix annotation to prefix any @FormParam from the annotated parameter container, but that wouldn't solve the case of List and Map which are very different.

In terms of implementation, I am not sure if it would be preferrable to adapt the current parameter container (aka @BeanParam) support or declare that this is not really a form encoding but its own serialisation format requiring a special MessageBodyReader, and only support @FormParam fields in the representation. On the other hand, pretty sure that the minute a File is required somewhere, we'll need multipart and suddenly everything will become super complex unless we adapt the current form support :(

I do wonder how commonly this format is used. I've never seen it myself.

FroMage commented 11 months ago

Perhaps @geoand or @Sgitario have opinions?

kucharzyk commented 11 months ago

I was trying to find a good example where this notation is used but it’s really hard. I was working with such forms about 10 years ago when jsp pages were still trendy.

In jsp (or plain html) we could use path property with dot notation like this:

 <form:form method="post" action="save.html" modelAttribute="contactForm">
    <table>
    <tr>
        <th>No.</th>
        <th>Name</th>
        <th>Lastname</th>
        <th>Email</th>
        <th>Phone</th>
    </tr>
    <c:forEach items="${contactForm.contacts}" var="contact" varStatus="status">
        <tr>
            <td align="center">${status.count}</td>
            <td><input name="contacts[${status.index}].firstname" value="${contact.firstname}"/></td>
            <td><input name="contacts[${status.index}].lastname" value="${contact.lastname}"/></td>
            <td><input name="contacts[${status.index}].email" value="${contact.email}"/></td>
            <td><input name="contacts[${status.index}].phone" value="${contact.phone}"/></td>
        </tr>
    </c:forEach>
</table>    
<br/>
<input type="submit" value="Save" />

</form:form>

https://www.viralpatel.net/spring-mvc-multi-row-submit-java-list/

This article have over 12 years. This notation is working out of the box since then. But these days most of the people are building JavaScript frameworks and SPA applications.

Now developers are coming back to simple server side rendered pages and libraries like htmx or unpoly are gaining popularity.

Notation like this is the easiest way to support complex forms with nested properties without manual mapping of those properties.

It would be great if we could support it in Quarkus

kucharzyk commented 11 months ago

@FroMage @geoand @Sgitario Any thoughts? Do you think it’s worth to implement this feature in Quarkus?

geoand commented 11 months ago

I am so far removed from what goes on in the frontend, so I don't have an opinion - I trust you folks will come up with the most sensinble solution :)

FroMage commented 11 months ago

OK, so, after thinking about this a lot, it represents some work. Not so much in terms of implementation, but it touches upon code generation, which is always tricky and often leads to invalid bytecode during the process of tweaking which typically take some time to figure out because they're hard to understand. Also, this will require writing enough tests, and making sure we didn't break anything from the TCK or our tests, which is possible.

But it's possible, and I think I've a design for it which makes sense. I won't have time to implement this, and frankly I'm not sure it's going to be very useful because you're the first one to ask for this, but I agree that it has merit because if you want to stick with HTML forms, as opposed to JSON or XML, then this is the only way to get structured recursion of data types in the message body.

What I can do though, is give you all the information needed for the design and implementation and coach you any time you need to get this working. Worst case, you don't have time to implement it and we can keep the design for later when someone has the time to get this done.

First, I've documented how we transform the bytecode of parameter containers (@BeanParam and friends) at https://github.com/quarkusio/quarkus/wiki/RESTEasy-Reactive-(Server)#beanparam-classes-aka-parameter-containers. This shows how we transform parameter container classes into classes we can inject into.

Now, a few design decisions:

An example

Here's an example of such a parameter container:

class Address {
    @RestForm
    String city;

    @RestForm
    String street;
}
class Role {
    @RestForm
    String name;

    @RestForm
    boolean enabled;    
}

class Person {
    @RestForm
    String name;

    // @BeanParam is optional and inferred ATM
    Address address;

    // @BeanParam can be made optional and inferred
    List<Role> roles;

    // @BeanParam can be made optional and inferred
    Map<String, Role> rolesByName;
}

MessageBodyReader or what?

This could be thought of as a new MessageBodyReader which would iterate over all keys and do its own stuff. It might appear as simpler to start it from scratch rather than adapt the existing parameter container system, BUT:

Adding support to the existing parameter container system

So instead, let's add support for this to the existing parameter container system.

Opting in the prefixed variant

For regular leaf @RestForm elements (not objects, not lists, not maps) the only difference is that keys can be prefixed: person.address.name instead of name, or person.roles[1].enabled instead of enabled.

We can turn that behaviour on with a new annotation @Prefixed which we can place at the root of the parameter container usage: either the endpoint field or the endpoint method parameter:

@Path("/")
public class Endpoint {

 // here
 @Prefixed
 Person user;

 // or here
 @POST
 public void something(@Prefixed Person user) {}

It does not make sense to repeat it at every nested inclusion point (such as the Person.roles field) because it cannot make sense to have the root non-prefixed and the nested elements prefixed, especially in the case of objects within lists or maps. Either the entire parameter container is prefixed, or it's not.

For the same reason, it doesn't make sense to annotate @Prefixed on the bean parameter container class itself unless all nested parameter containers are also similarly annotated. Although we could go this way, and validate it at build time, which would be less verbose if we use the parameter container more times than we declare it. Remains to be seen.

Nested parameter containers

In the case of single nested containers, such as user.address, this is simply adding on to the existing prefix for its nested properties.

Nested lists/arrays of parameter containers

Currently, lists/arrays of objects annotated with @FormParam must have the same key (foo=a&foo=b&foo=c for example) and values go via the regular ParameterConverter dance for each of them, which means that they cannot support nested parameter containers, because that interface takes a single String value (per item), and in the case of Role we need a tuple of values user.roles[0].name and user.roles[0].enabled.

So, while ParameterConverter can support deserialising any single String value into an object such as Role, it cannot access the context to build parameter containers, which require more than just one String.

As such, I don't think it makes sense to annotate List<Role> roles with @FormParam. It's not a terminal/leaf form parameter, and it can't go via the normal ParameterConverter system. Similarly, it also doesn't go via the normal multipart system. On the other hand, we must determine its length by reading the form elements, and @FormParam also allows us to override the name of the last part of the prefix, so perhaps it makes sense to make it explicit? Or we make it implicit unless the user wants to override the part name?

But we already know that Role is a parameter container, and if it was not in a List it would be handled properly, so I propose that if we see fields of type List of parameter container we handle them specially by obtaining the length from the form, and then iterating on each element and deserialise them as parameter containers by building on their prefix.

Nested maps of parameter containers

There's no support for Map types in @RestForm elements, because it makes no sense without prefixes.

Like for lists/arrays of parameter containers, we can handle maps of string to parameter container specially, by extracting the set of keys from the form using the current prefix, and using the parameter container system to instantiate every value.

This bypasses ParameterConverter and only allows for String keys.

Later, I suppose we might support other types of keys, by going via the ParameterContainer system just for the keys.

Implementation

We have to add a @Prefixed annotation, and modify ResteasyReactiveInjectionTarget so that we add an optional String prefix parameter to its __quarkus_rest_inject method.

The injection point (endpoint field or method parameter) will then decide if it passes a prefix based on the presence of the annotation. Each __quarkus_rest_inject method will then either work as usual for form parameters, or build on the current prefix, depending on whether it's null.

We have to detect list of parameter container and map of parameter container fields and treat them specially.

Current implementation

Here is the (roughly) current implementation of parameter container transformation for the example code (which obviously doesn't work):

class Address implements ResteasyReactiveInjectionTarget {
    @RestForm
    String city;

    @RestForm
    String street;

    @Override
    public void __quarkus_rest_inject(ResteasyReactiveInjectionContext ctx) {
        city = (String) ctx.getFormParameter("city", true, false);
        street = (String) ctx.getFormParameter("street", true, false);
    }
}

class io_quarkus_generated_boolean$quarkusrestparamConverter$ implements ParameterConverter {

    @Override
    public Object convert(Object parameter) {
        return Boolean.valueOf((String)parameter);
    }

}
class Role implements ResteasyReactiveInjectionTarget {
    @RestForm
    String name;

    @RestForm
    boolean enabled;    

    @Override
    public void __quarkus_rest_inject(ResteasyReactiveInjectionContext ctx) {
        name = (String) ctx.getFormParameter("city", true, false);
        Object val = ctx.getFormParameter("enabled", true, false);
        if(val != null) {
            val = __quarkus_converter__enabled.convert(val);
        }
        if(val != null) {
            enabled = (Boolean)val;
        }
    }

    private static ParameterConverter __quarkus_converter__enabled;

    // this will be called at startup
    public static void __quarkus_init_converter__enabled(Deployment deployment) {
        ParameterConverter converter = deployment.getRuntimeParamConverter(Role.class, "enabled", true);
        if(converter == null) {
            converter = new io_quarkus_generated_boolean$quarkusrestparamConverter$();
        }
        __quarkus_converter__enabled = converter;
    }
}

class Person implements ResteasyReactiveInjectionTarget {
    @RestForm
    String name;

    Address address;

    @RestForm
    List<Role> roles;

    private static ParameterConverter __quarkus_converter__roles;

    // this will be called at startup
    public static void __quarkus_init_converter__roles(Deployment deployment) {
        ParameterConverter converter = deployment.getRuntimeParamConverter(Person.class, "roles", true);
        // this is a collection
        converter = new ListConverter(converter);
        __quarkus_converter__roles = converter;
    }

    @Override
    public void __quarkus_rest_inject(ResteasyReactiveInjectionContext ctx) {
        name = (String) ctx.getFormParameter("name", true, false);

        address = new Address();
        address.__quarkus_rest_inject(ctx);

        // this is a list
        Object val = ctx.getFormParameter("roles", false, false);
        if(val != null) {
            val = __quarkus_converter__roles.convert(val);
        }
        roles = (List<Role>) val;       
    }
}

I didn't add rolesByName because we don't support Map.

Proposed implementation

This is what we should change if we want this to work:

class Address implements ResteasyReactiveInjectionTarget {
    @RestForm
    String city;

    @RestForm
    String street;

    @Override
    public void __quarkus_rest_inject(ResteasyReactiveInjectionContext ctx, String prefix) {
        city = (String) ctx.getFormParameter(SomeUtil.prefixIfNeeded(prefix, "city"), true, false);
        street = (String) ctx.getFormParameter(SomeUtil.prefixIfNeeded(prefix, "street"), true, false);
    }
}

class io_quarkus_generated_boolean$quarkusrestparamConverter$ implements ParameterConverter {

    @Override
    public Object convert(Object parameter) {
        return Boolean.valueOf((String)parameter);
    }

}
class Role implements ResteasyReactiveInjectionTarget {
    @RestForm
    String name;

    @RestForm
    boolean enabled;    

    @Override
    public void __quarkus_rest_inject(ResteasyReactiveInjectionContext ctx, String prefix) {
        name = (String) ctx.getFormParameter(SomeUtil.prefixIfNeeded(prefix, "city"), true, false);
        Object val = ctx.getFormParameter(SomeUtil.prefixIfNeeded(prefix, "enabled"), true, false);
        if(val != null) {
            val = __quarkus_converter__enabled.convert(val);
        }
        if(val != null) {
            enabled = (Boolean)val;
        }
    }

    private static ParameterConverter __quarkus_converter__enabled;

    // this will be called at startup
    public static void __quarkus_init_converter__enabled(Deployment deployment) {
        ParameterConverter converter = deployment.getRuntimeParamConverter(Role.class, "enabled", true);
        if(converter == null) {
            converter = new io_quarkus_generated_boolean$quarkusrestparamConverter$();
        }
        __quarkus_converter__enabled = converter;
    }
}

class Person implements ResteasyReactiveInjectionTarget {
    @RestForm
    String name;

    Address address;

    @RestForm
    List<Role> roles;

    @RestForm
    Map<String, Role> rolesByName;

    @Override
    public void __quarkus_rest_inject(ResteasyReactiveInjectionContext ctx, String prefix) {
        name = (String) ctx.getFormParameter(SomeUtil.prefixIfNeeded(prefix, "name"), true, false);

        address = new Address();
        address.__quarkus_rest_inject(ctx, SomeUtil.prefixIfNeeded(prefix, "address"));

        // we know it's a param container, so switch off the previous behaviour which makes no sense
        // for param containers
        // try to obtain them via the new prefixed name notation
        {
            int len = ctx.getPrefixedFormParametersLength(SomeUtil.prefixIfNeeded(prefix, "roles"));
            List<Role> val = new ArrayList<>();
            for(int i=0;i<len;i++) {
                Role item = new Role();
                item.__quarkus_rest_inject(ctx, SomeUtil.prefixIfNeeded(prefix, "roles["+i+"]"));
                val.add(item);
            }
            roles = val;
        }

        // we know it's a param container, so switch off the previous behaviour which makes no sense
        // for param containers
        // try to obtain them via the new prefixed name notation
        {
            Set<String> keys = ctx.getPrefixedFormParametersKeys(SomeUtil.prefixIfNeeded(prefix, "rolesByName"));
            Map<String,Role> val = new HashMap<>();
            for(String key : keys) {
                Role item = new Role();
                item.__quarkus_rest_inject(ctx, SomeUtil.prefixIfNeeded(prefix, "roles["+key+"]"));
                val.put(key, item);
            }
            rolesByName = val;
        }
    }
}

class SomeUtil {
    public static String prefixIfNeeded(String prefix, String suffix) {
        if(prefix != null) {
            return prefix + "." + suffix;
        }
        // not prefixed
        return suffix;
    }
}

I don't like that we're adding too much generated bytecode to build the lists and maps, but I'm not sure we can delegate that to a Util class due to the requirement to instantiate types such as Role in there. Or we pass them a lambda. TBD.

Open questions

Lists of non-parameter containers

My proposal doesn't mention special cases such as lists of non-parameter containers, for example List<String> or List<Boolean> which still go via the regular ParameterConverter route, and support prefixes but not the [index] suffix.

So for example, this:

class Person {
 @RestForm
 List<String> names;
}

Would be settable with this body (assuming we have @Prefixed at the root):

person.names=a
person.names=b
person.names=c

As opposed to:

person.names[0]=a
person.names[1]=b
person.names[2]=c

This is because String not a parameter container.

Do we want to support both types of input? We could.

Maps of non-parameter containers

My proposal doesn't mention special cases such as maps of non-parameter containers, for example Map<String,String> or Map<String,Boolean>.

So for example, this:

class Person {
 @RestForm
 Map<String, String> nickNames;
}

Since they have no ParameterConverter equivalent, we have no fallback. But we could support them, by using the same proposed implementation for building the Map by iterating the keys, and using ParameterConverter for the value.

kucharzyk commented 11 months ago

Thank you so much @FroMage for such deep analysis. I hope I will find enough time to work on it. I will try to start the implementation over the weekend. It won’t be easy but I hope I will manage to finish this.

kucharzyk commented 11 months ago

@FroMage Should we create new ParameterType for prefixed beans or it would be better to reuse BEAN type?

FroMage commented 10 months ago

I think BEAN should be enough. The fact that it's prefixed is a facet of the injection point only.