Skip to content

Conversation

@kwizzn
Copy link
Contributor

@kwizzn kwizzn commented Nov 9, 2015

We have a scenario where we have a "!" character inside our JSON request data:

{
  "username": "alice",
  "password": "cooper!00"
}

Swagger's asCurl method renders this as follows:

curl -X POST -d "{
  \"username\": \"alice\",
  \"password\": \"cooper!00\"
}" https://example.com/api

bash now tries to interpret the "!" here, which results in this error: -bash: !00: event not found.

There's no way to escape "!" in double quotes so we suggest the following output:

curl -X POST -d '{
  "username": "alice",
  "password": "cooper!00"
}' https://example.com/api

Does the trick and is also much easier to read.

Would you accept this PR? It fixes the problem and updates the corresponding 2 tests.

@ponelat
Copy link
Contributor

ponelat commented Nov 13, 2015

Thanks, that does look much better... I think this should be merged..
Do you think we should consider escaping headers as well? https://github.com/kwizzn/swagger-js/blob/single-quotes-in-curl-output/lib/types/operation.js#L864

@kwizzn
Copy link
Contributor Author

kwizzn commented Nov 16, 2015

@ponelat, you may be right and I will gladly make that change if it helps getting this issue merged.

@kwizzn
Copy link
Contributor Author

kwizzn commented Nov 16, 2015

OK done, @ponelat. I am confident my changes are ok. I've done lots of manual testing and npm test passes locally but Travis now seems to fail for a different reason. Looking at the Travis build logs, I see that the same tests are failing in the master branch due to some weird markup order difference involving an opening <div>.

Is that a known issue?

@ponelat
Copy link
Contributor

ponelat commented Nov 16, 2015

Ugh... html diff'ing is no man's idea of fun... that needs to be resolved, so that we don't hinder PRs.
I must just figure out, which is the correct one... expected or actual 😄

@ponelat
Copy link
Contributor

ponelat commented Nov 16, 2015

This is where failing test comes from... 7f0f8c5...cbc1879
@fehguy, do you know what's happening there?

we need to drop raw html diffing, or at least add a clearer diff.

@fehguy
Copy link
Contributor

fehguy commented Nov 16, 2015

yes, the html should be updated or just ignored for now. I need to get all this rendering out of the client asap.

@ponelat
Copy link
Contributor

ponelat commented Nov 16, 2015

Seeing as the its just the html tests, failing... I'm happy to merge, @fehguy ?

@fehguy
Copy link
Contributor

fehguy commented Nov 16, 2015

go for it

ponelat added a commit that referenced this pull request Nov 16, 2015
Output curl --data in single quotes (instead of double quotes)
@ponelat ponelat merged commit bea39f5 into swagger-api:master Nov 16, 2015
@kwizzn
Copy link
Contributor Author

kwizzn commented Nov 16, 2015

Thanks guys!

@ponelat
Copy link
Contributor

ponelat commented Nov 16, 2015

@kwizzn thanks for the PR 😄

@fehguy
Copy link
Contributor

fehguy commented Nov 16, 2015

@ponelat @kwizzn is there a reason why we would leave the URL in double quotes?

@kwizzn
Copy link
Contributor Author

kwizzn commented Nov 16, 2015

I thought about that too and I guess not. In fact, double quotes do not necessarily have to be escaped in a URL so we might be better off with single quote in this case, too. What do you guys think?

@ponelat
Copy link
Contributor

ponelat commented Nov 17, 2015

we should escape all of 'em... no reason not to. Mind making the change, @kwizzn ?

@kwizzn
Copy link
Contributor Author

kwizzn commented Nov 17, 2015

Happy to do it as soon as possible.

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.

3 participants