[Feature Request] Nested form attributes

[lancecarlson]

Feature Requests

1. Why you believe this feature is necessary.

Go, Rails, Express all have some way of parsing nested attributes easily from forms.

2. A convincing use-case for this feature.

I think it would be pretty nice to have the ability to able to specify a struct like:

#[derive(FromForm]
struct StarterApplication {
  user: User,
  business: Business,
}

#[post("/apply", data="<form>")]
fn post_apply(context: Context, form: StartedApplication) -> Flash<Redirect> {
  //..
}

And based on the dot notation or brackets, allow nested attributes in forms to parse automatically:

<input type="email" name="user[email]" />
<input type="email" name="business[name]" />

or

<input type="email" name="user.email" />
<input type="email" name="business.name" />

Instead I find myself doing this:

#[derive(FromForm, Serialize)]
struct StarterApplication {
    user_email: String,
    user_full_name: String,
//... user fields ...
    business_name: String,
//... business fields ...
}

and manually converting it to the individual structs.

That being said, if there was a way to possibly specify multiple structs on FromForm and get access to the form field name and not just the value, that might also be a viable option? I'm not sure what my options are regardless. Perhaps I'm missing something?

3. Why this feature can't or shouldn't exist outside of Rocket.

It could possibly be an extension of rocket if there was a way to get the values of the fields that come through. I'm not sure that is possible though.

Thanks!!

[lancecarlson]

@SergioBenitez what do you think the best option for implementation is? I can try to help flesh this out if you have a good idea of what it should look like and if it should be entirely in rocket or an extension.

[SergioBenitez]

@lancecarlson I haven't had plenty of time to think about this, but this seems like something that would be great to have, and so I'm inclined to accept this. Thus far, I'm thinking that fields can be labeled with #[form(nested)] to indicate that the field's type actually corresponds to a nested form. The FromForm derived code can then do the right thing, assuming something like what I've proposed in #242 (comment) is merged.

In your example, this might look like:

#[derive(FromForm]
struct StarterApplication {
  #[form(nested)]
  user: User,
  #[form(nested)]
  business: Business,
  not_nested: usize,
}

You would be able to use the nested and field inner-attributes simultaneously:

#[derive(FromForm]
struct StarterApplication {
  #[form(nested, field = "userSettings")]
  user_settings: User,
}

[SergioBenitez]

Implementation wise, a few things would need to happen:

1. The new attribute would need to be recognized in the derive(FromForm) plugin.

2. FormItems would need to be extended so that it knows about nested forms.

   I imagine this to mean that we add a .filtered(&self, name: &str) -> FormItems method to FormItems that returns a new iterator where only fields that start with {name}. are emitted from the iterator and so that the {name}. prefix is removed from keys. This enables infinite nesting as well, so forms can include something like user.account.id.

The implementation I site above means that the form string is scanned once per nested form. You might imagine a more optimized implementation that only goes through the form string twice, regardless of the number of nested forms. Though I like the simplicity of the above implementation.

[theduke]

Just a note: if you implement this, supporting arrays would be great too!

#[derive(FromForm]
struct Item {
  name: String,
}

#[derive(FromForm]
struct FormData {
  #[form(nested)]
  items: Vec<Item>,
}

<input type="text" name="items.0.name" />
<!-- OR -->
<input type="text" name="items[0][name]" />

[SergioBenitez]

@theduke I'm not sure about that. It's not clear what the semantics are; are we writing to index 0 of the items array? If so, how is the array getting sized? If we allow the user to control the index, which is what you proposed, and automatically size the array depending on the index, then this can trivially lead to a DOS attack via OOM. Further, what happens if index i and j are provided but aren't contiguous? I suppose we could force Item to be Default, but that seems error-prone.

The request in #205, which is similar to what you're proposing, is much more conventional and doesn't have any of these drawbacks. It's something I'd eventually like to get in.

[lancecarlson]

Is this a border line serde thing?

[SergioBenitez]

@lancecarlson Can you expand on that question? I'm not sure what you're asking.

[theduke]

