-
Notifications
You must be signed in to change notification settings - Fork 70
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
Introduce PropertyFormat to avoid validation errors that can lead to internal exceptions #248
Changes from all commits
253ba28
6aa5cfe
2b912f2
1b1188d
1ca53fd
8a9dbcf
b1d331f
47bb6e6
6149d80
6a36893
29f36c1
0a5c431
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,8 @@ public sealed record IntegrationProperty(string Name, PropertyType Type) | |
|
||
public string? Pattern { get; init; } | ||
|
||
public PropertyFormat Format { get; init; } = PropertyFormat.None; | ||
|
||
public bool IsValid(string? input, [MaybeNullWhen(true)] out string error) | ||
{ | ||
switch (Type) | ||
|
@@ -170,6 +172,32 @@ private bool TryGetString(string? input, [MaybeNullWhen(true)] out string error, | |
} | ||
} | ||
|
||
if (!string.IsNullOrWhiteSpace(input) && Format != PropertyFormat.None) | ||
{ | ||
switch (Format) | ||
{ | ||
case PropertyFormat.Email: | ||
if (!Regex.IsMatch(input, ValidationPatterns.Email)) | ||
{ | ||
error = Texts.IntegrationPropertyFormatEmail; | ||
return false; | ||
} | ||
|
||
break; | ||
case PropertyFormat.HttpUrl: | ||
// We only allow "http" and "https" schemas to enable the usage of URL field for HttpClient requests. | ||
if (!Uri.TryCreate(input, UriKind.Absolute, out var uri) | ||
|| (!string.Equals(uri.Scheme, "http", StringComparison.OrdinalIgnoreCase) && !string.Equals(uri.Scheme, "https", StringComparison.OrdinalIgnoreCase)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can just create a static readonly field with HttpSchemes, would have been shorter ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I do it in another pull request? I see you already merged this one :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, plz :) |
||
) | ||
{ | ||
error = Texts.IntegrationPropertyFormatHttpUrl; | ||
return false; | ||
} | ||
|
||
break; | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
// ========================================================================== | ||
// Notifo.io | ||
// ========================================================================== | ||
// Copyright (c) Sebastian Stehle | ||
// All rights reserved. Licensed under the MIT license. | ||
// ========================================================================== | ||
|
||
namespace Notifo.Domain.Integrations; | ||
|
||
public enum PropertyFormat | ||
{ | ||
None, | ||
Email, | ||
HttpUrl | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// ========================================================================== | ||
// Notifo.io | ||
// ========================================================================== | ||
// Copyright (c) Sebastian Stehle | ||
// All rights reserved. Licensed under the MIT license. | ||
// ========================================================================== | ||
|
||
namespace Notifo.Domain.Integrations; | ||
|
||
public static class ValidationPatterns | ||
{ | ||
// Taken from Yup's source code (validation library used on the front-end side). | ||
public const string Email = @"^[a-zA-Z0-9.!#$%&'*+\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$"; | ||
} |
This file was deleted.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,11 +42,11 @@ public sealed partial class SmtpIntegration : IIntegration | |
|
||
public static readonly IntegrationProperty FromEmailProperty = new IntegrationProperty("fromEmail", PropertyType.Text) | ||
{ | ||
Pattern = Patterns.Email, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you put this property at the end, just like in the other property?. My brain is annoying sometimes ;) |
||
EditorLabel = Texts.Email_FromEmailLabel, | ||
EditorDescription = Texts.Email_FromEmailDescription, | ||
IsRequired = true, | ||
Summary = true | ||
Summary = true, | ||
Format = PropertyFormat.Email, | ||
}; | ||
|
||
public static readonly IntegrationProperty FromNameProperty = new IntegrationProperty("fromName", PropertyType.Text) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -158,6 +158,77 @@ public void Should_not_fail_if_undefined_value_is_not_an_allowed_value(string? i | |
Assert.Equal("allowed", property.GetString(source)); | ||
} | ||
|
||
[Theory] | ||
[InlineData("localhost.com/test")] | ||
[InlineData("192.168.0.101")] | ||
[InlineData("randomString")] | ||
public void Should_fail_if_url_is_invalid(string? input) | ||
{ | ||
var source = new Dictionary<string, string> | ||
{ | ||
["key"] = input! | ||
}; | ||
|
||
var property = new IntegrationProperty("key", PropertyType.Text) | ||
{ | ||
Format = PropertyFormat.HttpUrl | ||
}; | ||
|
||
Assert.Throws<ValidationException>(() => property.GetString(source)); | ||
} | ||
|
||
[Theory] | ||
[InlineData("http://192.168.0.101/")] | ||
[InlineData("http://localhost/test")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A test with |
||
[InlineData("https://example.com/test?query=example")] | ||
[InlineData("http://login:[email protected]/random")] | ||
public void Should_get_url_if_value_is_valid(string? input) | ||
{ | ||
var source = new Dictionary<string, string> | ||
{ | ||
["key"] = input! | ||
}; | ||
|
||
var property = new IntegrationProperty("key", PropertyType.Text) | ||
{ | ||
Format = PropertyFormat.HttpUrl | ||
}; | ||
|
||
Assert.Equal(input, property.GetString(source)); | ||
} | ||
|
||
[Fact] | ||
public void Should_fail_if_email_is_invalid() | ||
{ | ||
var source = new Dictionary<string, string> | ||
{ | ||
["key"] = "invalidEmail" | ||
}; | ||
|
||
var property = new IntegrationProperty("key", PropertyType.Text) | ||
{ | ||
Format = PropertyFormat.Email | ||
}; | ||
|
||
Assert.Throws<ValidationException>(() => property.GetString(source)); | ||
} | ||
|
||
[Fact] | ||
public void Should_get_email_if_value_is_valid() | ||
{ | ||
var source = new Dictionary<string, string> | ||
{ | ||
["key"] = "[email protected]" | ||
}; | ||
|
||
var property = new IntegrationProperty("key", PropertyType.Text) | ||
{ | ||
Format = PropertyFormat.Email | ||
}; | ||
|
||
Assert.Equal(source["key"], property.GetString(source)); | ||
} | ||
|
||
[Fact] | ||
public void Should_get_value_if_all_requirements_are_met() | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validation is missing in the TryGetString method.