Refactor the webservice code #753

samj1912 · 2017-06-24T10:29:45Z

No description provided.

Sophist-UK · 2017-06-24T17:37:11Z

@samj1912 Are you planning a parallel PR to change any plugins which use webservices?

samj1912 · 2017-06-24T17:39:22Z

@Sophist-UK Yeah, I will be porting the plugins completely to be Picard 2.0 compatible.

mineo · 2017-07-03T18:59:14Z

picard/webservice/__init__.py

@@ -150,6 +79,8 @@ def __init__(self, parent=None):
        }
        self._init_queues()
        self._init_timers()
+        self.format_parser = None


I think the default parser should be a function that raises an exception (because if it's called, you've passed parse_format=True to some function on an object that's not configured for that).

(Nitpicking:) I also find the name format in this context not good, it actually refers to the either the content type or (in the case of the parse_format argument) the body of the response. Maybe just parser and parse_response or parse_response_body are better?

mineo · 2017-07-03T19:03:45Z

picard/webservice/__init__.py

-
-
-class XmlWebService(QtCore.QObject):
+class WebService(QtCore.QObject):


Is it a good idea to provide multiple WebService implementations? Each one would have its own set of queues, which I do find a bit weird...

mineo · 2017-07-03T19:04:53Z

picard/webservice/api_helpers.py

+                 queryargs=queryargs)
+
+
+class MBAPIHelper():


Why not make this one and the acoustid one simply inherit from APIHelper?

… parser

samj1912 · 2017-07-09T10:28:48Z

@mineo changes done

zas · 2017-07-09T11:47:41Z

picard/webservice/__init__.py

+                    response_parser = self.response_parser(parse_response_format)
+                    try:
+                        document = response_parser(reply)
+                    except Exception as e:


I think we should have a specific exception class here (ExceptionResponseParserUnknown?)

zas · 2017-07-09T11:50:31Z

picard/webservice/__init__.py

+        if name in cls.PARSERS:
+            return cls.PARSERS[name]['parser']
+        else:
+            log.error('Parser of type %s not found', name)


Raise ExceptionResponseParserUnknown here

zas · 2017-07-09T11:56:50Z

picard/webservice/__init__.py

 from picard import (PICARD_APP_NAME,
                    PICARD_ORG_NAME,
                    PICARD_VERSION_STR,
                    config,
                    log)
-from picard.const import (ACOUSTID_KEY,
-                          ACOUSTID_HOST,
+from picard.const import (ACOUSTID_HOST,


Those are now only used for request delays (REQUEST_DELAY dict below), i think it can be moved to API helpers as well. Not sure exactly how though ;)

zas · 2017-07-09T16:16:35Z

picard/webservice/api_helpers.py

+                          CAA_HOST,
+                          CAA_PORT)
+
+from picard.webservice import PICARD_VERSION_STR, CLIENT_STRING, REQUEST_DELAY


PICARD_VERSION_STR should be imported from picard

mineo · 2017-07-10T17:23:22Z

picard/webservice/__init__.py

@@ -60,79 +54,16 @@
                                                         PICARD_APP_NAME,
                                                         PICARD_VERSION_STR)))

+DEFAULT_RESPONSE_PARSER = "xml"


I have to admit that the name "Parser" always suggests to me that the thing in question itself is able to parse something.

mineo · 2017-07-10T17:24:47Z

picard/webservice/__init__.py

+                    response_parser = self.response_parser(parse_response_format)
+                    try:
+                        document = response_parser(reply)
+                    except UnknownResponseParserError as e:


Isn't it the call to self.response_parser above that would raise this exception?

mineo · 2017-07-10T17:25:33Z

picard/webservice/__init__.py


+class UnknownResponseParserError(Exception):


It would make sense to have this exception carry at least information about which parser is unknown.

mineo · 2017-07-10T17:27:49Z

picard/webservice/__init__.py

-        if xml:
-            request.setRawHeader(b"Accept", b"application/xml")
+        if parse_response_format:
+            request.setRawHeader(b"Accept", self.response_header(parse_response_format).encode('utf-8'))


For both the Accept and Content-Type headers it's more common to not refer to a format, but MIME or media types (see https://tools.ietf.org/html/rfc7231#section-3.1.1).

mineo · 2017-07-10T17:30:35Z

picard/webservice/__init__.py

+
+    @classmethod
+    def add_parser(cls, name, header, parser):
+        cls.PARSERS[name] = {'header': header, 'parser': parser}


For other cases where there's only a small set of dict keys (some in script.py and searchdialog.py) we're using collections.namedtuple.

mineo · 2017-07-10T17:31:33Z

picard/webservice/__init__.py

+        cls.PARSERS[name] = {'header': header, 'parser': parser}
+
+    @classmethod
+    def response_header(cls, name):


See above somewhere, this returns the mime type, not a complete header.

mineo · 2017-07-10T17:35:10Z

picard/webservice/__init__.py

