Skip to content

Support efficient parsing of Request ResourceParameters #62

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

Closed
btittelbach opened this issue Dec 1, 2019 · 5 comments · Fixed by #65
Closed

Support efficient parsing of Request ResourceParameters #62

btittelbach opened this issue Dec 1, 2019 · 5 comments · Fixed by #65
Assignees
Labels

Comments

@btittelbach
Copy link

btittelbach commented Dec 1, 2019

Is your feature request related to a problem? Please describe.
It seems that the recommend way to parse a requests RequestParameters, is to call isRequestParameterSet and then getRequestParameter for all the key-strings that might make up a query.
Both of these Methods translate to a for-loop which iterates though the whole list of parameters, doing string compare on each step.
Unfortunately getRequestParameter in itself is insufficient to check for the existence of a parameter since it will return "" when a parameter is unset or set to empty. getRequestParameterInt is even worse, since it will return 0 for unset parameters and thus one can't differentiate from parameters set to value 0.

Describe the solution you'd like
In order to improve response timing, I'd like a method that combines isRequestParameterSet and getRequestParameter. e.g. getRequestParameterIfExists which returns true/false and sets the parameter into a given argument.

Describe alternatives you've considered
ESP32AsyncWebserver let's people iterate for the parameters and do string comparison themselves, which is better than iterating twice but worse that a getRequestParameterIfExists method, since such a loop can't abort early and not exclude parameters from checking that have already been found.

@fhessel
Copy link
Owner

fhessel commented Dec 2, 2019

Thank you for the feedback. The behavior of the ResourceParameters class is indeed not optimal.

Unfortunately getRequestParameter in itself is insufficient to check for the existence of a parameter since it will return "" when a parameter is unset or set to empty.
[...] In order to improve response timing, I'd like a method that combines isRequestParameterSet and getRequestParameter. e.g. getRequestParameterIfExists which returns true/false and sets the parameter into a given argument.

I think I would go and replace the existing function in that case due to the ambiguity that you've mentioned. That would lead us to something with a signature like bool getRequestParameter(std::string const param, std::string &val) and calls like this:

  // ...
  std::string idVal;
  if (req->getParams()->getRequestParameter("id", idVal) {
    // handle request, idVal is set now
  } else {
    // handle missing parameter
  }
  // ...

getRequestParameterInt is even worse, since it will return 0 for unset parameters and thus one can't differentiate from parameters set to value 0.

I though about removing the getSomethingInt functions for some time now, as they are quite specific and only helpful in some special cases. This is one argument more for finally doing that. std::stoi, std::stol etc. allow more control by the programmer and handling such errors.

So I'd derive the following tasks:

  • Remove the getSomethingInt functions
  • Modify the getRequestParameter to differentiate between an unset parameter and an empty value
  • Provide an interface for the user to iterate through the existing parameters (that would allow handling arbitrary, unknown parameters, too).

Does that make sense from your point of view? Then I'd put it into the next major release, as the changes may break existing code.

@fhessel fhessel self-assigned this Dec 2, 2019
@fhessel fhessel added the feature label Dec 2, 2019
@btittelbach
Copy link
Author

Looks like a great good solution :-). Indeed since the get...Int function only converts unsigned 16bit integers there are likely more often cases where it will cause confusion than be helpful. (if all usecases for signed/unsigned 8,16,32bit are equally distributed) ;-)

Btw: If I could trouble you to add one more feature while you are at it:
It would be nice to know how many query parameters are available in total and if possible even get a list of their names.

Currently I might have to deal with a situation where I have to parse request parameters of the form "t10=123&t23=345,t40=456,choice=a" where the parameter name itself contains information. If so, I would have to guess and check for each possible parameter name.

@fhessel
Copy link
Owner

fhessel commented Dec 2, 2019

Btw: If I could trouble you to add one more feature while you are at it:
It would be nice to know how many query parameters are available in total and if possible even get a list of their names.

Getting the names should be possible by iterating through the parameters (the third task). The parameters are stored as std::pairs of std::strings at the moment. So for your example, you'd get an iterator that yields the pairs ("t10", "123"), ("t23", "345"), ("t40", "456") and ("choice", "a"). Adding something like ResourceParameters::requestParameterCount() shouldn't be a problem either.

Would that be sufficient for you?

@btittelbach
Copy link
Author

Giving access to an iterator to the string pairs would, i believe, be indeed the most straightforward way to cover all use-cases.

@fhessel
Copy link
Owner

fhessel commented Dec 5, 2019

Could you have a look at #65 and see if that works for you?

I'll merge that with the other pending PRs to create a new release, but that may take some time.

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

Successfully merging a pull request may close this issue.

2 participants