Skip to content

Conversation

@owenconti
Copy link
Contributor

Additional fix that needs to go along with swagger-api/swagger-ui#3516 to fix swagger-api/swagger-ui#3511.

Change swagger-js to resolve parameter values based on parameter.name plus parameter.in.

I believe this changes public APIs such as:

client.apis.pet.getPetById({petId: 3}).then((data) => {
    expect(data.body.id).toEqual(4)
    cb()
})

becomes:

client.apis.pet.getPetById({'petId-path': 3}).then((data) => {
    expect(data.body.id).toEqual(4)
    cb()
})

All the tests should be passing now, but I'd still appreciate manual verification that the solution is working as intended.

@owenconti owenconti requested review from shockey and webron August 3, 2017 13:09
@webron
Copy link
Contributor

webron commented Aug 3, 2017

The signature change seems like an issue. I'd expect the in to optional only for those cases where there's a name conflict. In OAS3 we have a similar issue, and we tackled it here. Can we follow a similar logic and structure?

@shockey
Copy link
Contributor

shockey commented Aug 3, 2017

I agree with the above. My suggestion:

  1. Keep supporting the old signature (avoids a breaking change)
  2. Interpret new name-in signature correctly (so we can use it in the UI)
  3. Define the behavior encountered when the old signature is used with colliding names... Refuse to fire/build the request? Optimistically use the first parameter in the array? Print a warning with a new signature hint?

We used a similar approach when refactoring some operationId code a couple of months ago (minus the warning hint).

@shockey shockey removed the request for review from webron September 8, 2017 18:42
Copy link
Contributor

@shockey shockey left a comment

Choose a reason for hiding this comment

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

I like the type.name style 😄

Code looks good, there are new tests, and no existing tests were modified or broken.

Approved!

@shockey shockey merged commit 9023d88 into swagger-api:master Sep 23, 2017
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.

Same parameter as a query and a formData parameter doesn't appear as a formData parameter

3 participants