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

Support custom json and base64 encoders for Token and Parser #301

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

Conversation

dcalsky
Copy link
Contributor

@dcalsky dcalsky commented Apr 2, 2023

This PR adds support for Token and Parser using custom JSON and base64 encoders to encode/decode related data for performance and experimentation purpose.

By implementing the Base64Encoder interface with EncodeToString and DecodeString methods to create a custom base64 encoder, while implementing JSONEncoder with Marshal and Unmarshal methods to create a custom json encoder.

import (
	"github.com/bytedance/sonic"
	asmbase64 "github.com/segmentio/asm/base64"
)

type sonicJsonEncoder struct{}

func (s *sonicJsonEncoder) Marshal(v any) ([]byte, error) {
	return sonic.Marshal(v)
}

func (s *sonicJsonEncoder) Unmarshal(data []byte, v any) error {
	return sonic.Unmarshal(data, v)
}

type asmBase64Encoder struct{}

func (s *asmBase64Encoder) EncodeToString(data []byte) string {
	return asmbase64.StdEncoding.EncodeToString(data)
}

func (s *asmBase64Encoder) DecodeString(data string) ([]byte, error) {
	return asmbase64.RawURLEncoding.DecodeString(data)
}

Then Using WithJSONEncoder and WithBase64Encoder to create a Parser, and using WithTokenJSONEncoder and WithTokenBase64Encoder to create a Token.

// create the token and parser  with json encoder based on sonic and base64 encoder based on asm/base64
token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.RegisteredClaims{}, jwt.WithTokenJSONEncoder(&sonicJsonEncoder{}), jwt.WithTokenBase64Encoder(&asmBase64Encoder{}))

parser := jwt.NewParser(jwt.WithJSONEncoder(&sonicJsonEncoder{}), jwt.WithBase64Encoder(&asmBase64Encoder{}))

The disadvantages of this PR are (Any suggestions are welcomed):

  • Introduce the third-party libs into the go module
  • due to almost all of functions and structs are under the same package, the optional methods naming might be confused, like WithJSONEncoder and WithTokenJSONEncoder

Resolves #168

@oxisto
Copy link
Collaborator

oxisto commented Apr 2, 2023

Very nice! I will do a review over the next days, but it’s basically the direction I envisioned. Just some general pointers: We want to avoid the inclusion of any third party libs, even if it’s only in the tests. I would suggest only testing the interface itself in the test without the actual sonic implementation.

@dcalsky dcalsky force-pushed the main branch 2 times, most recently from cd88b49 to 7c726ab Compare April 2, 2023 15:54
@dcalsky
Copy link
Contributor Author

dcalsky commented Apr 2, 2023

Very nice! I will do a review over the next days, but it’s basically the direction I envisioned. Just some general pointers: We want to avoid the inclusion of any third party libs, even if it’s only in the tests. I would suggest only testing the interface itself in the test without the actual sonic implementation.

I've updated the PR according to your advice.

token_option.go Outdated Show resolved Hide resolved
encoder.go Outdated Show resolved Hide resolved
parser.go Outdated Show resolved Hide resolved
parser.go Outdated Show resolved Hide resolved
parser.go Outdated Show resolved Hide resolved
parser.go Outdated Show resolved Hide resolved
parser.go Outdated Show resolved Hide resolved
parser_option.go Outdated
@@ -125,3 +125,16 @@ func WithStrictDecoding() ParserOption {
p.decodeStrict = true
}
}

// WithJSONEncoder supports
func WithJSONEncoder(enc JSONEncoder) ParserOption {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing docstring

parser_option.go Outdated Show resolved Hide resolved
token.go Outdated Show resolved Hide resolved
token.go Outdated Show resolved Hide resolved
@alam-chime
Copy link

thanks @dcalsky
+1 to this PR

Co-Authored-By: Christian Banse <oxisto@aybaze.com>
@oxisto
Copy link
Collaborator

oxisto commented Sep 13, 2023

Thanks for the initial work on this @dcalsky. I had another go at this, basically splitting up encoding and decoding and also converting most of the interfaces into function types. This should be pretty good integrated in the existing code base now. It even supports both parsing JSON with and without JSON number (as long as the underlying JSON library supports it).

I wonder whether I could also make our "special" base64 options (strict, allow padding) work with external libraries as well. For now, they are disabled when an external decoder is used.

If we are happy with this approach, I want to finalise docs and examples.

cc @mfridman @dcalsky

@oxisto
Copy link
Collaborator

oxisto commented Feb 1, 2024

@mfridman Do we still want to pursue this? It did gather some interest from the community. If yes, I would do another cleanup pass and add docs and examples/tests.

@mfridman
Copy link
Member

mfridman commented Feb 1, 2024

I'll review this in the next few days.

@mfridman
Copy link
Member

mfridman commented Feb 5, 2024

It seems useful, albeit the number of requests for this was quite low.

I was mainly waiting to see what comes of golang/go#63397. If the native std lib could be improved then bringing your own encoding package becomes unnecessary?

I'm a bit on the fence on this one, but if we get it into a good enough shape I wouldn't block. My main hesitation is all the plumbing we have to do to support this in the current /v5 API.

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.

Supporting alternative base64/json encoder/decoder
4 participants