Bug 1336452: Scheduled Changes First group of APIs #355

aksareen · 2017-07-20T20:03:11Z

No description provided.

bhearsum

In addition to the comments inline below, I found an issue when testing through the UI: When I tried to schedule a delete for a rule I got: "comment isn't required when scheduling a 'delete' change". The error didn't show up in the UI, only in the error console.

bhearsum · 2017-07-25T13:17:13Z

auslib/web/admin/swagger/api.yaml

+        - $ref: '#/definitions/CSRFModel'
+        - $ref: '#/definitions/RoleModel'
+      required:
+        - role


csrf_token should be required as well (it's always required).

it is already a required field specified in the CSRFModel. here's the link . Role is explicitly required in some APIs and not in others, hence specified on this line

Ah, I didn't realize that the "required" sections stacked like that. Sweet!

bhearsum · 2017-07-25T13:23:01Z

auslib/web/admin/swagger/api.yaml


-    # Cannot have additionalProperties as false for strict validation ,else it disallows csrf_token or any other model's properties to be added
-    #additionalProperties: false
+  RulesNonNullable:


priority and backgroundRate are also non-nullable, but they aren't in this model. Can you find a better name for this?

what should I re-name it into ?

Maybe RulesAlwaysRequired, if that's actually the case?

No that's not correct. Some fields aren't always required. Seems like a misnomer. Also priority and backgroundRate are actually nullable in many cases when handling request/response in /rules APIs.

Any name that can automatically signify that model's fields can be of any type but 'null' will do here

bhearsum · 2017-07-25T13:26:41Z

auslib/web/admin/swagger/api.yaml

+        url: "http://mozilla-balrog.readthedocs.io/en/latest/admin_api.html#scheduled-changes-object"
+        description: "create scheduled change for rule."
+      parameters:
+        - name: change_type


You've created a model for this (SCChangeType) - why not use it here?

because this one is the query (optional) parameter passed for the /sc/rules post operation and is handled here.

bhearsum · 2017-07-25T13:27:48Z

auslib/web/admin/swagger/api.yaml

+          maxLength: 20
+          x-nullable: true
+          required: false
+        - name: sc_rule_body


This field is required when creating new scheduled changes.

this is a query parameter which may not be required if the change_type is passed in the request.json body

Sorry - I think I added this comment to the wrong line. It was meant for change_type.

Not always. 'change_type' can be fetched from both query and body on this line . Therefore, cannot have change_type in 'required' list of fields since it will break client requests.

Instead this condition is handled explicitly in the web layer code here

bhearsum · 2017-07-25T13:30:32Z

auslib/web/admin/swagger/api.yaml

+        description: "create scheduled change for rule."
+      parameters:
+        - name: change_type
+          in: query


This parameter shouldn't be coming in via the query.

same reason as above.

Hm, I don't think we actually pass change_type in the query -- we put it in the body. The code you linked to was added to support this client class: https://github.com/mozilla/balrog/blob/master/client/balrogclient/api.py#L242.

that's why we have to define both types of parameters (json body and query) in this case else requests will start failing for the client class. Also the query parameter has 'required' set as False too , hence it is optional.

bhearsum · 2017-07-25T13:42:59Z

auslib/web/admin/swagger/api.yaml

        description: The number of signoffs required for a given role
        type: ["integer", "string"]
        format: signoffs_required
        example: 2

+  SignoffsRequiredNullable:


Are you sure this model is required? I only see it being used in GETs to /scheduled_changes/X, and as far as I know, there's no way it can be null -- it's a primary key field in those tables.

I did see instances of this field getting set as null when operating GET on /scheduled_changes/required_signoffs/permissions or /scheduled_changes/required_signoffs/product. Also , one of the test cases failed when field was set to non-nullable. here's the test case

Ah, I see. I'd forgotten about the DELETE case for the required signoffs table. Carry on!

bhearsum · 2017-07-25T13:45:56Z

auslib/web/admin/swagger/api.yaml

+        description: JSON field specifying the actions that user can perform and the prdouct on which it has current permission to do so
+        example: "{'actions':['modify']}"
+
+  OptionsModel:


Good work extracting all of these permissions fields into better models.

bhearsum · 2017-07-25T13:49:23Z

auslib/web/admin/views/permissions.py

    def __init__(self):
        super(PermissionScheduledChangesView, self).__init__("permissions", dbo.permissions)

    @requirelogin
    def _post(self, transaction, changed_by):
-        change_type = connexion.request.json.get("change_type")
+        what = connexion.request.json


The pattern I see with "what" (copying the entire object, and then removing certain fields from it) is a bit hard to read. It would be better to explicitly copy all of the common fields here, and then copy in the extra fields for the specific change_types later.

Same goes for all of the other object types.

in that case this will cause us to maintain two list of fields for each "change_type". First, the swagger list of allowed fields is the exhaustive/super-set and each change_type will need to have explicit list of fields allowed, rest will still have to be filtered out.

Copying extra fields will imply that 3 separate list of allowed field values for each change_type defined in python code. It will be same as maintaining a Form like before.

You can do this by inverting the logic a bit, and copying any field that's not dependent on change_type first, and then copying the change_type specific ones later.

copying specific fields based on change types is same as using explicit wtfform for each change_type. (which filters out specific fields from request.json and copies into form object).

Since wtfforms already had inheritance, they were easily reusing 'when' and 'telemtry_%s' forms from forms.py for every S.C. operation.

If we decide to Maintain copy-if-present-field-list-for-each-change-type'
Pros : Simple and easier to maintain in some cases where no. of fields are low like for sc/permissions and sc/releases.

Cons: Harder to maintain when dozens of lists containing field names need to be maintained for each change_type even if some have common fields between them. Each change_type needs an explicit list. Ex: /sc/rules.

Also, another issue is that additionalProperties has to be always true(in order to use allOf swagger feature to re-use fields via models) i.e. any no. of extra parameters can be passed into the requests (UI passes extra json body arguments in case of DELETE operations) therefore, filtering out only required fields (maintained in explicit list for each of 3 change_type) for given change_type value seems better option rather than throw error for extra-field-present-in-json-body.

But on other hand, it is easier to simply copy entire request.json and filter out some specific fields afterwards too. Also, in case extra parameters get passed into the 'what' dictionary won't matter, since in the DB layer only the fields that match name of DB columns will get updated, rest will be simply ignored. Just need to make sure that some specific fields values are changed or filtered out first in web layer (around 3-5 fields) and simply pass the rest of the request.json dictionary . Easier to maintain than having explicit lists for each change_type

bhearsum · 2017-07-25T13:54:03Z

auslib/web/admin/views/scheduled_changes.py

-        columns = {k: v.data for k, v in form._fields.iteritems()}
-        sc_id = self.sc_table.insert(changed_by, transaction, **columns)
+        # Validate 'when' is not in past
+        if what.get("when", None) and int(what.get("when")) < getMillisecondTimestamp():


Is there a reason you're not doing this in sc_when_validator?

sc_when_validator if declared on a field will automatically validate the request and response bodies both. In case of response bodies , it will always return an error when attempting to fetch a S.C.(which was enacted) and its 'when' value (stored in DB) was in past.

bhearsum · 2017-07-25T13:55:41Z

auslib/web/admin/views/scheduled_changes.py

@@ -123,7 +121,7 @@ def _delete(self, sc_id, transaction, changed_by):
        if not sc:
            return Response(status=404, response="Scheduled change does not exist")

-        form = DbEditableForm(request.args)
+        form = DbEditableForm(connexion.request.args)


Please get rid of the Form usage here - once we're done converting to swagger there's no reason we should be using them at all.

Removing flask.request caused the sc/:namespace/int:sc_id API operations to break since request package was no longer defined. This form is being used in the ScheduledChangeView group of APIs. They haven't been migrated in this commit.

Ah, so this will be removed when you finish converting this endpoint? If so, sounds good for now.

Can you confirm that you'll be removing the forms later?

this particular form is being used for "/scheduled_changes/:namespace/:sc_id" endpoints. These endpoints will be migrated in the upcoming PR. Then once all the form-usage instances have been purged from the code, we can safely delete the models from forms.py and by the end of all API migrations, the entire forms.py file

aksareen · 2017-07-25T18:02:50Z

@bhearsum : regarding the comment above

In addition to the comments inline below, I found an issue when testing through the UI: When I tried to schedule a delete for a rule I got: "comment isn't required when scheduling a 'delete' change". The error didn't show up in the UI, only in the error console.

Looks like the UI is broken and is sending extra fields in the change_type = 'delete' request. Hence 'comment' which is not needed in scheduling a delete SC is also passed in request.json body. I assume other Rule model fields must be getting passed too. Looks like a quick fix to this problem is to pop out the non-required fields when "change_type" == "delete" rather than throw error

bhearsum · 2017-07-27T16:57:36Z

To follow-up on @aksareen's previous comment - I put together a proposal for an alternative way to do filtering (bhearsum@0a0b7c2) and I think we're on the same page there now.

bhearsum · 2017-07-31T17:39:02Z

auslib/web/admin/views/permissions.py

+                what["data_version"] = int(what["data_version"])
+
+        elif change_type != "insert":
+            return problem(400, "Bad Request", "Invalid or missing change_type")


This error is better raised in scheduled_changes.py rather than repeating it in each object type.

Makes sense. Also I think we can safely remove the else check entirely since for both change_type fields either present in query or in body already have the enum list defined(insert, delete, update).

Just tested out the via proof of concept. It fails for /sc/rules APIs since it checks first for change_type in body then automatically uses the value passed in query args without validating whether the value was passed or is it using the default 'None' , since validation of query argument can only be done if the value was passed at all. Hence, an explicit check is needed in the Parent Class in case change_type wasn't passed at all.

bhearsum · 2017-07-31T17:45:56Z

auslib/web/admin/views/permissions.py

-            return Response(status=400, response="Invalid or missing change_type")
+        what = {}
+        for field in connexion.request.json:
+            if change_type == "delete" and field == "options" or change_type == "insert" and field == "data_version":


If I remember correctly, we decided to ignore options in this case because the UI currently passes it? If so, please add a comment as a reminder.

UI is passing 'options' as null for change_type == delete. I must have made some err while testing the UI initially. Seems only /sc/rules API is sending extra fields for delete change_type. Removing this check too.

Here's the request.json for reference:
balrogadmin_1 | {u'username': u'aks', u'csrf_token': u'IjhhMjFlMDVlZmE1ZTNmMWE4OTI3ODk4NjRiODg4Zjc5NTk3ZDcwNTUi.DGE83g.j8gUHwVEvIUg99Mu3XrlMlwN-eo', u'change_type': u'delete', u'permission': u'rule', u'when': 1501539302167, u'data_version': 1, u'options': None}

bhearsum · 2017-07-31T17:46:28Z

auslib/web/admin/views/permissions.py

-            return Response(status=400, response="Invalid or missing change_type")
+        what = {}
+        for field in connexion.request.json:
+            if change_type == "delete" and field == "options" or change_type == "insert" and field == "data_version":


I'm surprised to see data_version being ignored here - is there a case where that is actually passed when change_type is insert?

not via the UI forms AFAIK. However, any client can still pass it accidentally and the newly created rule can end up with a non-single (not 1) data version value. Hence the precaution

We don't have any non-UI clients for these endpoints with the exception of a script that creates a scheduled Rule change with change_type="update". I'd rather not encourage more bad client behaviour -- any new clients that are passing bad data should fix themselves.

So, please remove any precautionary filtering like this, except for the Rule case noted above, and for cases where we know the UI is already doing bad things.

removing these checks

bhearsum · 2017-07-31T17:53:00Z

auslib/web/admin/views/permissions.py

    def __init__(self):
        super(PermissionScheduledChangesView, self).__init__("permissions", dbo.permissions)

    @requirelogin
    def _post(self, transaction, changed_by):
        change_type = connexion.request.json.get("change_type")
+        connexion.request.json.pop("csrf_token", None)


Don't pop this here, ignore it when you iterate over it. Removing items from dicts like this can make debugging a lot more difficult. For example: if I'm looking at a client request and see csrf_token in it, but then don't see it if I print connexion.request.json in this function, it's very confusing.

I thought since I'm ignoring csrf_token everytime when constricting what dict, rather simply delete it. Will do the suggested change.

bhearsum · 2017-07-31T17:55:40Z

auslib/web/admin/views/rules.py

+            for key in list(what):
+                if key not in ["telemetry_product", "telemetry_channel", "telemetry_uptake", "when", "rule_id",
+                               "data_version", "change_type"]:
+                    what.pop(key, None)


As I said before, please do this before copying the values into "what", not by removing them after. This should be happening in the loop at line 300.

Oh mea culpa. Rather than popping it out, simply ignoring the rest of fields while constructing 'what 'if change_type == 'delete'

bhearsum · 2017-07-31T17:58:38Z

auslib/web/admin/views/scheduled_changes.py

@@ -123,7 +121,7 @@ def _delete(self, sc_id, transaction, changed_by):
        if not sc:
            return Response(status=404, response="Scheduled change does not exist")

-        form = DbEditableForm(request.args)
+        form = DbEditableForm(connexion.request.args)


Can you confirm that you'll be removing the forms later?

bhearsum

Just one thing I found while doing some final testing with the UI. Other than that, everything looks good, and seems to work fine!

bhearsum · 2017-08-01T15:07:42Z

auslib/web/admin/swagger/api.yaml

+              - $ref: '#/definitions/CSRFModel'
+              - $ref: '#/definitions/SCChangeType'
+              - $ref: '#/definitions/SCTime'
+              - $ref: '#/definitions/DataVersionModel'


I think using DataVersionModel here is causing some problems with the UI. When I tried to add a new role requirement for an existing signoff I got this: https://screenshots.firefox.com/XPk0lWEWCYG1HH15/localhost

The requests we make for changes like that contain the following POST data:

{"product":"Firefox","role":"relman","csrf_token":"xxxxxxx","data_version":null,"channel":"beta","when":1501599718293,"change_type":"insert","signoffs_required":"1"}

Note that data_version is null because adding a new required signoff ends up a new row in the database - not modifying an existing one.

Modifying it to Nullable-Data-Version-Model

I just realized that data_version as Null isn't supposed to be passed at all since for change_type == 'insert' code the wtfForm (ScheduledChangeNewProductRequiredSignoffForm) does not use data_version field at all. It looks like another UI (similar to rules) issue where extra arguments are passed as Null when they aren't supposed to be passed at all for a particular change_type. Looks like I need to add a TODO comment in swagger spec to fix UI later since data_version should not be passed as 'null' during a POST operation for 'update' or 'delete' change_types (since 'insert' change_type form does not use data_version field at all and other two have constraint of data_version as strictly non-null integer value).

aksareen force-pushed the scheduled_changes_first branch from fa850f9 to ddee69f Compare July 22, 2017 20:45

bhearsum requested changes Jul 25, 2017

View reviewed changes

bhearsum mentioned this pull request Jul 26, 2017

bug 1384269: Fix revocation when more than one signoff has already happened. #356

Merged

aksareen force-pushed the scheduled_changes_first branch from ddee69f to 6d5dc12 Compare July 28, 2017 22:42

bhearsum reviewed Jul 31, 2017

View reviewed changes

aksareen added 5 commits August 1, 2017 02:22

Bug 1336452: Scheduled Changes Base APIs

c199067

Bug 1336452: Scheduled Changes Enact APIs

96f603c

Bug 1336452: Scheduled Changes SignOff APIs

eafd9dc

Addressed comments

714dbca

Addressed comments 2

7429202

aksareen force-pushed the scheduled_changes_first branch from 6d5dc12 to 7429202 Compare July 31, 2017 20:53

Removed precautionary checks

c17e42a

bhearsum requested changes Aug 1, 2017

View reviewed changes

Modifying DataVersion to Nullable

e3b1aa0

bhearsum approved these changes Aug 1, 2017

View reviewed changes

bhearsum merged commit 8da1568 into mozilla-releng:master Aug 1, 2017

aksareen deleted the scheduled_changes_first branch August 1, 2017 17:46

Bug 1336452: Scheduled Changes First group of APIs #355

Bug 1336452: Scheduled Changes First group of APIs #355

Conversation

aksareen commented Jul 20, 2017

bhearsum 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

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

aksareen Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aksareen Jul 25, 2017 • edited Loading

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

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

aksareen Jul 26, 2017 • edited Loading

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

aksareen commented Jul 25, 2017 • edited Loading

bhearsum commented Jul 27, 2017

Choose a reason for hiding this comment

aksareen Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

aksareen Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aksareen Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aksareen Jul 31, 2017 • edited Loading

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

bhearsum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aksareen Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

aksareen Jul 25, 2017 •

edited

Loading

aksareen Jul 25, 2017 •

edited

Loading

aksareen Jul 26, 2017 •

edited

Loading

aksareen commented Jul 25, 2017 •

edited

Loading

aksareen Jul 31, 2017 •

edited

Loading

aksareen Jul 31, 2017 •

edited

Loading

aksareen Jul 31, 2017 •

edited

Loading

aksareen Jul 31, 2017 •

edited

Loading

aksareen Aug 1, 2017 •

edited

Loading