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

lib/options: BlacklistIPs json encoding/decoding #2085

Merged
merged 1 commit into from
Jul 12, 2021
Merged

lib/options: BlacklistIPs json encoding/decoding #2085

merged 1 commit into from
Jul 12, 2021

Conversation

codebien
Copy link
Contributor

@codebien codebien commented Jul 5, 2021

Support JSON encoding for the BlacklistIPs option.

Closes #2083

@codebien codebien requested review from mstoykov, imiric and na-- July 5, 2021 18:36
@codebien codebien self-assigned this Jul 5, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2021

Codecov Report

Merging #2085 (1298b30) into master (aa1fd6a) will decrease coverage by 0.01%.
The diff coverage is 37.50%.

❗ Current head 1298b30 differs from pull request most recent head 2b8be74. Consider uploading reports for the commit 2b8be74 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2085      +/-   ##
==========================================
- Coverage   72.11%   72.10%   -0.02%     
==========================================
  Files         180      180              
  Lines       14243    14251       +8     
==========================================
+ Hits        10272    10276       +4     
- Misses       3346     3349       +3     
- Partials      625      626       +1     
Flag Coverage Δ
ubuntu 72.03% <37.50%> (-0.02%) ⬇️
windows 71.72% <37.50%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
js/initcontext.go 87.87% <0.00%> (-4.68%) ⬇️
lib/options.go 84.00% <100.00%> (+0.21%) ⬆️
lib/executor/vu_handle.go 94.39% <0.00%> (-0.94%) ⬇️
output/cloud/output.go 81.73% <0.00%> (+0.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa1fd6a...2b8be74. Read the comment docs.

lib/options.go Outdated
// UnmarshalJSON decodes a JSON representation of IPNet using CIDR notation.
func (ipnet *IPNet) UnmarshalJSON(b []byte) error {
// Remove quotes and fallback to unmarshal text
return ipnet.UnmarshalText([]byte(strings.Trim(string(b), `"`)))
Copy link
Contributor

Choose a reason for hiding this comment

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

There is bytes.Trim.

lib/options.go Outdated
Comment on lines 184 to 187
// MarshalJSON returns the JSON representation of IPNet using CIDR notation.
func (ipnet *IPNet) MarshalJSON() ([]byte, error) {
return []byte(fmt.Sprintf("%q", ipnet.String())), nil
}

// UnmarshalJSON decodes a JSON representation of IPNet using CIDR notation.
func (ipnet *IPNet) UnmarshalJSON(b []byte) error {
// Remove quotes and fallback to unmarshal text
return ipnet.UnmarshalText([]byte(strings.Trim(string(b), `"`)))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine, but in reality, all you need is MarshalText as it seems that we fall back to that and if you just replace all of that with MarshalText that just return []byte(ipnet.String()) it seems still to work.

Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

I am not certain why the only MarshalText works fine, but if it does and it's simpler it's probably better then adding 2 methods ;)

@codebien codebien force-pushed the 2083 branch 3 times, most recently from 0e977d9 to 0942987 Compare July 8, 2021 08:57
@codebien
Copy link
Contributor Author

codebien commented Jul 8, 2021

I am not certain why the only MarshalText works fine, but if it does and it's simpler it's probably better then adding 2 methods ;)

@mstoykov I agree, some tests I did confuse me. The problem is just in the encoding part and we can fall back on TextMarshaler.

@codebien codebien requested a review from mstoykov July 8, 2021 09:21
mstoykov
mstoykov previously approved these changes Jul 8, 2021
lib/options.go Outdated Show resolved Hide resolved
The BlacklistIP configuration uses the CIDR notation.
Implemented the TextMarshaler interface to support correct
encoding based on the notation.
@na-- na-- added this to the v0.34.0 milestone Jul 12, 2021
@codebien codebien merged commit 412de38 into master Jul 12, 2021
@codebien codebien deleted the 2083 branch July 12, 2021 07:48
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.

blacklistIPs JS configuration isn't parsed correctly, pt. 2
4 participants