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

Dredd unable to correctly handle blueprint with multiple requests #78

Open
honzajavorek opened this Issue Jul 4, 2014 · 19 comments

Comments

Projects
None yet
5 participants
@honzajavorek
Copy link
Member

honzajavorek commented Jul 4, 2014

Example blueprint: http://docs.apiblueprintapi.apiary.io/ Possibly related to #25.

@netmilk

This comment has been minimized.

Copy link
Contributor

netmilk commented Jul 7, 2014

Thanks for the report! Can you provide here raw blueprint for replicating the issue?

@netmilk netmilk added the bug label Jul 7, 2014

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Jul 14, 2014

Sure, here: https://github.com/apiaryio/apiblueprintorg/blob/master/apiary.apib

Compose [POST] (line 214) has two possible inputs (two requests), but only one output (one response).

@netmilk

This comment has been minimized.

Copy link
Contributor

netmilk commented Jan 15, 2015

Ok, this is verified. But what's the proposed behaviour? Should Dredd test the response for each request?

$ curl -s https://raw.githubusercontent.com/apiaryio/apiblueprintorg/master/apiary.apib > multi-blueprint.md
$ dredd multi-blueprint.md http://localhost --names
warn: Runtime compilation warning: Multiple requests in example, using first.
on Blueprint Manipulation > Blueprint Composer > Compose
info: Beginning Dredd testing...
info: API > Service Root > List
info: Blueprint Manipulation > Blueprint Parser > Parse > Example 1
info: Blueprint Manipulation > Blueprint Parser > Parse > Example 2
info: Blueprint Manipulation > Blueprint Composer > Compose

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Jan 15, 2015

As there can be multiple requests as well as multiple responses, I think the algorithm would be following (high-level coffee-like pseudocode):

for example in examples
  for request in example.requests
    for response in example.responses
      test(request, response)

That's probably the best dredd can do.

@netmilk

This comment has been minimized.

Copy link
Contributor

netmilk commented Jan 15, 2015

Maybe I'm not right, but I don't think so. This code will mean that in the example if there is no request but only responses, no test is created and also if there is no response only request no test is created either.

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Jan 15, 2015

Well, yes, good catch. I did not think about these cases while writing the code. Depends on what is the intended behavior, what the blueprint author expects in such situation - one option is to skip cases where the pair is not complete (code above) or to provide a default request and default response:

for example in examples
  for request in (example.requests or [defaultRequest])
    for response in (example.responses or [defaultResponse])
      test(request, response)

Right now I don't know what's Apiary app's behavior in these cases and if there's a concept of default request/response already. I'd need to experiment with it for a while.

@obihann

This comment has been minimized.

Copy link

obihann commented Aug 28, 2015

poke This issue is coming up for me now, any progress?

@w-vi

This comment has been minimized.

Copy link
Member

w-vi commented Sep 1, 2015

Hi @obihann this hasn't been addressed yet, next week we'll be able to give you some ETA.

@obihann

This comment has been minimized.

Copy link

obihann commented Sep 2, 2015

@w-vi If this is a manpower issue let me know I can likely help out. Thanks for the reply though.

@w-vi

This comment has been minimized.

Copy link
Member

w-vi commented Sep 3, 2015

@obihann It is a manpower problem at least to some degree but first we have to decide how to handle the edge cases (missing request/response etc.), I guess we will discuss it once @netmilk is back in game.

@obihann

This comment has been minimized.

Copy link

obihann commented Sep 10, 2015

Just a poke to bring this back on the table. @w-vi @netmilk

@netmilk

This comment has been minimized.

Copy link
Contributor

netmilk commented Sep 10, 2015

Hey @obihann,

thank you very much for offering help! It's awesome! Would you be so kind and share a blueprint example with multiple request and responses and proposed test matrix for these examples and request -response pairs?

@obihann

This comment has been minimized.

Copy link

