Type mismatch in AppendUint

[gaby]

@erikdubbelboer We were implementing some changes in Fiber and noticed that AppendUint takes an int instead of an uint. We also noticed there's no AppendInt available.

In several parts of Fasthttp when a value below 0 is passed it throws a panic, instead of enforcing this by using uint.

Is this a bug, or was intended? Seems the code was added a long time ago

Affected code: https://github.com/valyala/fasthttp/blob/master/bytesconv.go#L124

Would it make sense to make the param uint and adding another function named AppendInt to fasthttp?

[erikdubbelboer]

This is from before I started maintaining this lib, so I don't know why this was done. And of course we can't change this without breaking backwards compatibility. So I'm afraid we are stuck with this behaviour.

[gaby]

@erikdubbelboer Thanks! We will fix it on our side then.

[ReneWerner87]

@erikdubbelboer we would have another function which can then also process negative values and we would adapt the other one so that it does not cause a breaking change

would this change be acceptable

// AppendUint appends n to dst and returns the extended dst.
func AppendUint(dst []byte, n int) []byte {
	if n < 0 {
		// developer sanity-check
		panic("BUG: int must be positive")
	}

	return AppendInt(dst, n)
}

func AppendInt(dst []byte, n int) []byte {
	isNegative := n < 0
	if isNegative {
		n = -n
	}

	var b [20]byte
	buf := b[:]
	i := len(buf)
	var q int
	for n >= 10 {
		i--
		q = n / 10
		buf[i] = '0' + byte(n-q*10)
		n = q
	}
	i--
	buf[i] = '0' + byte(n)

	dst = append(dst, buf[i:]...)
	if isNegative {
		// add '-' in front of the number
		dst = append(dst[:1], dst...)
		dst[0] = '-'
	}

	return dst
}

we will then benchmark and optimize the code, it's just a first draft for now

[erikdubbelboer]

That could work. What is the exact reason you need this? Why not add the AppendInt function to Fiber itself.

[gaby]

@erikdubbelboer We use the Append functions from fasthttp in the logger middleware. When we append Content-Length it returns -1 when streaming, this causes AppendUint to throw a panic. That's when we realized the mismatch of types between name and params

[ReneWerner87]

But there are other places where you could use this function and we wanted to make it possible to use it within fasthttp as well

[ReneWerner87]

the appendInt of strconv is almost as fast as your function
I noticed this after I created a clone with negative values
gofiber/utils#90 (comment)

https://pkg.go.dev/strconv#AppendInt

[erikdubbelboer]

That doesn't surprise me. I wouldn't mind a pull that just replaces the fasthttp implementation with AppendInt.

[gaby]

@erikdubbelboer While keeping the panic for negative values? I can do one.

[erikdubbelboer]

Yeah lets keep the panic for now.

[gaby]

Ok, I will do the PR today.

[gaby]

@erikdubbelboer PR submitted