Progress towards working /judgements/ path by halfak · Pull Request #4 · wiki-ai/jade

halfak · 2017-11-21T21:46:51Z

No description provided.

adamwight · 2017-11-22T19:25:23Z

+def configure(bp, config, trusted_clients, centralauth, state, stream):

-def configure(bp, trusted_clients, source):
+    @bp.route("/v1/judgements/<string:context>", methods=["get"])


Swagger might be the best place to experiment with routes...

adamwight · 2017-11-25T11:14:13Z


-from ... import responses

+def configure(bp, config, trusted_clients, centralauth, state, stream):


Nesting these route functions under a top-level function rather than a class will make testing difficult, won't it? Aren't these inner functions inaccessible?

They are inaccessible at first, but they become part of bp. This actually provides a nice framework for testing because the bp object is a general routing abstraction. Rather than using global variables, shared resources are enclosed in the configure() function. This allows us to, for example, safely pass an arbitrary configuration in testing or when constructing the routes to use. We can also easily include mocks for trusted_clients, centralauth, etc.

Okay, thanks it sounds fine in that case. The function is pretty much equivalent to a class, which means we can safely have slightly different opinions about the best way to encapsulate, since they'll behave in roughly the same way :)

Oh interesting. I wonder if this would be better as a class. I think bp takes the place of a class. But we could make the functions themselves available publicly/directly.

adamwight · 2017-11-25T11:15:40Z

