Mostly read-only REST API #45

bonzini · 2018-02-04T14:36:09Z

This pull request implements the beginning of #25 ("Add REST API").
The API is minimal on the write side (no login, no posting of messages), but it already includes:

the possibility to search and browse the database
a complex schema to easily retrieve the metadata for entire threads (aka "series") with a limited number of API calls
plugin integration
an embryo of a first-class "results" model, used by the git and testing plugins to hide the internal properties-based implementation

bonzini · 2018-02-04T14:56:17Z

The failure is only in Python 3.7 nightly builds, and is due to Django, not this pull request:

  File "/home/travis/virtualenv/python3.7-dev/lib/python3.7/site-packages/django/contrib/admin/widgets.py", line 152
    '%s=%s' % (k, v) for k, v in params.items(),
    ^
SyntaxError: Generator expression must be parenthesized

famz · 2018-02-06T02:23:09Z

api/rest.py

+
+class IsMaintainerOrReadOnly(permissions.BasePermission):
+    """
+    Allows access only to admin users.


"only to maintainers"?

"only to admin users or maintainers"

famz · 2018-02-06T02:57:15Z

api/rest.py

+        return results
+    results = SerializerMethodField()
+
+    def __init__(self, *args, **kwargs):


Is it easier to read if we move init to the first method in the class?

famz · 2018-02-06T03:00:23Z

I haven't looked in depth into django-rest-framework and the nested router plugin. But the PR looks great in general.

The question I have is testing. Is the plan to swtich patchew-cli or the web interface to be a client? (I think both are good things) And are you going to add cases to tests/?

bonzini · 2018-02-06T10:45:22Z

Is the plan to swtich patchew-cli or the web interface to be a client? (I think both are good things)

Yes and no respectively (I prefer the web interface to do everything server-side, I don't like single-page web apps and they are beyond my limited knowledge of Django!).

However, even for patchew-cli, people are using patchew-project/patchew's patchew-cli as a canonical source of "a command line tha works with patchew.org" so that should only be done after the REST API is deployed on patchew.org.

And are you going to add cases to tests/?

Yes, and I'll do that before pushing this.

bonzini · 2018-02-06T14:56:21Z

Needless to say, adding tests found some bugs. :) To compare between v1 and v2 you can use branches rest-v1 and rest in bonzini/patchew.

famz · 2018-02-08T09:27:58Z

tests/test_rest.py

+# Copyright 2017 Red Hat, Inc.
+#
+# Authors:
+#     Fam Zheng <famz@redhat.com>


I don't mind but you can update the year and the name if you want. :)

famz · 2018-02-09T00:57:14Z

tests/test_git.py

+If you prefer to skip this patch, run "git am --skip" instead.
+To restore the original branch and stop patching, run "git am --abort".
+Failed to apply patch:
+[Qemu-devel] [PATCH] bar patch""")


I think the actual error message depends on the git version. I'm getting this on Fedora (git version 2.14.3):

Switched to a new branch '20160628014747.20971-2-famz@redhat.com' Branch 20160628014747.20971-2-famz@redhat.com set up to track local branch test by rebasing. Applying: bar patch error: sha1 information is lacking or useless (foo). error: could not build fake ancestor Patch failed at 0001 bar patch The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Failed to apply patch: [Qemu-devel] [PATCH] bar patch

I.e. there is one extra line in the middle:

Branch 20160628014747.20971-2-famz@redhat.com set up to track local branch test by rebasing.

Maybe simply remove the self.assertEquals(log.content, expected_log) check?

I'll change it to a status code test (must be 200).

famz · 2018-02-09T00:58:00Z

tests/test_git.py