That's how most of the frameworks do it, since there is not really any other way to express nested arrays which contain structures.
The index could probably just be ignored on the Rust side and only used for detecting arrays.

I don't really care much about this feature anyway since nowadays I just create some JSON with Javascript and POST it.

[lancecarlson]

@SergioBenitez I was thinking maybe it's only the serialize part of serde to parse forms, but the more I think about it, it actually makes sense to have it in rocket.

[lancecarlson]

@SergioBenitez also, to @theduke's point, go does it the way he is describing:

http://www.gorillatoolkit.org/pkg/schema

[lancecarlson]

@SergioBenitez are you thinking that instead of allowing the index to be specified, that if you repeat the same input name (even if it is nested), that that would create an array instead?

items[].name
Items[].name

Instead of

items[0].name
items[1].name

I'm completely fine with that I think. The only nice thing with indexes is that it can be easier to fulfill Todo list style use cases where position is important instead of relying on placement in the DOM, you can rely on the index. Also, I assume that one would specify their struct to serialize into with the sizes predefined if they wanted to prevent large index attacks?

[SergioBenitez]

@lancecarlson Can you tell me more about the todo list style use cases you have in mind? I'm imagining that you'd have an "id" field in such a form anyway, so using the index wouldn't matter.

I'm not sure what you mean by "I assume that one would specify their struct to serialize into with the sizes predefined if they wanted to prevent large index attacks?". I think as long as the user can specify an index, we lose. Maybe you mean that you should be able to say something like, "ensure that this structure doesn't exceed this much allocated memory during deserialization." If so, well, maybe, but then we'd have to keep track of this during deserialization. That sounds a bit painful and not worthwhile. It also adds another configuration parameter.

[allan-simon]

to avoid somebody writing

<input type="text" name="items[0][name]" />
<!-- oh noz it's over 9000 --> 
<input type="text" name="items[9001][name]" />

to make the system OOM, does rust have an "ordered dict" structure ? in which case you could implement a sparse array that will actually contains only 2 elements

[shssoichiro]

does rust have an "ordered dict" structure ? in which case you could implement a sparse array that will actually contains only 2 elements

Not built in, but there is the ordermap crate.

[kardeiz]

You can get this behavior today with the following (defers form processing to serde_qs):

pub struct Qs<D>(pub D);
impl<'f, D> FromForm<'f> for Qs<D> where D: ::serde::de::DeserializeOwned {
    type Error = ::serde_qs::Error;

    fn from_form(items: &mut FormItems<'f>, _: bool) -> Result<Self, Self::Error> {
        items.mark_complete();
        Ok(Qs(::serde_qs::from_str(items.inner_str())?))
    }
}

And then wrapping your form struct in your handler signature like form: Qs<MyForm>.

