Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add LocalForm to make testing with form data more ergonomic #1605

Closed
wants to merge 286 commits into from
Closed

Add LocalForm to make testing with form data more ergonomic #1605

wants to merge 286 commits into from

Conversation

ELD
Copy link
Member

@ELD ELD commented Apr 11, 2021

This adds the testing improvement described in #1591.

The transformation of the form fields into the resulting body data is a bit clumsy and could probably use some improvement/cleanup. I believe the multipart form translation is correct, but there's only one, narrow test case that confirms it works as expected.

Additionally, I was unable to get something like this to work without ugly ergonomics:

let client = Client::tracked(rocket).unwrap();
let response = client.post("/")
    .form(&[("field", "value"), ("is it", "a cat?")])
    .dispatch();

I might be missing something, but attempting to do something like this:

impl<'v, F: Into<ValueField<'v>>, I: Iterator<Item = F>> From<I> for LocalForm { /* .. */ }

requires the caller to call the method in a way similar to this:

client.post("/")
    .form(&[("field", "value"), ("is it", "a cat?")].iter().cloned());

due to a slice reference not being coerced into an iterator and when it is iterated over, it returns &(&str, &str). ValueField doesn't have a present From<..> impl for &(&str, &str), so this is not possible.

Also, docs are likely not complete/elaborate enough at this stage, but an attempt at documenting relevant methods/structs was made in this first cut.

Please pick it apart so I can make the implementation better! :)

Copy link
Member

@SergioBenitez SergioBenitez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is handling percent-encoding incorrectly. It's decoding when it shouldn't be and failing to encode when it should be. Here are the conditions:

  • If a provided string is percent-encoded, it should be left alone.
  • If a provided string is not percent-encoded, it should be percent-encoded.

Specifically, note that for some s, encode(decode(s)) != s (for example, %).

Note that we have the UriDisplay<Query> trait that already knows how to do this for a bunch of types. We should probably use it here. Perhaps better type signatures are:

pub fn field<N, V>(mut self, name: N, value: V) -> Self
    where N: Into<NameBuf<'v>>, V: UriDisplay<Query>,

Let's start by fixing the incorrect code and style and we'll go from there.

@ELD
Copy link
Member Author

ELD commented Jun 5, 2021

@SergioBenitez This should be ready for another pass.

I updated the style to better match Rocket's. I think it's mostly there, but there might be some minor style issues remaining.

I changed the writing of the body data to a single Vec<u8> with the write! macro. There's a couple places where I call format!. There's probably a better way than what I did, so any help there would be great!

Error handling is a little weird as it currently silently outputs an empty buffer if the writing failed for some reason. This probably isn't a good user experience because an error would result in empty body data for the form request. Any pointers on how to do better error handling would be great!

@ELD ELD requested a review from SergioBenitez June 5, 2021 18:53
@slyons
Copy link

slyons commented Aug 31, 2021

Has anyone had a chance to look at this yet?

SergioBenitez and others added 26 commits December 27, 2022 13:02
Previously, 'serde_json::Value' was used to store the serialized
template context. This value does not represent all of serde's data
model. This means we may fail to serialize a valid Rust value into it,
for instance, 128-bit integers. This is reduced with Figment's 'Value',
which supports the majority if not all of the serde data model.

At present, all supported templating engines use 'serde_json::Value', so
in practice, this commit has no effect but to reduce local dependencies
and provide better error messages for bad contexts.
This makes a relative 'temp_dir' declared in a config file relative to
the config file itself.
Previously, recursion into rewriting 'self' in field validations would
cease after the first function call. This meant that internal uses of
'self' were not properly rewritten. This commit ameliorates the
situation.
The improvements are:

  * Point directly and immediately to the 'Responder' derive.
  * Provide more discussion on lifetimes.
  * Format documentation for easier scanning.
The syntax 'TypedStream![T + '_]' expands to:

  impl TypedStream<Item = T> + '_

This allows seamlessly borrowing in typed streams.

Also adds 'Event::empty()', for convenience.
SergioBenitez and others added 28 commits December 27, 2022 13:04
Prior to this commit, data guards were not being considered as eligible
to be sentinels. This commit resolves that.
This fixes UI tests on Linux nightly. Without the `rust-src` component,
an error message pointing to the core libraries is not emitted as
expected. Presumably by making the sources available, the compiler has
somewhere to point to.

This commit also re-enables CI failures for the Linux debug target.
Replace 'zero-downtown' with 'zero-downtime'.
Replace 'demonstratation' with 'demonstration'.
Replace 'installled' with 'installed'.
Use `allow(deadcode)` on structs that exist solely to be debug-printed.
Users experience confusion when the server appears to do "nothing" when
compiled in release mode. In reality, the server has started, but it
offers no indication in that direction via log message. Often users
misconfigure the port or address, but that information isn't displayed.

This commit makes it such that only the final "Rocket has launched!"
log message is displayed, which includes the listening address, port,
and protocol.
Regression introduced in 885cdfd resulted in items in logs messages not
being properly indented.
Prior to this commit, the `FromForm` derive could pair the incorrect
field name with a failing validation. The bug was caused by using two
mismatched iterators in a `quote!()` invocation. Specifically, the first
iterator emitted validation calls for all fields that had validation
applied, while the second emitted field names for all fields,
irrespective of whether the field had any validation applied. The two
iterators were effectively zipped to create the final error, creating
the bug.

This commit fixes the issue by correctly matching field names with their
validators at the expense of an additional allocation, necessitated by
the `quote` crate's inability to access subfields in a repetition.

Fixes #2394.
@ELD ELD closed this by deleting the head repository Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.