Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: add support for type parameter #272

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

costela
Copy link

@costela costela commented Feb 18, 2023

This PR introduces the type parameters to the API, allowing token claims to be accessed directly using their concrete types, without type assertions.

The change bumps the minimal go version to 1.18.

It also:

  • remove the claims parameter from all methods; replaced by the type
  • adds a jwt.NewParserFor generic initializer (keeping the jwt.NewParser as a jwt.MapClaims variant)
  • removes Parser.ParseWithClaims (claims can be derived from the parser)
  • removes request.ParseRequestWithClaims (requiring explicit typing)

⚠️ this is currently targeted against the v5 branch, but should probably be rebased on a new v6 branch if v5 gets released without it.

Benchmarks
goos: linux
goarch: amd64
pkg: github.com/golang-jwt/jwt/v5
cpu: 12th Gen Intel(R) Core(TM) i7-1260P
                                        │  v5.bench   │              v6.bench              │
                                        │   sec/op    │   sec/op     vs base               │
ParseUnverified/map_claims-16             2.657µ ± 4%   2.626µ ± 8%       ~ (p=0.481 n=10)
ParseUnverified/map_claims#01-16          3.042µ ± 4%   2.960µ ± 7%       ~ (p=0.353 n=10)
ParseUnverified/map_claims#02-16          3.101µ ± 6%   3.132µ ± 5%       ~ (p=0.684 n=10)
ParseUnverified/map_claims#03-16          3.441µ ± 7%   3.361µ ± 7%  -2.30% (p=0.050 n=10)
ParseUnverified/map_claims#04-16          2.834µ ± 6%   2.739µ ± 6%  -3.35% (p=0.007 n=10)
ParseUnverified/map_claims#05-16          2.864µ ± 5%   2.764µ ± 4%  -3.51% (p=0.004 n=10)
ParseUnverified/map_claims#06-16          2.860µ ± 5%   2.746µ ± 3%  -3.99% (p=0.000 n=10)
ParseUnverified/map_claims#07-16          2.882µ ± 3%   2.750µ ± 4%  -4.58% (p=0.001 n=10)
ParseUnverified/map_claims#08-16          2.894µ ± 3%   2.755µ ± 3%  -4.80% (p=0.001 n=10)
ParseUnverified/map_claims#09-16          2.844µ ± 7%   2.769µ ± 5%  -2.65% (p=0.043 n=10)
ParseUnverified/map_claims#10-16          2.863µ ± 6%   2.758µ ± 5%  -3.65% (p=0.005 n=10)
ParseUnverified/map_claims#11-16          2.891µ ± 6%   2.799µ ± 3%  -3.17% (p=0.001 n=10)
ParseUnverified/map_claims#12-16          2.887µ ± 6%   2.835µ ± 3%  -1.80% (p=0.029 n=10)
ParseUnverified/map_claims#13-16          3.132µ ± 3%   3.033µ ± 3%  -3.15% (p=0.001 n=10)
ParseUnverified/map_claims#14-16          3.147µ ± 7%   3.013µ ± 5%  -4.26% (p=0.005 n=10)
ParseUnverified/map_claims#15-16          3.489µ ± 7%   3.371µ ± 5%  -3.37% (p=0.035 n=10)
ParseUnverified/map_claims#16-16          3.131µ ± 6%   3.052µ ± 2%  -2.51% (p=0.012 n=10)
ParseUnverified/registered_claims-16      2.965µ ± 6%   2.913µ ± 7%       ~ (p=0.171 n=10)
ParseUnverified/registered_claims#01-16   2.935µ ± 6%   2.909µ ± 4%       ~ (p=0.306 n=10)
ParseUnverified/registered_claims#02-16   3.285µ ± 6%   3.166µ ± 6%  -3.62% (p=0.005 n=10)
ParseUnverified/registered_claims#03-16   2.832µ ± 5%   2.739µ ± 7%  -3.27% (p=0.015 n=10)
ParseUnverified/registered_claims#04-16   3.275µ ± 5%   3.127µ ± 4%  -4.52% (p=0.003 n=10)
ParseUnverified/registered_claims#05-16   3.012µ ± 5%   2.894µ ± 5%  -3.90% (p=0.035 n=10)
ParseUnverified/registered_claims#06-16   3.151µ ± 5%   3.016µ ± 4%       ~ (p=0.052 n=10)
geomean                                   3.010µ        2.920µ       -3.02%

                                        │   v5.bench   │              v6.bench               │
                                        │     B/op     │     B/op      vs base               │