+To XXXXXX/repo
+ * [new tag]         patchew/20160628014747.20971-1-famz@redhat.com -> patchew/20160628014747.20971-1-famz@redhat.com
+""")
+        self.assertEquals(log.content, expected_log)


The same error here for me.

famz · 2018-02-09T01:18:58Z

I am done with an incremental review, the code looks good. Thank you for working on this, and adding tests! I think we are almost ready to go!

Properties are handled differently between ListProjectView and GetProjectPropertiesView. ListProjectView was showing them even to non-authenticated users, while for GetProjectPropertiesView had no allowed_groups so only superuser was allowed. Project properties _can_ contain sensitive data, so they should be hidden from non-maintainers. However, GetProjectPropertiesView's behavior is too restrictive. After this patch, ListProjectView only includes the "properties" key when logged in as a maintainer, and GetProjectPropertiesView returns 403 (via a pre-existing, but pointless until now, maintained_by check) for non-maintainers. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Small English improvement before adding it to the REST API. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

This will make it a bit simpler to test the REST API, using the same idioms used by other test modules.

Start with user and project objects, just to test the setup of django-rest-framework. The bulk of the work is in the series and message API endpoints.

The series view is already pretty complicated. First, because the same series can exist in two projects, it has two endpoints: /api/v1/series and /api/v1/projects/NN/series/. List results from the former always point to the latter. This is handled via NestedDefaultRouter in urls.py and ProjectMessagesViewSetMixin in rest.py. Second, it includes the list of patches and the list of "replies" to the cover letter and the thread, but only in the "detailed" view. Therefore, this commit adds a whole hierarchy of serializers for messages.

Search support is trivial to add using a new filter backend. The code is mostly based on DRF's search filter, so that the search is available from the API browser, but of course it uses Patchew's existing code to convert from search terms to querysets.

The message endpoint only exists under /projects, since it is not listable--message URIs can be discovered via the series REST API, or via the /api/v1/projects/NN/messages/ID/replies route. Compared to the information under /series, it lets you retrieve the full mbox contents (and, when plugins support will be added, the tags that are parsed from replies). The mbox can also be retrieved as a simple text/plain response.

Plugins expose a JSON schema that is unrelated to project and message properties. They can add arbitrary fields to the serializers, or add to a "results" dictionary that could in the future become a first-class entity in the database.

We already do that for senders, do that for recipients and add a migration to fix the database.

bonzini · 2018-02-09T10:08:02Z

Done! As before, for an incremental review you can use branch rest-v2 (I rebased so that travis passes, but the changes to the series are identified easily).

As discussed on IRC, I've added a migration to fix UTF-8 recipients and a test case for that too. You still cannot search for UTF-8 recipients because of how they are encoded in JSON works, but that's a separate bug in api.search.

famz · 2018-02-12T13:05:59Z

Looks good. Except this error when migrating the live patchew.org database (let me know if you want access the server):

(venv) fam@lemon:~/work/patchew$ ./manage.py migrate
Loaded module: footer
Loaded module: diff
Loaded module: email
Loaded module: git
Loaded module: tags
Loaded module: testing
Operations to perform:
  Apply all migrations: admin, api, auth, contenttypes, sessions
Running migrations:
  Applying api.0020_auto_20180204_0647... OK
  Applying api.0021_fix_recipients_utf8...Traceback (most recent call last):
  File "./manage.py", line 19, in <module>
    execute_from_command_line(sys.argv)
  File "/stor/work/patchew/venv/lib64/python3.6/site-packages/django/core/management/__init__.py", line 364, in execute_from_command_line
    utility.execute()
  File "/stor/work/patchew/venv/lib64/python3.6/site-packages/django/core/management/__init__.py", line 356, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/stor/work/patchew/venv/lib64/python3.6/site-packages/django/core/management/base.py", line 283, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/stor/work/patchew/venv/lib64/python3.6/site-packages/django/core/management/base.py", line 330, in execute
    output = self.handle(*args, **options)
  File "/stor/work/patchew/venv/lib64/python3.6/site-packages/django/core/management/commands/migrate.py", line 204, in handle
    fake_initial=fake_initial,
  File "/stor/work/patchew/venv/lib64/python3.6/site-packages/django/db/migrations/executor.py", line 115, in migrate
    state = self._migrate_all_forwards(state, plan, full_plan, fake=fake, fake_initial=fake_initial)
  File "/stor/work/patchew/venv/lib64/python3.6/site-packages/django/db/migrations/executor.py", line 145, in _migrate_all_forwards
    state = self.apply_migration(state, migration, fake=fake, fake_initial=fake_initial)
  File "/stor/work/patchew/venv/lib64/python3.6/site-packages/django/db/migrations/executor.py", line 244, in apply_migration
    state = migration.apply(state, schema_editor)
  File "/stor/work/patchew/venv/lib64/python3.6/site-packages/django/db/migrations/migration.py", line 129, in apply
    operation.database_forwards(self.app_label, schema_editor, old_state, project_state)
  File "/stor/work/patchew/venv/lib64/python3.6/site-packages/django/db/migrations/operations/special.py", line 193, in database_forwards
    self.code(from_state.apps, schema_editor)
  File "/stor/work/patchew/api/migrations/0021_fix_recipients_utf8.py", line 24, in fix_utf8_recipients
    recipients = [[_parse_header(x[0]), x[1]] for x in recipients]
  File "/stor/work/patchew/api/migrations/0021_fix_recipients_utf8.py", line 24, in <listcomp>
    recipients = [[_parse_header(x[0]), x[1]] for x in recipients]
  File "/stor/work/patchew/api/migrations/0021_fix_recipients_utf8.py", line 12, in _parse_header
    h = h.decode(c or 'utf-8')
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xed in position 8: invalid continuation byte

bonzini · 2018-02-12T13:12:01Z

I'll drop the last patch then. I think the culprit is this patch.

bonzini · 2018-02-12T13:53:16Z

Oh, it's actually a wrong record in the database:

                "name": "=?UTF-8?q?Daniel=20D=EDaz?=",
                "address": "daniel.diaz@linaro.org"

Thunderbird "correctly" shows it as Daniel D�az.

bonzini added the feature This is a user-visible feature label Feb 4, 2018

bonzini requested a review from famz February 4, 2018 14:36

famz reviewed Feb 6, 2018

View reviewed changes

bonzini force-pushed the rest branch from 2146b64 to 543740d Compare February 6, 2018 14:54

famz reviewed Feb 8, 2018

View reviewed changes

famz reviewed Feb 9, 2018

View reviewed changes

bonzini added 13 commits February 9, 2018 10:05

rename message field "receivers" to "recipients"

e6dd30d

Small English improvement before adding it to the REST API. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

add cli_import to PatchewTestCase

2d3f2d3

use real mbox in test_testing

9850745

This will make it a bit simpler to test the REST API, using the same idioms used by other test modules.

initial REST framework support

2614f53

Start with user and project objects, just to test the setup of django-rest-framework. The bulk of the work is in the series and message API endpoints.

add plugin support to the REST API

e8fba4a

Plugins expose a JSON schema that is unrelated to project and message properties. They can add arbitrary fields to the serializers, or add to a "results" dictionary that could in the future become a first-class entity in the database.

add REST API support to the diff plugin

6f718ef

add REST API support to the tags plugin

443e54f

add REST API support to the testing plugin

223a0c2

add REST API support to the git plugin

4bded3e

bonzini force-pushed the rest branch from 543740d to 4bded3e Compare February 9, 2018 09:07

parse MIME words in recipients

7db12a2

We already do that for senders, do that for recipients and add a migration to fix the database.

bonzini closed this Feb 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mostly read-only REST API #45

Mostly read-only REST API #45

bonzini commented Feb 4, 2018

bonzini commented Feb 4, 2018

famz Feb 6, 2018

bonzini Feb 6, 2018

famz Feb 6, 2018

famz commented Feb 6, 2018

bonzini commented Feb 6, 2018

bonzini commented Feb 6, 2018 •

edited

famz Feb 8, 2018

famz Feb 9, 2018

bonzini Feb 9, 2018

famz Feb 9, 2018

famz commented Feb 9, 2018

bonzini commented Feb 9, 2018

famz commented Feb 12, 2018 •

edited

bonzini commented Feb 12, 2018

bonzini commented Feb 12, 2018

Mostly read-only REST API #45

Mostly read-only REST API #45

Conversation

bonzini commented Feb 4, 2018

bonzini commented Feb 4, 2018

famz Feb 6, 2018

Choose a reason for hiding this comment

bonzini Feb 6, 2018

Choose a reason for hiding this comment

famz Feb 6, 2018

Choose a reason for hiding this comment

famz commented Feb 6, 2018

bonzini commented Feb 6, 2018

bonzini commented Feb 6, 2018 • edited

famz Feb 8, 2018

Choose a reason for hiding this comment

famz Feb 9, 2018

Choose a reason for hiding this comment

bonzini Feb 9, 2018

Choose a reason for hiding this comment

famz Feb 9, 2018

Choose a reason for hiding this comment

famz commented Feb 9, 2018

bonzini commented Feb 9, 2018

famz commented Feb 12, 2018 • edited

bonzini commented Feb 12, 2018

bonzini commented Feb 12, 2018

bonzini commented Feb 6, 2018 •

edited

famz commented Feb 12, 2018 •

edited