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

parameter name and in required for referenced parameter #463

Closed
runtothesun opened this issue Jun 24, 2019 · 12 comments

Comments

@runtothesun
Copy link

commented Jun 24, 2019

Hi, I have just updated to version 2.0.0 of apispec and I've seen a whole load of new error messages 'Missing keys ['name', 'in'] for parameter' the check I think was added in PR #455. In my code I create a top level parameter using:

apispec.components.parameter('ParameterSchemaName','path',{ 'name': 'parameterName', 'schema': { 'type': 'string', }, 'description': '...', 'required': True })

and then reference it in the doc string:

parameters:
-   $ref: '#/components/parameters/ParameterSchemaName

Now I need to add 'in' and 'name' into the docstring for this parameter but I have already specified the location and name in the top level definition. I see that there has been recent activity in this area so apologies if this is something that is in hand - I just wanted to raise it in case it was overlooked.

@lafrech

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

What if you pass only the ref name?

post:
    requestBody:
        'ParameterSchemaName'

or

post:
    requestBody:
        ParameterSchemaName

(I don't use YAML, I'm not sure about the quotes.)

@lafrech

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

This is probably something we overlooked. Sorry about that and thanks for reporting.

On second thought, I don't think my suggestion above will make it work. I'm pretty sure, however, that apispec expects only a ref name, not the whole ref component path, as it handles the path itself (including OASv2 / OASv3 differences) but that is another story.

@lafrech

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

My suggestion might solve your case, I can't tell as I'm not sure what YAML parser does, but there's definitely an issue.

clean_parameters can be called several times on the same operation (see test_path_merges_paths), so it should be idempotent (or we should change the logic to ensure it is called only once, if that is feasible).

On the first call, string references are changed into dictionaries, and on the second call the function assumes those are not references but parameter descriptions and complains about missing fields.

Changing

    for parameter in [p for p in parameters if isinstance(p, dict)]:

into

    for parameter in [p for p in parameters if isinstance(p, dict) and '$ref' not in p]:

solves it. Maybe not the most elegant way.

@karec, any thoughts on this?

We might also have to update tests/docs about how to pass references because I think only the ref ID should be passed. Passing a complete ref won't trigger an exception but the reference is broken in the spec IIUC.

@lafrech

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

We can prevent clean_operations from being called twice on the same operations instance by deepcopying operations in path. It would probably be a good idea to deepcopy parameters as well for the same reasons.

    def path(
        self,
        path=None,
        operations=None,
        summary=None,
        description=None,
        parameters=None,
        **kwargs
    ):
        operations = deepcopy(operations) or OrderedDict()
        parameters = deepcopy(parameters) or []

Looks like a reasonable thing to do since those are mutated by helpers and cleanup functions.


Not sure this would solve the OP.

But this plus passing only ref name in YAML docstring should solve the OP AFAIU, so we also ought to fix test/docs to instruct users to only pass ref component name, not the whole $ref string.

@lafrech

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

The fact that complete references are not allowed anymore is an overlooked unintended side effect of #453, more specifically of e079401.

I don't think this should be fixed. Passing the ref name should be enough. We need to document that as a breaking change introduced in 2.0.0.

@lafrech

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

I submitted a PR to deepcopy operations and parameters.

Trivial change. 98% worktime spent writing the test...

@lafrech

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

@runtothesun, can you please try to pass component ID only and confirm it works?

Here's a PR with a doc fix: #465.

@runtothesun

This comment has been minimized.

Copy link
Author

commented Jun 24, 2019

Hi, thanks for the quick replies! Making the first change you suggested above has done the trick. My parameters in the docstring now look like this:

parameters:
-   ParameterSchemaName

No other changes required for me at least.

Thanks again

@karec

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Sorry for delay, just seen the issue.

Like you said, checking if parameter is actually a ref object could do the trick and actually allow users to enter whatever they want, but I agree with you that enforcing a single way to setup reference could also be a good idea.

My bad on double call, didn't check for it

@karec

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

@lafrech quick up on this, but what do you think of allowing full $ref objects (so interpreted as dict in yaml parser) inside parameters ? Even if we don't want to follow full path, this could be more compliant with specs and simpler for new users (since they just need to follow

This would require more checks on clean_parameters and maybe a little update of get_ref if we want to handle cases where users wrote a thing like that:

$ref: MySchemaName

But shouldn't be a big deal and shouldn't break compatibility with current version

@lafrech

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Thanks @karec for looking into this. No pb for the double call, I didn't see that coming either.

Passing the ref without the full path should be the recommended way as it lets apispec do the job of generating the path, and it makes the app more OAS version agnostic (this is not an objective that we absolutely want to reach, but I think it is nice). So I don't mind forbidding component paths in inputs as those are a bad habit anyway.

I don't mind the breaking change either as it is part of a major version. It's not to late to document it as part of 2.0.0.

About passing

$ref: MySchemaName

I think I get your point. Visually it is nice to the user as it makes it clear he's passing a ref. But this is rather cosmetic. It needs a (very small) change on our side and a (slightly) extended API surface for a small benefit.

Everything that allows simplifying the code is welcome. I'd rather not try to support many ways to do the same thing and end up managing unforeseen cases. E.g. #441. I'm pretty sure this is a side-effect in MarshmallowPlugin of passing the complete ref path.

If the reference is a ref to a schema and has a schema name as ID, passing just the string is also consistent with how Nested allows to pass schema names as strings. I admit this argument is a bit far-fetched.

Assuming we go on with #465 (documenting the breaking change), it's never to late to add another possibility if we discover some day that passing only the ref ID is blocking some use case, not just a cosmetic issue.

Overall I'm still in favor of merging #465 and I think I'll do that soon unless someone (@karec, @sloria,...) strongly feels otherwise.

@Pentusha

This comment has been minimized.

Copy link

commented Jun 26, 2019

I use custom apispec plugin, that inherited from MarshmallowPlugin. It contains logic to describe parameters in declarative way and give me ability to refer them when I need, so in my case is more preferable to specify $ref.
I realized that I can not use this functionality with new releases 2.0.0+, because I can not redefine the clean_operations and clean_parameters functions even if I create new class based on APISpec.
I think that functionality of cleaning should be either plugin specific or overritable.

Pentusha added a commit to Pentusha/apispec that referenced this issue Jun 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.