obihann commented Sep 10, 2015

No problem, I'll have that for you later today.

@obihann

This comment has been minimized.

Copy link

obihann commented Sep 10, 2015

### Delete a Object [DELETE /{id}{?permanent}]

+ Parameters

    + permanent: `0` (number, optional) - Set if the object will be permanently deleted
        + Members
            + `0` - Flag the object as deleted however do not remove it from our system
            + `1` - Permanently delete this object

+ Request

+ Response 202

So this is an example of some markdown I have, it represents a single API endpoint that accepts an optional URL param that can be either 0 or 1. Ideally I would like dredd to acknowledge this and execute the request for each of the possible URL param values.

An alternative solution that may required updates to the MSON spec would be a way to outline what URL param values are includes in each request.

@obihann

This comment has been minimized.

Copy link

obihann commented Sep 10, 2015

Another use case would be two request's one with a body one without, the one without would return something like a 400, perhaps with a explicit error. The request with a body would return something like a 200. The point here being to test that invalid requests do not break (500) the server and are handled properly.

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Sep 15, 2015

This is biting me again as well as #25. The use case is pretty much the same as before - I am working on similar parsing service as in my previous comments. It uses multiple requests and responses (M:N relation) heavily. I can not use Dredd for testing such API as it always takes just the first pair.

@netmilk netmilk referenced this issue Mar 1, 2016

Closed

Migration to API Elements #388

11 of 12 tasks complete

@honzajavorek honzajavorek added improvement and removed bug labels Mar 14, 2016

honzajavorek added a commit that referenced this issue Aug 24, 2016

fix: Use just the 1st req-res pair within APIB transaction examples
This change tests and fixes a problem introduced with migration to API Elements
in order to support Swagger in Dredd. Original implementation always selected
the first request-response pair from each transaction example. This wasn't
re-implemented correctly on top of API Elements. Instead, all specified responses
are appearing, which breaks Dredd's behavior in many ways. Respective test was
ported, but unfortunately with the same mistake. This commit fixes the situation.

Some early adopters discovered the issue and considered it to be a new
feature, but it really breaks how Dredd should work at the moment and needs
to be removed. It leads to duplicate transaction names and other undefined
behavior.

In order to implement #25 and #78,
which many believed happened when they discovered the bug, much more work needs
to be done. Namely designing and adopting a new way of addressing transactions
in Dredd #227.

Closes #615

BREAKING CHANGE

honzajavorek added a commit to apiaryio/dredd-transactions that referenced this issue Aug 24, 2016

fix: Use just the 1st req-res pair within APIB transaction examples
This change fixes a problem introduced with migration to API Elements in order
to support Swagger in Dredd. Original implementation always selected the first
request-response pair from each transaction example. This wasn't re-implemented
correctly on top of API Elements. Instead, all specified responses are appearing,
which breaks Dredd's behavior in many ways. Respective test was ported, but
unfortunately with the same mistake.

Some early adopters discovered the issue and considered it to be a new
feature, but it really breaks how Dredd should work at the moment and needs
to be removed. It leads to duplicate transaction names and other undefined
behavior.

In order to implement apiaryio/dredd#25 and apiaryio/dredd#78,
which many believed happened when they discovered the bug, much more work needs
to be done. Namely designing and adopting a new way of addressing transactions
in Dredd apiaryio/dredd#227.

BREAKING CHANGE

honzajavorek added a commit that referenced this issue Aug 24, 2016

fix: Use just the 1st req-res pair within APIB transaction examples
This change tests and fixes a problem introduced with migration to API Elements
in order to support Swagger in Dredd. Original implementation always selected
the first request-response pair from each transaction example. This wasn't
re-implemented correctly on top of API Elements. Instead, all specified responses
are appearing, which breaks Dredd's behavior in many ways. Respective test was
ported, but unfortunately with the same mistake. This commit fixes the situation.

