Loopback: Fix expire in explorer

[superkhau]

_From @superkhau on September 20, 2016 1:53_

KV connector expire feature isn't working properly in explorer:

Step to reproduce

1. Create a typical app with a model and browse to explorer
2. Create an object via set without TTL
3. Try to set TTL using expire

The request will fail with:

{
  "error": {
    "statusCode": 400,
    "name": "Error",
    "message": "ttl is a required argument"
  }
}

Workaround

None at the moment, we probably need to fix this one right away.

_Copied from original issue: strongloop-internal/scrum-loopback#1072_

[superkhau]

_From @bajtos on September 20, 2016 14:59_

@superkhau is there any particular reason for keeping this issue private? I would like us to be as transparent and open as possible, otherwise it's difficult to get community involvement. I would like us to default to filling issues in the public loopback project unless there is a good reason for keeping it private.

[superkhau]

_From @bajtos on September 20, 2016 15:0_

1. Create a typical app with a model and browse to explorer
2. Create an object via set without TTL
3. Try to set TTL using expire

Which request failed? The second one, setting a key without expire? Or the third one? For the third one, I think it's expected that the ttl argument is required, right?

[superkhau]

_From @bajtos on September 20, 2016 15:2_

@superkhau I am also adding missing Milestone: #Epic KV Connectors to this issue and the others you created recently.

[superkhau]

@bajtos No reason, I should've created in loopback in the first place -- I actually created all the CI issues in scrum-loopback and forgot to switch over for the KV connector issues. Moving.

[superkhau]

Which request failed? The second one, setting a key without expire? Or the third one? For the third one, I think it's expected that the ttl argument is required, right?

@bajtos First request. There is something wrong with explorer not passing the TTL entered in the textbox to the remote method.

[richardpringle]

@superkhau, what does your input look like?

[superkhau]

@richardpringle the remoting for expires is at https://github.com/strongloop/loopback/blob/master/common/models/key-value-model.js#L187-L195.

After some digging, the interesting part is API explorer with the above remoting settings doesn't submit any values in the curl request (missing -d ... contents or -F for form data). Meaning it doesn't submit any data to the backend.

That said, I did a bit more digging into PUT requests and it seems HTML forms only supports GET/POST in the spec (I think there is a proposal for the HTML5 spec -- http://programmers.stackexchange.com/questions/114156/why-are-there-are-no-put-and-delete-methods-on-html-forms).

In a nutshell, API Explorer might be failing because it doesn't support PUT in the form submission while our tests still pass since the API does allow put requests via supertest. Gonna use a workaround, but I think we'll need to create a longer term issue to figure out the cause of this.

I also did some digging into how replaceOrCreate does it in persisted model, but that one works because we submit values via source: 'body' instead of source: 'form'.

[bajtos]

That said, I did a bit more digging into PUT requests and it seems HTML forms only supports GET/POST in the spec (I think there is a proposal for the HTML5 spec -- http://programmers.stackexchange.com/questions/114156/why-are-there-are-no-put-and-delete-methods-on-html-forms).

In a nutshell, API Explorer might be failing because it doesn't support PUT in the form submission while our tests still pass since the API does allow put requests via supertest. Gonna use a workaround, but I think we'll need to create a longer term issue to figure out the cause of this.

There is one part that I don't understand. AFAIK, Explorer/swagger-ui should be sending JSON-encoded request bodies, not application/x-www-form-urlencoded. Therefore the HTML5 limitation allowing only POST for HTML forms should not be relevant here.

@superkhau could you please investigate more?

1) What is the swagger description of expire method?
2) How is this description handled by swagger-ui and swagger-js? Where is the code that excludes the ttl parameter from the request body?

[bajtos]

Also note that loopback-component-explorer is using our fork of swagger-ui/js, we may be able to implement the fix there.

[superkhau]

There is one part that I don't understand. AFAIK, Explorer/swagger-ui should be sending JSON-encoded request bodies, not application/x-www-form-urlencoded. Therefore the HTML5 limitation allowing only POST for HTML forms should not be relevant here.

Then I believe https://github.com/strongloop/loopback/blob/master/common/models/key-value-model.js#L192 may be wrong and should be changed from form to body?

1. https://github.com/strongloop/loopback/blob/master/common/models/key-value-model.js#L187-L195
2. They simply don't send any params (I simply opened network inspector and checked if any outgoing params are sent with the request -- they are not which leads me to suspect PUT with form type doesn't work with swagger-ui -- in addition, the curl output shown in the UI doesn't show the -d or -F flags if we set source: 'form').

[superkhau]

I've narrowed it down to:

1. Setting remoting to source: 'body' instead of form sends the correct object (Content-type: application/json), but shows in API explorer as a textbox instead of an input field -- meaning you can pass in "60000" which is a not a JSON object and it will work correctly.
2. Setting remoting to source: 'form' correctly shows a input field, but doesn't send the data as part of it's request.

I am blocked on this (already spoke to @richardpringle and @gunjpan), will try and see if you're online monday night to see if you can give me a hand.

[richardpringle]

This is definitely a problem with explorer. The content-type: application/x-www-form-urlencoded header is not set by the swagger UI. Using Postman, the http.source form works fine.

[superkhau]

@bajtos Had a meeting with @raymondfeng today regarding this issue and we decided to go with the query string route.

1. Simple one liner change to get it working in LB
2. Found a bug in loopback-swagger (we need to coerce 'form' to 'formData' -- we have other tests in various projects setting source: 'form', but they are all returning false positives as form is not actually supported by Swagger.
3. The set API already uses query string to set TTL, so this would make it more consistent for the end users (if we went with formData now, the user experience would be terrible because all endpoints would accept application/json while the one expires endpoint will take x-www-form-urlencoded -- confusing).

Will submit the following PRs:

• [ ] loopback -- change key-value-model.js expires remoting metadata to query instead of form
• [ ] loopback-swagger -- submit fix to coerce form into formData

The Swagger fix should keep the other tests passing, but in case they fail, might need to fix CI for those too.

[bajtos]

we decided to go with the query string route.

I believe this is a breaking change. How are we going to ensure existing apps don't break after upgrade?

Found a bug in loopback-swagger (we need to coerce 'form' to 'formData' -- we have other tests in various projects setting source: 'form', but they are all returning false positives as form is not actually supported by Swagger.

Will submit the following PRs:
loopback-swagger -- submit fix to coerce form into formData

+1

Note there is a catch here: because Swagger's formData type expects url-encoded form data body (as opposed to LoopBack, which allows JSON too), arguments are not allowed to have a an object type. This does not affect ttl, which is a number, but we need to handle this case in loopback-swagger in general, otherwise we may produce invalid swagger.

TBH, I think a better solution is to use the same approach we use for functions that have multiple named return arguments, where the swagger generator creates a new schema describing the JSON object.

I.e. in our case, we should describe expire method as accepting a JSON body containing a ttl property of type number (integer?).

See convertReturnsToSwagger and https://github.com/strongloop/loopback-swagger/commit/b23e08cfe110db539ae6c3b8a4ec82ad58a999e8.

if we went with formData now, the user experience would be terrible because all endpoints would accept application/json while the one expires endpoint will take x-www-form-urlencoded -- confusing).

As far as LoopBack is concerned, source:formData works for application/json requests too. source:body means the argument is set to the full JSON request body. source:formData means that the argument is looked up as a property value in the JSON request body.

Request:

{
  "ttl": 100
}

source:body sets ttl={ttl:100}, source:form sets ttl=100. See rest-coercion tests.

We also have a unit-test in LoopBack (link) to confirm.

---

Honestly, I think the current design of the REST API is a good one and we should fix loopback-swagger to correctly support this API flavour, instead of bending our API to work around current loopback-swagger limitations.

@superkhau @raymondfeng

[superkhau]

@bajtos Last time I spoke to @raymondfeng regarding this issue, he mentioned https://github.com/strongloop/loopback-swagger/blob/master/lib/specgen/route-helper.js#L276 line failing since swagger expects formData. Basically change that line from:

if (accepts.http && accepts.http.source) {
  paramType = accepts.http.source;
}

to:

// coerce 'form' into 'formData'
if (accepts.http && accepts.http.source) {
  paramType = accepts.http.source === 'form' ? 'formData' : accepts.http.source;
  // or
  if (accepts.http.source === 'form')
    accepts.http.source = 'formData';
    paramType = accepts.http.source;
  }
}

What are your thoughts on this before I update the PR? Is this a viable solution?

[bajtos]

@superkhau +1 for fixing loopback-swagger to produce in: 'formData' instead of in: 'form'. BTW I think this is a bug introduced while upgrading our specgen to Swagger 2.0. In Swagger 1.2, paramType: 'form' is valid.

[superkhau]

Submitted fix at https://github.com/strongloop/loopback-swagger/pull/69