1432044 - comma separate fields in rules should strip whitespace from the beginning and end of each entry

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (backend), enhancement, P1)

Description

[Justin Wood (:Callek)]

So for Firefox 58 we specified some rules in balrog where it would match based on locale, but failed due to whitespace in the field.

The Locale field looked something like:

"de, en-US, hu, it, zh-TW"

When the application queries balrog the locale is only "en-US" without that surrounding whitespace.

I suggest we strip beginning and trailing whitespace around each locale, since no actual locale-code contains whitespace.

Comment 1

[[e-mail] (:bhearsum)]

My only hesitation here is that it might make things less legible - but we can improve that in the UI if it becomes an issue. I think it would be good to apply this to version and instructionSet as well - where we also support comma separated values.

The fix for this will need to both adjust any validators that parse the lists (such as https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/forms.py#L107), and adjust any web layer code to strip away the spaces before calling the database layer. There might be some clever way to do the latter with swagger, or it can be done in https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/rules.py

Comment 2

[Vikas Prasad Mahato [:vikas_mahato]]

Can I work on this?

Comment 3

[[e-mail] (:bhearsum)]

(In reply to Vikas Prasad Mahato [:vikas_mahato] from comment #2)
> Can I work on this?

Sure

Comment 4

[Vikas Prasad Mahato [:vikas_mahato]]

(In reply to Ben Hearsum (:bhearsum) from comment #1)
> The fix for this will need to both adjust any validators that parse the
> lists (such as
> https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/forms.
> py#L107), and adjust any web layer code to strip away the spaces before
> calling the database layer. There might be some clever way to do the latter
> with swagger, or it can be done in
> https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/rules.py

So I have to remove the spaces from the supplied locale value in https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/rules.py
Did I get it right?

Comment 5

[[e-mail] (:bhearsum)]

(In reply to Vikas Prasad Mahato [:vikas_mahato] from comment #4)
> (In reply to Ben Hearsum (:bhearsum) from comment #1)
> > The fix for this will need to both adjust any validators that parse the
> > lists (such as
> > https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/forms.
> > py#L107), and adjust any web layer code to strip away the spaces before
> > calling the database layer. There might be some clever way to do the latter
> > with swagger, or it can be done in
> > https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/rules.py
> 
> So I have to remove the spaces from the supplied locale value in
> https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/rules.py
> Did I get it right?

Yep. Be sure to update all of the endpoints that accept it. And please add a test or two.

Comment 6

[Vikas Prasad Mahato [:vikas_mahato]]

I have updated https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/rules.py to save "de, en-US, hu, it, zh-TW" as "de,en-US,hu,it,zh-TW" (without quotes). 
However I am not able to figure out what is the exact requirement of this bug. I have a few questions which require clarification:

1. What should the endpoints do when they receive comma separated locale info?
2. What are these endpoints we are referring to?

Comment 7

[[e-mail] (:bhearsum)]

(In reply to Vikas Prasad Mahato [:vikas_mahato] from comment #6)
> I have updated
> https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/rules.
> py to save "de, en-US, hu, it, zh-TW" as "de,en-US,hu,it,zh-TW" (without
> quotes). 
> However I am not able to figure out what is the exact requirement of this
> bug. I have a few questions which require clarification:
> 
> 1. What should the endpoints do when they receive comma separated locale
> info?

Strip it away (like you're doing above) before passing it to the database layer.

> 2. What are these endpoints we are referring to?

All endpoints that accept "locale". You can figure this out by looking at the swagger specs. For example, the "SCRulesItemBase" defines "locale" and is included by other endpoints:
https://github.com/mozilla/balrog/blob/master/auslib/web/admin/swagger/api.yaml#L4393

And is also defined in another file as part of RuleObject: https://github.com/mozilla/balrog/blob/master/auslib/web/common/swagger/definitions.yml#L344

Comment 8

[Vikas Prasad Mahato [:vikas_mahato]]

"SCRulesItemBase" is reference by 

 1. https://github.com/mozilla/balrog/blob/master/auslib/web/admin/swagger/api.yaml#L2129
 2. https://github.com/mozilla/balrog/blob/master/auslib/web/admin/swagger/api.yaml#L2283

The implementation for [1] lies in https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/rules.py#L195 which calls process_rule_form() 

So adding code to remove whitespaces in process_rule_form() should be enough as in the following commit ?
https://github.com/vikasmahato/balrog/blob/1432044/auslib/web/admin/views/rules.py#L26

Did I miss something?

Comment 9

[[e-mail] (:bhearsum)]

(In reply to Vikas Prasad Mahato [:vikas_mahato] from comment #8)
> "SCRulesItemBase" is reference by 
> 
>  1.
> https://github.com/mozilla/balrog/blob/master/auslib/web/admin/swagger/api.
> yaml#L2129
>  2.
> https://github.com/mozilla/balrog/blob/master/auslib/web/admin/swagger/api.
> yaml#L2283
> 
> The implementation for [1] lies in
> https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/rules.
> py#L195 which calls process_rule_form() 
> 
> So adding code to remove whitespaces in process_rule_form() should be enough
> as in the following commit ?
> https://github.com/vikasmahato/balrog/blob/1432044/auslib/web/admin/views/
> rules.py#L26
> 
> Did I miss something?

It looks like that should cover everything - yep!

Comment 10

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/balrog/pull/463

Comment 11

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/74fbddee6170c5084d92d5818a496dbbe56db6ef
Bug 1432044: Remove spaces from locale in process_rule_form (#463). r=bhearsum,allan-silva

Comment 12

[[e-mail] (:bhearsum)]

Vikas, it looks like this broke rule editing when the "locale" field is null. We get tracebacks like this:
balrogadmin_1  | [2018-03-05 19:25:39,955] ERROR in app: Exception on /scheduled_changes/rules [POST]
balrogadmin_1  | Traceback (most recent call last):
balrogadmin_1  |   File "/usr/local/lib/python2.7/site-packages/flask/app.py", line 1982, in wsgi_app
balrogadmin_1  |     response = self.full_dispatch_request()
balrogadmin_1  |   File "/usr/local/lib/python2.7/site-packages/flask/app.py", line 1614, in full_dispatch_request
balrogadmin_1  |     rv = self.handle_user_exception(e)
balrogadmin_1  |   File "/usr/local/lib/python2.7/site-packages/flask/app.py", line 1517, in handle_user_exception
balrogadmin_1  |     reraise(exc_type, exc_value, tb)
balrogadmin_1  |   File "/usr/local/lib/python2.7/site-packages/flask/app.py", line 1612, in full_dispatch_request
balrogadmin_1  |     rv = self.dispatch_request()
balrogadmin_1  |   File "/usr/local/lib/python2.7/site-packages/flask/app.py", line 1598, in dispatch_request
balrogadmin_1  |     return self.view_functions[rule.endpoint](**req.view_args)
balrogadmin_1  |   File "/usr/local/lib/python2.7/site-packages/connexion/decorators/decorator.py", line 66, in wrapper
balrogadmin_1  |     response = function(request)
balrogadmin_1  |   File "/usr/local/lib/python2.7/site-packages/connexion/decorators/validation.py", line 122, in wrapper
balrogadmin_1  |     response = function(request)
balrogadmin_1  |   File "/usr/local/lib/python2.7/site-packages/connexion/decorators/validation.py", line 293, in wrapper
balrogadmin_1  |     return function(request)
balrogadmin_1  |   File "/usr/local/lib/python2.7/site-packages/connexion/decorators/response.py", line 84, in wrapper
balrogadmin_1  |     response = function(request)
balrogadmin_1  |   File "/usr/local/lib/python2.7/site-packages/connexion/decorators/decorator.py", line 42, in wrapper
balrogadmin_1  |     response = function(request)
balrogadmin_1  |   File "/usr/local/lib/python2.7/site-packages/connexion/decorators/parameter.py", line 219, in wrapper
balrogadmin_1  |     return function(**kwargs)
balrogadmin_1  |   File "./auslib/web/admin/views/mapper.py", line 213, in scheduled_changes_rules_post
balrogadmin_1  |     return RuleScheduledChangesView().post()
balrogadmin_1  |   File "./auslib/web/admin/views/base.py", line 70, in decorated
balrogadmin_1  |     ret = request_handler(*args, transaction=trans, **kwargs)
balrogadmin_1  |   File "./auslib/web/admin/views/base.py", line 24, in decorated
balrogadmin_1  |     return f(*args, **kwargs)
balrogadmin_1  |   File "./auslib/web/admin/views/base.py", line 95, in post
balrogadmin_1  |     return self._post(*args, **kwargs)
balrogadmin_1  |   File "./auslib/web/admin/views/base.py", line 16, in decorated
balrogadmin_1  |     return f(*args, changed_by=username, **kwargs)
balrogadmin_1  |   File "./auslib/web/admin/views/rules.py", line 245, in _post
balrogadmin_1  |     rule_dict, mapping_values, fallback_mapping_values = process_rule_form(what)
balrogadmin_1  |   File "./auslib/web/admin/views/rules.py", line 28, in process_rule_form
balrogadmin_1  |     form_data['locale'] = ''.join(locale.split())
balrogadmin_1  | AttributeError: 'NoneType' object has no attribute 'split'


Can you look into a fix when you have a chance?

Comment 13

[Vikas Prasad Mahato [:vikas_mahato]]

Yes sure!

Comment 14

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/balrog/pull/464

Comment 15

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/b21546cb78b1c91a9d6de8c1e3558cb52936be36
bug 1432044: Fixed crash on updating rules with empty locale (#464). r=bhearsum

Comment 16

[[e-mail] (:bhearsum)]

This is now in production, thank you Vikas!

Comment 17

[Vikas Prasad Mahato [:vikas_mahato]]

My pleasure Ben!! :)