Skip to content

Conversation

@harpocrates
Copy link
Contributor

@harpocrates harpocrates commented Jul 21, 2020

Escape $ in generated curl commands

Description

This address a bug where a $ character in a request body or header
would not be properly escaped in string literals in the generated curl command.

Motivation and Context

We have a REST endpoint which is sending Cypher queries. In particular, our API uses parametrized queries where the dollar sign is critically important (here is a sample of such a query, taken from the link above MATCH (n:Person) WHERE n.name = $name RETURN n)

Fixes #5390

How Has This Been Tested?

Locally checked that the produced curl command now works.

Screenshots (if appropriate):

N/A

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested. This is a tiny very simple fix
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

Copy link
Contributor

@tim-lai tim-lai left a comment

Choose a reason for hiding this comment

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

@harpocrates Thanks for the PR. Change itself looks fine, however, we should have Cypress tests that reflects the fix to #5390.

@harpocrates harpocrates force-pushed the alec/escape-dollar-requests branch from 4bc8ac4 to b71151e Compare July 29, 2020 21:18
This address a bug where a `$` character in a request body or header
would not be properly escaped in a string in the generated curl command.

Fixes swagger-api#5390
@harpocrates harpocrates force-pushed the alec/escape-dollar-requests branch from b71151e to 29593c3 Compare July 29, 2020 21:20
@harpocrates
Copy link
Contributor Author

harpocrates commented Jul 29, 2020

@tim-lai It looks like there is already a pretty big Mocha suite to test curlify, so I tacked on the test there. Does that work?

@harpocrates harpocrates requested a review from tim-lai August 1, 2020 18:03
@tim-lai tim-lai merged commit 225a915 into swagger-api:master Aug 3, 2020
@tim-lai
Copy link
Contributor

tim-lai commented Aug 3, 2020

@harpocrates Thanks for the update with curlify mocha test. This PR is now merged! Thanks for the contribution!

@harpocrates harpocrates deleted the alec/escape-dollar-requests branch August 3, 2020 16:50
harpocrates added a commit to harpocrates/swagger-ui that referenced this pull request Sep 1, 2020
In swagger-api#6245 I forgot that calling `replace` with a string as the first
argument replaces only the first occurrence of the string. In order to
replace all occurrences, we need to use a regex with the global (g)
modifier.
harpocrates added a commit to harpocrates/swagger-ui that referenced this pull request Sep 10, 2020
In swagger-api#6245 I forgot that calling `replace` with a string as the first
argument replaces only the first occurrence of the string. In order to
replace all occurrences, we need to use a regex with the global (g)
modifier.
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.

Dollar sign not works on copy / paste of curl command line

2 participants