AMP Command specification format

[teratorn]

Proposing a standard convention for specifying AMP Command structures for sharing between multiple applications, with clients and servers using different implementations of AMP in different languages.

JSON is chosen as the basic serialization format for maximum portability and ease of parsing.

An example of the proposed format:

{"commands" : {
    "Sum" : {
        "arguments" : [["a", "Integer"],
                       ["b", "Integer"]],
        "response"  : [["total", "Integer"]]
            },

    "Divide" : {
        "arguments" : [["numerator", "Float"],
                       ["denominator", "Float"]],
        "response"  : [["result", "Float"]],
            
        "errors"    : [["ZERO_DIVISION", "ZeroDivisionError"]]
               },

    "SumList" : {
        "arguments" : [["args", {"type" : "ListOf", "of" : "Integer"}],
                       ["basevalue", {"type" : "Integer",
                                      "optional" : true}]
                      ],
        "response"  : [["total", "Integer"]]
                },

    "SumPairs" : {
        "arguments" : [["args", {"type" : "TupleList",
                                 "of" : ["Integer", "Integer"]}]],
        "response"  : [["total", "Integer"]]
                 }
    }
}

Which, when parsed and loaded results in access to Command classes equivalent to:

class Sum(amp.Command):
    arguments = [['a', amp.Integer()], ['b', amp.Integer()]]
    response  = [['total', amp.Integer()]]
    
class Divide(amp.Command):
    arguments = [['numerator', amp.Float()],
                 ['denominator', amp.Float()]]
    response  = [['result', amp.Float()]]
    errors    = [['ZERO_DIVISION', ZeroDivisionError]]

class SumList(amp.Command):
    arguments = [['args', amp.ListOf(amp.Integer())],
                 ['basevalue', amp.Integer(optional=True)]]

    response = [['total', amp.Integer()]]
        
class SumPairs(amp.Command):
    arguments = [['args', TupleList(amp.Integer(), amp.Integer())]]
    response  = [['total', amp.Integer()]]

[teratorn]

(In [33707]) Branching to 'amp-json-spec-5532'

[teratorn]

Ready for review and feedback on JSON format

[teratorn]

Documentation of this feature is in #5543

[washort]

A few thoughts:

• fromFile seems like it oughta be a class method that creates a Spec.
• Seems kinda weird to have the type mapping defined in the code loading the spec. Should there be a default one that you can override as needed?

Review comments:

• SpecTestCase and SpecIntegrationTestCase need docstrings.
• Test methods in SpecTestCase need docstrings.
• MyAMP.sum in SpecIntegrationSetupTestCase.setUp needs whitespace around +.
• Coding standard says to put multiline addCallbacks etc chaining in parens and put dots at the start of the line.
• The integration tests make a connection over loopback. How hard would it be to use a fake transport?
• You're calling str() on a unicode object in Spec._loadCommand. This seems likely to surprise someone or at the very least tempt someone to change it later. If you really mean "convert to ASCII always without question" then typeSpec.encode("ascii") seems like a better choice. Alternately you could just leave it as a unicode string. and not call str.

• Oh, you're doing it in _parseArgSpec and _resolveType too.

[Jean-Paul Calderone]

A few extra notes:

1. Can we replace the name Spec too?
2. And maybe fromFile could be fromJSONFile? Or maybe fromJSON, and it could take a string instead of a file. Or even an already parsed json structure?
3. FilePath is better than os.path for the unit tests.
4. trial provides assertIsInstance
5. coding standard calls for assertEqual and not assertEquals.
6. There's a handful of other places where the tests don't follow the coding standard, too.

[teratorn]

(In [37600]) Branching to 'amp-json-spec-5532-2'

[teratorn]

I've addressed all the review comments in the current branch. My eyes can't pick out any more coding standards violations, but that doesn't mean I caught them all..