+        return flask.json.jsonify(judgements_doc)
+
+    @bp.route("/v1/judgements/<int:judgement_id>/preference", methods=["put"])
+    @util.authorized_user_action(


I like the decorator-based security.

adamwight · 2017-11-25T11:18:09Z

+        """
+        Sets the preference bit for a specific judgement and returns the event
+        """
+        preference = json.loads(request_values['preference'])


This could also fail to be well-formed json, what would happen in that case? I think it throws a ValueError, which we should catch.

Right, I don't do all the error handling when we're here. I think we might want to consider adding a decorator that will handle many error types and return a specific type of structured response. I haven't worked out all of the details for that yet. I'm not sure it will make sense (DRY, KISS) until it is half-implemented.

That sounds great.

Updates made! I put the logic for formatting an exc in the Exception classes themselves. I'm not sure how I feel about that. What do you think?

It looks really nice to use once implemented!

adamwight · 2017-11-25T11:19:25Z

+        return util.execute_and_log_or_error(state, proto_event)
+
+    @bp.route("/v1/judgements/", methods=["post"])
+    @util.authorized_user_action(


Per our IRC conversation, maybe we need to check authorization based on the wiki context instead.

Right. We should be able to access the request values (that include "context") inside of the authorized_user_action decorator.

That would work, and I guess it's fine to split request parsing, it's a good trade-off in exchange for nice encapsulation of the security aspect.

Note that authorized_user_action does all of the request pre-processing too (since that's necessary for decrypting data from trusted clients). So in a way, we're just sequencing request parsing. The request_values map roughly mimics flask.request.values with the same accessor methods (MultiDict).

adamwight · 2017-11-25T11:20:39Z

+    return decorator
+
+
+def execute_and_log_or_error(state, proto_event):


adamwight · 2017-11-25T11:24:51Z

+                gu_id = values['gu_id']
+            elif 'mwoauth_access_token' in flask.session:
+                values = flask.request.values
+                gu_id = flask.session.get('mwoauth_identity')['id']


I missed how we validated the access token here?

We don't. This will need to happen in connection with mwoauth. Given that that is easy and we've done it multiple times, I've skipped spec'ing it out.

Great. Maybe just a comment here saying that it's not really authenticating, so we don't accidentally deploy like this?

Ohh! Even better, I'll raise NotImplementedError()

Just noting that this still isn't protected, should raise the NotImplementedError.

halfak · 2017-11-27T15:33:34Z


-class TrustedClientVerificationError(RuntimeError):
-    pass
+class RequestError(RuntimeError):


implements JSONFormattable (make an interface)

halfak · 2017-11-27T15:40:58Z

+centralauth:
+  host: https://mediawiki.org
+
+actions:


Put rights inside of action type -- flexibility for other types of constraints.

adamwight

Happy to merge if you want to start a new PR to refine...

adamwight · 2017-11-27T15:31:20Z

-    pass
+class RequestError(RuntimeError):
+    "An error occured inside of JADE while processing a request"
+    TYPE = None


Please include codes in the base class, 'unspecified_type' and 'unspecified_subtype' or something. My reasoning is that we might accidentally throw one of these from code or fail to inherit TYPE, and the receiving end won't have certainty that the type and subtype are intentionally missing, vs. lost somehow.

Hmm. It seems that "None" is the right value here. Missing will cause a crash (AttributeError). We could check to see if the attribute exists before trying to access the value. Ultimately this will need to be represent-able in JSON. None is convenient because None --> null. If we were to use something like NotImplemented, then it would error when converting to JSON.

It seems to me that intentionally missing == not specified and preparing for lost somehow seems overbearing. Can you imagine a scenario where the TYPE or SUBTYPE is lost somehow?

I see now that you were proposing strings containing "unspecified_type" and "unspecified_subtype". It seems that one could use "not_specified" when they'd like to intentionally not specify whereas intentionally missing would be None/null.

If you feel strongly about this, no problem. My perspective is just that error handling needs to be absolutely hailstormproof, and leaving the potential for a value to be either string or null is risky for both the server and client. You're right about "unspecified" sending the wrong signal, perhaps "unimplemented_type" would be more clear, if we decide to take this string-only approach?

The whole point of null/None is mixing it with other types. Are you against the use of null/None in general?

We can pick this up later, no I'm not opposed to null in general, but in this specific case I am opposed. I'd like the error to be clear and not up for misinterpretation.

adamwight · 2017-11-27T15:34:37Z

+    def format_detail(self):
+        doc = {'exception': str(self.e)}
+        if __debug__:
+            ex_type, ex, tb = sys.exc_info()


Repeating this code doesn't seem right. Would be better to call a superclass method to get additional debug info, or even have the superclass call into the subclass to get class-specific details.

:P one liner repeats. We could definitely handle that better.

Yeah the repetition itself isn't what bothers me, it's that * author of a new error class needs to figure out whether this line will be necessary or not, which is not clear from the code, and * as seen below, it's important to have consistency between these lines.

adamwight · 2017-11-27T15:37:11Z

+        doc = {'exception': str(self.e)}
+        if __debug__:
+            ex_type, ex, tb = sys.exc_info()
+            return {'traceback': list(traceback.extract_tb(tb))}


Lost the rest of doc here. See the next implementation for correct doc['traceback'] = ...

Nice catch!

adamwight · 2017-11-27T15:44:55Z

+            return json2multidict(jwt.decode(
+                encoded_values, self.key_secrets[auth_key],
+                algorithms=[HASH_ALGORITHM]))
+        except (jwt.exceptions.InvalidTokenError,


I'm not 100% certain about the syntax or whether it makes sense, but we could define this list as a constant.

This was a pain because not all JWT errors inherit from a common base error type.

$ python Python 3.5.1+ (default, Mar 30 2016, 22:46:26) [GCC 5.3.1 20160330] on linux Type "help", "copyright", "credits" or "license" for more information. >>> error_types = (RuntimeError, ValueError) >>> try: ... raise RuntimeError() ... except error_types as e: ... print("It worked") ... except Exception as e: ... print("It didn't work") ... It worked

I can't believe that worked. :D

lol. Python and Ruby are amazing in that everything is a first-class object.

adamwight · 2017-11-27T15:45:24Z

+def error(e):
+    if not isinstance(e, errors.RequestError):
+        e = error.UnknownError(e)
+    return e.HTTP_CODE, flask.jsonify(e.format_json())


This scares me. Want to wrap it in a try-catch just in case?

Hey look what I found,
http://flask.pocoo.org/docs/0.12/patterns/apierrors/

If we're using enough of the flask machinery, we can implement the errorhandler decorator to catch RequestErrors.

Ladsgroup · 2017-12-02T19:57:52Z

+        return doc
+
+
+###############################################################################


I guess this #### should be removed :D

It's nice for visual separation of different groups of exceptions.

That could be done by supplying the groups of exceptions in different modules, then including each module here.

Ladsgroup · 2017-12-02T20:01:10Z

+    @util.authorized_user_action(
+        config, trusted_clients, centralauth, "new_judgement")
+    def new_judgement(gu_id, request_values):
+        """Creates a new judgement and returns the event"""


Per PEP257, it should be "Create" (with trailing s) as action verb

Ladsgroup · 2017-12-02T20:02:16Z

+        schema = request_values['schema']
+        data = json.loads(request_values['data'])
+
+        # Check that data represents a valid JSON blob based on the latest


Comments are usually bad sign and needed when the code is not explanatory enough, this comment is redundant because "state.schemas.validate(schema, data)" conveys the comment fully.

Ladsgroup · 2017-12-02T20:02:29Z

+        # schema version
+        state.schemas.validate(schema, data)
+
+        # Construct a new proto-event for the judgement


Ladsgroup · 2017-12-02T20:02:39Z

+
+def configure(config, bp, score_processor):
+
+    # /spec/


Ladsgroup · 2017-12-02T20:03:21Z

+
+
+def execute_and_log_or_error(state, proto_event):
+    # Try to execute and log the event


Ladsgroup · 2017-12-02T20:04:20Z

+
+    @bp.route("/v1/judgements/<string:context>/<string:type>/<int:id>/<string:schema>", methods=["get"])  # noqa
+    def get_entity_schema_judgements(context, type_, id_, schema):
+        """Gets all judgements for a specific entity schema"""


s/Gets/Get/

Add period sign at the end (PEP257)

Ladsgroup · 2017-12-02T20:08:24Z

+        # Execute the proto_event and construct full event.
+        return util.execute_and_log_or_error(state, proto_event)
+
+    @bp.route("/v1/judgements/", methods=["post"])


s/post/POST. I like that it support POST requests only.

Ladsgroup · 2017-12-02T20:08:41Z

+            context, type_, id_, schema)
+        return flask.json.jsonify(judgements_doc)
+
+    @bp.route("/v1/judgements/<int:judgement_id>/preference", methods=["put"])


Ladsgroup · 2017-12-02T20:09:26Z

+        return flask.json.jsonify(judgements_doc)
+
+    @bp.route("/v1/judgements/<string:context>/<string:type>", methods=["get"])
+    def get_entity_type_judgements(context, type_):


I like trailing _ here, so PEP8y <3

Ladsgroup · 2017-12-02T20:10:27Z

Lots and lots of nitpicky stuff, the logic looks okay to me, also would love to see tests :D

halfak · 2017-12-11T22:15:55Z

@adamwight and/or @Ladsgroup, take another look plz :)

Ladsgroup · 2017-12-16T17:54:48Z

LGTM

adamwight

This prototype is looking great.

Two broad comments,

This begs for tests
http://klen.github.io/py-frameworks-bench/ It might be worth looking at a more efficient mini-framework like falcon over flask, since we're already nicely decoupled from the specific choice of framework.

adamwight · 2017-12-18T16:40:54Z

@@ -0,0 +1,125 @@
+# Use lists


These examples would be better as test case expected outputs. It's unclear what API is being demonstrated here.

adamwight · 2017-12-18T16:42:41Z

-    pass
+class RequestError(RuntimeError):
+    "An error occured inside of JADE while processing a request"
+    TYPE = None


We can pick this up later, no I'm not opposed to null in general, but in this specific case I am opposed. I'd like the error to be clear and not up for misinterpretation.

adamwight · 2017-12-18T16:43:54Z

+        return doc
+
+
+###############################################################################


That could be done by supplying the groups of exceptions in different modules, then including each module here.

adamwight · 2017-12-18T16:48:05Z

+
+from . import v1
+
+PWD = os.path.dirname(os.path.abspath(__file__))


PWD is a confusing name, since it usually refers to the shell's cwd at application launch time, which will not be the same directory we're talking about here. Better to call this WSGI_CODE_DIR or something

adamwight · 2017-12-18T16:52:13Z

+def configure(config, bp, trusted_clients, centralauth, state):

-def configure(bp, trusted_clients, source):
+    @bp.route("/v1/judgements", methods=["GET"])


There should be some general TODOs documented, e.g. we'll need result set paging.

adamwight · 2017-12-18T16:56:20Z

+                gu_id = values['gu_id']
+            elif 'mwoauth_access_token' in flask.session:
+                values = flask.request.values
+                gu_id = flask.session.get('mwoauth_identity')['id']


Just noting that this still isn't protected, should raise the NotImplementedError.

Progress towards working /judgements/ path

cb9d678

adamwight requested changes Nov 25, 2017

View reviewed changes

Uniform error handling and in-context group checks.

30ce02a

halfak commented Nov 27, 2017

View reviewed changes

adamwight requested changes Nov 27, 2017

View reviewed changes

halfak added 2 commits November 30, 2017 17:23

Adds error handling, basic server structure, and swagger skeleton.

09425cd

Completes basic structure of judgements/ response in swagger.

24c2be0

Ladsgroup reviewed Dec 2, 2017

View reviewed changes

Comment thread jade/wsgi/routes/v1/spec.py

def configure(config, bp, score_processor):

# /spec/

Copy link
Copy Markdown

Member

Ladsgroup Dec 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

Ladsgroup reviewed Dec 2, 2017

View reviewed changes

Some PEP fixes.

a4ba24d

adamwight approved these changes Dec 18, 2017

View reviewed changes

adamwight merged commit 5ae9877 into master Dec 18, 2017


		from ... import responses

		def configure(bp, config, trusted_clients, centralauth, state, stream):

		return decorator


		def execute_and_log_or_error(state, proto_event):

		return doc


		###############################################################################



		def execute_and_log_or_error(state, proto_event):
		# Try to execute and log the event


		from . import v1

		PWD = os.path.dirname(os.path.abspath(__file__))

Conversation

halfak commented Nov 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamwight left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halfak Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halfak Nov 27, 2017 •

edited

Loading