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

Allow the specification to be specified as a URL. #1871

Merged
merged 4 commits into from
Feb 18, 2024

Conversation

mjp4
Copy link
Contributor

@mjp4 mjp4 commented Feb 9, 2024

Changes proposed in this pull request:

  • Allow a specification to be specified as a URL that is downloaded when the App runs. In combination with the existing mock features, this makes it a single command to run a mock server for any published API.

@perrystallings
Copy link

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.

@RobbeSneyders
Copy link
Member

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.

@mjp4
Copy link
Contributor Author

mjp4 commented Feb 10, 2024

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:

class DictResolver(connexion.Resolver):
    def __init__(self, handler_dict):
        def function_resolver(operation_id):
            function = handler_dict.get(operation_id)
            if function is not None:
                return function
            msg = f'Cannot resolve operationId "{operation_id}"! Supported operations: {list(handler_dict.keys())}'
            raise ValueError(msg)

        self.function_resolver = function_resolver

OPERATIONS = {
    "OperationOne": lambda: ("Body Text", 200)
}

resolver = DictResolver(OPERATIONS),

@coveralls
Copy link

coveralls commented Feb 11, 2024

Coverage Status

coverage: 94.306% (+0.09%) from 94.214%
when pulling c19743e on mjp4:allow-url-spec
into 3e64fe4 on spec-first:main.

Copy link
Member

@RobbeSneyders RobbeSneyders left a 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.

Comment on lines 168 to 172
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)
Copy link
Member

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?

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 (
Copy link
Member

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.

@mjp4
Copy link
Contributor Author

mjp4 commented Feb 17, 2024

@RobbeSneyders I think this is ready to go in?

Copy link
Member

@RobbeSneyders RobbeSneyders left a 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!

@RobbeSneyders RobbeSneyders merged commit 994f53f into spec-first:main Feb 18, 2024
8 checks passed
@mjp4 mjp4 deleted the allow-url-spec branch February 20, 2024 09:34
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

Successfully merging this pull request may close these issues.

None yet

4 participants