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

Improve consistency of looking up query and form parameters #12277

Open
cowwoc opened this issue Sep 16, 2024 · 8 comments
Open

Improve consistency of looking up query and form parameters #12277

cowwoc opened this issue Sep 16, 2024 · 8 comments

Comments

@cowwoc
Copy link
Contributor

cowwoc commented Sep 16, 2024

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
Copy link
Contributor

joakime commented Sep 16, 2024

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
Copy link
Contributor

joakime commented Sep 16, 2024

Right now ...

  • Request.extractQueryParameters(Request) - exists in Request, as the query is part of the Request metadata.
  • FormFields.from(Request) - exists in FormFields as it parses application/x-www-form-urlencoded
  • MultiPartFormData.Parser - exists in MultPartFormData as it parses multipart/form-data

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
Copy link
Contributor

gregw commented Sep 16, 2024

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
Copy link
Contributor Author

cowwoc commented Sep 17, 2024

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
Copy link
Contributor

gregw commented Sep 17, 2024

@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
Copy link
Contributor Author

cowwoc commented Sep 18, 2024

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
Copy link
Contributor

joakime commented Sep 18, 2024

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
Copy link
Contributor Author

cowwoc commented Sep 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants