DXCDT-349: Relax url fields validation to allow to be set to empty #451
Conversation
sergiught
changed the title
sergiught
force-pushed
the
issue-429/DXCDT-349-fix-empty-urls
branch
from
a497558 to
cdfeee0
Compare
| Optional: true, | ||
| Computed: true, | ||
| ValidateFunc: validation.All( | ||
| internalValidation.IsURLWithNoFragment, |
There was a problem hiding this comment.
After some investigating in the api/v2 schemas it appears that we do allow urls with fragments. They just have to be https or empty.
There was a problem hiding this comment.
If this type of issue is frequent, we can consider in the future checking the validations directly against the ones in API2 code.
| t.Run(fmt.Sprintf("test case #%d", i), func(t *testing.T) { | ||
| var errorsAsString []string | ||
| _, actualErrors := IsURLWithHTTPSorEmptyString(testCase.inputURL, "theTestURL") | ||
| for _, actualError := range actualErrors { |
There was a problem hiding this comment.
We need here something we can compare as all the errors returned are pointers so we're transforming everything in a slice of strings.
Codecov ReportBase: 87.16% // Head: 87.31% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #451 +/- ##
==========================================
+ Coverage 87.16% 87.31% +0.14%
==========================================
Files 42 42
Lines 9264 9245 -19
==========================================
- Hits 8075 8072 -3
+ Misses 914 902 -12
+ Partials 275 271 -4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
internal/validation/validation.go
Outdated
|
|
||
| if u.Fragment != "" { | ||
| errors = append(errors, fmt.Errorf("expected %q to have a url with an empty fragment. %s", k, v)) | ||
| func IsURLWithHTTPSorEmptyString(i interface{}, s string) ([]string, []error) { |
There was a problem hiding this comment.
Should this be IsURLWithHTTPSOrEmptyString?
There was a problem hiding this comment.
IMO no, as the "O" is not part of "HTTPS" and it also reads easier.
A similar example was set in https://github.com/hashicorp/terraform-plugin-sdk/blob/8ffdd1ea219bc0bbb64c0e7a87bf081ad40ca3ee/helper/validation/web.go#L17.
internal/validation/validation.go
Outdated
| func IsURLWithHTTPSorEmptyString(i interface{}, s string) ([]string, []error) { | ||
| _, errors := validation.IsURLWithHTTPS(i, s) | ||
| for _, err := range errors { | ||
| if !strings.Contains(err.Error(), "url to not be empty") { |
There was a problem hiding this comment.
Should we check this some other way, instead of looking for a hardcoded error string? Maybe checking for empty string before the IsURLWithHTTPS validation?
There was a problem hiding this comment.
Wondering the same thing. Although that seems like too easy of a solution so wondering what we're missing.
There was a problem hiding this comment.
I wanted to avoid duplicating logic from within the IsURLWithHTTPS, specifically checking for the string type and then returning nil on an empty string. But I can skip using IsURLWithHTTPS and implement our own logic that's a tiny itsy bitsy bit more performant.
There was a problem hiding this comment.
Can't we keep IsURLWithHTTPS and do an empty string check before? No need to remove it. Note that it's not about performance, it's about depending on the error message string
There was a problem hiding this comment.
Though it's not a blocker for me.
There was a problem hiding this comment.
I updated this in 1007111 (#451) without making use of the IsURLWithHTTPS func from the SDK. I personally didn't like duplicating the type check and returning immediately if the url is empty as there was another check inside the sdk func for empty urls.
sergiught
force-pushed
the
issue-429/DXCDT-349-fix-empty-urls
branch
from
11aacd7 to
1007111
Compare
🔧 Changes
In this PR we are changing the validation for url fields to only allow HTTPS and empty strings.
📚 References
initiate_login_urionauth0_client#429🔬 Testing
Tests were updated to check for empty strings as well.
📝 Checklist