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

fix null pattern #9

Merged
merged 1 commit into from
Dec 26, 2023
Merged

fix null pattern #9

merged 1 commit into from
Dec 26, 2023

Conversation

pyama86
Copy link
Contributor

@pyama86 pyama86 commented Dec 26, 2023

ref: https://go.dev/play/p/2vZE4pIC-ai

In Go, accessing a[0] when a is an empty string (a := "") will result in a panic.

@@ -34,7 +34,7 @@ func NewBaseLimiter(
targetExtensionsMap := make(map[string]struct{}, len(targetExtensions))
if len(targetExtensions) > 0 {
for _, ext := range targetExtensions {
if ext[0] != '.' {
if len(ext) > 0 && ext[0] != '.' {
ext = "." + ext
}
targetExtensionsMap[strings.ToLower(ext)] = struct{}{}
Copy link
Member

Choose a reason for hiding this comment

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

It seems better to exclude illegal ext.
https://go.dev/play/p/tAFv_4iuCHr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying that having a file extension as "" (empty string) is an abnormal case? I thought that in the context of the web, there are cases where files don't have extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a confusing behavior that the root path is limited to the target file with no file extension.
How about this?

https://github.com/2manymws/rlutils/blob/49d6079f9ac4e30a15aba4df45d1c42bb64711db/base_limiter.go#L68C44-L68C54

Copy link
Member

@k1LoW k1LoW Dec 26, 2023

Choose a reason for hiding this comment

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

There may be a way to change isTargetExtensions as a fix to https://github.com/2manymws/rlutils/pull/9/files#r1436334082

Copy link
Member

@k1LoW k1LoW left a comment

Choose a reason for hiding this comment

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

@pyama86 pyama86 merged commit e0a68d6 into main Dec 26, 2023
3 checks passed
@pyama86 pyama86 deleted the fix-bugs-null branch December 26, 2023 09:16
@github-actions github-actions bot mentioned this pull request Dec 26, 2023
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.

2 participants