gin icon indicating copy to clipboard operation
gin copied to clipboard

refactor(context): refactor `Keys` type to `map[any]any`

Open flc1125 opened this issue 1 year ago • 11 comments

  • ⚠️ Destructive adjustments require evaluation.
  • Enable Keys to support more types.
package main

import "github.com/gin-gonic/gin"

func main() {
	var ctx *gin.Context
	type key struct{}
	
	
	ctx.Set(111, "111")
	ctx.Get(111)
	ctx.Get(key{})
	ctx.Get("111")
	// ……
}

The first step in solving https://github.com/gin-gonic/gin/issues/1123#issuecomment-876642837

flc1125 avatar May 10 '24 13:05 flc1125

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.05%. Comparing base (3dc1cd6) to head (0336111). Report is 59 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3963      +/-   ##
==========================================
- Coverage   99.21%   99.05%   -0.16%     
==========================================
  Files          42       44       +2     
  Lines        3182     2751     -431     
==========================================
- Hits         3157     2725     -432     
+ Misses         17       15       -2     
- Partials        8       11       +3     
Flag Coverage Δ
?
-tags "sonic avx" 99.04% <100.00%> (?)
-tags go_json 99.04% <100.00%> (?)
-tags nomsgpack 99.03% <100.00%> (?)
go-1.18 ?
go-1.19 ?
go-1.20 ?
go-1.21 99.05% <100.00%> (-0.16%) :arrow_down:
go-1.22 99.05% <100.00%> (?)
macos-latest 99.05% <100.00%> (-0.16%) :arrow_down:
ubuntu-latest 99.05% <100.00%> (-0.16%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 10 '24 13:05 codecov[bot]

Why not just make it comparable, or just any?

jarrodhroberson avatar May 14 '24 14:05 jarrodhroberson

Why not just make it comparable, or just any?

I think it's just like the key in context.WithValue(ctx, key, value).

It can be supported, so why not?

Moreover, when the key is set to struct{}, it saves more memory, ex:

type customKey struct{}

ctx.Set(customKey{}, "xxx")

package main

import (
	"fmt"
	"unsafe"
)

func main() {
	var a int
	var b string
	var c struct{}

	fmt.Println("a:", unsafe.Sizeof(a))
	fmt.Println("b:", unsafe.Sizeof(b))
	fmt.Println("c:", unsafe.Sizeof(c))
}

output:

a: 8
b: 16
c: 0

flc1125 avatar May 14 '24 15:05 flc1125

In Golang, maps can only have keys of types that are considered comparable. This means the type needs to support the == and != operators for comparison.

which means that structs have to have nothing but comparable types as their fields.

it introduces confusion when someone tries to use a key that is not comparable.

*comparable *is broken as far as I am concerned, not being able to provide a function that makes any struct comparable is a serious oversight and flaw with the language.

I have ended up having to write a library that generates a unique hash of any struct so that I can pass an array of structs I want to use as keys and array of values and generate a map with consistent hashing of the identity (fingerprint) of a struct as a key and the struct as the value. This allows me to generate keys from structs and look them up in the map without having to worry about if something I want to store in a map key is a valid key or not.

On Tue, May 14, 2024 at 11:24 AM Flc゛ @.***> wrote:

Why not just make it comparable, or just any?

I think it's just like the key in context.WithValue(ctx, key, value).

It can be supported, so why not?

Moreover, when the key is set to struct{}, it saves more memory, ex:

type customKey struct{} ctx.Set(customKey{}, "xxx")

— Reply to this email directly, view it on GitHub https://github.com/gin-gonic/gin/pull/3963#issuecomment-2110524488, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABF7754OZ67JDDN5LWVVBDZCIUERAVCNFSM6AAAAABHQT7FSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJQGUZDINBYHA . You are receiving this because you commented.Message ID: @.***>

-- Jarrod Roberson 678.551.2852

jarrodhroberson avatar May 20 '24 16:05 jarrodhroberson

In Golang, maps can only have keys of types that are considered comparable.

This means the type needs to support the == and != operators for comparison.

which means that structs have to have nothing but comparable types as

their fields.

it introduces confusion when someone tries to use a key that is not

comparable.

*comparable *is broken as far as I am concerned, not being able to provide

a function that makes any struct comparable is a serious oversight and

flaw with the language.

I have ended up having to write a library that generates a unique hash of

any struct so that I can pass an array of structs I want to use as keys and

array of values and generate a map with consistent hashing of the identity

(fingerprint) of a struct as a key and the struct as the value. This allows

me to generate keys from structs and look them up in the map without having

to worry about if something I want to store in a map key is a valid key or

not.

On Tue, May 14, 2024 at 11:24 AM Flc゛ @.***> wrote:

Why not just make it comparable, or just any?

I think it's just like the key in context.WithValue(ctx, key, value).

It can be supported, so why not?

Moreover, when the key is set to struct{}, it saves more memory, ex:

type customKey struct{}

ctx.Set(customKey{}, "xxx")

Reply to this email directly, view it on GitHub

https://github.com/gin-gonic/gin/pull/3963#issuecomment-2110524488, or

unsubscribe

https://github.com/notifications/unsubscribe-auth/AABF7754OZ67JDDN5LWVVBDZCIUERAVCNFSM6AAAAABHQT7FSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJQGUZDINBYHA

.

You are receiving this because you commented.Message ID:

@.***>

--

Jarrod Roberson

678.551.2852

Great reminder, I will add some single test trials.

flc1125 avatar May 20 '24 23:05 flc1125

@jarrodhroberson It is indeed recommended to change to map[comparable]any for better practice, but unfortunately, Go does not support this format. After checking the official documentation, no better solution has been found. Therefore, I have maintained the map[any]any format and added some unit test scripts.

https://github.com/gin-gonic/gin/pull/3963/files#diff-e6ce689a25eaef174c2dd51fe869fabbe04a6c6afbd416b23eda138c82e761baR220-R247

Some references: https://github.com/golang/go/issues/51384

flc1125 avatar May 21 '24 16:05 flc1125

@jarrodhroberson It is indeed recommended to change to map[comparable]any for better practice, but unfortunately, Go does not support this format. After checking the official documentation, no better solution has been found. Therefore, I have maintained the map[any]any format and added some unit test scripts.

https://github.com/gin-gonic/gin/pull/3963/files#diff-e6ce689a25eaef174c2dd51fe869fabbe04a6c6afbd416b23eda138c82e761baR220-R247

Some references: golang/go#51384

Yep went over source and there's no exposed func that could be turned into interface type with the given signature. Closest thing was cmp package but builtin types don't conform to its Equal.

bound2 avatar May 24 '24 06:05 bound2

@jarrodhroberson It is indeed recommended to change to map[comparable]any for better practice, but unfortunately, Go does not support this format. After checking the official documentation, no better solution has been found. Therefore, I have maintained the map[any]any format and added some unit test scripts. https://github.com/gin-gonic/gin/pull/3963/files#diff-e6ce689a25eaef174c2dd51fe869fabbe04a6c6afbd416b23eda138c82e761baR220-R247 Some references: golang/go#51384

Yep went over source and there's no exposed func that could be turned into interface type with the given signature. Closest thing was cmp package but builtin types don't conform to its Equal.

That is one of the inherit flaws of Go in reaction to all the insanely stupid things that other languages (JS cough cough) allow you to do. The inability to define an comparable function for your own types is one of them. Thanks for all the work on trying to so some kind of qualify life enhancements. If the Gin team would just make this an Interface it would make all this irrelevant.

jarrodhroberson avatar May 24 '24 13:05 jarrodhroberson

break backwards. looks not good for me. if somebody write code like this, this pr make it build fail

func handleTest(c *gin.Context)  {
	var cpKeys = make(map[string]any, len(c.Keys))
	for s, a := range c.Keys {
		cpKeys[s] = a
	}
	// ....
}

xiaotushaoxia avatar Jul 02 '24 05:07 xiaotushaoxia

if somebody write code like this, this pr make it build fail

But gin.Context is an implementation of context.Context.

And it is a broken implementation, because keys should be any, like in the original context.Context.

And so this change is more important, because it fixes this bug.

dmitry-novozhilov avatar Jul 30 '24 09:07 dmitry-novozhilov

Yes, the official context source code: https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/context/context.go;drc=f966695ccea356e4e4e8cc0328276e2d00c9fc1e;l=162

VILJkid avatar Feb 16 '25 23:02 VILJkid