fix: API address parameter validation #1311

orpheuslummis · 2023-04-09T18:27:05Z

Relevant issue(s)

Resolves #1310

Description

Fix the bug through improving the related validation.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

test suite and manual try.

codecov · 2023-04-09T18:32:22Z

Codecov Report

Merging #1311 (44f75ce) into develop (2d27a10) will increase coverage by 0.00%.
The diff coverage is 77.77%.

@@           Coverage Diff            @@
##           develop    #1311   +/-   ##
========================================
  Coverage    70.57%   70.57%           
========================================
  Files          184      184           
  Lines        17803    17820   +17     
========================================
+ Hits         12564    12577   +13     
- Misses        4283     4285    +2     
- Partials       956      958    +2

Impacted Files	Coverage Δ
config/errors.go	`45.16% <ø> (-3.23%)`	⬇️
config/config.go	`74.24% <71.42%> (-0.15%)`	⬇️
api/http/server.go	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

source-devs · 2023-04-09T18:39:07Z

Benchmark Results

Summary

0 Benchmarks successfully compared.
0 Benchmarks were ✅ Better.
0 Benchmarks were ❌ Worse .
0 Benchmarks were ✨ Unchanged.

✅ See Better Results...

	time/op		delta

❌ See Worse Results...

	time/op		delta

✨ See Unchanged Results...

	time/op		delta

🐋 See Full Results...

AndrewSisley

LGTM :)

fredcarle · 2023-04-11T15:43:42Z

Did you ensure that this doesn't affect autocert with a valid domain?

orpheuslummis · 2023-04-11T20:09:48Z

Did you ensure that this doesn't affect autocert with a valid domain?

thanks for asking Fred, in fact I just tested it now and it doesn't work. I'll adjust the code

config/config.go

shahzadlone

LGTM

source-devs · 2023-04-11T21:44:28Z

Benchmark Results

Summary

0 Benchmarks successfully compared.
0 Benchmarks were ✅ Better.
0 Benchmarks were ❌ Worse .
0 Benchmarks were ✨ Unchanged.

✅ See Better Results...

	time/op		delta

❌ See Worse Results...

	time/op		delta

✨ See Unchanged Results...

	time/op		delta

🐋 See Full Results...

config/config.go

Co-authored-by: Shahzad Lone <[email protected]>

config/config.go

source-devs · 2023-04-11T23:13:53Z

Benchmark Results

Summary

0 Benchmarks successfully compared.
0 Benchmarks were ✅ Better.
0 Benchmarks were ❌ Worse .
0 Benchmarks were ✨ Unchanged.

✅ See Better Results...

	time/op		delta

❌ See Worse Results...

	time/op		delta

✨ See Unchanged Results...

	time/op		delta

🐋 See Full Results...

config/config.go

source-devs · 2023-04-11T23:27:15Z

Benchmark Results

Summary

0 Benchmarks successfully compared.
0 Benchmarks were ✅ Better.
0 Benchmarks were ❌ Worse .
0 Benchmarks were ✨ Unchanged.

✅ See Better Results...

	time/op		delta

❌ See Worse Results...

	time/op		delta

✨ See Unchanged Results...

	time/op		delta

🐋 See Full Results...

config/errors.go

source-devs · 2023-04-11T23:33:20Z

Benchmark Results

Summary

0 Benchmarks successfully compared.
0 Benchmarks were ✅ Better.
0 Benchmarks were ❌ Worse .
0 Benchmarks were ✨ Unchanged.

✅ See Better Results...

	time/op		delta

❌ See Worse Results...

	time/op		delta

✨ See Unchanged Results...

	time/op		delta

🐋 See Full Results...

source-devs · 2023-04-11T23:34:42Z

Benchmark Results

Summary

0 Benchmarks successfully compared.
0 Benchmarks were ✅ Better.
0 Benchmarks were ❌ Worse .
0 Benchmarks were ✨ Unchanged.

✅ See Better Results...

	time/op		delta

❌ See Worse Results...

	time/op		delta

✨ See Unchanged Results...

	time/op		delta

🐋 See Full Results...

source-devs · 2023-04-11T23:47:53Z

Benchmark Results

Summary

0 Benchmarks successfully compared.
0 Benchmarks were ✅ Better.
0 Benchmarks were ❌ Worse .
0 Benchmarks were ✨ Unchanged.

✅ See Better Results...

	time/op		delta

❌ See Worse Results...

	time/op		delta

✨ See Unchanged Results...

	time/op		delta

🐋 See Full Results...

fredcarle

Thanks for taking care of this and dealing with all my comments. Just one final suggestion bellow.

config/errors.go

source-devs · 2023-04-12T01:34:06Z

Benchmark Results

Summary

0 Benchmarks successfully compared.
0 Benchmarks were ✅ Better.
0 Benchmarks were ❌ Worse .
0 Benchmarks were ✨ Unchanged.

✅ See Better Results...

	time/op		delta

❌ See Worse Results...

	time/op		delta

✨ See Unchanged Results...

	time/op		delta

🐋 See Full Results...

source-devs · 2023-04-12T02:12:28Z

Benchmark Results

Summary

0 Benchmarks successfully compared.
0 Benchmarks were ✅ Better.
0 Benchmarks were ❌ Worse .
0 Benchmarks were ✨ Unchanged.

✅ See Better Results...

	time/op		delta

❌ See Worse Results...

	time/op		delta

✨ See Unchanged Results...

	time/op		delta

🐋 See Full Results...

source-devs · 2023-04-12T02:13:07Z

Benchmark Results

Summary

0 Benchmarks successfully compared.
0 Benchmarks were ✅ Better.
0 Benchmarks were ❌ Worse .
0 Benchmarks were ✨ Unchanged.

✅ See Better Results...

	time/op		delta

❌ See Worse Results...

	time/op		delta

✨ See Unchanged Results...

	time/op		delta

🐋 See Full Results...

orpheuslummis added bug Something isn't working area/config Related to configuration labels Apr 9, 2023

orpheuslummis added this to the DefraDB v0.5 milestone Apr 9, 2023

orpheuslummis requested a review from a team April 9, 2023 18:27

orpheuslummis self-assigned this Apr 9, 2023

AndrewSisley approved these changes Apr 11, 2023

View reviewed changes

fix: API address param. validation

b7d8162

handle domain name validation

fd9f23c

orpheuslummis force-pushed the orpheus/fix/config-url-validation branch from 1122bac to fd9f23c Compare April 11, 2023 21:33

shahzadlone reviewed Apr 11, 2023

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

shahzadlone approved these changes Apr 11, 2023

View reviewed changes

shahzadlone reviewed Apr 11, 2023

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

orpheuslummis and others added 3 commits April 11, 2023 17:53

Shahzad's suggestion

233941d

Co-authored-by: Shahzad Lone <[email protected]>

Merge branch 'develop' into orpheus/fix/config-url-validation

0f30a69

require TLS to use port 443

8ee19ce

fredcarle reviewed Apr 11, 2023

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

orpheuslummis added 2 commits April 11, 2023 23:18

new error for TLS mismatch

cc2a6c3

Merge branch 'develop' into orpheus/fix/config-url-validation

c2a50fd

fredcarle requested changes Apr 11, 2023

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

apply feedback

30c16b5

fredcarle reviewed Apr 11, 2023

View reviewed changes

config/errors.go Outdated Show resolved Hide resolved

adjust to the non-localhost domain name requires port 80 rule

7a955ee

apply feedback by Fred

4b89488

fredcarle approved these changes Apr 12, 2023

View reviewed changes

config/errors.go Outdated Show resolved Hide resolved

orpheuslummis added 2 commits April 12, 2023 02:04

fix WithAddress

62be2aa

remove unneeded errors

44f75ce

orpheuslummis merged commit bbe8408 into develop Apr 12, 2023

orpheuslummis deleted the orpheus/fix/config-url-validation branch April 12, 2023 13:03

shahzadlone pushed a commit that referenced this pull request Apr 13, 2023

fix: API address parameter validation (#1311)

fe46cfb

shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024

fix: API address parameter validation (sourcenetwork#1311)

8d609ad

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: API address parameter validation #1311

fix: API address parameter validation #1311

orpheuslummis commented Apr 9, 2023

codecov bot commented Apr 9, 2023 •

edited

Loading

source-devs commented Apr 9, 2023

AndrewSisley left a comment

fredcarle commented Apr 11, 2023

orpheuslummis commented Apr 11, 2023

shahzadlone left a comment

source-devs commented Apr 11, 2023

source-devs commented Apr 11, 2023

source-devs commented Apr 11, 2023

source-devs commented Apr 11, 2023

source-devs commented Apr 11, 2023

source-devs commented Apr 11, 2023

fredcarle left a comment

source-devs commented Apr 12, 2023

source-devs commented Apr 12, 2023

source-devs commented Apr 12, 2023

fix: API address parameter validation #1311

fix: API address parameter validation #1311

Conversation

orpheuslummis commented Apr 9, 2023

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Apr 9, 2023 • edited Loading

Codecov Report

source-devs commented Apr 9, 2023

Benchmark Results

Summary

AndrewSisley left a comment

Choose a reason for hiding this comment

fredcarle commented Apr 11, 2023

orpheuslummis commented Apr 11, 2023

shahzadlone left a comment

Choose a reason for hiding this comment

source-devs commented Apr 11, 2023

Benchmark Results

Summary

source-devs commented Apr 11, 2023

Benchmark Results

Summary

source-devs commented Apr 11, 2023

Benchmark Results

Summary

source-devs commented Apr 11, 2023

Benchmark Results

Summary

source-devs commented Apr 11, 2023

Benchmark Results

Summary

source-devs commented Apr 11, 2023

Benchmark Results

Summary

fredcarle left a comment

Choose a reason for hiding this comment

source-devs commented Apr 12, 2023

Benchmark Results

Summary

source-devs commented Apr 12, 2023

Benchmark Results

Summary

source-devs commented Apr 12, 2023

Benchmark Results

Summary

codecov bot commented Apr 9, 2023 •

edited

Loading