launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/app/webservice/marshallers.py'
> --- lib/lp/app/webservice/marshallers.py	2012-01-01 02:58:52 +0000
> +++ lib/lp/app/webservice/marshallers.py	2018-09-25 18:05:37 +0000
> @@ -31,3 +44,49 @@
>          if (value is not None and getUtility(ILaunchBag).user is None):
>              return obfuscate_email(value)
>          return value
> +
> +
> +class InlineObjectFieldMarshaller(SimpleFieldMarshaller):
> +    """A marshaller that represents an object as a dict.
> +
> +    lazr.restful represents objects as URL references by default, but that
> +    isn't what we want in all cases.
> +
> +    To use this marshaller to read JSON input data, you must register an
> +    adapter from the expected top-level type of the loaded JSON data
> +    (usually `dict`) to the `InlineObject` field's schema.  The adapter will
> +    be called with the deserialised input data, with all inner fields
> +    already converted as indicated by the schema.
> +    """
> +
> +    def unmarshall(self, entry, value):
> +        """See `IFieldMarshaller`."""
> +        result = {}
> +        for name in self.field.schema.names(all=True):
> +            field = self.field.schema[name]
> +            if IField.providedBy(field):
> +                marshaller = getMultiAdapter(
> +                    (field, self.request), IFieldMarshaller)
> +                sub_value = getattr(value, name, field.default)
> +                try:
> +                    sub_entry = getMultiAdapter(
> +                        (sub_value, self.request), IEntry)
> +                except ComponentLookupError:
> +                    sub_entry = entry
> +                result[name] = marshaller.unmarshall(sub_entry, sub_value)
> +        return result
> +
> +    def _marshall_from_json_data(self, value):
> +        """See `SimpleFieldMarshaller`."""
> +        template = {}
> +        for name in self.field.schema.names(all=True):
> +            field = self.field.schema[name]
> +            if IField.providedBy(field):
> +                if field.required and name not in value:
> +                    raise RequiredMissing(name)
> +                if name in value:
> +                    marshaller = getMultiAdapter(
> +                        (field, self.request), IFieldMarshaller)
> +                    template[name] = marshaller.marshall_from_json_data(
> +                        value[name])
> +        return self.field.schema(template)

Should these both use representation_name from the marshaller rather than the normal field name, so they end up as foo_link rather than just foo for object references?

