-
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 6 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,29 @@ 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.Url: | ||
if (!Uri.TryCreate(input, UriKind.Absolute, out var uri) || !((string[])["http", "https"]).Contains(uri.Scheme, StringComparer.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. Please introduce a static or improve the query. But having an allocation for this is really weird ;) 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. Is there a test for that? 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. What is about mqtt:// and all other URL properties? 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. Hey! Sure, what other URL schemes do you think should be supported, besides http, https, mqtt? 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. The URL should support all schemes. If an integration needs a special URL, it could just use patterns I think. You usually also get a runtime error so we should not validate too much. 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. For me the advantage of the propertyFormat is that we can also use it in the UI and use specialized inputs for example. If we are just using that for validation we would be probably fine with regex. I don't want to make it too complicated. But if we introduce base classes and stuff like that it makes the UI super complicated. 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. Okay, sure, you're right. Therefore - I'll rename the "Url" 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. If a URL is not http or https they very like need additional validation, e.g. if it starts with "mongo://" or something that. So HttpUrl is fine. 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. There is still the extra allocation per call. 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. Oh, yeah, sure, I'm so sorry about forgetting to change that😅 |
||
{ | ||
error = Texts.IntegrationPropertyFormatUrl; | ||
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, | ||
Url | ||
} |
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,7 @@ | ||
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,76 @@ 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.Url | ||
}; | ||
|
||
Assert.Throws<ValidationException>(() => property.GetString(source)); | ||
} | ||
|
||
[Theory] | ||
[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.Url | ||
}; | ||
|
||
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.