feature/query-and-error-handler #1
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "vickodev/web:feature/query-and-error-handler"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Hi,
This looks really good! Let me quickly take a look at the details.
@ -21,2 +21,4 @@
QueryParam(name string) string
}
type QueryParams 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
).@ -70,1 +74,4 @@
// QueryParam retrieves a parameter.
func (req *request) QueryParam(name string) string {
if value, exists := req.queryParams[name]; exists {
Minor nitpick: I personally don't like these "do 2 things in 1 line" constructs.
@ -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))
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".
@ -225,1 +234,4 @@
// parseQueryParams parses the query string into a slice of parameters.
func parseQueryParams(query string) QueryParams {
params := make(QueryParams)
Multiple options here:
nil
value would need to be handled then.newContext
, that way we can reuse the existing memory. Use clear when finishing the request.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.@ -226,0 +240,4 @@
return params
}
pairs := strings.Split(query, "&")
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).@ -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 != "" {
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.
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:
The advantages of this approach are:
Let me know which of this approach is a better implementation for you and I'll update the PR.
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 withy
.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.
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.
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
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.
b3af477793
to13051c7ae9
Hi,
there are a couple things that still need to be done.
https://url.spec.whatwg.org/#urlencoded-parsing
query = strings.ReplaceAll(query, "+", " ")
before parsing the query.for pair := strings.SplitSeq(query, "&")
.strings.IndexByte(pair, '=')
.=
is missing) as mentioned in the document.trimSpaceInPlace
is not mentioned in the document, do you have any reason to include this? I think it should be removed.@ed wrote in #1 (comment):
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:
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.
Getting closer to merging. A few more things:
//
, the code is self-explanatory.For example, to combine the last 6 commits into 1 commit:
The
-S
is for signing. How to create and register a key:98e9e3e19b
to80a670d5bf
@ed wrote in #1 (comment):
Hey good man, regarding to:
//
, the code is self-explanatory.I didn't find it, but let me know if you see it in the code.
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. 💡
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 myselfb) don't merge the PR yet and I'll wait for you to add
type Query string
here@ed wrote in #1 (comment):
b. Man, give me a few minutes!!
80a670d5bf
toe7a807c28b
Thank you!