It might be nice if Rocket just deferred form parsing entirely to serde_qs (or serde_urlencoded, but that one doesn't support nesting)...

[SergioBenitez]

@kardeiz Deferring to serde simply isn't an option. We want to have custom validation per field which means calling from_form_value for every field in the structure; this isn't something serde can do, of course. We also want it to be simple to implement the equivalent of FromFormValue; implementing Deserialize is much more complicated.

[kardeiz]

@SergioBenitez, okay, that makes sense. Sorry, I'm not very familiar with the Rocket ecosystem at the moment. I do appreciate how easy it is to defer to serde with FromForm when it is necessary, though!

[SergioBenitez]

Unfortunately, we won't be able to get this in for 0.4: pushing to 0.5.

[TotalKrill]

There seems to be a commit on this here from 2018, but it was never merged. This is blocking a 0.5 release currently. Is the original author still around and can comment on the code and its state and/or what is missing?

[lancecarlson]

@TotalKrill It's been so long, I'm sure this code is no longer relevant and I'm missing a lot of context on how to get it up to speed.

[TotalKrill]

Maybe this feature could be pushed to 0.6, from what I gather, the move to async in 0.5 has made the codebase a lot different and it seems a lot of work for 0.4 is no longer relevant

[jebrosen]

@TotalKrill see #106 (comment). It's already been mostly implemented; IIRC there were a few rustc bugs (which were hopefully fixed in the most recent stable) and a few bugs in dependencies.

[SergioBenitez]

All of the issue are now resolved! I should be able to land this very soon!

[ittorchbearer]

This is more of POC, but I ran into trouble with recursive dependencies causing compilation failure. Simple Box around the contexts Generic storage allowed them to build the layouts separately during MIR for more flexible inputs. Leaving hear as a future topical note.
see comment below for corrected link
Plato-solutions@b13a664

[SergioBenitez]

This is more of POC, but I ran into trouble with recursive dependencies causing compilation failure. Simple Box around the contexts Generic storage allowed them to build the layouts separately during MIR for more flexible inputs. Leaving hear as a future topical note.

Plato-solutions@b13a664

Can you be more specific about what problem you had? This really shouldn't be required unless you have a custom FromForm type in which case you can Box that type's Context instead.

[ittorchbearer]

The important part of the code snippet is the Configuration struct recursion with struct and its' final field.

#[derive(FromForm, JsonSchema, UriDisplayQuery))]
pub struct Configuration {
    pub path: String,
    pub path_type: PathType,
    pub processor: Processor,
    pub parameters: HashMap<String, Value>,
    pub log: LogConfig,
    pub store: Option<Store>,
    pub response: Option<ResponseConfig>,
    pub post_activities: Vec<Configuration>,
}

#[derive(FromForm, JsonSchema, UriDisplayQuery))]
pub struct ExecutionDb {
    pub configuration: Json<Configuration>,
}

I suspect the second part is unnecessary, and any structure in a container recursion like this will cause the error. I do not have a minimum reproducible version, and this already has a lot stripped out--hopefully not too much.

The error I was getting:

error[E0391]: cycle detected when computing layout of `rocket::form::from_form::VecContext<configuration::Configuration>`
   |
   = note: ...which requires computing layout of `core::option::Option<<configuration::Configuration as rocket::form::from_form::FromForm>::Context>`...
   = note: ...which requires computing layout of `core::option::Option<configuration::_::FromFormGeneratedContext>`...
   = note: ...which requires computing layout of `configuration::_::FromFormGeneratedContext`...
   = note: ...which requires computing layout of `core::option::Option<<alloc::vec::Vec<configuration::Configuration> as rocket::form::from_form::FromForm>::Context>`...
   = note: ...which requires computing layout of `core::option::Option<rocket::form::from_form::VecContext<configuration::Configuration>>`...
   = note: ...which again requires computing layout of `rocket::form::from_form::VecContext<configuration::Configuration>`, completing the cycle
note: cycle used when optimizing MIR for `configuration::_::<impl at src/configuration.rs:30:45: 30:53>::push_value::{closure#7}`
  --> src/configuration.rs:39:26
   |
39 |     pub post_activities: Vec<Configuration>,

Also, I moved the Box from items to Context in this new commit. The error returned today using the prior one and also seemed cleaner than the last patch.
Plato-solutions@2cdd5c1

[SergioBenitez]

Does this change also fix compilation?

#[derive(FromForm, JsonSchema, UriDisplayQuery))]
pub struct Configuration {
    pub path: String,
    pub path_type: PathType,
    pub processor: Processor,
    pub parameters: HashMap<String, Value>,
    pub log: LogConfig,
    pub store: Option<Store>,
    pub response: Option<ResponseConfig>,
-   pub post_activities: Vec<Configuration>,
+   pub post_activities: Vec<Box<Configuration>>,
}

[ittorchbearer]

It probably would, but most macros don't have generic implementation support for Box. In those cases, I would have to patch all other libraries to work around what seems like a limitation in Rocket's internal data structure.

Another solution is to support From with duplicate structs fitting each library, ie Adapters--which I sometimes do. The ugly thing about this solution is that is essentially what library Macros are doing. Now there is a bunch of duplicate code to maintain, taking away key advantages of macros.

I don't believe it's good to have the object that tightly coupled in memory either. I suspect they are semi-frequently operated on and used in separate functions--so it may reduce overhead in real-world complex structures. Lots of assumptions and guesses in this last paragraph.

What downside do you see to this change?