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

Feature/path arguments #5214

Closed
wants to merge 17 commits into from
Closed

Conversation

Bmooij
Copy link
Contributor

@Bmooij Bmooij commented Oct 6, 2018

Adding path arguments to WebServer. (In respond to #2019)
Ex.

server.on("/led/2/actions/{}", []() {
    String action = server.pathArg(0);
    ...
 });

I don't know if this is the correct approach, feel free to give comments.

@Bmooij
Copy link
Contributor Author

Bmooij commented Oct 8, 2018

One note: the order of the paths are key!

Given

ESP8266WebServer server(80);
server.on("/users/{}", handleUsersCall);
server.on("/users/{}/devices/{}", handleDevicesCall);
server.begin();

When calling http://YOUR_IP/users/3/devices/1 only the function handleUsersCall will be called.
With server.pathArg(0) == "3/devices/1"

In order to make this work:

ESP8266WebServer server(80);
server.on("/users/{}/devices/{}", handleDevicesCall);
server.on("/users/{}", handleUsersCall);
server.begin();

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 8, 2018

In order to make this work:

Could you add this explanation in your example ?

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Some questions at this point before I do a full review.

@Bmooij
Copy link
Contributor Author

Bmooij commented Oct 9, 2018

One note: the order of the paths are key!

Given

ESP8266WebServer server(80);
server.on("/users/{}", handleUsersCall);
server.on("/users/{}/devices/{}", handleDevicesCall);
server.begin();

When calling http://YOUR_IP/users/3/devices/1 only the function handleUsersCall will be called.
With server.pathArg(0) == "3/devices/1"

In order to make this work:

ESP8266WebServer server(80);
server.on("/users/{}/devices/{}", handleDevicesCall);
server.on("/users/{}", handleUsersCall);
server.begin();

The order no longer matters.
Path arguments may not contain '/'.
If a '/' needs to be a path argument, an URL encoded value can be used (%2F)

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

There is a trailing space on line 602 in the example.
I just realized that there would be a lot more constref to modify in these classes.
I'm OK to merge this now and it could be revisited later as part of a cleaning process (aka our ram is not gruyère)

@d-a-v d-a-v added this to the 2.5.0 milestone Oct 12, 2018
@d-a-v d-a-v self-assigned this Nov 30, 2018
@devyte devyte self-requested a review December 4, 2018 15:03
@devyte
Copy link
Collaborator

devyte commented Dec 6, 2018

@Bmooij I've been looking at this from different angles and... the {} syntax bothers me, it's just not standard. Extending it would require extending the custom parser code, which again would not be standard, so I'd prefer to avoid it.
I'd personally prefer regex, but that can get complicated to use, and the patterns tend to be a bit error prone.
How about using glob (glob.h is available)? It's simple and standard, and makes use of a standard lib for the algorithm, so no custom parsing/string manipulation.

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Consider using glob matching instead of custom syntax/code.

@Bmooij
Copy link
Contributor Author

Bmooij commented Dec 6, 2018

@devyte I'm open for every thing. I'm not familiair with glob. Can you show an example how the path would look like?

@devyte
Copy link
Collaborator

devyte commented Dec 6, 2018

/users/*/devices
If you've ever done something like this in a Linux console, you've used glob patterns:
ls -l myfile*
or:
ls -l *cpp

glob is actually used for filenames in a dir. I think the underlying function to use is fnmatch (fnmatch.h). It returns zero or non-zero.
The following works in cpp.sh:

// Example program
#include <iostream>

#include <fnmatch.h>

int main()
{
    const char * pattern = "/users/*/devices/*";
    const char * path = "/users/lol/devices/lel";
    
    int ret = fnmatch(pattern, path, 0);
    
    std::cout << "ret=" << ret << std::endl;
}

Printout is ret=0, which means match.

@devyte
Copy link
Collaborator

devyte commented Dec 10, 2018

This requires further discussion, so pushing back.

@Bmooij
Copy link
Contributor Author

Bmooij commented Dec 24, 2018

After using glob patterns, my personal preference goes to regex.
I added the regex support. Please let me know your thoughts on this.

usage:

server.on("^\\/users\\/([0-9]+)\\/devices\\/([0-9]+)$", []() {
    String user = server.pathArg(0);
    String device = server.pathArg(1);
    server.send(200, "text/plain", "User: '" + user + "' and Device: '" + device + "'");
  });

note:
The ^ upfront and $ at the end are required to detect it is a regex pattern.

@earlephilhower earlephilhower added merge-conflict PR has a merge conflict that needs manual correction waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. labels Oct 1, 2019
@devyte
Copy link
Collaborator

devyte commented Oct 29, 2019

This has a merge conflict, and needs further consideration to evaluate against the alternatives.
As a side note, I'm evaluating coexistence of all three.

Pushing back.

@devyte devyte modified the milestones: 2.6.0, 2.7.0 Oct 29, 2019
@Bmooij
Copy link
Contributor Author

Bmooij commented Oct 31, 2019

I forgot this pull request. Tomorrow I will fix the merge conflicts and implement the glob patterns, like @devyte proposed.

A problem that I encountered from a similar pull request, was that regex is a large include (100k).

@devyte
Copy link
Collaborator

devyte commented Oct 31, 2019

@Bmooij #5467 I already implemented glob. All 3 alternatives are in PRs.
The question is: which do we use.
I was thinking of all three, with a different method nameing scheme, e. g. :

onRegEx() 
onGlob() 
onDontKnowNameOfThisOne()

With this, the regex bin increase should happen only if it is actually used, and yet all three alternatives are available.

@Bmooij
Copy link
Contributor Author

Bmooij commented Oct 31, 2019

@devyte What do you think of the following solution:

server.on("a/static/uri", callbackfn); // this will automatically transform to Uri("a/static/uri")
server.on(Uri("an/other/static/uri"), callbackfn);
server.on(UriRegex("^\\/users\\/([0-9]+)\\/devices\\/([0-9]+)$"), callbackfn);
server.on(UriGlob("/users/*/devices/*"), callbackfn);
server.on(UriBracket("/users/{}/devices/{}"), callbackfn);

@devyte
Copy link
Collaborator

devyte commented Oct 31, 2019

That is a very good idea!
Notes for the uri objects:

  • the constructors need to be explicit
  • overloads in each for const char * and String args
  • probably const String member
  • UriBrace or UriBraces instead of brackets
  • mind move semantics
  • getter methods

@devyte
Copy link
Collaborator

devyte commented Oct 31, 2019

@Bmooij could you please pick up the three implementations from the PRs and make a new one with your idea? That new one would then supersede the three.

@Bmooij
Copy link
Contributor Author

Bmooij commented Nov 1, 2019

There is an issue when I want to include <regex>

lib\ESP8266WebServer\src/detail/RequestHandlersImpl.h:25:13: error: 'regex' is not a member of 'std'

@devyte
Copy link
Collaborator

devyte commented Nov 6, 2019

Closing in favor of #6696.

@devyte devyte closed this Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-conflict PR has a merge conflict that needs manual correction waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants