opensupplyhub / open-apparel-registry

An application for searching, matching, uploading factories.
MIT License
32 stars 13 forks source link

The curl command for POSTing new facilty generated by Swagger UI can't be copeid and run in a terminal #923

Open jwalgran opened 4 years ago

jwalgran commented 4 years ago

Overview

The curl command for POSTing new facilty generated by Swagger UI contains \ characters intended to be interpreted as line continuations but are instead being included in the literal JSON string, resulting in an invalid request.

$ curl -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' --header 'Authorization: Token abc123...' -d '{ \
   "country": "US", \
   "name": "Azalea", \
   "address": "990 Spring Garden Philadelphia" \
 }' 'http://localhost:8081/api/facilities/?create=false'
{"detail":"JSON parse error - Expecting property name enclosed in double quotes: line 1 column 3 (char 2)"}

Removing the \ characters results in a successful request

$ curl -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' --header 'Authorization: Token abc123...' -d '{
   "country": "US",
   "name": "Azalea",
   "address": "990 Spring Garden Philadelphia"
 }' 'http://localhost:8081/api/facilities/?create=false'
{... match details ...}
$ curl --version
curl 7.54.0 (x86_64-apple-darwin18.0) libcurl/7.54.0 LibreSSL/2.6.5 zlib/1.2.11 nghttp2/1.24.1

Expected Behavior

The curl example can be copy-pasted into a terminal and run successfully.

Actual Behavior

The copy-pasted command makes an invalid request.

Steps to Reproduce

Additional Context

If it is not possible to easily modify the generated curl we should at least include a bold message at the bottom of the example text, just above the "Try it out" section, that clearly states that the \ characters need to be removed.

katieashaw commented 4 years ago

Two additional users have encountered problems connecting with the API due to this bug:

1) On 5th May 2) On 22nd June

On both occasions their issues were resolved by following the instructions in the additional documentation created by @jwalgran

https://docs.google.com/document/d/1ZKCN84Eu9WDAXUokojOw7Dcg5TAJw0vKnVk7RPrTPZ0/edit?usp=sharing

katieashaw commented 4 years ago

We're receiving increased levels of interest in our API and therefore more users encountering this issue. Could we include this fix in the next update, if at all possible @jwalgran ?

cc @dboyer

dboyer commented 4 years ago

@katieashaw We picked PPE related tasks for this sprint rather than this issue but have added it to our priority backlog to get to if we have time.

katieashaw commented 4 years ago

Got it, thank you!