Closed Bug 1454737 Opened Last year Closed Last year

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

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: chutten, Assigned: is2ei, Mentored)

References

Details

(Whiteboard: [lang=python][good-first-bug])

Attachments

(1 file, 6 obsolete files)

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

notification_emails
  - user1@domain.com, user2@domain.com

The yaml format for lists is

notification_emails
  - user1@domain.com
  - user2@domain.com

We ought to be able to do something to catch this. Maybe catch whitespace characters in email address list items?
Set up for mentoring!
Flags: needinfo?(alessio.placitelli)
Priority: -- → P3
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
Mentor: alessio.placitelli
Flags: needinfo?(alessio.placitelli)
Whiteboard: [lang=python][good-first-bug]
Hi,

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

Sure! Is anything unclear?
Assignee: nobody → is2ei.horie
Attached patch bug-1454737.patch (obsolete) — Splinter Review
I attached a patch.
Could you review it, please?
Attachment #8973244 - Flags: review?(alessio.placitelli)
Status: NEW → ASSIGNED
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, ["test01@mozilla.com", "test02@mozilla.com"])
> +
> +    def test_invalid_email_address(self):
> +        SAMPLE_SCALAR_TYPE = """

nit: let's call this |SAMPLE_SCALAR_INVALID_ADDRESSES|
Attachment #8973244 - Flags: review?(alessio.placitelli) → feedback+
Attached patch bug-1454737.patch (obsolete) — Splinter Review
Thanks!
I fixed the patch. Could you check it, please?

BTW, how do we deal with existing broken email addresses in the yaml file?
Attachment #8974007 - Flags: review?(alessio.placitelli)
Attachment #8973244 - Attachment is obsolete: true
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."
Attachment #8974007 - Flags: review?(alessio.placitelli) → feedback+
Attached patch bug-1454737.patch (obsolete) — Splinter Review
Thanks! I fixed the comment.
Could you review it, please?
Attachment #8974674 - Flags: review?(alessio.placitelli)
Attachment #8974007 - Attachment is obsolete: true
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.
Attachment #8974674 - Flags: review?(alessio.placitelli) → review+
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.
Flags: needinfo?(is2ei.horie)
Attached patch bug-1454737.patch (obsolete) — Splinter Review
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.
Attachment #8974674 - Attachment is obsolete: true
Flags: needinfo?(is2ei.horie)
Attachment #8976118 - Flags: review?(alessio.placitelli)
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.
Attachment #8976118 - Flags: review?(alessio.placitelli) → review-
Thanks! It seems my branch is old. I will create branch from default again.
Attached patch bug-1454737.patch (obsolete) — Splinter Review
Thanks Alessio!
I updated the patch. Could you please review it?
Attachment #8976118 - Attachment is obsolete: true
Attachment #8979651 - Flags: review?(alessio.placitelli)
Attached patch bug-1454737.patch (obsolete) — Splinter Review
Forgot to add new line...
Attachment #8979651 - Attachment is obsolete: true
Attachment #8979651 - Flags: review?(alessio.placitelli)
Attachment #8979652 - Flags: review?(alessio.placitelli)
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.
Attachment #8979652 - Flags: review?(alessio.placitelli) → feedback+
Thanks for your comment! I updated the patch.
Attachment #8979652 - Attachment is obsolete: true
Attachment #8979947 - Flags: review?(alessio.placitelli)
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 :)
Attachment #8979947 - Flags: review?(alessio.placitelli) → review+
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
https://hg.mozilla.org/mozilla-central/rev/521f86964bcc
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.