-        host, port = ACOUSTID_HOST, ACOUSTID_PORT
-        body = self._encode_acoustid_args(args, format='json')
-        return self.post(host, port, '/v2/submit', body, handler, priority=True, important=False, mblogin=False)
+            log.error('Parser of type %s not found', name)


This will implicitly return None if the parser is not found (unlike response_parser, which raises an exception). Since this method is called earlier during the lifetime of an HTTP request, wouldn't it make sense to signal that error here already with something more than a log message?

mineo · 2017-07-10T17:40:48Z

picard/webservice/api_helpers.py

+            '<metadata xmlns="http://musicbrainz.org/ns/mmd-2.0#">%s</metadata>' % data)
+
+
+class APIHelper(object):


Deriving from object is not necessary in Python 3 anymore (but I fail to find the documentation about it everytime I look for it)

mineo · 2017-07-10T17:48:34Z

test/test_webservice.py

+import unittest
+from picard import config
+from picard.webservice import WebService
+from mock import patch, MagicMock


I'm a bit surprised the test passes on travis, the mock module included with Python 3 is only available as unittest.mock?

mineo · 2017-07-10T17:50:41Z

test/test_webservice.py

+        mock_add_task.assert_called()
+        self.assertIn(host, mock_add_task.call_args[0])
+        self.assertIn(port, mock_add_task.call_args[0])
+        self.assertIn("'GET'", repr(mock_add_task.call_args))


I'm 100% sure there's a better way to test this than looking at the repr of call_args :-) self.assertIn("GET", mock_add_task.call_args[0][0].args) will do it, for example.

samj1912 added 2 commits June 24, 2017 15:57

Refactor webservice code

2f3cdba

Update the refactored webservice changes in other files

d5287b4

samj1912 requested review from zas, mineo and phw June 24, 2017 10:29

Fix erronous code: Return data from api functions

6ba3d13

metabrainz deleted a comment Jun 24, 2017

Remove unexpected argument from post api helper

04a987b

metabrainz deleted a comment Jun 24, 2017

samj1912 force-pushed the wsref branch from 3201b4b to c7df13c Compare June 24, 2017 11:04

Fix codacy errors and style issues

d9f5a22

samj1912 force-pushed the wsref branch from c7df13c to d9f5a22 Compare June 24, 2017 11:08

Add file header

d5d31d5

samj1912 force-pushed the wsref branch 3 times, most recently from 1bf7eb3 to 1641d8c Compare June 24, 2017 11:53

Fix copy errors

ee97c4e

samj1912 force-pushed the wsref branch from 1641d8c to ee97c4e Compare June 24, 2017 11:54

zas approved these changes Jul 3, 2017

View reviewed changes

mineo reviewed Jul 3, 2017

View reviewed changes

samj1912 added 3 commits July 8, 2017 22:28

Refactor code for api helpers

ad96473

Rename variables appropriately and add exception on improper response…

c469531

… parser

Refactor the webservice code to allow hooking of response parsers

181d04e

zas reviewed Jul 9, 2017

View reviewed changes

samj1912 added 2 commits July 9, 2017 17:44

Move API constants and add Unknown parser error

0ef8fd0

Add basic code to test Webservice

dcaa3f4

samj1912 force-pushed the wsref branch from f17d622 to dcaa3f4 Compare July 9, 2017 16:08

zas reviewed Jul 9, 2017

View reviewed changes

samj1912 added 2 commits July 9, 2017 21:49

Fix imports

6145b0d

Add more tests for WebService

9df4041

zas merged commit 333b249 into metabrainz:master Jul 10, 2017

mineo reviewed Jul 10, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the webservice code #753

Refactor the webservice code #753

samj1912 commented Jun 24, 2017

Sophist-UK commented Jun 24, 2017

samj1912 commented Jun 24, 2017

mineo Jul 3, 2017

mineo Jul 3, 2017

mineo Jul 3, 2017

samj1912 commented Jul 9, 2017

zas Jul 9, 2017

zas Jul 9, 2017

zas Jul 9, 2017

zas Jul 9, 2017

samj1912 Jul 9, 2017

mineo Jul 10, 2017

mineo Jul 10, 2017

mineo Jul 10, 2017

mineo Jul 10, 2017

mineo Jul 10, 2017

mineo Jul 10, 2017

mineo Jul 10, 2017

mineo Jul 10, 2017

mineo Jul 10, 2017

mineo Jul 10, 2017



		class XmlWebService(QtCore.QObject):
		class WebService(QtCore.QObject):

		'<metadata xmlns="http://musicbrainz.org/ns/mmd-2.0#">%s</metadata>' % data)


		class APIHelper(object):

Refactor the webservice code #753

Refactor the webservice code #753

Conversation

samj1912 commented Jun 24, 2017

Sophist-UK commented Jun 24, 2017

samj1912 commented Jun 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samj1912 commented Jul 9, 2017

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