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

flc1125 · 2024-05-10T13:18:10Z

⚠️ 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 #1123 (comment)

Signed-off-by: Flc゛ <four_leaf_clover@foxmail.com>

codecov · 2024-05-10T13:19:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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%)`	⬇️
go-1.22	`99.05% <100.00%> (?)`
macos-latest	`99.05% <100.00%> (-0.16%)`	⬇️
ubuntu-latest	`99.05% <100.00%> (-0.16%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jarrodhroberson · 2024-05-14T14:34:52Z

Why not just make it comparable, or just any?

flc1125 · 2024-05-14T15:24:33Z

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

jarrodhroberson · 2024-05-20T16:23:39Z

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 <#3963 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABF7754OZ67JDDN5LWVVBDZCIUERAVCNFSM6AAAAABHQT7FSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJQGUZDINBYHA> . You are receiving this because you commented.Message ID: ***@***.***>

-- Jarrod Roberson 678.551.2852

flc1125 · 2024-05-20T23:48:30Z

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

#3963 (comment), 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.

Signed-off-by: Flc゛ <four_leaf_clover@foxmail.com>

flc1125 · 2024-05-21T16:14:44Z

@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

bound2 · 2024-05-24T05:20:01Z

context_test.go

+		c.Set([]int{1}, 1)
+		c.Set(func() {}, 1)
+		c.Set(make(chan int), 1)
+	})
 }

 func TestContextSetGetValues(t *testing.T) {


Should this test also check that it's possible to retrieve pointer by key? I know that it's not part of the scope of your task changes but I see that it's missing from here.

Is your meaning nil?

bound2 · 2024-05-24T05:22:34Z

context_test.go

@@ -217,6 +217,34 @@ func TestContextSetGet(t *testing.T) {

 	assert.Equal(t, "bar", c.MustGet("foo"))
 	assert.Panics(t, func() { c.MustGet("no_exist") })
+
+	// other types
+	type key struct{}


Maybe worth extracting into

func TestContextSetGetAnyKey(t *testing.T)

bound2 · 2024-05-24T05:23:09Z

context_test.go

+	}
+
+	// no comparable
+	assert.Panics(t, func() {


Maybe worth extracting into

func TestContextSetGetPanicsWhenKeyNotComparable(t *testing.T)

bound2 · 2024-05-24T06:16:37Z

@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.

jarrodhroberson · 2024-05-24T13:47:59Z

@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.

xiaotushaoxia · 2024-07-02T05:08:27Z

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
	}
	// ....
}

dmitry-novozhilov · 2024-07-30T09:48:22Z

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.

refactor(context): refactor keys to map[any]any

42a712b

Signed-off-by: Flc゛ <four_leaf_clover@foxmail.com>

flc1125 changed the title ~~refactor(context): refactor Keys to map[any]any~~ refactor(context): refactor Keys type to map[any]any May 10, 2024

flc1125 mentioned this pull request May 10, 2024

gin.Context is not a interface, how to extend it? #1123

Open

flc1125 added 2 commits May 21, 2024 23:37

Merge branch 'gin-gonic:master' into refactor-keys

1188f19

refactor(context): refactor keys to map[any]any

0336111

Signed-off-by: Flc゛ <four_leaf_clover@foxmail.com>

bound2 reviewed May 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

flc1125 commented May 10, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading

jarrodhroberson commented May 14, 2024

flc1125 commented May 14, 2024 •

edited

Loading

jarrodhroberson commented May 20, 2024 via email

flc1125 commented May 20, 2024

flc1125 commented May 21, 2024

bound2 May 24, 2024

flc1125 May 24, 2024

bound2 May 24, 2024

bound2 May 24, 2024

bound2 commented May 24, 2024

jarrodhroberson commented May 24, 2024

xiaotushaoxia commented Jul 2, 2024 •

edited

Loading

dmitry-novozhilov commented Jul 30, 2024

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

Are you sure you want to change the base?

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

Conversation

flc1125 commented May 10, 2024 • edited Loading

codecov bot commented May 10, 2024 • edited Loading

Codecov Report

jarrodhroberson commented May 14, 2024

flc1125 commented May 14, 2024 • edited Loading

jarrodhroberson commented May 20, 2024 via email

flc1125 commented May 20, 2024

flc1125 commented May 21, 2024

bound2 May 24, 2024

Choose a reason for hiding this comment

flc1125 May 24, 2024

Choose a reason for hiding this comment

bound2 May 24, 2024

Choose a reason for hiding this comment

bound2 May 24, 2024

Choose a reason for hiding this comment

bound2 commented May 24, 2024

jarrodhroberson commented May 24, 2024

xiaotushaoxia commented Jul 2, 2024 • edited Loading

dmitry-novozhilov commented Jul 30, 2024

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

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

flc1125 commented May 10, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading

flc1125 commented May 14, 2024 •

edited

Loading

xiaotushaoxia commented Jul 2, 2024 •

edited

Loading