Skip to content

shouldnt remove 0 values from query params #938

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 12, 2017
Merged

shouldnt remove 0 values from query params #938

merged 1 commit into from
Jan 12, 2017

Conversation

cesine
Copy link
Contributor

@cesine cesine commented Jan 12, 2017 •
edited
Loading

Hi guys,

We pulled in the latest version 2.1.31 and found a breaking change from 2.1.2x

It seems to be converting bar=0 into bar=, I added a test to show with a sample parameter and resulting url

$ npm test

> swagger-client@2.1.31 test /Users/gina/git/swagger-js
> gulp test

[12:52:09] Using gulpfile ~/git/swagger-js/gulpfile.js
[12:52:09] Starting 'test'...


  SwaggerClient
    1) doesnt remove 0 from query params


  0 passing (85ms)
  1 failing

  1) SwaggerClient doesnt remove 0 from query params:

      AssertionError: 'http://localhost:8000/double/foo?bar=' === 'http://localhost:8000/double/foo?bar=0'
      + expected - actual

      -http://localhost:8000/double/foo?bar=
      +http://localhost:8000/double/foo?bar=0

If the value is not falsy, (for example 1) the test passes we found that https://github.com/swagger-api/swagger-js/pull/935/files#diff-e9afd75580721a5b884cc82272c23984R1245 is cleaning falsey values

$ npm test

> swagger-client@2.1.31 test /Users/gina/git/swagger-js
> gulp test

[12:52:53] Using gulpfile ~/git/swagger-js/gulpfile.js
[12:52:53] Starting 'test'...


  SwaggerClient
    ✓ doesnt remove 0 from query params


  1 passing (80ms)

@fehguy
Copy link
Contributor

fehguy commented Jan 12, 2017

Hmm. The change was to support allowEmptyValues as a query parameter option. But it's possible that there's a check like this:

if(something) {
  // it exists
}

Which would fail if the value is zero or false. Will look into this.

@cesine cesine force-pushed the bug/0_query_params_are_cleaned branch from a26b54f to 8a0b1cb Compare January 12, 2017 17:58
@cesine
Copy link
Contributor Author

cesine commented Jan 12, 2017 •
edited
Loading

@fehguy yeah i found that line, updated the description and pushed the fix in this PR. lets see if travis passes

@cesine cesine force-pushed the bug/0_query_params_are_cleaned branch from 8a0b1cb to 2cdccc0 Compare January 12, 2017 18:00
@jvivs
Copy link
Contributor

jvivs commented Jan 12, 2017

👍 looks good to me.

@fehguy fehguy merged commit 84962fb into swagger-api:master Jan 12, 2017
@fehguy
Copy link
Contributor

fehguy commented Jan 12, 2017

I'll push out a 2.1.32 release with this. Swagger-ui is waiting for it as well.

@fehguy fehguy modified the milestone: v2.1.32 Jan 12, 2017
@cesine cesine deleted the bug/0_query_params_are_cleaned branch January 12, 2017 20:08
@dyang-7
Copy link

dyang-7 commented May 2, 2017

3.x ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.1.32
Development

Successfully merging this pull request may close these issues.
4 participants
@cesine @fehguy @jvivs @dyang-7