apiaryio / dredd

Language-agnostic HTTP API Testing Tool
https://dredd.org
MIT License
4.18k stars 279 forks source link

Incorrect detection of Ambiguous parameters? #1590

Open jaydiablo opened 8 years ago

jaydiablo commented 8 years ago

I've been dealing with an issue in Dredd for a certain API document setup. I thought I'd open the issue here rather than in Dredd or the deprecated blueprint-transactions because it seems that Dredd is moving to use this, and I believe this is where the error occurs (it exists in blueprint-transactions as well).

I'll start by referencing this old api-blueprint ticket: https://github.com/apiaryio/api-blueprint/issues/40

The API Blueprint syntax that I'm using is basically what is described by the OP in that ticket, but here's a snippet of the relevant pieces:

# Group Approvals

## Approval Collection [/api/endpoint/approvals{?page,required_param}]

+ Parameters
    + required_param: 10000 (required, number) - Something that is required for all requests

+ Attributes
    + count: 1 (number) - Number of results in this response
    + approvals: [] (array) - The approvals

### List Approvals [GET]

+ Parameters
  + page: 1 (optional, number) - Page number of results, only needed in the List request

+ Response 200 (application/json)
    + Attributes (Approval Collection)

### Create an Approval [POST]

+ Request (application/json)

        {
            "some_param": 2
        }

+ Response 200 (application/hal+json)

The important bits here, the URL template defined on the Collection has two parameters specified. One is required in all requests in this grouping, the other is only defined/used by the GET request (the POST request doesn't use the page parameter at all).

This blueprint, in the Apiary.io system renders as one would expect. The required parameter shows up for both the GET and POST requests, and the page parameter only shows up for the GET request.

However, when this is processed by Dredd, warnings are emitted:

warn: Compilation warning in file './documentation/apiary.apib': Ambiguous URI parameter in template: /api/endpoint/approvals{?page,required_param} Parameter not defined in blueprint:'page' on Approvals > Approval Collection > >Create an Approval

(this is using Dredd version 1.1.0-pre.0, but similar warnings are emitted in other versions (1.0.5, 1.0.6, 1.0.10))

In versions of Dredd 1.0.10 and below, these POST requests aren't even attempted by Dredd, they're just silently ignored. In this pre-release version of Dredd the POSTs are attempted, but against a "null" URL:

fail: POST null duration: 161ms

I can make Dredd process these requests fine if I move that page parameter definition up into the main Collection grouping, rather than in the GET request block, but then in Apiary.io's viewer, the page parameter will show for the POST request, which is incorrect.

I think that the undefined parameter detection here needs to be modified to allow parameters that are defined on the URI template, defined elsewhere in the group, but aren't used for this particular request method.

Has this been discussed at all? I'm not sure exactly what the spec is for this, but I am assuming it should be allowed since the apiary.io viewer renders it correctly. Also, I use the api-blueprint Sublime plugin and the drafter integration it uses to show errors and warnings doesn't complain about these lines, fwiw.

Let me know if you need any more info or examples, thanks!

jaydiablo commented 8 years ago

Just trying some different things here.

If I change the definition around a little bit, by removing the page parameter from the URI Template like so:

# Group Approvals

## Approval Collection [/api/endpoint/approvals{?required_param}]

+ Parameters
    + required_param: 10000 (required, number) - Something that is required for all requests

+ Attributes
    + count: 1 (number) - Number of results in this response
    + approvals: [] (array) - The approvals

### List Approvals [GET]

+ Parameters
  + page: 1 (optional, number) - Page number of results, only needed in the List request

+ Response 200 (application/json)
    + Attributes (Approval Collection)

### Create an Approval [POST]

+ Request (application/json)

        {
            "some_param": 2
        }

+ Response 200 (application/hal+json)

I get a warning in Sublime, Dredd and Apiary.io, but it does render as I'd expect in Apiary.io AND the dredd tests run properly. Here are the warnings that Dredd emits:

warn: Parser warning in file './documentation/apiary.apib': (warning code 8) parameter 'page' not specified in 'Approval Collection' its '/api/endpoint/approvals{?required_param}' URI template on line 122 warn: Compilation warning in file './documentation/apiary.apib': URI template: /api/endpoint/approvals{?required_param} Doesn't contain expression for parameter 'page' on Approvals > Approval Collection > List Approvals

Still not ideal I suppose, but at least I can get the tests to run and have the documentation represent the requests properly.

honzajavorek commented 8 years ago

@jaydiablo Hi, thanks for reporting this and for discussing the issue! You've chosen the right repository.

This seems like bug in Dredd Transactions. If the behavior is according to https://github.com/apiaryio/api-blueprint/issues/40#issuecomment-33798856 and Dredd doesn't behave, it is a bug. Moreover, the pre release might not behave exactly the same as previous version (but it should!), there are regressions, e.g. https://github.com/apiaryio/dredd/issues/469.

Dredd has more requirements than documentation, so there might be errors even if the parser or documentation behaves correctly. To be able to perform requests, it always needs to know what value to use for required parameters. If it doesn't find value to use, it prints the warning/error about ambiguous parameters. Since all your required parameters have default/sample values, it seems that

Ambiguous URI parameter in template: /api/endpoint/approvals{?page,required_param}
Parameter not defined in blueprint:'page' on Approvals > Approval Collection > >Create an Approval

is a bug in Dredd Transactions.


By the way,

+ approvals: [] (array) - The approvals

isn't correct MSON, it should be just

+ approvals (array) - The approvals
jaydiablo commented 8 years ago

Thanks for the reply, I'll certainly be keeping an eye on this, let me know if you want me to test anything.

Also, just fyi, I tested the "remove the params from the URI template" change I did in the last comment against Dredd 1.0.10 and it does perform the POST actions (there doesn't seem to be much difference between 1.0.10 and 1.1.0-pre.0 in this case).

hueter commented 8 years ago

I just experienced this today warn: Runtime compilation warning: Ambigous URI parameter in template because of query parameters which were not specified under a POST method. dredd v1.0.11

By definition, query params should be included for GET requests only.

When I ran dredd --names the POST transactions were excluded. So I fixed it by doing what @jaydiablo suggested in his second post, just removed them from the specification.

Edit: Also on dredd v1.2.0