-
-
Notifications
You must be signed in to change notification settings - Fork 769
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
Allow the specification to be specified as a URL. #1871
Conversation
Do you have a few tests for this PR? In particular I'm interested in how this would work for specs without defined operation ids. For context, I could see this being awesome for mocking external API service during development or even as part of a testing pipeline so that you automatically get the default/example response combined with request validation. |
Thanks @mjp4, I'm indeed wondering the same as @perrystallings. Another option would be to include this in the CLI instead, if that's the only place where it would be useful. |
Thanks both, I've added a test. I don't have an immediate proposal for handling APIs without Operation IDs, the ones I'm interested in all do. As it stands, and especially combined with #1874 and #1870, it has the potential to provide useful dummy output in many situations. I was also thinking of submitting a PR for a resolver that maps arbitrary Operation IDs to functions without making assumptions about module structure. Something like:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it stands, and especially combined with #1874 and #1870, it has the potential to provide useful dummy output in many situations.
Really appreciate the effort across these PRs 👍
I was also thinking of submitting a PR for a resolver that maps arbitrary Operation IDs to functions without making assumptions about module structure.
Happy to accept it if you can fit it cleanly in the current Resolver
interface.
connexion/spec.py
Outdated
spec_content = requests.get(spec).content | ||
with tempfile.NamedTemporaryFile() as tfile: | ||
tfile.write(spec_content) | ||
tfile.flush() | ||
return cls.from_file(tfile.name, arguments=arguments, base_uri=base_uri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we reuse the code we already use to resolve remote references?
connexion/connexion/json_schema.py
Lines 56 to 62 in 5de6dcc
def __call__(self, uri): | |
response = requests.get(uri) | |
response.raise_for_status() | |
data = io.StringIO(response.text) | |
with contextlib.closing(data) as fh: | |
return yaml.load(fh, ExtendedSafeLoader) |
I like that it raises a clear error for non-successful requests and doesn't use a tempfile.
@@ -408,7 +408,11 @@ def add_api( | |||
if self.middleware_stack is not None: | |||
raise RuntimeError("Cannot add api after an application has started") | |||
|
|||
if isinstance(specification, (pathlib.Path, str)): | |||
if isinstance(specification, str) and ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add in the docstrings that this can be a URL? The docstring is repeated in a couple of places across the code base, so would be good to do a find and replace.
Probably also good to explicitly mention this in the CLI docs.
@RobbeSneyders I think this is ready to go in? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good indeed, thanks @mjp4!
Changes proposed in this pull request: