feature/query-and-error-handler #1

Merged
ed merged 1 commit from vickodev/web:feature/query-and-error-handler into main 2025-07-17 17:12:13 +00:00
Contributor
  • Query params support was added
  • Custom error handler option was added
- Query params support was added - Custom error handler option was added
requested review from ed 2025-07-13 04:52:59 +00:00
Owner

Hi,

This looks really good! Let me quickly take a look at the details.

Hi, This looks really good! Let me quickly take a look at the details.
ed requested changes 2025-07-13 08:45:52 +00:00
Request.go Outdated
@ -21,2 +21,4 @@
QueryParam(name string) string
}
type QueryParams map[string]string
Owner

This doesn't need to be exported if we have an interface function to access them.

The whole type alias could probably be removed because I can't see a reason to alias it (the map[string]string).

This doesn't need to be exported if we have an interface function to access them. The whole type alias could probably be removed because I can't see a reason to alias it (the `map[string]string`).
vickodev marked this conversation as resolved
Request.go Outdated
@ -70,1 +74,4 @@
// QueryParam retrieves a parameter.
func (req *request) QueryParam(name string) string {
if value, exists := req.queryParams[name]; exists {
Owner

Minor nitpick: I personally don't like these "do 2 things in 1 line" constructs.

Minor nitpick: I personally don't like these "do 2 things in 1 line" constructs.
vickodev marked this conversation as resolved
Server.go Outdated
@ -23,6 +23,7 @@ type Server interface {
Router() *router.Router[Handler]
Run(address string) error
Use(handlers ...Handler)
UseErrorHandler(handler func(ctx Context, err error))
Owner

Since I've used the term "Use" above as an append-style function, I think the term "Set" would fit better here because we're not appending to a list of error handlers (though that could be an interesting implementation).

Therefore the full name I'm suggesting, if we keep the single handler, is "SetErrorHandler".

Since I've used the term "Use" above as an append-style function, I think the term "Set" would fit better here because we're not appending to a list of error handlers (though that could be an interesting implementation). Therefore the full name I'm suggesting, if we keep the single handler, is "SetErrorHandler".
vickodev marked this conversation as resolved
Server.go Outdated
@ -225,1 +234,4 @@
// parseQueryParams parses the query string into a slice of parameters.
func parseQueryParams(query string) QueryParams {
params := make(QueryParams)
Owner

Multiple options here:

  1. This memory allocation can be avoided for requests that don't have queries. The nil value would need to be handled then.
  2. Always allocate the map when we request a new object from the pool in Server.go newContext, that way we can reuse the existing memory. Use clear when finishing the request.
  3. Clearing a map and clearing a slice (a = a[:0]) could be compared for performance. Their speed in object retrieval is honestly not that different for the number of elements they're usually used for here. If slices provide better performance, switch to slices.
Multiple options here: 1. This memory allocation can be avoided for requests that don't have queries. The `nil` value would need to be handled then. 2. Always allocate the map when we request a new object from the pool in Server.go `newContext`, that way we can reuse the existing memory. Use [clear](https://tip.golang.org/ref/spec#Clear) when finishing the request. 3. Clearing a map and clearing a slice (`a = a[:0]`) could be compared for performance. Their speed in object retrieval is honestly not that different for the number of elements they're usually used for here. If slices provide better performance, switch to slices.
vickodev marked this conversation as resolved
Server.go Outdated
@ -226,0 +240,4 @@
return params
}
pairs := strings.Split(query, "&")
Owner

This is a memory allocation. Avoid at all costs. It's also trivial to rewrite it to a non-allocating algorithm.

This could potentially be a good fit for the new generators that Go offers now (yield on every substring).

This is a memory allocation. Avoid at all costs. It's also trivial to rewrite it to a non-allocating algorithm. This could potentially be a good fit for the new generators that Go offers now (`yield` on every substring).
vickodev marked this conversation as resolved
Server.go Outdated
@ -227,3 +264,4 @@
func (s *server) handleRequest(ctx *context, method string, url string, writer io.Writer) {
ctx.method = method
ctx.scheme, ctx.host, ctx.path, ctx.query = parseURL(url)
if ctx.query != "" {
Owner

This is an extremely hot function so I don't like that branch here.

The condition is also handled inside the function. Either we keep the check here and remove it in the function (can save 1 call) or we apply any of the alternative options mentioned earlier.

This is an extremely hot function so I don't like that branch here. The condition is also handled inside the function. Either we keep the check here and remove it in the function (can save 1 call) or we apply any of the alternative options mentioned earlier.
Author
Contributor

Hey bro thanks for your advices. I understand what you mean, so I moved all the implementation about QueryParams to the Request file as follow:

// Request.go
// parseQueryParams parses the query string into a slice of parameters.
func parseQueryParams(query string, params map[string]string) {
	start := 0
	for i := 0; i <= len(query); i++ {
		if i == len(query) || query[i] == '&' {
			if i > start {
				pair := query[start:i]
				if pair != "" {
					parts := strings.SplitN(pair, "=", 2)
					if len(parts) == 2 {
						key := strings.TrimSpace(parts[0])
						value := strings.TrimSpace(parts[1])
						params[key] = value
					}
				}
			}
			start = i + 1
		}
	}
}

// QueryParam retrieves a parameter.
func (req *request) QueryParam(name string) string {
	if req.query == "" {
		return ""
	}

	if req.queryParams == nil {
		req.queryParams = make(map[string]string)
		parseQueryParams(req.query, req.queryParams)
	}

	value, exists := req.queryParams[name]
	if exists {
		return value
	}

	return ""
}

The advantages of this approach are:

  1. Avoid hot functions in handler request.
  2. Only if user request the value, and query has data, the query params map is going to be builded (Once)
  3. By other hand, let me know if is better option manage the query params like the Path Params using the type router.Parameter, I mean something like this:
func parseQueryParams(query string) []router.Parameter {
	params := make([]router.Parameter, 0, 8)
	pairs := strings.Split(query, "&")

	for _, pair := range pairs {
		if pair == "" {
			continue
		}

		parts := strings.SplitN(pair, "=", 2)
		if len(parts) != 2 {
			continue
		}

		params = append(params, router.Parameter{
			Key:   parts[0],
			Value: parts[1],
		})
	}

	return params
}
  1. Or maybe you'll prefer this approach that avoid manage queryParams value in memory
// Request.go
func getQueryParams(query string, name string) (string, map[string]string) {
	// Pre-allocate map with estimated capacity
	params := make(map[string]string, 8)
	paramValue := ""

	start := 0
	for i := 0; i <= len(query); i++ {
		if i == len(query) || query[i] == '&' {
			if i > start {
				// Find the '=' separator
				eqPos := -1
				for j := start; j < i; j++ {
					if query[j] == '=' {
						eqPos = j
						break
					}
				}

				if eqPos != -1 && eqPos > start && eqPos < i-1 {
					key := strings.TrimSpace(query[start:eqPos])
					value := strings.TrimSpace(query[eqPos+1 : i])

					if key == name {
						paramValue = value
					} else {
						params[key] = value
					}
				}
			}
			start = i + 1
		}
	}

	return paramValue, params
}

// QueryParam retrieves a parameter.
func (req *request) QueryParam(name string) (string, map[string]string) {
	if req.query == "" {
		return "", nil
	}

	return getQueryParams(req.query, name)
}

// In some request 
        myParam1, queryParams := ctx.Request().QueryParam("myParam1")
		myParam2 := queryParams["myParam2"]
		myEmptyValue := queryParams["noExistingKey"]

Let me know which of this approach is a better implementation for you and I'll update the PR.

Hey bro thanks for your advices. I understand what you mean, so I moved all the implementation about QueryParams to the Request file as follow: ```go // Request.go // parseQueryParams parses the query string into a slice of parameters. func parseQueryParams(query string, params map[string]string) { start := 0 for i := 0; i <= len(query); i++ { if i == len(query) || query[i] == '&' { if i > start { pair := query[start:i] if pair != "" { parts := strings.SplitN(pair, "=", 2) if len(parts) == 2 { key := strings.TrimSpace(parts[0]) value := strings.TrimSpace(parts[1]) params[key] = value } } } start = i + 1 } } } // QueryParam retrieves a parameter. func (req *request) QueryParam(name string) string { if req.query == "" { return "" } if req.queryParams == nil { req.queryParams = make(map[string]string) parseQueryParams(req.query, req.queryParams) } value, exists := req.queryParams[name] if exists { return value } return "" } ``` The advantages of this approach are: 1. Avoid hot functions in handler request. 2. Only if user request the value, and query has data, the query params map is going to be builded (Once) 3. By other hand, let me know if is better option manage the query params like the Path Params using the type router.Parameter, I mean something like this: ```go func parseQueryParams(query string) []router.Parameter { params := make([]router.Parameter, 0, 8) pairs := strings.Split(query, "&") for _, pair := range pairs { if pair == "" { continue } parts := strings.SplitN(pair, "=", 2) if len(parts) != 2 { continue } params = append(params, router.Parameter{ Key: parts[0], Value: parts[1], }) } return params } ``` 4. Or maybe you'll prefer this approach that avoid manage queryParams value in memory ```go // Request.go func getQueryParams(query string, name string) (string, map[string]string) { // Pre-allocate map with estimated capacity params := make(map[string]string, 8) paramValue := "" start := 0 for i := 0; i <= len(query); i++ { if i == len(query) || query[i] == '&' { if i > start { // Find the '=' separator eqPos := -1 for j := start; j < i; j++ { if query[j] == '=' { eqPos = j break } } if eqPos != -1 && eqPos > start && eqPos < i-1 { key := strings.TrimSpace(query[start:eqPos]) value := strings.TrimSpace(query[eqPos+1 : i]) if key == name { paramValue = value } else { params[key] = value } } } start = i + 1 } } return paramValue, params } // QueryParam retrieves a parameter. func (req *request) QueryParam(name string) (string, map[string]string) { if req.query == "" { return "", nil } return getQueryParams(req.query, name) } // In some request myParam1, queryParams := ctx.Request().QueryParam("myParam1") myParam2 := queryParams["myParam2"] myEmptyValue := queryParams["noExistingKey"] ``` Let me know which of this approach is a better implementation for you and I'll update the PR.
Owner

It's almost what I was thinking of.

I was thinking we don't need to build the map at all.
If the user requests x we can just iterate over all query params and only return the value for x.
If the user requests y we iterate again and do the same with y.

Usually the number of parameters on a website is pretty low so I think this approach works better because it avoids memory allocations completely.

Bonus points for using SplitSeq instead of Split.

It's _almost_ what I was thinking of. I was thinking we don't need to build the map at all. If the user requests `x` we can just iterate over all query params and only return the value for x. If the user requests `y` we iterate again and do the same with `y`. Usually the number of parameters on a website is pretty low so I think this approach works better because it avoids memory allocations completely. Bonus points for using [SplitSeq](https://pkg.go.dev/strings#SplitSeq) instead of Split.
vickodev marked this conversation as resolved
ed self-assigned this 2025-07-13 08:55:06 +00:00
Owner

Idea / food for thought:

We can probably combine the best of all options and just make a generator for all query params.

The generator would avoid a) memory allocations when queries are not present and b) even avoid memory allocations in cases where queries exist because they are "lazily parsed" just-in-time, when the generator runs.

Idea / food for thought: We can probably combine the best of all options and just make a generator for all query params. The generator would avoid a) memory allocations when queries are not present and b) even avoid memory allocations in cases where queries exist because they are "lazily parsed" just-in-time, when the generator runs.
Owner
I just saw that this work has already been done: https://cs.opensource.google/go/go/+/refs/tags/go1.24.5:src/strings/iter.go;l=70-76
Author
Contributor

Hey bro, I tryed to do my best, I improved the performance to get the query params value in a good manner, so feel free to make suggestions.

Hey bro, I tryed to do my best, I improved the performance to get the query params value in a good manner, so feel free to make suggestions.
vickodev force-pushed feature/query-and-error-handler from b3af477793 to 13051c7ae9 2025-07-14 14:38:31 +00:00 Compare
[CHANGE]: fix documentation about SetErrorHandler option
All checks were successful
/ test (pull_request) Successful in 21s
0fbc84cc28
Owner

Hi,

there are a couple things that still need to be done.

  1. I've looked up the official WhatWG algorithm, please read it carefully:
    https://url.spec.whatwg.org/#urlencoded-parsing
  2. Use query = strings.ReplaceAll(query, "+", " ") before parsing the query.
  3. Use the standard library strings.SplitSeq for pair := strings.SplitSeq(query, "&") .
  4. Avoid manual search. Use strings.IndexByte strings.IndexByte(pair, '=').
  5. Handle the edge cases (name is missing or = is missing) as mentioned in the document.
  6. trimSpaceInPlace is not mentioned in the document, do you have any reason to include this? I think it should be removed.
Hi, there are a couple things that still need to be done. 1. I've looked up the official WhatWG algorithm, please read it carefully: https://url.spec.whatwg.org/#urlencoded-parsing 2. Use `query = strings.ReplaceAll(query, "+", " ")` before parsing the query. 3. Use the standard library [strings.SplitSeq](https://pkg.go.dev/strings#SplitSeq) `for pair := strings.SplitSeq(query, "&")` . 4. Avoid manual search. Use [strings.IndexByte](https://pkg.go.dev/strings#IndexByte) `strings.IndexByte(pair, '=')`. 5. Handle the edge cases (name is missing or `=` is missing) as mentioned in the document. 4. `trimSpaceInPlace` is not mentioned in the document, do you have any reason to include this? I think it should be removed.
Author
Contributor

@ed wrote in #1 (comment):

Hi,

there are a couple things that still need to be done.

  1. I've looked up the official WhatWG algorithm, please read it carefully:
    https://url.spec.whatwg.org/#urlencoded-parsing
  2. Use query = strings.ReplaceAll(query, "+", " ") before parsing the query.
  3. Use the standard library strings.SplitSeq for pair := strings.SplitSeq(query, "&") .
  4. Avoid manual search. Use strings.IndexByte strings.IndexByte(pair, '=').
  5. Handle the edge cases (name is missing or = is missing) as mentioned in the document.
  6. trimSpaceInPlace is not mentioned in the document, do you have any reason to include this? I think it should be removed.

Hey man, I'm learning a lot of things with this shore!!

Regarding point 1, nice resource, tk!! 🚀🙌

Regarding point 6, I don't know the answer, but I guess it's because of development errors when building the structure of the queryParams in the url, it can happen, it's the reality! 🥲
For example:

response := s.Request("GET", "/search?query-one=go&query-two=lang\x20", nil, nil)
assert.Equal(t, string(response.Body()), "golang") // Fail
assert.Equal(t, string(response.Body()), "golang ") // Pass

I believe this trimSpace feature is to help developers, but let me know what do you think about!! 👀

Hey bro, I follow your advices so let me know if this implementation words for you.

// getQueryParam get the value from the query string and return it when it's found.
func getQueryParam(query string, name string) string {
	query = strings.ReplaceAll(query, "+", " ")

	for pair := range strings.SplitSeq(query, "&") {
		if pair == "" {
			continue
		}

		var keyPart, valuePart string = pair, "" 
        // With the previous line, I avoid to create and else after `if eqPos != -1` to set keyPart with pair, like this: 
        // } else {
        //     // If = is not found, name has value of bytes, value is empty
        //     keyPart = pair
        //     valuePart = ""
        // }

		eqPos := strings.IndexByte(pair, '=')
		if eqPos != -1 {
			keyPart = pair[:eqPos]
			valuePart = pair[eqPos+1:]
		}

		if keyPart == name {
			return valuePart
		}
	}

	return ""
}

// QueryParam retrieves a parameter.
func (req *request) QueryParam(name string) string {
	if req.query == "" {
		return ""
	}

	return getQueryParam(req.query, name)
}
@ed wrote in https://git.urbach.dev/go/web/pulls/1#issuecomment-21: > Hi, > > there are a couple things that still need to be done. > > 1. I've looked up the official WhatWG algorithm, please read it carefully: > https://url.spec.whatwg.org/#urlencoded-parsing > 2. Use `query = strings.ReplaceAll(query, "+", " ")` before parsing the query. > 3. Use the standard library [strings.SplitSeq](https://pkg.go.dev/strings#SplitSeq) `for pair := strings.SplitSeq(query, "&")` . > 4. Avoid manual search. Use [strings.IndexByte](https://pkg.go.dev/strings#IndexByte) `strings.IndexByte(pair, '=')`. > 5. Handle the edge cases (name is missing or `=` is missing) as mentioned in the document. > 6. `trimSpaceInPlace` is not mentioned in the document, do you have any reason to include this? I think it should be removed. Hey man, I'm learning a lot of things with this shore!! Regarding point 1, nice resource, tk!! 🚀🙌 Regarding point 6, I don't know the answer, but I guess it's because of development errors when building the structure of the queryParams in the url, it can happen, it's the reality! 🥲 For example: ```go response := s.Request("GET", "/search?query-one=go&query-two=lang\x20", nil, nil) assert.Equal(t, string(response.Body()), "golang") // Fail assert.Equal(t, string(response.Body()), "golang ") // Pass ``` I believe this trimSpace feature is to help developers, but let me know what do you think about!! 👀 Hey bro, I follow your advices so let me know if this implementation words for you. ```go // getQueryParam get the value from the query string and return it when it's found. func getQueryParam(query string, name string) string { query = strings.ReplaceAll(query, "+", " ") for pair := range strings.SplitSeq(query, "&") { if pair == "" { continue } var keyPart, valuePart string = pair, "" // With the previous line, I avoid to create and else after `if eqPos != -1` to set keyPart with pair, like this: // } else { // // If = is not found, name has value of bytes, value is empty // keyPart = pair // valuePart = "" // } eqPos := strings.IndexByte(pair, '=') if eqPos != -1 { keyPart = pair[:eqPos] valuePart = pair[eqPos+1:] } if keyPart == name { return valuePart } } return "" } // QueryParam retrieves a parameter. func (req *request) QueryParam(name string) string { if req.query == "" { return "" } return getQueryParam(req.query, name) } ```
[UPDATE]: getQueryParam function was simplified and improved
All checks were successful
/ test (pull_request) Successful in 18s
98e9e3e19b
Owner

Getting closer to merging. A few more things:

  1. Remove the comments starting with //, the code is self-explanatory.
  2. Combine all commits into a single commit.
  3. Call it "Added a method to access query parameters" so it fits to the rest of the git log.
  4. Sign your commit.

For example, to combine the last 6 commits into 1 commit:

git reset --soft HEAD~6
git commit -S -m "Added a method to access query parameters"

The -S is for signing. How to create and register a key:


Note

For the final version, I will probably use Query() to return a custom string-based type with a Param(string) method.

type Query string
func (q Query) Param(name string) string

// in the request interface:
Query() Query

// for the user
req.Query().Param("q1")

Let me know if this would be interesting to work on for you..?
If you just want to finish the PR with the current code that's perfectly fine.
We can do the interface changes later.

Getting closer to merging. A few more things: 1. Remove the comments starting with `//`, the code is self-explanatory. 2. Combine all commits into a **single commit**. 3. Call it "Added a method to access query parameters" so it fits to the rest of the git log. 4. Sign your commit. For example, to combine the last 6 commits into 1 commit: ```shell git reset --soft HEAD~6 git commit -S -m "Added a method to access query parameters" ``` The `-S` is for signing. How to create and register a key: - https://docs.github.com/en/authentication/managing-commit-signature-verification/generating-a-new-gpg-key - https://git.urbach.dev/user/settings/keys --- > [!NOTE] > For the final version, I will probably use `Query()` to return a custom string-based type with a `Param(string)` method. > ``` > type Query string > func (q Query) Param(name string) string > > // in the request interface: > Query() Query > > // for the user > req.Query().Param("q1") > ``` > > Let me know if this would be interesting to work on for you..? > If you just want to finish the PR with the current code that's perfectly fine. > We can do the interface changes later.
vickodev force-pushed feature/query-and-error-handler from 98e9e3e19b to 80a670d5bf 2025-07-17 13:39:21 +00:00 Compare
Author
Contributor

@ed wrote in #1 (comment):

Getting closer to merging. A few more things:

  1. Remove the comments starting with //, the code is self-explanatory.
  2. Combine all commits into a single commit.
  3. Call it "Added a method to access query parameters" so it fits to the rest of the git log.
  4. Sign your commit.

For example, to combine the last 6 commits into 1 commit:

git reset --soft HEAD~6
git commit -S -m "Added a method to access query parameters"

The -S is for signing. How to create and register a key:

Note

For the final version, I will probably use Query() to return a custom string-based type with a Param(string) method.

type Query string
func (q Query) Param(name string) string

// in the request interface:
Query() Query

// for the user
req.Query().Param("q1")

Let me know if this would be interesting to work on for you..?
If you just want to finish the PR with the current code that's perfectly fine.
We can do the interface changes later.

Hey good man, regarding to:

  1. Remove the comments starting with //, the code is self-explanatory.
    I didn't find it, but let me know if you see it in the code.
  2. Let me know if this would be interesting to work on for you..?
    Yes, I want to work on it. 🚀

By other hand, I want to implement other features in order to improve the web server, but let me organize the ideas and then I'm going to create a discussion with you about that. 💡

@ed wrote in https://git.urbach.dev/go/web/pulls/1#issuecomment-24: > Getting closer to merging. A few more things: > > 1. Remove the comments starting with `//`, the code is self-explanatory. > 2. Combine all commits into a **single commit**. > 3. Call it "Added a method to access query parameters" so it fits to the rest of the git log. > 4. Sign your commit. > > For example, to combine the last 6 commits into 1 commit: > > ```shell > git reset --soft HEAD~6 > git commit -S -m "Added a method to access query parameters" > ``` > > The `-S` is for signing. How to create and register a key: > > * https://docs.github.com/en/authentication/managing-commit-signature-verification/generating-a-new-gpg-key > * https://git.urbach.dev/user/settings/keys > > > **Note** > > For the final version, I will probably use `Query()` to return a custom string-based type with a `Param(string)` method. > > ```text > > type Query string > > func (q Query) Param(name string) string > > > > // in the request interface: > > Query() Query > > > > // for the user > > req.Query().Param("q1") > > ``` > > > > Let me know if this would be interesting to work on for you..? > > If you just want to finish the PR with the current code that's perfectly fine. > > We can do the interface changes later. Hey good man, regarding to: 1. Remove the comments starting with `//`, the code is self-explanatory. I didn't find it, but let me know if you see it in the code. 2. Let me know if this would be interesting to work on for you..? Yes, I want to work on it. 🚀 By other hand, I want to implement other features in order to improve the web server, but let me organize the ideas and then I'm going to create a discussion with you about that. 💡
Owner

Heya,

we have 2 options, let me know what you'd prefer - any option is fine!

a) merge the PR and I'll add type Query string by myself
b) don't merge the PR yet and I'll wait for you to add type Query string here

Heya, we have 2 options, let me know what you'd prefer - any option is fine! a) merge the PR and I'll add `type Query string` by myself b) don't merge the PR yet and I'll wait for you to add `type Query string` here
Author
Contributor

@ed wrote in #1 (comment):

Heya,

we have 2 options, let me know what you'd prefer - any option is fine!

a) merge the PR and I'll add type Query string by myself b) don't merge the PR yet and I'll wait for you to add type Query string here

b. Man, give me a few minutes!!

@ed wrote in https://git.urbach.dev/go/web/pulls/1#issuecomment-27: > Heya, > > we have 2 options, let me know what you'd prefer - any option is fine! > > a) merge the PR and I'll add `type Query string` by myself b) don't merge the PR yet and I'll wait for you to add `type Query string` here b. Man, give me a few minutes!!
vickodev force-pushed feature/query-and-error-handler from 80a670d5bf to e7a807c28b 2025-07-17 14:45:05 +00:00 Compare
ed merged commit e7a807c28b into main 2025-07-17 17:12:13 +00:00
ed deleted branch feature/query-and-error-handler 2025-07-17 17:12:13 +00:00
Owner

Thank you!

Thank you!
Sign in to join this conversation.
No reviewers
ed
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: go/web#1
No description provided.