> 
> === modified file 'lib/lp/app/webservice/tests/test_marshallers.py'
> --- lib/lp/app/webservice/tests/test_marshallers.py	2012-01-01 02:58:52 +0000
> +++ lib/lp/app/webservice/tests/test_marshallers.py	2018-09-25 18:05:37 +0000
> @@ -128,3 +149,56 @@
>          webservice = LaunchpadWebServiceCaller()
>          etag_logged_out = webservice(ws_url(bug)).getheader('etag')
>          self.assertNotEqual(etag_logged_in, etag_logged_out)
> +
> +
> +class IInlineExample(Interface):
> +
> +    person = PersonChoice(vocabulary="ValidPersonOrTeam")
> +
> +    status = Choice(vocabulary=JobStatus)
> +
> +
> +@implementer(IInlineExample)
> +class InlineExample:
> +
> +    def __init__(self, person, status):
> +        self.person = person
> +        self.status = status
> +
> +
> +@adapter(dict)
> +@implementer(IInlineExample)
> +def inline_example_from_dict(template):
> +    return InlineExample(**template)
> +
> +
> +class TestInlineObjectFieldMarshaller(TestCaseWithFactory):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_unmarshall(self):
> +        field = InlineObject(schema=IInlineExample)
> +        request = WebServiceTestRequest()
> +        request.setVirtualHostRoot(names=["devel"])
> +        marshaller = InlineObjectFieldMarshaller(field, request)
> +        obj = InlineExample(self.factory.makePerson(), JobStatus.WAITING)
> +        result = marshaller.unmarshall(None, obj)
> +        self.assertThat(result, MatchesDict({
> +            "person": Equals(canonical_url(obj.person, request=request)),

e.g. this seems like it should be person_link.

> +            "status": Equals("Waiting"),
> +            }))
> +
> +    def test_marshall_from_json_data(self):
> +        self.useFixture(ZopeAdapterFixture(inline_example_from_dict))
> +        field = InlineObject(schema=IInlineExample)
> +        request = WebServiceTestRequest()
> +        request.setVirtualHostRoot(names=["devel"])
> +        marshaller = InlineObjectFieldMarshaller(field, request)
> +        person = self.factory.makePerson()
> +        data = {
> +            "person": canonical_url(person, request=request),
> +            "status": "Running",
> +            }
> +        obj = marshaller.marshall_from_json_data(data)
> +        self.assertThat(obj, MatchesStructure.byEquality(
> +            person=person, status=JobStatus.RUNNING))
> 
> === modified file 'lib/lp/code/interfaces/gitref.py'
> --- lib/lp/code/interfaces/gitref.py	2018-08-20 23:33:01 +0000
> +++ lib/lp/code/interfaces/gitref.py	2018-09-25 18:05:37 +0000
> @@ -392,6 +389,36 @@
>          """
>  
>  
> +class IGitRefEdit(Interface):
> +    """IGitRef methods that require launchpad.Edit permission."""
> +
> +    @export_read_operation()
> +    @operation_for_version("devel")
> +    def getGrants():
> +        """Get the access grants for this reference."""
> +
> +    @operation_parameters(
> +        grants=List(
> +            title=_("Grants"),
> +            # Really IGitNascentRuleGrant, patched in
> +            # _schema_circular_imports.py.
> +            value_type=InlineObject(schema=Interface)))
> +    @call_with(user=REQUEST_USER)
> +    @export_write_operation()
> +    @operation_for_version("devel")
> +    def setGrants(grants, user):
> +        """Set the access grants for this reference."""

May be worth calling out that other grants may apply via wildcards.

> +
> +
> +class IGitRef(IGitRefView, IGitRefEdit):
> +    """A reference in a Git repository."""
> +
> +    # XXX cjwatson 2015-01-19 bug=760849: "beta" is a lie to get WADL
> +    # generation working.  Individual attributes must set their version to
> +    # "devel".
> +    export_as_webservice_entry(as_of="beta")
> +
> +
>  class IGitRefBatchNavigator(ITableBatchNavigator):
>      pass
>  
> 
> === modified file 'lib/lp/code/model/gitref.py'
> --- lib/lp/code/model/gitref.py	2018-08-23 12:34:24 +0000
> +++ lib/lp/code/model/gitref.py	2018-09-25 18:05:37 +0000
> @@ -453,7 +475,10 @@
>  
>      @property
>      def commit_message_first_line(self):
> -        return self.commit_message.split("\n", 1)[0]
> +        if self.commit_message is not None:
> +            return self.commit_message.split("\n", 1)[0]
> +        else:
> +            return None

Is this meant to be here?

>  
>      @property
>      def has_commits(self):
> 
> === modified file 'lib/lp/code/model/gitrule.py'
> --- lib/lp/code/model/gitrule.py	2018-09-25 18:05:37 +0000
> +++ lib/lp/code/model/gitrule.py	2018-09-25 18:05:37 +0000
> @@ -117,6 +132,54 @@
>          getUtility(IGitActivitySet).logGrantAdded(grant, grantor)
>          return grant
>  
> +    def _validateGrants(self, grants):
> +        """Validate a new iterable of access grants."""
> +        for grant in grants:
> +            if grant.grantee_type == GitGranteeType.PERSON:
> +                if grant.grantee is None:
> +                    raise ValueError(
> +                        "Permission grant for %s has grantee_type 'Person' "
> +                        "but no grantee" % self.ref_pattern)
> +            else:
> +                if grant.grantee is not None:
> +                    raise ValueError(
> +                        "Permission grant for %s has grantee_type '%s', "
> +                        "contradicting grantee ~%s" %
> +                        (self.ref_pattern, grant.grantee_type,
> +                         grant.grantee.name))
> +
> +    def setGrants(self, grants, user):
> +        """See `IGitRule`."""
> +        self._validateGrants(grants)
> +        existing_grants = {
> +            (grant.grantee_type, grant.grantee): grant
> +            for grant in self.grants}
> +        new_grants = OrderedDict(
> +            ((grant.grantee_type, grant.grantee), grant)
> +            for grant in grants)
> +
> +        for grant_key, grant in existing_grants.items():
> +            if grant_key not in new_grants:
> +                grant.destroySelf(user)
> +
> +        for grant_key, new_grant in new_grants.items():
> +            grant = existing_grants.get(grant_key)
> +            if grant is None:
> +                new_grantee = (
> +                    new_grant.grantee
> +                    if new_grant.grantee_type == GitGranteeType.PERSON
> +                    else new_grant.grantee_type)
> +                grant = self.addGrant(new_grantee, user)

Should this set the permissions from the start, rather than firing a creation event and then a modification one?

> +            grant_before_modification = Snapshot(
> +                grant, providing=providedBy(grant))
> +            edited_fields = []
> +            for field in ("can_create", "can_push", "can_force_push"):
> +                if getattr(grant, field) != getattr(new_grant, field):
> +                    setattr(grant, field, getattr(new_grant, field))
> +                    edited_fields.append(field)
> +            notify(ObjectModifiedEvent(
> +                grant, grant_before_modification, edited_fields))
> +
>      def destroySelf(self, user):
>          """See `IGitRule`."""
>          getUtility(IGitActivitySet).logRuleRemoved(self, user)
> @@ -213,8 +276,41 @@
>          return "<GitRuleGrant [%s] to %s> for %r" % (
>              ", ".join(permissions), grantee_name, self.rule)
>  
> +    def toDataForJSON(self, media_type):
> +        """See `IJSONPublishable`."""
> +        if media_type != "application/json":
> +            raise ValueError("Unhandled media type %s" % media_type)
> +        request = get_current_browser_request()
> +        return {
> +            "grantee_type": self.grantee_type,
> +            "grantee": (
> +                canonical_url(self.grantee, request=request)
> +                if self.grantee is not None else None),

Similar to above, should this be grantee_link to match normal links?

> +            "can_create": self.can_create,
> +            "can_push": self.can_push,
> +            "can_force_push": self.can_force_push,
> +            }
> +

Does InlineObjectFieldMarshaller.unmarshall not work here?

>      def destroySelf(self, user=None):
>          """See `IGitRuleGrant`."""
>          if user is not None:
>              getUtility(IGitActivitySet).logGrantRemoved(self, user)
>          Store.of(self).remove(self)
> +
> +
> +@implementer(IGitNascentRuleGrant)
> +class GitNascentRuleGrant:
> +
> +    def __init__(self, grantee_type, grantee=None, can_create=False,
> +                 can_push=False, can_force_push=False):
> +        self.grantee_type = grantee_type
> +        self.grantee = grantee
> +        self.can_create = can_create
> +        self.can_push = can_push
> +        self.can_force_push = can_force_push
> +
> +
> +@adapter(dict)
> +@implementer(IGitNascentRuleGrant)
> +def nascent_rule_grant_from_dict(template):
> +    return GitNascentRuleGrant(**template)


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-permissions-webservice-ref/+merge/355608
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.