From d0fab6098147c1f2b83d5897a51811157fc991e8 Mon Sep 17 00:00:00 2001 From: Miccah Date: Mon, 1 Mar 2021 04:50:52 -0600 Subject: [PATCH] vweb: refactor routing logic (#9025) --- cmd/tools/vtest-self.v | 5 + examples/vweb/vweb_example.v | 7 +- vlib/net/http/header.v | 5 + vlib/vweb/request.v | 32 ++-- vlib/vweb/route_test.v | 263 ++++++++++++++++++++++++++++++ vlib/vweb/vweb.v | 300 ++++++++++++++++------------------- 6 files changed, 434 insertions(+), 178 deletions(-) create mode 100644 vlib/vweb/route_test.v diff --git a/cmd/tools/vtest-self.v b/cmd/tools/vtest-self.v index ae0f601f8a..0b455dffbf 100644 --- a/cmd/tools/vtest-self.v +++ b/cmd/tools/vtest-self.v @@ -19,6 +19,7 @@ const ( 'vlib/v/tests/orm_sub_struct_test.v', 'vlib/vweb/tests/vweb_test.v', 'vlib/vweb/request_test.v', + 'vlib/vweb/route_test.v', 'vlib/x/websocket/websocket_test.v', ] skip_with_fsanitize_address = [ @@ -138,6 +139,7 @@ const ( 'vlib/v/tests/working_with_an_empty_struct_test.v', 'vlib/vweb/tests/vweb_test.v', 'vlib/vweb/request_test.v', + 'vlib/vweb/route_test.v', 'vlib/x/json2/any_test.v', 'vlib/x/json2/decoder_test.v', 'vlib/x/json2/json2_test.v', @@ -312,6 +314,7 @@ const ( 'vlib/v/vcache/vcache_test.v', 'vlib/vweb/tests/vweb_test.v', 'vlib/vweb/request_test.v', + 'vlib/vweb/route_test.v', 'vlib/v/compiler_errors_test.v', 'vlib/v/tests/map_enum_keys_test.v', 'vlib/v/tests/tmpl_test.v', @@ -341,6 +344,7 @@ const ( 'vlib/clipboard/clipboard_test.v', 'vlib/vweb/tests/vweb_test.v', 'vlib/vweb/request_test.v', + 'vlib/vweb/route_test.v', 'vlib/x/websocket/websocket_test.v', 'vlib/net/http/http_httpbin_test.v', 'vlib/net/http/header_test.v', @@ -357,6 +361,7 @@ const ( 'vlib/x/websocket/websocket_test.v', 'vlib/vweb/tests/vweb_test.v', 'vlib/vweb/request_test.v', + 'vlib/vweb/route_test.v', ] skip_on_non_windows = []string{} skip_on_macos = []string{} diff --git a/examples/vweb/vweb_example.v b/examples/vweb/vweb_example.v index 9cfbf83803..d0531848ea 100644 --- a/examples/vweb/vweb_example.v +++ b/examples/vweb/vweb_example.v @@ -1,6 +1,7 @@ module main import vweb +import rand const ( port = 8082 @@ -21,8 +22,10 @@ pub fn (mut app App) init_once() { app.handle_static('.', false) } -pub fn (mut app App) json_endpoint() vweb.Result { - return app.json('{"a": 3}') +['/users/:user'] +pub fn (mut app App) user_endpoint(user string) vweb.Result { + id := rand.intn(100) + return app.json('{"$user": $id}') } pub fn (mut app App) index() vweb.Result { diff --git a/vlib/net/http/header.v b/vlib/net/http/header.v index 6fb49e82c5..641dec31e2 100644 --- a/vlib/net/http/header.v +++ b/vlib/net/http/header.v @@ -419,6 +419,11 @@ pub fn (h Header) values_str(key string) []string { return h.data[k] } +// Gets all header keys as strings +pub fn (h Header) keys() []string { + return h.data.keys() +} + // Validate and canonicalize an HTTP header key // A canonical header is all lowercase except for the first character // and any character after a `-`. Example: `Example-Header-Key` diff --git a/vlib/vweb/request.v b/vlib/vweb/request.v index 9485bf7465..ee29657bad 100644 --- a/vlib/vweb/request.v +++ b/vlib/vweb/request.v @@ -10,26 +10,28 @@ pub fn parse_request(mut reader io.BufferedReader) ?http.Request { method, target, version := parse_request_line(line) ? // headers - mut headers := map[string][]string{} + mut h := http.new_header() line = reader.read_line() ? for line != '' { - key, values := parse_header(line) ? - headers[key] << values + key, value := parse_header(line) ? + h.add_str(key, value) ? line = reader.read_line() ? } - mut http_headers := map[string]string{} - mut http_lheaders := map[string]string{} - for k, v in headers { - values := v.join('; ') - http_headers[k] = values - http_lheaders[k.to_lower()] = values + // create map[string]string from headers + // TODO: replace headers and lheaders with http.Header type + mut headers := map[string]string{} + mut lheaders := map[string]string{} + for key in h.keys() { + values := h.values_str(key).join('; ') + headers[key] = values + lheaders[key.to_lower()] = values } // body mut body := [byte(0)] - if 'content-length' in http_lheaders { - n := http_lheaders['content-length'].int() + if length := h.get(.content_length) { + n := length.int() body = []byte{len: n, cap: n + 1} reader.read(mut body) or { } body << 0 @@ -38,8 +40,8 @@ pub fn parse_request(mut reader io.BufferedReader) ?http.Request { return http.Request{ method: method url: target.str() - headers: http_headers - lheaders: http_lheaders + headers: headers + lheaders: lheaders data: string(body) version: version } @@ -60,7 +62,7 @@ fn parse_request_line(s string) ?(http.Method, urllib.URL, http.Version) { return method, target, version } -fn parse_header(s string) ?(string, []string) { +fn parse_header(s string) ?(string, string) { if ':' !in s { return error('missing colon in header') } @@ -69,7 +71,7 @@ fn parse_header(s string) ?(string, []string) { return error('invalid character in header name') } // TODO: parse quoted text according to the RFC - return words[0], words[1].trim_left(' \t').split(';').map(it.trim_space()) + return words[0], words[1].trim_left(' \t') } // TODO: use map for faster lookup (untested) diff --git a/vlib/vweb/route_test.v b/vlib/vweb/route_test.v new file mode 100644 index 0000000000..31a88ba10e --- /dev/null +++ b/vlib/vweb/route_test.v @@ -0,0 +1,263 @@ +module vweb + +struct RoutePair { + url string + route string +} + +fn (rp RoutePair) test() ?[]string { + url := rp.url.split('/').filter(it != '') + route := rp.route.split('/').filter(it != '') + return route_matches(url, route) +} +fn (rp RoutePair) test_match() { + rp.test() or { panic('should match: $rp') } +} +fn (rp RoutePair) test_no_match() { + rp.test() or { return } + panic('should not match: $rp') +} +fn (rp RoutePair) test_param(expected []string) { + res := rp.test() or { panic('should match: $rp') } + assert res == expected +} + +fn test_route_no_match() { + tests := [ + RoutePair{ + url: '/a' + route: '/a/b/c' + }, + RoutePair{ + url: '/a/' + route: '/a/b/c' + }, + RoutePair{ + url: '/a/b' + route: '/a/b/c' + }, + RoutePair{ + url: '/a/b/' + route: '/a/b/c' + }, + RoutePair{ + url: '/a/c/b' + route: '/a/b/c' + }, + RoutePair{ + url: '/a/c/b/' + route: '/a/b/c' + }, + RoutePair{ + url: '/a/b/c/d' + route: '/a/b/c' + }, + ] + for test in tests { + test.test_no_match() + } +} + +fn test_route_exact_match() { + tests := [ + RoutePair{ + url: '/a/b/c' + route: '/a/b/c' + }, + RoutePair{ + url: '/a/b/c/' + route: '/a/b/c' + }, + RoutePair{ + url: '/a' + route: '/a' + }, + RoutePair{ + url: '/' + route: '/' + }, + ] + for test in tests { + test.test_match() + } +} + +fn test_route_params_match() { + RoutePair{ + url: '/a/b/c' + route: '/:a/b/c' + }.test_match() + + RoutePair{ + url: '/a/b/c' + route: '/a/:b/c' + }.test_match() + + RoutePair{ + url: '/a/b/c' + route: '/a/b/:c' + }.test_match() + + RoutePair{ + url: '/a/b/c' + route: '/:a/b/:c' + }.test_match() + + RoutePair{ + url: '/a/b/c' + route: '/:a/:b/:c' + }.test_match() + + RoutePair{ + url: '/one/two/three' + route: '/:a/:b/:c' + }.test_match() + + RoutePair{ + url: '/one/b/c' + route: '/:a/b/c' + }.test_match() + + RoutePair{ + url: '/one/two/three' + route: '/:a/b/c' + }.test_no_match() + + RoutePair{ + url: '/one/two/three' + route: '/:a/:b/c' + }.test_no_match() + + RoutePair{ + url: '/one/two/three' + route: '/:a/b/:c' + }.test_no_match() + + RoutePair{ + url: '/a/b/c/d' + route: '/:a/:b/:c' + }.test_no_match() + + RoutePair{ + url: '/1/2/3/4' + route: '/:a/:b/:c' + }.test_no_match() + + RoutePair{ + url: '/a/b' + route: '/:a/:b/:c' + }.test_no_match() + + RoutePair{ + url: '/1/2' + route: '/:a/:b/:c' + }.test_no_match() +} + +fn test_route_params() { + RoutePair{ + url: '/a/b/c' + route: '/:a/b/c' + }.test_param(['a']) + + RoutePair{ + url: '/one/b/c' + route: '/:a/b/c' + }.test_param(['one']) + + RoutePair{ + url: '/one/two/c' + route: '/:a/:b/c' + }.test_param(['one', 'two']) + + RoutePair{ + url: '/one/two/three' + route: '/:a/:b/:c' + }.test_param(['one', 'two', 'three']) + + RoutePair{ + url: '/one/b/three' + route: '/:a/b/:c' + }.test_param(['one', 'three']) +} + +fn test_route_params_array_match() { + // array can only be used on the last word (TODO: add parsing / tests to ensure this) + + RoutePair{ + url: '/a/b/c' + route: '/a/b/:c...' + }.test_match() + + RoutePair{ + url: '/a/b/c/d' + route: '/a/b/:c...' + }.test_match() + + RoutePair{ + url: '/a/b/c/d/e' + route: '/a/b/:c...' + }.test_match() + + RoutePair{ + url: '/one/b/c/d/e' + route: '/:a/b/:c...' + }.test_match() + + RoutePair{ + url: '/one/two/c/d/e' + route: '/:a/:b/:c...' + }.test_match() + + RoutePair{ + url: '/one/two/three/four/five' + route: '/:a/:b/:c...' + }.test_match() + + RoutePair{ + url: '/a/b' + route: '/:a/:b/:c...' + }.test_no_match() + + RoutePair{ + url: '/a/b/' + route: '/:a/:b/:c...' + }.test_no_match() +} + +fn test_route_params_array() { + RoutePair{ + url: '/a/b/c' + route: '/a/b/:c...' + }.test_param(['c']) + + RoutePair{ + url: '/a/b/c/d' + route: '/a/b/:c...' + }.test_param(['c/d']) + + RoutePair{ + url: '/a/b/c/d/' + route: '/a/b/:c...' + }.test_param(['c/d']) + + RoutePair{ + url: '/a/b/c/d/e' + route: '/a/b/:c...' + }.test_param(['c/d/e']) + + RoutePair{ + url: '/one/b/c/d/e' + route: '/:a/b/:c...' + }.test_param(['one', 'c/d/e']) + + RoutePair{ + url: '/one/two/c/d/e' + route: '/:a/:b/:c...' + }.test_param(['one', 'two', 'c/d/e']) + + RoutePair{ + url: '/one/two/three/d/e' + route: '/:a/:b/:c...' + }.test_param(['one', 'two', 'three/d/e']) +} diff --git a/vlib/vweb/vweb.v b/vlib/vweb/vweb.v index 2f0be31b7c..734b85ff10 100644 --- a/vlib/vweb/vweb.v +++ b/vlib/vweb/vweb.v @@ -304,7 +304,10 @@ fn handle_conn(mut conn net.TcpConn, mut app T) { } mut reader := io.new_buffered_reader(reader: io.make_reader(conn)) page_gen_start := time.ticks() - req := parse_request(mut reader) or { return } + req := parse_request(mut reader) or { + eprintln('error parsing request: $err') + return + } app.Context = Context{ req: req conn: conn @@ -326,191 +329,166 @@ fn handle_conn(mut conn net.TcpConn, mut app T) { } } // Serve a static file if it is one - // TODO: handle url parameters properly - for now, ignore them - mut static_file_name := app.req.url - // TODO: use urllib methods instead of manually parsing - if static_file_name.contains('?') { - static_file_name = static_file_name.all_before('?') - } - static_file := app.static_files[static_file_name] - mime_type := app.static_mime_types[static_file_name] - if static_file != '' && mime_type != '' { - data := os.read_file(static_file) or { - send_string(mut conn, vweb.http_404) or { } - return - } - app.send_response_to_client(mime_type, data) - unsafe { data.free() } + // TODO: get the real path + url := urllib.parse(app.req.url.to_lower()) or { + eprintln('error parsing path: $err') return } + if serve_static(mut app, url) { + // successfully served a static file + return + } + app.init() // Call the right action $if debug { println('route matching...') } - mut route_words_a := [][]string{} - // TODO: use urllib methods instead of manually parsing - mut url_words := req.url.split('/').filter(it != '') - // Parse URL query - if url_words.len > 0 && url_words.last().contains('?') { - words := url_words.last().after('?').split('&') - tmp_query := words.map(it.split('=')) - url_words[url_words.len - 1] = url_words.last().all_before('?') - for data in tmp_query { - if data.len == 2 { - app.query[data[0]] = data[1] - } - } + url_words := url.path.split('/').filter(it != '') + // copy query args to app.query + for k, v in url.query().data { + app.query[k] = v.data[0] } - mut vars := []string{cap: route_words_a.len} - mut action := '' + $for method in T.methods { $if method.return_type is Result { - attrs := method.attrs - route_words_a = [][]string{} - // Get methods - // Get is default - mut req_method_str := '$req.method' - if req.method == .post { - if 'post' in attrs { - route_words_a = attrs.filter(it.to_lower() != 'post').map(it[1..].split('/')) - } - } else if req.method == .put { - if 'put' in attrs { - route_words_a = attrs.filter(it.to_lower() != 'put').map(it[1..].split('/')) - } - } else if req.method == .patch { - if 'patch' in attrs { - route_words_a = attrs.filter(it.to_lower() != 'patch').map(it[1..].split('/')) - } - } else if req.method == .delete { - if 'delete' in attrs { - route_words_a = attrs.filter(it.to_lower() != 'delete').map(it[1..].split('/')) - } - } else if req.method == .head { - if 'head' in attrs { - route_words_a = attrs.filter(it.to_lower() != 'head').map(it[1..].split('/')) - } - } else if req.method == .options { - if 'options' in attrs { - route_words_a = attrs.filter(it.to_lower() != 'options').map(it[1..].split('/')) - } - } else { - route_words_a = attrs.filter(it.to_lower() != 'get').map(it[1..].split('/')) + mut method_args := []string{} + // TODO: move to server start + http_methods, route_path := parse_attrs(method.name, method.attrs) or { + eprintln('error parsing method attributes: $err') + return } - if attrs.len == 0 || (attrs.len == 1 && route_words_a.len == 0) { - if url_words.len > 0 { - // No routing for this method. If it matches, call it and finish matching - // since such methods have a priority. - // For example URL `/register` matches route `/:user`, but `fn register()` - // should be called first. - if (req_method_str == '' && url_words[0] == method.name && url_words.len == 1) - || (req_method_str == req.method.str() && url_words[0] == method.name - && url_words.len == 1) { - $if debug { - println('easy match method=$method.name') - } - app.$method(vars) - return - } - } else if method.name == 'index' { - // handle / to .index() - $if debug { - println('route to .index()') - } - app.$method(vars) + // Used for route matching + route_words := route_path.split('/').filter(it != '') + // Skip if the HTTP request method does not match the attributes + if app.req.method in http_methods { + // Route immediate matches first + // For example URL `/register` matches route `/:user`, but `fn register()` + // should be called first. + if !route_path.contains('/:') && url_words == route_words { + // We found a match + app.$method(method_args) return } - } else { - mut req_method := []string{} - if route_words_a.len > 0 { - for route_words_ in route_words_a { - // cannot move to line initialize line because of C error with map(it.filter(it != '')) - route_words := route_words_.filter(it != '') - if route_words.len == 1 && route_words[0] in vweb.methods_without_first { - req_method << route_words[0] - } - if url_words.len == route_words.len || (url_words.len >= route_words.len - 1 - && route_words.len > 0 && route_words.last().ends_with('...')) { - if req_method.len > 0 { - if req_method_str.to_lower()[1..] !in req_method { - continue - } - } - // match `/:user/:repo/tree` to `/vlang/v/tree` - mut matching := false - mut unknown := false - mut variables := []string{cap: route_words.len} - if route_words.len == 0 && url_words.len == 0 { - // index route - matching = true - } - for i in 0 .. route_words.len { - if url_words.len == i { - variables << '' - matching = true - unknown = true - break - } - if url_words[i] == route_words[i] { - // no parameter - matching = true - continue - } else if route_words[i].starts_with(':') { - // is parameter - if i < route_words.len && !route_words[i].ends_with('...') { - // normal parameter - variables << url_words[i] - } else { - // array parameter only in the end - variables << url_words[i..].join('/') - } - matching = true - unknown = true - continue - } else { - matching = false - break - } - } - if matching && !unknown { - // absolute router words like `/test/site` - app.$method(vars) - return - } else if matching && unknown { - // router words with paramter like `/:test/site` - action = method.name - vars = variables.clone() - } - req_method = []string{} - } + if url_words.len == 0 && route_words == ['index'] && method.name == 'index' { + app.$method(method_args) + return + } + + if params := route_matches(url_words, route_words) { + method_args = params.clone() + if method_args.len != method.args.len { + eprintln('warning: uneven parameters count ($method.args.len) in `$method.name`, compared to the vweb route `$method.attrs` ($method_args.len)') } + app.$method(method_args) + return } } } } - if action == '' { - // site not found - send_string(mut conn, vweb.http_404) or { } - return + // site not found + send_string(mut conn, vweb.http_404) or { } +} + +fn route_matches(url_words []string, route_words []string) ?[]string { + // URL path should be at least as long as the route path + if url_words.len < route_words.len { + return none } - $for method in T.methods { - $if method.return_type is Result { - // search again for method - if action == method.name && method.attrs.len > 0 { - // call action method - if method.args.len == vars.len { - app.$method(vars) - return - } else { - eprintln('warning: uneven parameters count ($method.args.len) in `$method.name`, compared to the vweb route `$method.attrs` ($vars.len)') - } + + mut params := []string{cap: url_words.len} + if url_words.len == route_words.len { + for i in 0 .. url_words.len { + if route_words[i].starts_with(':') { + // We found a path paramater + params << url_words[i] + } else if route_words[i] != url_words[i] { + // This url does not match the route + return none } } + return params } + + // The last route can end with ... indicating an array + if !route_words[route_words.len - 1].ends_with('...') { + return none + } + + for i in 0 .. route_words.len - 1 { + if route_words[i].starts_with(':') { + // We found a path paramater + params << url_words[i] + } else if route_words[i] != url_words[i] { + // This url does not match the route + return none + } + } + params << url_words[route_words.len - 1..url_words.len].join('/') + return params +} + +// parse function attribute list for methods and a path +fn parse_attrs(name string, attrs []string) ?([]http.Method, string) { + if attrs.len == 0 { + return [http.Method.get], '/$name' + } + + mut x := attrs.clone() + mut methods := []http.Method{} + mut path := '' + + for i := 0; i < x.len; { + attr := x[i] + attru := attr.to_upper() + m := http.method_from_str(attru) + if attru == 'GET' || m != .get { + methods << m + x.delete(i) + continue + } + if attr.starts_with('/') { + if path != '' { + return error('Expected at most one path attribute') + } + path = attr + x.delete(i) + continue + } + i++ + } + if x.len > 0 { + return error('Encountered unexpected extra attributes: $x') + } + if methods.len == 0 { + methods = [http.Method.get] + } + if path == '' { + path = '/$name' + } + // Make path lowercase for case-insensitive comparisons + return methods, path.to_lower() +} + +// check if request is for a static file and serves it +// returns true if we served a static file, false otherwise +fn serve_static(mut app T, url urllib.URL) bool { + // TODO: handle url parameters properly - for now, ignore them + static_file := app.static_files[url.path] + mime_type := app.static_mime_types[url.path] + if static_file == '' || mime_type == '' { + return false + } + data := os.read_file(static_file) or { + send_string(mut app.conn, vweb.http_404) or { } + return true + } + app.send_response_to_client(mime_type, data) + unsafe { data.free() } + return true } // vweb intern function