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

Implement initial multi api file changes. #126

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

alex-zywicki
Copy link
Contributor

@alex-zywicki alex-zywicki commented Nov 18, 2021

First draft of multi api file support #108.

server stores list of apis.
When server_name is provided, checks are performed to make sure that they resolve to the same "path".
Checks for namespace conflicts are performed.

@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2021

Codecov Report

Merging #126 (da901f3) into main (390412f) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #126      +/-   ##
==========================================
+ Coverage   97.09%   97.20%   +0.10%     
==========================================
  Files          11       11              
  Lines         895      929      +34     
==========================================
+ Hits          869      903      +34     
  Misses         26       26              
Flag Coverage Δ
unittests 97.20% <100.00%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
asynction/mock_server.py 93.44% <100.00%> (+0.05%) ⬆️
asynction/server.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 390412f...da901f3. Read the comment docs.

@@ -29,8 +29,10 @@ def make_raw_spec_view(spec: AsyncApiSpec) -> View:
return lambda: jsonify(spec.to_dict())


def make_docs_blueprint(spec: AsyncApiSpec, url_prefix: Path) -> Blueprint:
bp = Blueprint("asynction_docs", __name__, url_prefix=str(url_prefix))
def make_docs_blueprint(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dedoussis I'm not sure what to do about the name here. I am passing in the spec.info.title but that only works if your title only contains the correct characters. I think for the multi API it may be a situation where the use of a new Jinja template that can handle multiple specs would be better?

@dedoussis
Copy link
Owner

@alex-zywicki I think it would be simpler to approach this problem from another POV. As in: keep the core of asynction aligned with Flask-SocketIO, which does not have multi API capabilities, and instead of iterating through a sequence of APIs/Specs, provide a utility that bundles multiple files together into a single object. For example:

def bundle_asyncapi_specs(spec_paths: Sequence[Path]) -> AsyncApiSpec:
    """Merges multiple spec files into a single AsyncApiSpec structure"""
    ...

Users of asynction will import this method, and pass its output to the from_spec static method that is already supported.

I feel this is going to make the implementation of this feature way simpler.

@alex-zywicki
Copy link
Contributor Author

alex-zywicki commented Dec 15, 2021

@alex-zywicki I think it would be simpler to approach this problem from another POV. As in: keep the core of asynction aligned with Flask-SocketIO, which does not have multi API capabilities, and instead of iterating through a sequence of APIs/Specs, provide a utility that bundles multiple files together into a single object. For example:

def bundle_asyncapi_specs(spec_paths: Sequence[Path]) -> AsyncApiSpec:
    """Merges multiple spec files into a single AsyncApiSpec structure"""
    ...

Users of asynction will import this method, and pass its output to the from_spec static method that is already supported.

I feel this is going to make the implementation of this feature way simpler.

@dedoussis I'm fine with that. That was my original thought, but you seemed averse to that initially when you said 'I think merging of spec files is a recipe for disaster and I'm keen to avoid it.'

If that is the approach you would prefer I can test out some implementations on my end and maybe open another PR when I have something working with my application

@alex-zywicki
Copy link
Contributor Author

@alex-zywicki I think it would be simpler to approach this problem from another POV. As in: keep the core of asynction aligned with Flask-SocketIO, which does not have multi API capabilities, and instead of iterating through a sequence of APIs/Specs, provide a utility that bundles multiple files together into a single object. For example:

def bundle_asyncapi_specs(spec_paths: Sequence[Path]) -> AsyncApiSpec:
    """Merges multiple spec files into a single AsyncApiSpec structure"""
    ...

Users of asynction will import this method, and pass its output to the from_spec static method that is already supported.

I feel this is going to make the implementation of this feature way simpler.

I suppose it's also worth noting that AFAIK SocketIO has no concept of an API in the way we are using the term. The entire API concept in question here applies to asynction and how asynction is mapping asyncapi on to SocketIO

@dedoussis
Copy link
Owner

dedoussis commented Dec 16, 2021

@dedoussis I'm fine with that. That was my original thought, but you seemed averse to that initially when you said 'I think merging of spec files is a recipe for disaster and I'm keen to avoid it.'
If that is the approach you would prefer I can test out some implementations on my end and maybe open another PR when I have something working with my application

Right, sorry for mis-directing you. I still believe that each of the spec file(s) should be a stand-alone API. Each yaml file should be able to be deserialised into a complete AsyncApiSpec structure that can launch a functional AsynctionSocketIO server. We can still validate that within the bundle_asyncapi_specs method. For example, before merging some spec YAML data with the rest, we can assert that it can deserialise to an AsyncApiSpec object. However, thinking about it again, I am not sure how Asynction would benefit from such a validation. For sure it enforces some good practice, but this is a good practice that is only relevant to the user of asynction and not to asynction itself. So maybe we can just merge whatever number of YAML files we're given without caring what's included within each of them and only complain if the merged data structure does not comply with the AsyncApiSpec class (which yes, it is essentially what you proposed in the first place). It's up to the user of asynction if the spec files are complete or not. Personally, I feel that having them incomplete is a bad practice, but restricting it will not provide extra utility value to asynction.

EDIT: We could add an extra flag parameter(s) in the bundle_asyncapi_specs to enable additional validation rules, such as:

  1. No 2 AsyncAPI's should document the same namespace
  2. Each AsyncAPI file should be independent of the rest and capable of spinning up a stand-alone and functional Socket.IO server.

But this validation will be opt-in

 def bundle_asyncapi_specs(spec_paths: Sequence[Path], validation: bool = False) -> AsyncApiSpec:
    """Merges multiple spec files into a single AsyncApiSpec structure"""
    ...

No need to implement validation now of course, we can start with a bundle_asyncapi_specs that does not offer any validation, and iterate upon it.

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

3 participants