ParseUnverified/map_claims-16             2.141Ki ± 0%   2.117Ki ± 0%  -1.09% (p=0.000 n=10)
ParseUnverified/map_claims#01-16          2.273Ki ± 0%   2.250Ki ± 0%  -1.03% (p=0.000 n=10)
ParseUnverified/map_claims#02-16          2.272Ki ± 0%   2.249Ki ± 0%  -1.03% (p=0.000 n=10)
ParseUnverified/map_claims#03-16          2.374Ki ± 0%   2.351Ki ± 0%  -0.99% (p=0.000 n=10)
ParseUnverified/map_claims#04-16          2.141Ki ± 0%   2.117Ki ± 0%  -1.09% (p=0.000 n=10)
ParseUnverified/map_claims#05-16          2.141Ki ± 0%   2.117Ki ± 0%  -1.09% (p=0.000 n=10)
ParseUnverified/map_claims#06-16          2.141Ki ± 0%   2.117Ki ± 0%  -1.09% (p=0.000 n=10)
ParseUnverified/map_claims#07-16          2.141Ki ± 0%   2.117Ki ± 0%  -1.09% (p=0.000 n=10)
ParseUnverified/map_claims#08-16          2.141Ki ± 0%   2.117Ki ± 0%  -1.09% (p=0.000 n=10)
ParseUnverified/map_claims#09-16          2.141Ki ± 0%   2.117Ki ± 0%  -1.09% (p=0.000 n=10)
ParseUnverified/map_claims#10-16          2.141Ki ± 0%   2.117Ki ± 0%  -1.09% (p=0.000 n=10)
ParseUnverified/map_claims#11-16          2.141Ki ± 0%   2.117Ki ± 0%  -1.09% (p=0.000 n=10)
ParseUnverified/map_claims#12-16          2.141Ki ± 0%   2.117Ki ± 0%  -1.09% (p=0.000 n=10)
ParseUnverified/map_claims#13-16          2.273Ki ± 0%   2.250Ki ± 0%  -1.03% (p=0.000 n=10)
ParseUnverified/map_claims#14-16          2.273Ki ± 0%   2.250Ki ± 0%  -1.03% (p=0.000 n=10)
ParseUnverified/map_claims#15-16          2.375Ki ± 0%   2.352Ki ± 0%  -0.99% (p=0.000 n=10)
ParseUnverified/map_claims#16-16          2.273Ki ± 0%   2.250Ki ± 0%  -1.03% (p=0.000 n=10)
ParseUnverified/registered_claims-16      2.163Ki ± 0%   2.140Ki ± 0%  -1.08% (p=0.000 n=10)
ParseUnverified/registered_claims#01-16   2.211Ki ± 0%   2.188Ki ± 0%  -1.06% (p=0.000 n=10)
ParseUnverified/registered_claims#02-16   2.273Ki ± 0%   2.250Ki ± 0%  -1.03% (p=0.000 n=10)
ParseUnverified/registered_claims#03-16   2.140Ki ± 0%   2.116Ki ± 0%  -1.10% (p=0.000 n=10)
ParseUnverified/registered_claims#04-16   2.266Ki ± 0%   2.242Ki ± 0%  -1.03% (p=0.000 n=10)
ParseUnverified/registered_claims#05-16   2.156Ki ± 0%   2.133Ki ± 0%  -1.09% (p=0.000 n=10)
ParseUnverified/registered_claims#06-16   2.156Ki ± 0%   2.133Ki ± 0%  -1.09% (p=0.000 n=10)
geomean                                   2.202Ki        2.179Ki       -1.06%

                                        │  v5.bench  │             v6.bench              │
                                        │ allocs/op  │ allocs/op   vs base               │
ParseUnverified/map_claims-16             35.00 ± 0%   33.00 ± 0%  -5.71% (p=0.000 n=10)
ParseUnverified/map_claims#01-16          41.00 ± 0%   39.00 ± 0%  -4.88% (p=0.000 n=10)
ParseUnverified/map_claims#02-16          41.00 ± 0%   39.00 ± 0%  -4.88% (p=0.000 n=10)
ParseUnverified/map_claims#03-16          46.00 ± 0%   44.00 ± 0%  -4.35% (p=0.000 n=10)
ParseUnverified/map_claims#04-16          35.00 ± 0%   33.00 ± 0%  -5.71% (p=0.000 n=10)
ParseUnverified/map_claims#05-16          35.00 ± 0%   33.00 ± 0%  -5.71% (p=0.000 n=10)
ParseUnverified/map_claims#06-16          35.00 ± 0%   33.00 ± 0%  -5.71% (p=0.000 n=10)
ParseUnverified/map_claims#07-16          35.00 ± 0%   33.00 ± 0%  -5.71% (p=0.000 n=10)
ParseUnverified/map_claims#08-16          35.00 ± 0%   33.00 ± 0%  -5.71% (p=0.000 n=10)
ParseUnverified/map_claims#09-16          35.00 ± 0%   33.00 ± 0%  -5.71% (p=0.000 n=10)
ParseUnverified/map_claims#10-16          35.00 ± 0%   33.00 ± 0%  -5.71% (p=0.000 n=10)
ParseUnverified/map_claims#11-16          35.00 ± 0%   33.00 ± 0%  -5.71% (p=0.000 n=10)
ParseUnverified/map_claims#12-16          35.00 ± 0%   33.00 ± 0%  -5.71% (p=0.000 n=10)
ParseUnverified/map_claims#13-16          41.00 ± 0%   39.00 ± 0%  -4.88% (p=0.000 n=10)
ParseUnverified/map_claims#14-16          41.00 ± 0%   39.00 ± 0%  -4.88% (p=0.000 n=10)
ParseUnverified/map_claims#15-16          46.00 ± 0%   44.00 ± 0%  -4.35% (p=0.000 n=10)
ParseUnverified/map_claims#16-16          41.00 ± 0%   39.00 ± 0%  -4.88% (p=0.000 n=10)
ParseUnverified/registered_claims-16      35.00 ± 0%   33.00 ± 0%  -5.71% (p=0.000 n=10)
ParseUnverified/registered_claims#01-16   39.00 ± 0%   37.00 ± 0%  -5.13% (p=0.000 n=10)
ParseUnverified/registered_claims#02-16   42.00 ± 0%   40.00 ± 0%  -4.76% (p=0.000 n=10)
ParseUnverified/registered_claims#03-16   34.00 ± 0%   32.00 ± 0%  -5.88% (p=0.000 n=10)
ParseUnverified/registered_claims#04-16   41.00 ± 0%   39.00 ± 0%  -4.88% (p=0.000 n=10)
ParseUnverified/registered_claims#05-16   35.00 ± 0%   33.00 ± 0%  -5.71% (p=0.000 n=10)
ParseUnverified/registered_claims#06-16   35.00 ± 0%   33.00 ± 0%  -5.71% (p=0.000 n=10)
geomean                                   37.66        35.65       -5.34%

@oxisto
Copy link
Collaborator

oxisto commented Feb 18, 2023

I like the idea of having a seperate NewParserFor that is generic and keeping the other one as MapClaims or just the Claims interface. Another idea that I had is basically having two types: one as generic and a second one without genetics as an alias to replace the existing Keyfunc/token type. This could even make it almost backwards compatible with some twisting. Or at least not break existing code too much for people who do not want/need generics.

Something like:

type TokenFor[T Claims] struct{ /*… */ }
type Token = TokenFor[Claims]

The „new“ Token should the pretty much behave the same as the old one.

@costela costela force-pushed the switch-to-generics-for-claims branch from 4374bd1 to b3d339a Compare February 19, 2023 01:03
@oxisto oxisto deleted the branch golang-jwt:main February 21, 2023 13:32
@oxisto oxisto closed this Feb 21, 2023
@oxisto oxisto reopened this Feb 21, 2023
@oxisto oxisto changed the base branch from v5 to main February 21, 2023 13:38
@costela
Copy link
Author

