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

Always use openapi-spec-validator in utils.validate_spec #299

Closed
wants to merge 1 commit into from

Conversation

lafrech
Copy link
Member

@lafrech lafrech commented Oct 4, 2018

Closes #282

@lafrech lafrech force-pushed the dev_prance_openapi_spec_validator branch from ddf9c05 to 0cc2746 Compare October 4, 2018 12:19
@lafrech
Copy link
Member Author

lafrech commented Oct 6, 2018

Looks like maybe flex is more reliable so we could keep it for 2.0. (See #282).

Concerning flex:

@lafrech
Copy link
Member Author

lafrech commented Oct 6, 2018

Why not let the user choose by making it a validate_spec parameter?

def validate_spec(spec, backend=None):
    if backend is None:
        if spec.openapi_version.major < 3:
            backend = 'flex'
        else:
            backend = 'openapi-spec-validation'
        prance.BaseParser(
            spec_string=json.dumps(spec.to_dict()),
            backend=backend,
        )

@sloria
Copy link
Member

sloria commented Oct 6, 2018

Why not let the user choose by making it a validate_spec parameter?

Fine by me. @lafrech Would you like to change this PR?

@lafrech
Copy link
Member Author

lafrech commented Oct 6, 2018

I've been applying the change to my branch, and now that I'm considering what to change in setup.py and docs, I'm realizing the whole point of apispec[validation] and validate_spec is to abstract the details of the validation.

If the user can use his backend of choice, then he must install it manually and we should not force installation of an unused backend, then apispec[validation] should only install prance, then what's the point? The user could also call prance directly. Also, it is weird to provide an agnostic validation (validate_spec) function with a prance specific parameter (backend).

def validate_spec(spec, backend=None):
    """Validate the output of an :class:`APISpec` object against the
    OpenAPI specification.

    Note: Requires installing apispec with the ``[validation]`` extras.
    ::

        pip install 'apispec[validation]'

    :param str backend: prance backend to use  # <--- prance specific parameter
    :raise: apispec.exceptions.OpenAPIError if validation fails.
    """
    if backend is None:
        if spec.openapi_version.major < 3:
            backend = 'flex'
        else:
            backend = 'openapi-spec-validation'
    try:
        import prance
    except ImportError as error:  # re-raise with a more verbose message
        exc_class = type(error)
        raise exc_class(
            'validate_spec requires prance to be installed. '
            'You can install all validation requirements using:\n'
            "    pip install 'apispec[validation]'",
        )
    try:
        prance.BaseParser(
            spec_string=json.dumps(spec.to_dict()),
            backend='openapi-spec-validator',
        )
    except prance.ValidationError as err:
        raise exceptions.OpenAPIError(*err.args)
    else:
        return True

Maybe the details of validation (discrepancies between validators) are too important to the user to be hidden in apispec. We could remove the whole feature and add a note in the docs showing how to proceed with prance. Kinda sucks, but oh well...

@lafrech lafrech added this to the 1.0 milestone Oct 6, 2018
@sloria
Copy link
Member

sloria commented Oct 8, 2018

If the user can use his backend of choice, then he must install it manually and we should not force installation of an unused backend, then apispec[validation] should only install prance, then what's the point? The user could also call prance directly. Also, it is weird to provide an agnostic validation (validate_spec) function with a prance specific parameter (backend).

Both good points. Adding the prance-specific parameter certainly makes the abstraction leak.

We could remove the whole feature and add a note in the docs showing how to proceed with prance.

Perhaps. I don't feel strongly either way, but I hesitate to remove it because the current implementation should be fine for most users. If it works for 80% of users, should we really remove it for the 20%? Those few users can still use prance directly if they want.

So maybe for now, we close this PR, continue to help @lbeaufort resolve her issue in #282, and open a new issue to discuss removing validate_spec.

@lafrech
Copy link
Member Author

lafrech commented Oct 8, 2018

Yes.

@lafrech lafrech closed this Oct 8, 2018
@lafrech lafrech deleted the dev_prance_openapi_spec_validator branch October 8, 2018 21:17
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.

2 participants