1454737 - Validate notification_emails lists in YAML files are parsed to lists of email addresses

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Chris H-C :chutten]

Some Scalar definitions in Scalars.yaml had notification_emails lists of the form:

notification_emails
  - [e-mail], [e-mail]

The yaml format for lists is

notification_emails
  - [e-mail]
  - [e-mail]

We ought to be able to do something to catch this. Maybe catch whitespace characters in email address list items?

Comment 1

[Alessio Placitelli [:Dexter]]

Set up for mentoring!

Comment 2

[Alessio Placitelli [:Dexter]]

To fix this:

- figure a way to detect if multiple email addresses are on the same line; while this might seem simple (at first), rules for email addresses [2] are fairly broad! Since we're basically allowing just @mozilla.com email addresses, we have some control on what goes in there; one simple way to fix this could be to detect spaces or commas in email addresses;
- create a function in parse_scalars.py, |validate_email_address|, that takes an input string and checks the logic above;
- let's add a new check in [1] that makes use of |validate_email_address|;
- let's add test coverage in [3]; create a new test file, for example "test_parse_scalars.py": see [4] for a rough guideline; try to parse a sample scalar definition with a "broken" email address (see comment 0) and one with a correct list of email addresses.
- make sure all the tests pass:

> ./mach test toolkit/components/telemetry/tests/python
> ./mach test toolkit/components/telemetry/tests/unit

[1] - https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/toolkit/components/telemetry/parse_scalars.py#161
[2] - https://stackoverflow.com/a/2049510/261698
[3] - https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/python
[4] - https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py#28

Comment 3

[Issei Horie [:is2ei]]

Hi,

Could I work on this issue?

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Issei Horie from comment #3)
> Hi,
> 
> Could I work on this issue?

Sure! Is anything unclear?

Comment 5

[Issei Horie [:is2ei]]

Attachment: bug-1454737.patch

I attached a patch.
Could you review it, please?

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8973244 [details] [diff] [review]
bug-1454737.patch

Review of attachment 8973244 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Issel, thank you for your contribution! This looks good, with a few things to change below. Let me know if anything is uncler.

Did you make sure all the tests pass?

::: toolkit/components/telemetry/parse_scalars.py
@@ +164,5 @@
>          if not self._strict_type_checks:
>              return
>  
> +        def validate_notification_email(notification_email):
> +            if any(c in notification_email for c in [',', ' ']):

can't this simply |return not any(...)|?

Moreover, let's add a comment above this line to explain that, while email validations rules are fairly broad, we manage the addresses here and we can just do this easy check.

@@ +174,5 @@
> +        notification_emails = definition.get('notification_emails')
> +        for notification_email in notification_emails:
> +            if not validate_notification_email(notification_email):
> +                raise ParserError(self._name + ' - invalid email address: ' + notification_email +
> +                                  '.\nSee: {}'.format(BASE_DOC_URL))

We recently landed a change that enables the parser to fail after detecting *all* the errors in the probe registry file. To enable that, let's add |.handle_later()| after |.format(BASE_DOC_URL))|.