Some early adopters discovered the issue and considered it to be a new
feature, but it really breaks how Dredd should work at the moment and needs
to be removed. It leads to duplicate transaction names and other undefined
behavior.

In order to implement #25 and #78,
which many believed happened when they discovered the bug, much more work needs
to be done. Namely designing and adopting a new way of addressing transactions
in Dredd #227.

Closes #615

BREAKING CHANGE

honzajavorek added a commit that referenced this issue Aug 24, 2016

fix: Use just the 1st req-res pair within APIB transaction examples
BREAKING CHANGE: This change tests and fixes a problem introduced with migration to API Elements
in order to support Swagger in Dredd. Original implementation always selected
the first request-response pair from each transaction example. This wasn't
re-implemented correctly on top of API Elements. Instead, all specified responses
are appearing, which breaks Dredd's behavior in many ways. Respective test was
ported, but unfortunately with the same mistake. This commit fixes the situation.

Some early adopters discovered the issue and considered it to be a new
feature, but it really breaks how Dredd should work at the moment and needs
to be removed. It leads to duplicate transaction names and other undefined
behavior.

In order to implement #25 and #78,
which many believed happened when they discovered the bug, much more work needs
to be done. Namely designing and adopting a new way of addressing transactions
in Dredd #227.

Closes #615
@pksunkara

This comment has been minimized.

Copy link
Member

pksunkara commented Aug 18, 2017

Since we have moved to API Description Refract, we can support this in a backward compatible way.

Assume, we have request response pairs (RRP from now) in an API Blueprint as shown below:

+ Request A
+ Request B
+ Response 200
+ Response 400
+ Request C
+ Response 403

API Description refract generates 5 RRP. They are as follows:

RRP 1
====
+ Request A
+ Response 200

RRP 2
====
+ Request B
+ Response 200

RRP 3
====
+ Request A
+ Response 400

RRP 4
=====
+ Request B
+ Response 400

RRP 5
====
+ Request C
+ Response 403

Actual Behaviour

Dredd currently only tests RRP 1 and RRP 5 as the first pair and the second pair.

Expected Behaviour

Dredd needs to test all 5 pairs. But to ensure the backward compatibility of the order of the pairs that are being tested, we will add the newer pairs to the end. Which means dredd will test them in the following order: RRP 1, RRP 5, RRP 2, RRP 3, RRP 4

@honzajavorek

This comment has been minimized.

Copy link
Member

honzajavorek commented Aug 18, 2017

@pksunkara As we discussed previously, I think adding new transactions is still a breaking change, but that could be solved. We could either make this breaking change, or to skip the transactions by default and allow people to unskip them in hooks.

And when I was thinking about writing you the sentence above, I just realized a problem with this. What would be transaction names of the added requests? There is no way how to distinguish such transactions in current transaction names. According to how it works now, the transaction names would be:

A-200 ... Lorem > Ipsum > Lorem Ipsum > Example 1
A-400 ... Lorem > Ipsum > Lorem Ipsum > Example 1
B-200 ... Lorem > Ipsum > Lorem Ipsum > Example 1
B-400 ... Lorem > Ipsum > Lorem Ipsum > Example 1
C-403 ... Lorem > Ipsum > Lorem Ipsum > Example 2

That's it. They wouldn't be addressable. So I'm really afraid this will be possible only after #227 are done 😞

@pksunkara

This comment has been minimized.

Copy link
Member

pksunkara commented Aug 18, 2017

The transaction names would be:

A-200 ... Lorem > Ipsum > Lorem Ipsum > Example 1
A-400 ... Lorem > Ipsum > Lorem Ipsum > Example 3
B-200 ... Lorem > Ipsum > Lorem Ipsum > Example 4
B-400 ... Lorem > Ipsum > Lorem Ipsum > Example 5
C-403 ... Lorem > Ipsum > Lorem Ipsum > Example 2

I thought that part was clear from my comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment