jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.84k stars 1.91k forks source link

Improve consistency of looking up query and form parameters #12277

Open cowwoc opened 1 week ago

cowwoc commented 1 week ago

Jetty version(s) 12

Enhancement Description To look up query parameters, one has to invoke Request.extractQueryParameters(Request). To look up form parameters, one has to invoke FormFields.from(Request).

I only figured out the latter by digging into Jetty's source-code.

Ideally, the two APIs should be similar, accessible from the same class, and easily discoverable.

joakime commented 1 week ago

Don't forget multipart/form-data too, accessed via org.eclipse.jetty.http.MultiPartFormData...

          String boundary = MultiPart.extractBoundary(contentType);
          MultiPartFormData.Parser formData = new MultiPartFormData.Parser(boundary);
          formData.setFilesDirectory(workDir);

          // we are assuming a simple form with no binary data (like a file upload)
          Map<String, String> form = new HashMap<>();

          // May block waiting for multipart form data.
          try (MultiPartFormData.Parts parts = formData.parse(request).join())
          {
              parts.forEach(part ->
              {
                  if (StringUtil.isNotBlank(part.getFileName()))
                      return; // skip files

                  String value = part.getContentAsString(StandardCharsets.UTF_8);
                  form.put(part.getName(), value);
              });
          }
joakime commented 1 week ago

Right now ...

There are also parsers in Jetty for message/html and multipart/byte-range request body contents too. (just to name a few common ones)

gregw commented 1 week ago

Form Field parsing is done by org.eclipse.jetty.server.FormFields#parse and query field parsing is done by org.eclipse.jetty.util.UrlEncoded#decodeUtf8To(java.lang.String, int, int, java.util.function.BiConsumer<java.lang.String,java.lang.String>). The key difference is that the former is written against a possibly asynchronous stream of content chunks of bytes, whilst the later decodes from a String that is already converted to characters.

Whilst the parsing itself is the same, there are significant different concerns of needing to be able to read a form asynchronously in bytes vs having the full query available as a string. Perhaps the query string could be converted back to bytes, wrapped as a chunk and the normal form parser used... but that is a lot of extra work for a very frequent component of requests.

So whilst I do not like code duplication.... performance is probably more important in this case and I think UrlEncoded and FormFields both have their place. Perhaps the two classes could be merged, but the different styles of IO need to be preserved efficiently.

cowwoc commented 1 week ago

At this point, I care more about discoverability than invocation styles (though, yes, long-term it would be nice to make that consistent as well).

gregw commented 1 week ago

@cowwoc So tell us how you go about discovery? You want a class to parse form fields, so are you searching javadoc, other documentation or just looking for a class that is named appropriately?

In this case, I'm guessing it is probably the UrlEncoded class that you want to use, which is a util class and documented as Handles coding of MIME "x-www-form-urlencoded".. Tell us where you looked and didn't find this class and we will see if we can make it more discoverable.

cowwoc commented 1 week ago

I was hoping to look at the Request class and find methods for accessing query, form, multipart, etc parameters. I typically look for such methods using the IDE's method name auto-complete functionality. If that fails, I google. If that fails, I check the programmer's manual.

joakime commented 1 week ago

I was hoping to look at the Request class and find methods for accessing query, form, multipart, etc parameters. I typically look for such methods using the IDE's method name auto-complete functionality. If that fails, I google. If that fails, I check the programmer's manual.

When using your IDE, does it show the static methods on Request (like Request.extractQueryParameters(Request request)? or just the non-static instance methods?

cowwoc commented 1 week ago

When using your IDE, does it show the static methods on Request (like Request.extractQueryParameters(Request request)? or just the non-static instance methods?

I'm using IntelliJ, and yes it shows the static methods as well.

To find the method in question, I typed Request.query and triggered auto-complete. It returned all the methods that contained "query" somewhere in their name, including the static methods.

This is how I usually start my search for new methods.