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

✨ enhancement: use msgp for flash message encoding/decoding #3099

Merged
merged 9 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ctx.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ type DefaultCtx struct {
pathOriginal string // Original HTTP path
pathBuffer []byte // HTTP path buffer
detectionPathBuffer []byte // HTTP detectionPath buffer
redirectionMessages []string // Messages of the previous redirect
flashMessages redirectionMsgs // Flash messages
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved
indexRoute int // Index of the current route
indexHandler int // Index of the current handler
methodINT int // HTTP method INT equivalent
Expand Down Expand Up @@ -1896,7 +1896,7 @@ func (c *DefaultCtx) release() {
c.route = nil
c.fasthttp = nil
c.bind = nil
c.redirectionMessages = c.redirectionMessages[:0]
c.flashMessages = c.flashMessages[:0]
c.viewBindMap = sync.Map{}
if c.redirect != nil {
ReleaseRedirect(c.redirect)
Expand Down
217 changes: 119 additions & 98 deletions redirect.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import (
"errors"
"strings"
"sync"

"github.com/gofiber/fiber/v3/binder"
Expand All @@ -19,7 +18,7 @@
New: func() any {
return &Redirect{
status: StatusFound,
oldInput: make(map[string]string, 0),
messages: make(redirectionMsgs, 0),
}
},
}
Expand All @@ -32,13 +31,37 @@
CookieDataAssigner = ":"
)

// redirectionMsgs is a struct that used to store flash messages and old input data in cookie using MSGP.
// msgp -file="redirect.go" -o="redirect_msgp.go" -unexported
//
//msgp:ignore Redirect RedirectConfig OldInputData FlashMessage
type redirectionMsg struct {
key string
value string
level uint8
isOldInput bool
}

type redirectionMsgs []redirectionMsg

// OldInputData is a struct that holds the old input data.
type OldInputData struct {
Key string
Value string
}

// FlashMessage is a struct that holds the flash message data.
type FlashMessage struct {
Key string
Value string
Level uint8
}

// Redirect is a struct that holds the redirect data.
type Redirect struct {
c *DefaultCtx // Embed ctx
oldInput map[string]string // Old input data

messages []string // Flash messages
status int // Status code of redirection. Default: StatusFound
c *DefaultCtx // Embed ctx
messages redirectionMsgs // Flash messages and old input data
status int // Status code of redirection. Default: StatusFound
}

// RedirectConfig A config to use with Redirect().Route()
Expand Down Expand Up @@ -71,10 +94,6 @@
func (r *Redirect) release() {
r.status = 302
r.messages = r.messages[:0]
// reset map
for k := range r.oldInput {
delete(r.oldInput, k)
}
r.c = nil
}

Expand All @@ -90,8 +109,28 @@
// They will be sent as a cookie.
// You can get them by using: Redirect().Messages(), Redirect().Message()
// Note: You must use escape char before using ',' and ':' chars to avoid wrong parsing.
func (r *Redirect) With(key, value string) *Redirect {
r.messages = append(r.messages, key+CookieDataAssigner+value)
func (r *Redirect) With(key, value string, level ...uint8) *Redirect {
// Get level
var msgLevel uint8
if len(level) > 0 {
msgLevel = level[0]
}

// Override old message if exists
for i, msg := range r.messages {
if msg.key == key && !msg.isOldInput {
r.messages[i].value = value
r.messages[i].level = msgLevel

Check warning on line 123 in redirect.go

View check run for this annotation

Codecov / codecov/patch

redirect.go#L122-L123

Added lines #L122 - L123 were not covered by tests

return r

Check warning on line 125 in redirect.go

View check run for this annotation

Codecov / codecov/patch

redirect.go#L125

Added line #L125 was not covered by tests
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved
}
}

r.messages = append(r.messages, redirectionMsg{
key: key,
value: value,
level: msgLevel,
})

return r
}
Expand All @@ -105,76 +144,92 @@
ctype := utils.ToLower(utils.UnsafeString(r.c.Context().Request.Header.ContentType()))
ctype = binder.FilterFlags(utils.ParseVendorSpecificContentType(ctype))

oldInput := make(map[string]string)
switch ctype {
case MIMEApplicationForm:
_ = r.c.Bind().Form(r.oldInput) //nolint:errcheck // not needed
_ = r.c.Bind().Form(oldInput) //nolint:errcheck // not needed

Check warning on line 150 in redirect.go

View check run for this annotation

Codecov / codecov/patch

redirect.go#L150

Added line #L150 was not covered by tests
case MIMEMultipartForm:
_ = r.c.Bind().MultipartForm(r.oldInput) //nolint:errcheck // not needed
_ = r.c.Bind().MultipartForm(oldInput) //nolint:errcheck // not needed

Check warning on line 152 in redirect.go

View check run for this annotation

Codecov / codecov/patch

redirect.go#L152

Added line #L152 was not covered by tests
default:
_ = r.c.Bind().Query(r.oldInput) //nolint:errcheck // not needed
_ = r.c.Bind().Query(oldInput) //nolint:errcheck // not needed
}
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved

// Add old input data
for k, v := range oldInput {
r.messages = append(r.messages, redirectionMsg{
key: k,
value: v,
isOldInput: true,
})
}

return r
}

// Messages Get flash messages.
func (r *Redirect) Messages() map[string]string {
msgs := r.c.redirectionMessages
flashMessages := make(map[string]string, len(msgs))

for _, msg := range msgs {
k, v := parseMessage(msg)

if !strings.HasPrefix(k, OldInputDataPrefix) {
flashMessages[k] = v
func (r *Redirect) Messages() []FlashMessage {
flashMessages := make([]FlashMessage, 0)

for _, msg := range r.c.flashMessages {
if !msg.isOldInput {
flashMessages = append(flashMessages, FlashMessage{
Key: msg.key,
Value: msg.value,
Level: msg.level,
})
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved
}
}

return flashMessages
}

// Message Get flash message by key.
func (r *Redirect) Message(key string) string {
msgs := r.c.redirectionMessages
func (r *Redirect) Message(key string) FlashMessage {
msgs := r.c.flashMessages

for _, msg := range msgs {
k, v := parseMessage(msg)

if !strings.HasPrefix(k, OldInputDataPrefix) && k == key {
return v
if msg.key == key && !msg.isOldInput {
return FlashMessage{
Key: msg.key,
Value: msg.value,
Level: msg.level,
}
}
}
return ""

return FlashMessage{}

Check warning on line 200 in redirect.go

View check run for this annotation

Codecov / codecov/patch

redirect.go#L200

Added line #L200 was not covered by tests
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved
}

// OldInputs Get old input data.
func (r *Redirect) OldInputs() map[string]string {
msgs := r.c.redirectionMessages
oldInputs := make(map[string]string, len(msgs))

for _, msg := range msgs {
k, v := parseMessage(msg)

if strings.HasPrefix(k, OldInputDataPrefix) {
// remove "old_input_data_" part from key
oldInputs[k[len(OldInputDataPrefix):]] = v
func (r *Redirect) OldInputs() []OldInputData {
inputs := make([]OldInputData, 0)

for _, msg := range r.c.flashMessages {
if msg.isOldInput {
inputs = append(inputs, OldInputData{
Key: msg.key,
Value: msg.value,
})
}
}
return oldInputs

return inputs
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved
}

// OldInput Get old input data by key.
func (r *Redirect) OldInput(key string) string {
msgs := r.c.redirectionMessages
func (r *Redirect) OldInput(key string) OldInputData {
msgs := r.c.flashMessages

for _, msg := range msgs {
k, v := parseMessage(msg)

if strings.HasPrefix(k, OldInputDataPrefix) && k[len(OldInputDataPrefix):] == key {
return v
if msg.key == key && msg.isOldInput {
return OldInputData{
Key: msg.key,
Value: msg.value,
}
}
}
return ""

return OldInputData{}

Check warning on line 232 in redirect.go

View check run for this annotation

Codecov / codecov/patch

redirect.go#L232

Added line #L232 was not covered by tests
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved
}

// To redirect to the URL derived from the specified path, with specified status.
Expand Down Expand Up @@ -240,66 +295,32 @@
return r.To(location)
}

// parseAndClearFlashMessages is a method to get flash messages before removing them
// parseAndClearFlashMessages is a method to get flash messages before they are getting removed
func (r *Redirect) parseAndClearFlashMessages() {
// parse flash messages
cookieValue := r.c.Cookies(FlashCookieName)

var commaPos int
for {
commaPos = findNextNonEscapedCharsetPosition(cookieValue, []byte(CookieDataSeparator))
if commaPos == -1 {
r.c.redirectionMessages = append(r.c.redirectionMessages, utils.Trim(cookieValue, ' '))
break
}
r.c.redirectionMessages = append(r.c.redirectionMessages, utils.Trim(cookieValue[:commaPos], ' '))
cookieValue = cookieValue[commaPos+1:]
_, err := r.c.flashMessages.UnmarshalMsg(r.c.app.getBytes(cookieValue))
if err != nil {
return

Check warning on line 305 in redirect.go

View check run for this annotation

Codecov / codecov/patch

redirect.go#L305

Added line #L305 was not covered by tests
Comment on lines +298 to +305
Copy link
Contributor

Choose a reason for hiding this comment

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

Add error handling to parseAndClearFlashMessages method.

The method lacks error handling for the unmarshaling process. Consider adding error handling to ensure robustness.

Apply this diff to add error handling:

303,305c303,307
< 	_, err := r.c.flashMessages.UnmarshalMsg(r.c.app.getBytes(cookieValue))
< 	if err != nil {
< 		return
---
> 	if _, err := r.c.flashMessages.UnmarshalMsg(r.c.app.getBytes(cookieValue)); err != nil {
> 		// Handle error appropriately
> 		r.c.app.LogError("Failed to parse flash messages", err)
> 		return
> 	}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// parseAndClearFlashMessages is a method to get flash messages before they are getting removed
func (r *Redirect) parseAndClearFlashMessages() {
// parse flash messages
cookieValue := r.c.Cookies(FlashCookieName)
var commaPos int
for {
commaPos = findNextNonEscapedCharsetPosition(cookieValue, []byte(CookieDataSeparator))
if commaPos == -1 {
r.c.redirectionMessages = append(r.c.redirectionMessages, utils.Trim(cookieValue, ' '))
break
}
r.c.redirectionMessages = append(r.c.redirectionMessages, utils.Trim(cookieValue[:commaPos], ' '))
cookieValue = cookieValue[commaPos+1:]
_, err := r.c.flashMessages.UnmarshalMsg(r.c.app.getBytes(cookieValue))
if err != nil {
return
// parseAndClearFlashMessages is a method to get flash messages before they are getting removed
func (r *Redirect) parseAndClearFlashMessages() {
// parse flash messages
cookieValue := r.c.Cookies(FlashCookieName)
if _, err := r.c.flashMessages.UnmarshalMsg(r.c.app.getBytes(cookieValue)); err != nil {
// Handle error appropriately
r.c.app.LogError("Failed to parse flash messages", err)
return
}
Tools
GitHub Check: codecov/patch

[warning] 305-305: redirect.go#L305
Added line #L305 was not covered by tests

}

r.c.ClearCookie(FlashCookieName)
}

// processFlashMessages is a helper function to process flash messages and old input data
// and set them as cookies
func (r *Redirect) processFlashMessages() {
// Flash messages
if len(r.messages) > 0 || len(r.oldInput) > 0 {
messageText := bytebufferpool.Get()
defer bytebufferpool.Put(messageText)

// flash messages
for i, message := range r.messages {
messageText.WriteString(message)
// when there are more messages or oldInput -> add a comma
if len(r.messages)-1 != i || (len(r.messages)-1 == i && len(r.oldInput) > 0) {
messageText.WriteString(CookieDataSeparator)
}
}
r.messages = r.messages[:0]

// old input data
i := 1
for k, v := range r.oldInput {
messageText.WriteString(OldInputDataPrefix + k + CookieDataAssigner + v)
if len(r.oldInput) != i {
messageText.WriteString(CookieDataSeparator)
}
i++
}

r.c.Cookie(&Cookie{
Name: FlashCookieName,
Value: r.c.app.getString(messageText.Bytes()),
SessionOnly: true,
})
if len(r.messages) == 0 {
return
}
}

// parseMessage is a helper function to parse flash messages and old input data
func parseMessage(raw string) (string, string) { //nolint: revive // not necessary
if i := findNextNonEscapedCharsetPosition(raw, []byte(CookieDataAssigner)); i != -1 {
return RemoveEscapeChar(raw[:i]), RemoveEscapeChar(raw[i+1:])
val, err := r.messages.MarshalMsg(nil)
if err != nil {
return

Check warning on line 318 in redirect.go

View check run for this annotation

Codecov / codecov/patch

redirect.go#L318

Added line #L318 was not covered by tests
}

return RemoveEscapeChar(raw), ""
r.c.Cookie(&Cookie{
Name: FlashCookieName,
Value: r.c.app.getString(val),
SessionOnly: true,
})
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +312 to +325
Copy link
Contributor

Choose a reason for hiding this comment

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

Add error handling to processFlashMessages method.

The method lacks error handling for the marshaling process. Consider adding error handling to ensure robustness.

Apply this diff to add error handling:

316,318c316,320
< 	val, err := r.messages.MarshalMsg(nil)
< 	if err != nil {
< 		return
---
> 	if val, err := r.messages.MarshalMsg(nil); err != nil {
> 		// Handle error appropriately
> 		r.c.app.LogError("Failed to marshal flash messages", err)
> 		return
> 	}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if len(r.messages) == 0 {
return
}
}
// parseMessage is a helper function to parse flash messages and old input data
func parseMessage(raw string) (string, string) { //nolint: revive // not necessary
if i := findNextNonEscapedCharsetPosition(raw, []byte(CookieDataAssigner)); i != -1 {
return RemoveEscapeChar(raw[:i]), RemoveEscapeChar(raw[i+1:])
val, err := r.messages.MarshalMsg(nil)
if err != nil {
return
}
return RemoveEscapeChar(raw), ""
r.c.Cookie(&Cookie{
Name: FlashCookieName,
Value: r.c.app.getString(val),
SessionOnly: true,
})
if len(r.messages) == 0 {
return
}
if val, err := r.messages.MarshalMsg(nil); err != nil {
// Handle error appropriately
r.c.app.LogError("Failed to marshal flash messages", err)
return
}
r.c.Cookie(&Cookie{
Name: FlashCookieName,
Value: r.c.app.getString(val),
SessionOnly: true,
})
Tools
GitHub Check: codecov/patch

[warning] 318-318: redirect.go#L318
Added line #L318 was not covered by tests

}
Loading
Loading