costela commented Feb 22, 2023

@oxisto yes, that's a good point with the type aliases!

I'll rebase and see if we can still make this backward compatible. Sounds good?

@oxisto
Copy link
Collaborator

oxisto commented Feb 22, 2023

@oxisto yes, that's a good point with the type aliases!

I'll rebase and see if we can still make this backward compatible. Sounds good?

Sure!

@costela costela force-pushed the switch-to-generics-for-claims branch from 20b02ba to fea6835 Compare February 25, 2023 20:45
@costela
Copy link
Author

costela commented Feb 25, 2023

Rebase done ✔️

I don't think we can achieve 100% backward-compatibility since jwt.Parse returned a token with a jwt.Claims, but used jwt.MapClaims internally. But IMHO the "new" API feels a bit cleaner. WDYT?

The other breaking change is more of an opinion: I personally always found the passing-in of claims a bit weird. But of course we can just put it back in, if the general opinion wants it.

@costela costela marked this pull request as ready for review February 25, 2023 20:59
@costela costela force-pushed the switch-to-generics-for-claims branch from fea6835 to c47e5da Compare February 26, 2023 20:04
@costela
Copy link
Author

costela commented Mar 16, 2023

@oxisto did you (or any other maintainer) have a chance to look at this? 😇

Copy link
Collaborator

@oxisto oxisto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial assessment done. Looks good so far. I still do not quite know what to do with it in our roadmap though. I guess we won't include this in the initial v5 release.

Possibly we can get it to like 99,999% backwards compatibility, I will try to check this in a second round of reviews

Signature string // Signature is the third segment of the token. Populated when you Parse a token
Valid bool // Valid specifies if the token is valid. Populated when you Parse/Verify a token
}

// Token is an alias for TokenFor[Claims], for backward compatibility.
type Token = TokenFor[MapClaims]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want something like this for even more compatibility:

Suggested change
type Token = TokenFor[MapClaims]
type Token = TokenFor[Claims]


// Token represents a JWT Token. Different fields will be used depending on
// Keyfunc is an alias for KeyfuncFor[Claims], for backward compatibility.
type Keyfunc = KeyfuncFor[MapClaims]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type Keyfunc = KeyfuncFor[MapClaims]
type Keyfunc = KeyfuncFor[Claims]

@@ -111,8 +117,8 @@ func Parse(tokenString string, keyFunc Keyfunc, options ...ParserOption) (*Token
// embed a non-pointer version of the claims or b) if you are using a pointer,
// allocate the proper memory for it before passing in the overall claims,
// otherwise you might run into a panic.
func ParseWithClaims(tokenString string, claims Claims, keyFunc Keyfunc, options ...ParserOption) (*Token, error) {
return NewParser(options...).ParseWithClaims(tokenString, claims, keyFunc)
func ParseWithClaims[T Claims](tokenString string, keyFunc KeyfuncFor[T], options ...ParserOption) (*TokenFor[T], error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably keep the claims parameter here at least for now, maybe remove it in a future update. The reason is that there are also some cases in which you need to properly initialise an embedded claims struct (in a custom claim), when using a pointer and this is only possible if the claim is provided in the function.

func NewParser(options ...ParserOption) *Parser {
p := &Parser{
validator: &validator{},
func NewParser(options ...ParserOption) *Parser[MapClaims] {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func NewParser(options ...ParserOption) *Parser[MapClaims] {
func NewParser(options ...ParserOption) *Parser[Claims] {

@oxisto oxisto mentioned this pull request Aug 14, 2023
10 tasks
@hantonelli
Copy link

This is a nice feature, it would be great if it could be merged

@hantonelli
Copy link

Any updates on when this will be merged?

@costela
Copy link
Author

costela commented Jan 28, 2024

@hantonelli sorry, this totally got lost in the day-to-day work.

I'll try to rebase and address the points above 🤞

@mfridman
Copy link
Member

This is a nice feature, it would be great if it could be merged

Outside of generics, does this PR have a feature that's missing from the current /v5 library?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants