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

Arciiix · 2024-07-04T16:19:41Z

Fixes #247 , more info there.

Can you please review my code for now so that I can make some appropriate changes?

Cheers

SebastianStehle

The main thing is that you have forgotten the server side validation and I am not so happy with client side validation yet. See comments for more infos. Continue with the good work :)

SebastianStehle · 2024-07-04T16:30:34Z

backend/src/Notifo.Domain.Integrations/Smtp/SmtpIntegration.cs

@@ -42,7 +42,6 @@ public sealed partial class SmtpIntegration : IIntegration

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


Can you put this property at the end, just like in the other property?. My brain is annoying sometimes ;)

SebastianStehle · 2024-07-04T16:30:49Z

frontend/src/app/pages/integrations/IntegrationDialog.tsx

+                if (property.format && property.format !== "None") {
+                    const format = FORMAT_REGEXPS.get(property.format);
+
+


Remove double line.

SebastianStehle · 2024-07-04T16:31:47Z

frontend/src/app/pages/integrations/IntegrationDialog.tsx

+
+
+                    if (format) {  
+                        propertyType = propertyType.matches(format, texts.validation.formatFn);


Yup already provides validation method for email and url. Just use that. There is also emailI18n and perhaps url18n for localized methods in the code base.

Heyy @SebastianStehle! I'm back.

I just realized that the Yup's provided URL validator doesn't accept localhost (and there is no option to change that - e.g. in validate.js you can set the config to { allowLocal: true }, but there is no such thing here since it's a wrapper around .match(urlRegex))

Here's how it looks like in Yup's source code

https://github.com/jquense/yup/blob/5a22c16dbba610050e85f123d389ddacaa92a0ad/src/string.ts#L24-L26

There is also an open pull request and issue on Yup's repo that suggests adding it.

Therefore - can I modify the codebase's urli18n method and use my own RegEx, inspired by the original one but accepting localhost? And in the future, if Yup implements it, we can migrate back to the provided method.

On one hand, maybe people wouldn't use localhost in a production environment of a deployed app, but there is always a chance if they do self-deploy (instead of using http://notifo.io) or just test, like I do.

What do you think?

It makes perfectly sense to support localhost :)

Go for the regex, good job. But if you can use a custom function or so I would prefer that to make the regex an internal implementation detail.

SebastianStehle · 2024-07-04T16:34:00Z

frontend/src/app/texts/en.ts

@@ -441,6 +441,7 @@ export const EN = {
        minItemsFn: (p: { label?: string; min: number }) => `${p.label} must have at least ${p.min || 0} items.`,
        minLengthFn: (p: { label?: string; min: number }) => `${p.label} must have at least ${p.min} characters.`,
        moreThanFn: (p: { label?: string; more: number }) => `${p.label} must be greater than ${p.more || 0}.`,
+        formatFn: (p: { label?: string }) => `${p.label} is not in a valid format for this type of field.`,


We have introduced the format property to provide better validation messages. Otherwise we could just use "pattern", which has a very generic method.

SebastianStehle · 2024-07-04T16:34:24Z

backend/src/Notifo.Domain.Integrations.Abstractions/IntegrationProperty.cs

@@ -39,6 +39,8 @@ public sealed record IntegrationProperty(string Name, PropertyType Type)

    public string? Pattern { get; init; }

+    public PropertyFormat Format { get; init; } = PropertyFormat.None;


Validation is missing in the TryGetString method.

Arciiix · 2024-07-17T11:32:29Z

Hey, just pushed the changes, can you please take a look if it's alright?

SebastianStehle

I have only 2 small things to improve.

Good job :)

SebastianStehle · 2024-07-17T14:11:27Z

backend/tests/Notifo.Domain.Tests/Integrations/IntegrationPropertyTests.cs

+        }
+
+        [Theory]
+        [InlineData("http://localhost/test")]


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

SebastianStehle · 2024-07-17T14:17:36Z

frontend/src/app/shared/utils/yup.ts

@@ -57,13 +61,25 @@ function atLeastOneStringI18n(this: Yup.ObjectSchema<any>) {
        });
 }

+function formatI18n(this: Yup.StringSchema, format: PropertyFormat) {


I would not make this. It creates a very weird dependency from a utility class to the domain layer (service). You only need this method in one place anyway, so it does not reduces the amount of code.

… test

Arciiix · 2024-07-17T16:16:41Z

Hey! Thank you so much! I'm happy that there were some things I could've improved :)

Just pushed the changes

Arciiix · 2024-07-17T16:21:02Z

Btw,
Do you think we should add a default case handling here with a simple console.warn() that the property format is not recognized?
You didn't do that in other places, so I didn't do it as well - I'd just like to hear your opinion.

https://github.com/Arciiix/notifo/blob/b1d331f386f0f62999666c4b9983d15384517dfd/frontend/src/app/pages/integrations/IntegrationDialog.tsx#L127-L136

SebastianStehle · 2024-07-17T16:44:45Z

No, I dont think so.

Arciiix · 2024-07-17T21:04:16Z

Okay.
So do you think everything's ready to merge? Or do you want me to do something more (I will unify the phone numbers in another PR)?

SebastianStehle · 2024-07-17T21:25:56Z

The build fails ;) ... so it is not ready yet.

Arciiix · 2024-07-18T11:29:24Z

I think I forgot to update the C#'s SDK (I only updated the TypeScript one)... stupid me ;)
Thank you for noting!

Arciiix · 2024-07-18T11:34:35Z

Actually never mind - I updated it as well.
I'll investigate a bit because this error seems to be weird - everything was working in my local env.

Arciiix · 2024-07-18T14:04:22Z

It's especially weird that the same tests that fail in the GitHub Action actually pass on my local machine

Do you think I should create a dummy (e.g. adding a comment) commit to re-run it again?

SebastianStehle · 2024-07-18T16:59:37Z

It also failed with previous commits. Just try it

SebastianStehle · 2024-07-18T20:58:47Z

backend/src/Notifo.Domain.Integrations.Abstractions/IntegrationProperty.cs

@@ -185,6 +185,7 @@ private bool TryGetString(string? input, [MaybeNullWhen(true)] out string error,

                    break;
                case PropertyFormat.Url:
+                    // We only allow http and https to enable the usage of URL field for HttpClient requests.
                    if (!Uri.TryCreate(input, UriKind.Absolute, out var uri) || !((string[])["http", "https"]).Contains(uri.Scheme, StringComparer.OrdinalIgnoreCase))


Please introduce a static or improve the query. But having an allocation for this is really weird ;)

Is there a test for that?

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

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?

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.

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.

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)

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 is still the extra allocation per call.

Oh, yeah, sure, I'm so sorry about forgetting to change that😅

Arciiix · 2024-07-18T20:59:09Z

Oh, so it apparently worked 🤣

I think I can quote the popular "My code doesn't work, I have no idea why" and "My code works, I have no idea why".

Also, it actually worked on the 2nd previous commit (8a9dbcf), but didn't work on the previous one which actually didn't change anything on the back-end side (b1d331f). That's weird.

Have a nice evening!

Arciiix · 2024-08-26T09:30:35Z

Just renamed the "Url" PropertyFormat to "HttpUrl". Can you take a look if everything is looking good?

SebastianStehle · 2024-08-26T13:36:54Z

Only the one thing with allocations.

SebastianStehle · 2024-08-26T18:05:37Z

backend/src/Notifo.Domain.Integrations.Abstractions/IntegrationProperty.cs

-                    if (!Uri.TryCreate(input, UriKind.Absolute, out var uri) || !((string[])["http", "https"]).Contains(uri.Scheme, StringComparer.OrdinalIgnoreCase))
+                    // 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))


You can just create a static readonly field with HttpSchemes, would have been shorter ;)

Should I do it in another pull request? I see you already merged this one :)

Yes, plz :)

Arciiix added 4 commits July 3, 2024 19:54

Add field format to integration property

253ba28

Remove global predefined patterns

6aa5cfe

Remove a useless value

2b912f2

Avoid checking for format if it's None

1b1188d

SebastianStehle reviewed Jul 4, 2024

View reviewed changes

Arciiix added 2 commits July 17, 2024 13:11

Apply code review changes, add server-side validation

1ca53fd

Add some tests for the PropertyFormat

8a9dbcf

SebastianStehle requested changes Jul 17, 2024

View reviewed changes

Remove unnecessary dependency between domain layer and utilities, add…

b1d331f

… test

Add some explanation comments

47bb6e6

SebastianStehle reviewed Jul 18, 2024

View reviewed changes

Rename Url PropertyFormat to HttpUrl

6149d80

Arciiix added 3 commits August 26, 2024 18:55

Refactor the URL schema checking code to avoid unnecessary allocation

6a36893

Move down the using statements for consistency

29f36c1

Add a missing file header

0a5c431

SebastianStehle reviewed Aug 26, 2024

View reviewed changes

SebastianStehle merged commit c9d7cf7 into notifo-io:main Aug 26, 2024
2 checks passed

Arciiix mentioned this pull request Aug 28, 2024

Introduce static field for the PropertyFormat to improve readability, add a test case #251

Merged

		if (property.format && property.format !== "None") {
		const format = FORMAT_REGEXPS.get(property.format);



		if (format) {
		propertyType = propertyType.matches(format, texts.validation.formatFn);

		@@ -39,6 +39,8 @@ public sealed record IntegrationProperty(string Name, PropertyType Type)

		public string? Pattern { get; init; }

		public PropertyFormat Format { get; init; } = PropertyFormat.None;

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

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

Conversation

Arciiix commented Jul 4, 2024

SebastianStehle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arciiix Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

SebastianStehle Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arciiix commented Jul 17, 2024

SebastianStehle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arciiix commented Jul 17, 2024

Arciiix commented Jul 17, 2024

SebastianStehle commented Jul 17, 2024

Arciiix commented Jul 17, 2024

SebastianStehle commented Jul 17, 2024

Arciiix commented Jul 18, 2024

Arciiix commented Jul 18, 2024

Arciiix commented Jul 18, 2024

SebastianStehle commented Jul 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arciiix commented Jul 18, 2024

Arciiix commented Aug 26, 2024

SebastianStehle commented Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arciiix Jul 16, 2024 •

edited

Loading

SebastianStehle Jul 16, 2024 •

edited

Loading

SebastianStehle commented Aug 26, 2024 •

edited

Loading