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

Introduce PropertyFormat to avoid validation errors that can lead to internal exceptions #248

Merged
merged 12 commits into from
Aug 26, 2024
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ public sealed record IntegrationProperty(string Name, PropertyType Type)

public string? Pattern { get; init; }

public PropertyFormat Format { get; init; } = PropertyFormat.None;
Copy link
Collaborator

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.


public bool IsValid(string? input, [MaybeNullWhen(true)] out string error)
{
switch (Type)
Expand Down Expand Up @@ -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))
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a test for that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is about mqtt:// and all other URL properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?
I didn't add mqtt and other schemes (e.g. smtp) at first since this URL field was specifically supposed to be used for HTTP requests, but sure - we can make it be more universal. Or maybe should we create two PropertyFormats - one for HTTP URLs, one for other types?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, sure, you're right. Therefore - I'll rename the "Url" PropertyFormat to "HttpUrl" and only add http and https as allowed schemas.
Should I also add a generic "Url" PropertyFormat? (for input[type="url"] on front-end, so in the end, there will be a separate Url and HttpUrl)

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is still the extra allocation per call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
}

Expand Down
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
Expand Up @@ -120,6 +120,12 @@
<data name="IntegrationPropertyAllowedValue" xml:space="preserve">
<value>Field is not an allowed value.</value>
</data>
<data name="IntegrationPropertyFormatEmail" xml:space="preserve">
<value>Field is not a valid e-mail.</value>
</data>
<data name="IntegrationPropertyFormatUrl" xml:space="preserve">
<value>Field is not a valid URL (remember about the protocol).</value>
</data>
<data name="IntegrationPropertyInvalidBoolean" xml:space="preserve">
<value>Not a valid boolean value.</value>
</data>
Expand Down
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])?)*$";
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ public sealed class IntegratedAmazonSESIntegration : IIntegration, IInitializabl

public static readonly IntegrationProperty FromEmailProperty = new IntegrationProperty("fromEmail", PropertyType.Text)
{
Pattern = Patterns.Email,
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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ public sealed partial class HttpIntegration : IIntegration
EditorLabel = Texts.Webhook_URLLabel,
EditorDescription = Texts.Webhook_URLHints,
IsRequired = true,
Summary = true
Summary = true,
Format = PropertyFormat.Url,
};

private static readonly IntegrationProperty HttpMethodProperty = new IntegrationProperty("Method", PropertyType.Text)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ public sealed partial class MailchimpIntegration : IIntegration

public static readonly IntegrationProperty FromEmailProperty = new IntegrationProperty("fromEmail", PropertyType.Text)
{
Pattern = Patterns.Email,
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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ public sealed partial class MailjetIntegration : IIntegration

public static readonly IntegrationProperty FromEmailProperty = new IntegrationProperty("fromEmail", PropertyType.Text)
{
Pattern = Patterns.Email,
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)
Expand Down
13 changes: 0 additions & 13 deletions backend/src/Notifo.Domain.Integrations/Patterns.cs

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
Expand Up @@ -438,7 +438,7 @@ When you have done this send me an /update command.</value>
<value>Send Always</value>
</data>
<data name="Webhook_URLHints" xml:space="preserve">
<value>The URL to your server endpoint.</value>
<value>The URL to your server endpoint. Remember about the protocol (http, https).</value>
</data>
<data name="Webhook_URLLabel" xml:space="preserve">
<value>URL</value>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,11 @@ public sealed partial class SmtpIntegration : IIntegration

public static readonly IntegrationProperty FromEmailProperty = new IntegrationProperty("fromEmail", PropertyType.Text)
{
Pattern = Patterns.Email,
Copy link
Collaborator

Choose a reason for hiding this comment

The 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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ public sealed class IntegrationPropertyDto
/// </summary>
public string? Pattern { get; set; }

/// <summary>
/// Format of the field, used to both validate the input and to provide hints to the user.
/// </summary>
public PropertyFormat Format { get; set; }

/// <summary>
/// The default value.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

A test with http://<IP> might be useful. Perhaps you also have it above

[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()
{
Expand Down
18 changes: 18 additions & 0 deletions frontend/src/app/pages/integrations/IntegrationDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,10 @@ export const IntegrationDialog = (props: IntegrationDialogProps) => {
propertyType = propertyType.max(property.maxLength, texts.validation.maxLengthFn);
}

if (property.format && property.format !== "None") {
propertyType = propertyType.formatI18n(property.format);
}

if (property.pattern) {
propertyType = propertyType.matches(new RegExp(property.pattern), texts.validation.patternFn);
}
Expand Down Expand Up @@ -271,6 +275,20 @@ export const FormField = ({ property }: { property: IntegrationPropertyDto }) =>
label={label} hints={property.editorDescription} />
);
} else {
if (property.format && property.format !== 'None') {
switch (property.format) {
case 'Email':
return (
<Forms.Email name={name}
label={label} hints={property.editorDescription} />
);
case 'Url':
return (
<Forms.Url name={name}
label={label} hints={property.editorDescription} />
);
}
}
return (
<Forms.Text name={name}
label={label} hints={property.editorDescription} />
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/app/pages/users/UserDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export const UserDialog = (props: UserDialogProps) => {
<Forms.Text name='emailAddress'
label={texts.common.emailAddress} />

<Forms.Text name='phoneNumber'
<Forms.Phone name='phoneNumber'
label={texts.common.phoneNumber} />

<Forms.Select name='preferredLanguage' options={coreLanguages}
Expand Down
3 changes: 3 additions & 0 deletions frontend/src/app/service/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7430,11 +7430,14 @@ export interface IntegrationPropertyDto {
maxLength?: number | undefined;
/** The pattern (for strings). */
pattern?: string | undefined;
/** Format of the field, used to both validate the input and to provide hints to the user. */
format: PropertyFormat;
/** The default value. */
defaultValue?: any | undefined;
}

export type PropertyType = "Text" | "Number" | "MultilineText" | "Password" | "Boolean";
export type PropertyFormat = "None" | "Email" | "Url";

export interface IntegrationCreatedDto {
/** The ID of the integration. */
Expand Down
8 changes: 8 additions & 0 deletions frontend/src/app/shared/components/Forms.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,14 @@ export module Forms {
);
};

export const Phone = ({ placeholder, ...other }: FormEditorProps) => {
return (
<Forms.Row {...other}>
<InputSpecial type='tel' name={other.name} placeholder={placeholder} />
</Forms.Row>
);
}

export const Url = ({ placeholder, ...other }: FormEditorProps) => {
return (
<Forms.Row {...other}>
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/app/shared/utils/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* Copyright (c) Sebastian Stehle. All rights reserved.
*/

import { ChannelCondition, ChannelRequired, ChannelSend, ConfirmMode, IsoDayOfWeek, SchedulingType, TopicChannel } from '@app/service';
import { ChannelCondition, ChannelRequired, ChannelSend, ConfirmMode, IsoDayOfWeek, PropertyFormat, SchedulingType, TopicChannel } from '@app/service';
import { texts } from '@app/texts';

export const CHANNELS = [
Expand Down Expand Up @@ -106,4 +106,4 @@ export const WEEK_DAYS: ReadonlyArray<Mode<IsoDayOfWeek | undefined>> = [{
}, {
value: 'Saturday',
label: texts.common.weekDays.saturday,
}];
}];
Loading