I've changed the proposed JSON format such that Command names are top-level keys under the "commands" key. This seems more natural to me (thanks dreid), and should map more cleanly to formats like YAML where indentation indicates structure.

Ready for review.

[Tom Prince]

(In [38006]) Cleanup some trailing whitespace and missing trailing newlines.

Refs: #5532

[Tom Prince]

1. Comments on the specification format.
   1. The exception names look python specific to me, which means that the specification isn't entirely language neutral. I'm not sure how best to address this.
   2. A related point, the format is unversioned. It would probably be good to give some thought on how it can be versioned.
   3. (followup tickets) The format can easily be turned into amp boxes
      1. It would be reasonable to define amp types for holding command specifications.
      2. Using the above specification, it should be possible to make amp responders introsepctable. That would make things like dash's json-to-amp bridge workable.
   4. There is a comment about the command name needing to be a valid python identifier.
      1. The amp implementation doesn't seem to enforce this, it accepts arbitrary bytestrings.
      2. The code in question actually just enforces that name be valid ascii.
      3. It is probably reasonable to limit it somehow. But we should be clear in what we require, and test that we require exactly that.
   5. The format of specifications should be documented (in doc/core/specifications/ perhaps)?
   6. 'of' seems somewhat ad-hoc. The current paramaterized types are containers, so of makes sense. Is that always going to be the case? I think the design could be improved here.
2. Comments on the implementation.
   1. Should the mappings for types and errors be seperate?
   2. There should also be some usage information in the amp howto.
   3. An instance of SchemaLoader doesn't actually load anything. It seems perhaps like it might be more appropriate to call it Schema? .fromJSON is actually the loader.
      1. This seems like it might relate to 1.4.1
      2. If this is called Schema, one might want to create instances from existing commands, rather than loading from a file. Relatedly, typeMapping doesn't seem like a relevant init argument for a Schmea`.
   4. I'm wary of trying to protect the user from modifying defaultTypeMapping, at least in the ad-hoc way it is done here. A quick look at help(dict) suggested a number of ways of evading the protection done in the branch.
      • I'm not opposed to having a frozen dict type, but I'm wary of putting a partial hack like this in.
   5. There doesn't appear to be any way to inspect the loaded commands, other than looking at dir(spec).
   6. (followup ticket) There doesn't seem to be any way to create a JSON specification from existing Command classes.
   7. SchemaLoaderIntegrationTestCase.test_sum_call should use successResultOf
      • The name doesn't follow the coding standard either.
   8. SchemaLoaderTestCase: The test docstrings should specify what 'correctly' means.
      • But, it might be easier to test smaller units of functionality.
        1. We don't care json.loads works.
        2. The command and argument parsing can probably be tested individally.

[Jean-Paul Calderone]

I wonder if there's any hope of getting this released as a third-party library before it's integrated into Twisted. I would love to try it out and provide feedback, and it's a lot more likely I'll be able to try out a separately released project rather than a development branch of Twisted.

[Tom Prince]

I'll second that suggestion. It does seem like something that could benefit from a short release cycle. (This seems like a common theme, with code like this. Perhaps something worth making more explicit?)

[teratorn]

Replying to Jean-Paul Calderone:

I wonder if there's any hope of getting this released as a third-party library before it's integrated into Twisted. I would love to try it out and provide feedback, and it's a lot more likely I'll be able to try out a separately released project rather than a development branch of Twisted.

if it's my ticket or branch, feel free not to wait, and branch it on github or wherever.

I would like to work more on this, but AMP isn't exactly a priority at the moment..

[teratorn]

Replying to Tom Prince:

I'll second that suggestion. It does seem like something that could benefit from a short release cycle. (This seems like a common theme, with code like this. Perhaps something worth making more explicit?)

note my reply above ^{- a lot of "small" things in twisted really would benefit from being quickly worked on outside of Twisted trunk - what do you suppose is an appropriate workflow these days? it's probably easier to just collaborate on github now than uh 7 years ago?}