canonical / acceptable Goto Github PK

There seems to be a big in the linter. If you add a new api with an introduced_at without a change_log entry it will be ok but once the api.json is updated it will start failing, there seems to be an issue where the 'new' metadata code path ignores this check.

It would benefit from a rewrite and use of a better approach.

We use black elsewhere, it would be helpful to add it to this project too.

• Close all the very old issues
• Move project to canonical
• Rename master to main
• Release on GitHub
• Release on PyPI
• Update a single internal service without breaking something

SN-1142

From https://openapi.tools we have identified potential consumers of OpenAPI schemas, but they only support OpenAPI 3.0. Options include:

1. translate 3.1 schemas to 3.0, consume from there
2. update consumers to ingest 3.1 schemas

This ticket explores option 2. (It's probably worth trialing these tools first via option 1 to confirm they add value.)

Connexion 🐍 📝

• spec-first/connexion#1396
  • python-jsonschema/jsonschema#782
  • python-jsonschema/jsonschema#817
  • python-openapi/openapi-spec-validator#111
  • Other issues for 3.1 support in https://github.com/p1c2u/openapi-spec-validator ?
    • None. Now supports 3.1.
  • See also spec-first/connexion#1395

Fuzzers

• https://github.com/matusf/openapi-fuzzer (Rust 🦀 ) no clear plan to adopt 3.1
• https://github.com/vwt-digital/openapi3-fuzzer/tree/master (Python 🐍 ) no clear plan to adopt 3.1
• https://github.com/KissPeter/APIFuzzer (Python 🐍 ) no clear plan to adopt 3.1

See also

https://www.openapis.org/blog/2021/02/16/migrating-from-openapi-3-0-to-3-1-0

The changelog entries for API docs that acceptable created are currently sorted reverse ASCII-betically. Not as one would expect.

e.g. 9, 8, 7, 10, 11 instead of 11, 10, 9, 8, 7

Running on a focal system, with python 3.7 installed from the deadsnakes ppa, I updated tox to also test the python3.7 and 3.8 interpreters.
The python 3.7 works fine, but the python 3.8 is having some failed unittests.

Most (all?) seem to be related by an offset in the generated files, which cause a off by one error. I.e. results are expected on line 11, but are now found on line 12. I do think is only affects the tests and not the actual code.

It would be nice if 'make test' generated nothing except the regular unittest output, but, the following are warnings printed to the terminal from a successful 'make test'

1. ❌ TODO

WARNING: Testing via this command is deprecated and will be removed in a future version. Users looking for a generic test entry point independent of test runner are encouraged to use tox.
/snap/bin/documentation-builder

2. ✅ Fixed: https://github.com/canonical-ols/acceptable/pull/98/files

/home/jhartley/src/acceptable/acceptable/tests/test_main.py:315:
    json_dict = json.load(open(json_file.name))
ResourceWarning: unclosed file <_io.TextIOWrapper name='/tmp/tmps8bl8hth' mode='r' encoding='UTF-8'>

3. ❌ TODO

/snap/documentation-builder/42/ubuntudesign/documentation_builder/operations.py:135:
    'content': yaml.load(metadata_file.read()) or {}
YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated,
as the default Loader is unsafe. Please read https://msg.pyyaml.org/load for full details.

4. ❌ TODO

Notice: No folder found at '/home/jhartley/tmp175zcv_d/media' - not copying media

5. ✅ Fixed: https://github.com/canonical-ols/acceptable/pull/98/files

/home/jhartley/src/acceptable/env/lib/python3.5/site-packages/testtools/testcase.py:723:
    return self._get_test_method()()
ResourceWarning: unclosed file <_io.TextIOWrapper name='examples/api.json' mode='r' encoding='UTF-8'>

Acceptable should allow manually written documentation to be included in the output, and linked to easily within docstrings.

It might be best to assume we are generating a documentation subtree for the API rather than the entire documentation.

The CommandParser class has changed and will no longer accept the cmd parameter on init. This makes this part of the code fail when using a newer version of django

  File "/home/woutervb/projects/software-center-agent/env/lib/python3.8/site-packages/acceptable/management/commands/acceptable.py", line 38, in add_arguments
    metadata_parser = subparser.add_parser(
  File "/usr/lib/python3.8/argparse.py", line 1135, in add_parser
    parser = self._parser_class(**kwargs)
  File "/home/woutervb/projects/software-center-agent/env/lib/python3.8/site-packages/acceptable/management/commands/acceptable.py", line 33, in __init__
    super().__init__(cmd, **kwargs)
TypeError: __init__() takes 1 positional argument but 2 were given

If you want to share parts of schemas across APIs you will find that introduced_at is a problem. If you have an older and newer API the older API may have introduced_at version that are not relevant to new one (because it was added after that version).

A simple fix would be to ignore/strip introduced_at when it is less than the parents implied introduced_at.

When a field is complex and repeated several times, it makes total sense to "factor it out" to a module variable, and use it several times in the different validations.

This improve readability, makes easier to understand the API by humans, and reduces the "error pronning".

The problem is: eval_literal fails if Names are found.

So, let's resolve them! I hacked this for a real case, and it worked just fine, let me know if this has a future:

--- env/lib/python3.5/site-packages/acceptable/_build_doubles.py	2018-07-19 09:47:46.446747916 -0300
+++ _build_doubles.py	2018-07-19 09:47:29.503077728 -0300
@@ -140,6 +140,30 @@
             return schemas
 
 
+def get_simple_assignments(tree):
+    """Get simple assignments from node tree."""
+    result = {}
+    for node in ast.walk(tree):
+        if isinstance(node, ast.Assign):
+            for target in node.targets:
+                if isinstance(target, ast.Name):
+                    result[target.id] = node.value
+    return result
+
+
+class SimpleNamesResolver(ast.NodeTransformer):
+
+    def __init__(self, names_values):
+        super().__init__()
+        self.names_values = names_values
+
+    def visit_Name(self, node):
+        print("============ node, its there", node.id, node.id in self.names_values)
+        if node.id in self.names_values:
+            node = self.names_values[node.id]
+        return node
+
+
 def extract_schemas_from_source(source, filename='<unknown>'):
     """Extract schemas from 'source'.
 
@@ -156,6 +180,8 @@
     acceptable_views = {}
     schemas_found = []
     ast_tree = ast.parse(source, filename)
+    simple_names = get_simple_assignments(ast_tree)
+    print("============ simple names", simple_names)
 
     assigns = [n for n in ast_tree.body if isinstance(n, ast.Assign)]
     call_assigns = [n for n in assigns if isinstance(n.value, ast.Call)]
@@ -238,8 +264,11 @@
                     # TODO: Check that nothing in the tree below
                     # decorator.args[0] is an instance of 'ast.Name', and
                     # print a nice error message if it is.
+                    print("========== dec args", decorator.args[0])
+                    SimpleNamesResolver(simple_names).visit(decorator.args[0])
                     input_schema = ast.literal_eval(decorator.args[0])
                 if decorator_name == 'validate_output':
+                    SimpleNamesResolver(simple_names).visit(decorator.args[0])
                     output_schema = ast.literal_eval(decorator.args[0])
         for api_options in api_options_list:
             schema = ViewSchema(

acceptable lint is asking for an introduced_at on all nodes under a new property. Only the top level should need one when its new.
Errors on:
"common-ids": {
"type": "array",
"items": {"type": "string"},
"introduced_at": 20,
},
Wants:
"common-ids": {
"type": "array",
"items": {"type": "string", "introduced_at": 20},
"introduced_at": 20,
},

SN-1114

Our service includes this single endpoint supporting multiple methods:

create_perms_api = service.api('/permissions', 'create_permissions', methods=['POST'])

delete_perms_api = service.api('/permissions', 'delete_permissions', methods=['DELETE'])

get_perms_api = service.api('/permissions', 'get_permissions', methods=['GET'])

But the produced openapi.yaml only includes the GET schema:

paths:
  /permissions:
    get:
      ...
  /another-path:
...

The permissions entry should include the schemas for post and delete.

Problem statement: We have observed at least one case of a regenerating an openapi.yaml file just to reorder elements. This causes diff noise.

Proposed solution: Alphabetize the YAML. Ideally we would sort/tidy according to OpenAPI conventions, but that is not required to close this ticket.

This is a double bug (using a real life case in the examples):

1. the URL in the CONTENTS section at the right is badly built (right now it's https://api.snapcraft.io/docs/metadata.html#click_metadata-deprecated, when it should be https://api.snapcraft.io/docs/metadata.html#click_metadata.

2. the anchor in the "center web page" is also wrong, right now is <a class="anchor" href="#click_metadata-"></a>, it should be <a class="anchor" href="#click_metadata"></a>

The test doubles that acceptable creates use AST parsing to try and understand what the schema is for a given API endpoint, but this is limited and hard to do without duplicating a full blown Python interpreter.

There should be enough information in the api.json files that acceptable creates to correctly create the test doubles, and allow doubles to be created on APIs that AST parsing fails on.

Looks like AcceptableAPI is missing some attributes to match the expected API(?) used by flask-limiter. This is probably true for other extension as it seems reasonable for extesions/decorators to get the name of a view function.

When trying to use flask-limiter alongside acceptable the error I get is:

 File "/.../env/lib/python3.5/site-packages/flask/testing.py", line 127, in open
   follow_redirects=follow_redirects)
 File "/.../env/lib/python3.5/site-packages/werkzeug/test.py", line 764, in open
   response = self.run_wsgi_app(environ, buffered=buffered)
 File "/.../env/lib/python3.5/site-packages/werkzeug/test.py", line 677, in run_wsgi_app
   rv = run_wsgi_app(self.application, environ, buffered=buffered)
 File "/.../env/lib/python3.5/site-packages/werkzeug/test.py", line 884, in run_wsgi_app
   app_rv = app(environ, start_response)
 File "/.../env/lib/python3.5/site-packages/flask/app.py", line 1994, in __call__
   return self.wsgi_app(environ, start_response)
 File "/.../env/lib/python3.5/site-packages/flask/app.py", line 1985, in wsgi_app
   response = self.handle_exception(e)
 File "/.../env/lib/python3.5/site-packages/flask/app.py", line 1540, in handle_exception
   reraise(exc_type, exc_value, tb)
 File "/.../env/lib/python3.5/site-packages/flask/_compat.py", line 33, in reraise
   raise value
 File "/.../env/lib/python3.5/site-packages/flask/app.py", line 1982, in wsgi_app
   response = self.full_dispatch_request()
 File "/.../env/lib/python3.5/site-packages/flask/app.py", line 1614, in full_dispatch_request
   rv = self.handle_user_exception(e)
 File "/.../env/lib/python3.5/site-packages/flask/app.py", line 1517, in handle_user_exception
   reraise(exc_type, exc_value, tb)
 File "/.../env/lib/python3.5/site-packages/flask/_compat.py", line 33, in reraise
   raise value
 File "/.../env/lib/python3.5/site-packages/flask/app.py", line 1610, in full_dispatch_request
   rv = self.preprocess_request()
 File "/.../env/lib/python3.5/site-packages/flask/app.py", line 1831, in preprocess_request
   rv = func()
 File "/.../env/lib/python3.5/site-packages/flask_limiter/extension.py", line 366, in __check_request_limit
   if view_func else ""
AttributeError: 'AcceptableAPI' object has no attribute '__name__'

Given a view with:

@validate_params(
    {
        'type': 'object',
        'properties': {
            'account_id': {'type': 'string'},
            'brand_id': {'type': 'string'},
        },  
        'required': ['brand_id', 'account_id'],
    }
)

Then run:

$ env/bin/acceptable lint api.json myapp.webapi --quiet --update

Produces api.json with the expected "params_schema" for that view. But openapi.yaml otoh, contains just get.parameters: [].

A lot of boiler plate is needed to use the new mocks with unit tests using the testtools and fixtures libraries.

I added new endpoints, declared thus:

service = acceptable.AcceptableService('snapdevicegw', group='Charm Info')
charm_info_api = service.api('/v2/charms/info/<name>', 'charm_info')
charm_info_api.changelog(22, "Endpoint created.")

@charm_info_api.view(introduced_at=22)
@acceptable.validate_output({ .... })
def charm_info(name):
    ...

https://git.launchpad.net/~tartley/snapdevicegw/tree/snapdevicegw/webapi_info_charm.py?h=find-charm-fake-data&id=d125e23cbef61fbcadcc6ed4f6615a29567e1b55

As expected, this does generate documentation with a "Charm Info" group.
But the "Charm Info" entry in the left nav pane is a link to:
http://api.staging.snapcraft.io/docs/Charm%20Info.md
which is 404. (It should end ".html")

http://api.staging.snapcraft.io/docs/
(apologies if this is out of date when you read this)

Line 79 in responses.py

   def __call__(self, func):
        # responses.get_wrapper creates a function which has the same
        # signature etc.  as `func`. It execs `wrapper_template`
        # in a seperate namespace to do this. See get_wrapper code.
        wrapper_template = """\
def wrapper%(signature)s:
    with responses_mock_context:
        return func%(funcargs)s
"""
        namespace = {'responses_mock_context': self, 'func': func}
        return responses.get_wrapped(func, wrapper_template, namespace)

Passes 3 arguments, while responses library >= 11 only expect 2 in the get_wrapped call

If you create a new endpoint with required request or response fields then linting fails with "Cannot require new field FIELD".

This seems to be due to walk_schema not distinguishing between changes on an existing endpoint and a completely new one.

Creating an API with like:
api = service.api(
"/something", "something", methods=["GET"], introduced_at=7
)

The introduced_at in the generated metadata will be 1 and not 7 as specified.

It needs to be trivial to assert on the payload sent to a service double. Currently this is a PITA because:

• responses stores all mock calls in a single list, and does not support starting more than one mock at once
• responses stores the PreparedRequest, which means callers need to decode request body etc. This should be simpler, ideally allowing a caller to access the request json directly.

Probably this means switching to something other than responses under the hood. Thankfully responses is pretty small and well written, so we can crib from them easily enough.

The linter does not check that new APIs have a changelog with the right version (current+1) .

Without a changelog entry introduced_at defaults to 1.