::: toolkit/components/telemetry/tests/python/test_parse_scalars.py
@@ +42,5 @@
> +                                        strict_type_checks=True)
> +        self.assertEqual(sclr.notification_emails, ["[e-mail]", "[e-mail]"])
> +
> +    def test_invalid_email_address(self):
> +        SAMPLE_SCALAR_TYPE = """

nit: let's call this |SAMPLE_SCALAR_INVALID_ADDRESSES|

Comment 7

[Issei Horie [:is2ei]]

Attachment: bug-1454737.patch

Thanks!
I fixed the patch. Could you check it, please?

BTW, how do we deal with existing broken email addresses in the yaml file?

Comment 8

[Alessio Placitelli [:Dexter]]

Comment on attachment 8974007 [details] [diff] [review]
bug-1454737.patch

Review of attachment 8974007 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Issel, this looks good with the nit below addressed.

From a quick look there shouldn't be any wrong address in Scalars.yaml: if you can find one, let's fix it in another patch on this bug!

::: toolkit/components/telemetry/parse_scalars.py
@@ +167,5 @@
> +        def validate_notification_email(notification_email):
> +            return not any(c in notification_email for c in [',', ' '])
> +
> +        # Validate Email address
> +        # We just do easy check to make sure it does not contain space or comma.

nit: let's move this comment inside the validation function. Let's also rephrase it a bit "Perform simple email validation to make sure it doesn't contain spaces or commas."

Comment 9

[Issei Horie [:is2ei]]

Attachment: bug-1454737.patch

Thanks! I fixed the comment.
Could you review it, please?

Comment 10

[Alessio Placitelli [:Dexter]]

Comment on attachment 8974674 [details] [diff] [review]
bug-1454737.patch

Review of attachment 8974674 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me now, thanks! Let me push it to try for you.

Comment 11

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d9a03f473e6d2dbf94cce5d70c9948168f4eeb4

Comment 12

[Alessio Placitelli [:Dexter]]

Hey Issei,

looks like try highlighted a few different failures, could you please check them and fix the patch accordingly? See comment 11 for a link to the try push.

Comment 13

[Issei Horie [:is2ei]]

Attachment: bug-1454737.patch

Thanks! I updated the patch. Could you review it?
BTW, is it possible for me to run the try? If possible, I would like to run it before asking you for reviewing. I would like to make sure the patch will pass the try.

Comment 14

[Alessio Placitelli [:Dexter]]

Comment on attachment 8976118 [details] [diff] [review]
bug-1454737.patch

Review of attachment 8976118 [details] [diff] [review]:
-----------------------------------------------------------------

This is still failing both locally and on try. Please kindly test your patch following the instructions in comment 2 before attaching the updated one :)

::: toolkit/components/telemetry/tests/python/python.ini
@@ +1,2 @@
>  [test_histogramtools_non_strict.py]
> +[test_parse_scalars.py]
\ No newline at end of file

For some reason this chunk fails to apply and produces a weird result: `[test_parse_scalars.py][test_histogramtools_strict.py]`, which makes the test runner fail.

Comment 15

[Issei Horie [:is2ei]]

Thanks! It seems my branch is old. I will create branch from default again.

Comment 16

[Issei Horie [:is2ei]]

Attachment: bug-1454737.patch

Thanks Alessio!
I updated the patch. Could you please review it?

Comment 17

[Issei Horie [:is2ei]]

Attachment: bug-1454737.patch

Forgot to add new line...

Comment 18

[Alessio Placitelli [:Dexter]]

Comment on attachment 8979652 [details] [diff] [review]
bug-1454737.patch

Review of attachment 8979652 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, one last nit and we're good to go :)

::: toolkit/components/telemetry/tests/python/test_parse_scalars.py
@@ +23,5 @@
> +
> +
> +class TestParser(unittest.TestCase):
> +    def test_valid_email_address(self):
> +        SAMPLE_SCALAR_INVALID_ADDRESSES = """

nit: this should be SAMPLE_SCALAR_VALID_ADDRESSES, since we're testing valid ones in this.

Comment 19

[Issei Horie [:is2ei]]

Attachment: bug-1454737.patch

Thanks for your comment! I updated the patch.

Comment 20

[Alessio Placitelli [:Dexter]]

Comment on attachment 8979947 [details] [diff] [review]
bug-1454737.patch

Review of attachment 8979947 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good now, thanks! Let's try.. try again :)

Comment 21

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe20879048c3e67951113bb068cfa0b22c4521cb

Comment 22

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/521f86964bcc0becb8463fc2b4a206b410d5d099
Bug 1454737 - Validate notification_emails lists in YAML files are parsed to lists of email addresses r=Dexter

Comment 23

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/521f86964bcc