Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rebar can generate invalid OpenAPI specs: non-unique operationId's #250

Open
jab opened this issue Jul 27, 2021 · 0 comments
Open

Rebar can generate invalid OpenAPI specs: non-unique operationId's #250

jab opened this issue Jul 27, 2021 · 0 comments

Comments

@jab
Copy link
Collaborator

jab commented Jul 27, 2021

https://swagger.io/docs/specification/paths-and-operations/#operationid

operationId is an optional unique string used to identify an operation. If provided, these IDs must be unique among all operations described in your API.

This is important because things downstream of the spec (such as generated clients) may use the operationId (e.g. to code-generate methods) in such a way that depends on unique operationIds.

However, it looks like Rebar's OpenAPI spec generation uses the function name to generate operationIds, which need not be unique:

# my_rebar_registry.py 
from flask_rebar import Rebar

rebar = Rebar()
registry = rebar.create_handler_registry(prefix="/api")
# handlers1.py 
from my_rebar_registry import registry

@registry.handles(
    rule="/foo",
)
def foo():
    return "foo"
# handlers2.py 
from my_rebar_registry import registry

@registry.handles(
    rule="/bar",
)
def foo():
    return "bar"
# my_api_spec.py
import json

from my_rebar_registry import registry
import handlers1
import handlers2

if __name__ == "__main__":
    print(json.dumps(registry.swagger_generator.generate_swagger(registry), indent=2))
> python -m my_api_spec | grep operationId  # thither be duplicates
        "operationId": "foo",
        "operationId": "foo",
(Click for full spec output)
{
  "consumes": [
    "application/json"
  ],
  "definitions": {
    "Error": {
      "properties": {
        "errors": {
          "type": "object"
        },
        "message": {
          "type": "string"
        }
      },
      "required": [
        "message"
      ],
      "title": "Error",
      "type": "object"
    }
  },
  "host": "localhost",
  "info": {
    "description": "",
    "title": "My API",
    "version": "1.0.0"
  },
  "paths": {
    "/api/bar": {
      "get": {
        "operationId": "foo",
        "responses": {
          "default": {
            "description": "Error",
            "schema": {
              "$ref": "#/definitions/Error"
            }
          }
        }
      }
    },
    "/api/foo": {
      "get": {
        "operationId": "foo",
        "responses": {
          "default": {
            "description": "Error",
            "schema": {
              "$ref": "#/definitions/Error"
            }
          }
        }
      }
    }
  },
  "produces": [
    "application/json"
  ],
  "schemes": [],
  "securityDefinitions": {},
  "swagger": "2.0"
}

This may not be a terribly big deal in practice, since the moment you try to use such a registry with a Flask app, Flask's own duplication checking (of endpoint names) prevents you from doing so:

# app.py 
from flask import Flask

from my_rebar_registry import rebar
import handlers1
import handlers2

app = Flask("app")
rebar.init_app(app)  # kaboom
> python -m app
Traceback (most recent call last):
  File "/usr/local/Cellar/python@3.9/3.9.6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/local/Cellar/python@3.9/3.9.6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/Users/jab/tmp/rebarv2/app.py", line 9, in <module>
    rebar.init_app(app)  # kaboom
  File "/Users/jab/tmp/rebarv2/.venv/lib/python3.9/site-packages/flask_rebar/rebar.py", line 784, in init_app
    registry.register(app=app)
  File "/Users/jab/tmp/rebarv2/.venv/lib/python3.9/site-packages/flask_rebar/rebar.py", line 554, in register
    self._register_routes(app=app)
  File "/Users/jab/tmp/rebarv2/.venv/lib/python3.9/site-packages/flask_rebar/rebar.py", line 576, in _register_routes
    app.add_url_rule(
  File "/Users/jab/tmp/rebarv2/.venv/lib/python3.9/site-packages/flask/app.py", line 98, in wrapper_func
    return f(self, *args, **kwargs)
  File "/Users/jab/tmp/rebarv2/.venv/lib/python3.9/site-packages/flask/app.py", line 1282, in add_url_rule
    raise AssertionError(
AssertionError: View function mapping is overwriting an existing endpoint function: api.foo

However, it still seems preferable to make Rebar's OpenAPI spec generation smart enough to not generate invalid specs in this way. I think Rebar should instead raise an error telling the user to provide a unique function name to avoid generating duplicate operationIds, or perhaps better yet, the @registry.handles() decorators (and so forth) could accept an operation_id param that allows the user to explicitly set the associated operationId that gets generated into the spec, making the operationId no longer tied to the Python function name 1-to-1. (A non-option, IMO, would be for Rebar to silently append some number to deduplicate what would otherwise be a duplicate operationId, which I've seen some Swagger tooling do, and it causes all kinds of madness.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant